go/analysis: harmonize flags across all checkers

The -json and -c=N flags, formerly belonging only to the
go/packages-based {single,multi}checkers, are now supported by
unitchecker as well.

The no-op -source, -v, -all, and -tags flags, formerly belonging only
to unitchecker, have moved to the analysisflags package, which is
common to all checkers.

The -flags flag now reports all registered flags (except the
{single,multi}checker-only debugging flags) rather than just those
related to analyzers, allowing one to say: 'go vet -json' or 'go vet -c=1'.

The code for printing diagnostics, either plain or in JSON, has been
factored and moved into the common analysisflags package.

This CL depends on https://go-review.googlesource.com/c/go/+/149960 to
cmd/go, which causes 'go vet' to populate the ID field of the *.cfg.
This field is used as a key in the JSON tree.

Added basic tests of the new -json and -c unitchecker flags.

Change-Id: Ia7a3a9adc86de067de060732d2c200c58be3945a
Reviewed-on: https://go-review.googlesource.com/c/150038
Reviewed-by: Michael Matloob <matloob@golang.org>

go/analysis/cmd/vet-lite/main.go

@@ -7,8 +7,6 @@
 package main
 
 import (
-	"flag"
-
 	"golang.org/x/tools/go/analysis/unitchecker"
 
 	"golang.org/x/tools/go/analysis/passes/asmdecl"
@@ -34,11 +32,6 @@ import (
 	"golang.org/x/tools/go/analysis/passes/unusedresult"
 )
 
-// Flags for legacy vet compatibility.
-//
-// These flags, plus the shims in analysisflags, enable
-// existing scripts that run vet to continue to work.
-//
 // Legacy vet had the concept of "experimental" checkers. There
 // was exactly one, shadow, and it had to be explicitly enabled
 // by the -shadow flag, which would of course disable all the
@@ -53,12 +46,6 @@ import (
 // Alternatively, one could build a multichecker containing all
 // the desired checks (vet's suite + shadow) and run it in a
 // single "go vet" command.
-func init() {
-	_ = flag.Bool("source", false, "no effect (deprecated)")
-	_ = flag.Bool("v", false, "no effect (deprecated)")
-	_ = flag.Bool("all", false, "no effect (deprecated)")
-	_ = flag.String("tags", "", "no effect (deprecated)")
-}
 
 func main() {
 	unitchecker.Main(

go/analysis/internal/analysisflags/flags.go

@@ -11,27 +11,29 @@ import (
 	"encoding/json"
 	"flag"
 	"fmt"
+	"go/token"
 	"io"
+	"io/ioutil"
 	"log"
 	"os"
 	"strconv"
+	"strings"
 
 	"golang.org/x/tools/go/analysis"
 )
 
+// flags common to all {single,multi,unit}checkers.
+var (
+	JSON    = false // -json
+	Context = -1    // -c=N: if N>0, display offending line plus N lines of context
+)
+
 // Parse creates a flag for each of the analyzer's flags,
 // including (in multi mode) a flag named after the analyzer,
 // parses the flags, then filters and returns the list of
 // analyzers enabled by flags.
 func Parse(analyzers []*analysis.Analyzer, multi bool) []*analysis.Analyzer {
 	// Connect each analysis flag to the command line as -analysis.flag.
-	type analysisFlag struct {
-		Name  string
-		Bool  bool
-		Usage string
-	}
-	var analysisFlags []analysisFlag
-
 	enabled := make(map[*analysis.Analyzer]*triState)
 	for _, a := range analyzers {
 		var prefix string
@@ -44,7 +46,6 @@ func Parse(analyzers []*analysis.Analyzer, multi bool) []*analysis.Analyzer {
 			enableUsage := "enable " + a.Name + " analysis"
 			flag.Var(enable, a.Name, enableUsage)
 			enabled[a] = enable
-			analysisFlags = append(analysisFlags, analysisFlag{a.Name, true, enableUsage})
 		}
 
 		a.Flags.VisitAll(func(f *flag.Flag) {
@@ -55,12 +56,6 @@ func Parse(analyzers []*analysis.Analyzer, multi bool) []*analysis.Analyzer {
 
 			name := prefix + f.Name
 			flag.Var(f.Value, name, f.Usage)
-
-			var isBool bool
-			if b, ok := f.Value.(interface{ IsBoolFlag() bool }); ok {
-				isBool = b.IsBoolFlag()
-			}
-			analysisFlags = append(analysisFlags, analysisFlag{name, isBool, f.Usage})
 		})
 	}
 
@@ -68,7 +63,16 @@ func Parse(analyzers []*analysis.Analyzer, multi bool) []*analysis.Analyzer {
 	printflags := flag.Bool("flags", false, "print analyzer flags in JSON")
 	addVersionFlag()
 
-	// Add shims for legacy vet flags.
+	// flags common to all checkers
+	flag.BoolVar(&JSON, "json", JSON, "emit JSON output")
+	flag.IntVar(&Context, "c", Context, `display offending line with this many lines of context`)
+
+	// Add shims for legacy vet flags to enable existing
+	// scripts that run vet to continue to work.
+	_ = flag.Bool("source", false, "no effect (deprecated)")
+	_ = flag.Bool("v", false, "no effect (deprecated)")
+	_ = flag.Bool("all", false, "no effect (deprecated)")
+	_ = flag.String("tags", "", "no effect (deprecated)")
 	for old, new := range vetLegacyFlags {
 		newFlag := flag.Lookup(new)
 		if newFlag != nil && flag.Lookup(old) == nil {
@@ -80,11 +84,7 @@ func Parse(analyzers []*analysis.Analyzer, multi bool) []*analysis.Analyzer {
 
 	// -flags: print flags so that go vet knows which ones are legitimate.
 	if *printflags {
-		data, err := json.MarshalIndent(analysisFlags, "", "\t")
+		printFlags()
-		if err != nil {
-			log.Fatal(err)
-		}
-		os.Stdout.Write(data)
 		os.Exit(0)
 	}
 
@@ -122,6 +122,33 @@ func Parse(analyzers []*analysis.Analyzer, multi bool) []*analysis.Analyzer {
 	return analyzers
 }
 
+func printFlags() {
+	type jsonFlag struct {
+		Name  string
+		Bool  bool
+		Usage string
+	}
+	var flags []jsonFlag = nil
+	flag.VisitAll(func(f *flag.Flag) {
+		// Don't report {single,multi}checker debugging
+		// flags as these have no effect on unitchecker
+		// (as invoked by 'go vet').
+		switch f.Name {
+		case "debug", "cpuprofile", "memprofile", "trace":
+			return
+		}
+
+		b, ok := f.Value.(interface{ IsBoolFlag() bool })
+		isBool := ok && b.IsBoolFlag()
+		flags = append(flags, jsonFlag{f.Name, isBool, f.Usage})
+	})
+	data, err := json.MarshalIndent(flags, "", "\t")
+	if err != nil {
+		log.Fatal(err)
+	}
+	os.Stdout.Write(data)
+}
+
 // addVersionFlag registers a -V flag that, if set,
 // prints the executable version and exits 0.
 //
@@ -247,3 +274,70 @@ var vetLegacyFlags = map[string]string{
 	"unusedfuncs":         "unusedresult.funcs",
 	"unusedstringmethods": "unusedresult.stringmethods",
 }
+
+// ---- output helpers common to all drivers ----
+
+// PrintPlain prints a diagnostic in plain text form,
+// with context specified by the -c flag.
+func PrintPlain(fset *token.FileSet, diag analysis.Diagnostic) {
+	posn := fset.Position(diag.Pos)
+	fmt.Fprintf(os.Stderr, "%s: %s\n", posn, diag.Message)
+
+	// -c=N: show offending line plus N lines of context.
+	if Context >= 0 {
+		data, _ := ioutil.ReadFile(posn.Filename)
+		lines := strings.Split(string(data), "\n")
+		for i := posn.Line - Context; i <= posn.Line+Context; i++ {
+			if 1 <= i && i <= len(lines) {
+				fmt.Fprintf(os.Stderr, "%d\t%s\n", i, lines[i-1])
+			}
+		}
+	}
+}
+
+// A JSONTree is a mapping from package ID to analysis name to result.
+// Each result is either a jsonError or a list of jsonDiagnostic.
+type JSONTree map[string]map[string]interface{}
+
+// Add adds the result of analysis 'name' on package 'id'.
+// The result is either a list of diagnostics or an error.
+func (tree JSONTree) Add(fset *token.FileSet, id, name string, diags []analysis.Diagnostic, err error) {
+	var v interface{}
+	if err != nil {
+		type jsonError struct {
+			Err string `json:"error"`
+		}
+		v = jsonError{err.Error()}
+	} else if len(diags) > 0 {
+		type jsonDiagnostic struct {
+			Category string `json:"category,omitempty"`
+			Posn     string `json:"posn"`
+			Message  string `json:"message"`
+		}
+		var diagnostics []jsonDiagnostic
+		for _, f := range diags {
+			diagnostics = append(diagnostics, jsonDiagnostic{
+				Category: f.Category,
+				Posn:     fset.Position(f.Pos).String(),
+				Message:  f.Message,
+			})
+		}
+		v = diagnostics
+	}
+	if v != nil {
+		m, ok := tree[id]
+		if !ok {
+			m = make(map[string]interface{})
+			tree[id] = m
+		}
+		m[name] = v
+	}
+}
+
+func (tree JSONTree) Print() {
+	data, err := json.MarshalIndent(tree, "", "\t")
+	if err != nil {
+		log.Panicf("internal error: JSON marshalling failed: %v", err)
+	}
+	fmt.Printf("%s\n", data)
+}

go/analysis/internal/checker/checker.go

@@ -7,12 +7,10 @@ package checker
 import (
 	"bytes"
 	"encoding/gob"
-	"encoding/json"
 	"flag"
 	"fmt"
 	"go/token"
 	"go/types"
-	"io/ioutil"
 	"log"
 	"os"
 	"reflect"
@@ -25,12 +23,11 @@ import (
 	"time"
 
 	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/internal/analysisflags"
 	"golang.org/x/tools/go/packages"
 )
 
 var (
-	JSON = false
-
 	// Debug is a set of single-letter flags:
 	//
 	//	f	show [f]acts as they are created
@@ -41,17 +38,16 @@ var (
 	//
 	Debug = ""
 
-	Context = -1 // if >=0, display offending line plus this many lines of context
-
 	// Log files for optional performance tracing.
 	CPUProfile, MemProfile, Trace string
 )
 
 // RegisterFlags registers command-line flags used the analysis driver.
 func RegisterFlags() {
-	flag.BoolVar(&JSON, "json", JSON, "emit JSON output")
+	// When adding flags here, remember to update
+	// the list of suppressed flags in analysisflags.
+
 	flag.StringVar(&Debug, "debug", Debug, `debug flags, any subset of "lpsv"`)
-	flag.IntVar(&Context, "c", Context, `display offending line with this many lines of context`)
 
 	flag.StringVar(&CPUProfile, "cpuprofile", "", "write CPU profile to this file")
 	flag.StringVar(&MemProfile, "memprofile", "", "write memory profile to this file")
@@ -276,50 +272,18 @@ func printDiagnostics(roots []*action) (exitcode int) {
 		}
 	}
 
-	if JSON {
+	if analysisflags.JSON {
-		tree := make(map[string]map[string]interface{}) // ID -> analysis -> result
+		// JSON output
-
+		tree := make(analysisflags.JSONTree)
 		print = func(act *action) {
-			m, existing := tree[act.pkg.ID]
+			var diags []analysis.Diagnostic
-			if !existing {
+			if act.isroot {
-				m = make(map[string]interface{})
+				diags = act.diagnostics
-				// Insert m into tree later iff non-empty.
-			}
-			if act.err != nil {
-				type jsonError struct {
-					Err string `json:"error"`
-				}
-				m[act.a.Name] = jsonError{act.err.Error()}
-			} else if act.isroot {
-				type jsonDiagnostic struct {
-					Category string `json:"category,omitempty"`
-					Posn     string `json:"posn"`
-					Message  string `json:"message"`
-				}
-				var diagnostics []jsonDiagnostic
-				for _, f := range act.diagnostics {
-					diagnostics = append(diagnostics, jsonDiagnostic{
-						Category: f.Category,
-						Posn:     act.pkg.Fset.Position(f.Pos).String(),
-						Message:  f.Message,
-					})
-				}
-				if diagnostics != nil {
-					m[act.a.Name] = diagnostics
-				}
-			}
-			if !existing && len(m) > 0 {
-				tree[act.pkg.ID] = m
 			}
+			tree.Add(act.pkg.Fset, act.pkg.ID, act.a.Name, diags, act.err)
 		}
 		visitAll(roots)
-
+		tree.Print()
-		data, err := json.MarshalIndent(tree, "", "\t")
-		if err != nil {
-			log.Panicf("internal error: JSON marshalling failed: %v", err)
-		}
-		os.Stdout.Write(data)
-		fmt.Println()
 	} else {
 		// plain text output
 
@@ -340,30 +304,18 @@ func printDiagnostics(roots []*action) (exitcode int) {
 				return
 			}
 			if act.isroot {
-				for _, f := range act.diagnostics {
+				for _, diag := range act.diagnostics {
 					// We don't display a.Name/f.Category
 					// as most users don't care.
 
-					posn := act.pkg.Fset.Position(f.Pos)
+					posn := act.pkg.Fset.Position(diag.Pos)
-
+					k := key{posn, act.a, diag.Message}
-					k := key{posn, act.a, f.Message}
 					if seen[k] {
 						continue // duplicate
 					}
 					seen[k] = true
 
-					fmt.Fprintf(os.Stderr, "%s: %s\n", posn, f.Message)
+					analysisflags.PrintPlain(act.pkg.Fset, diag)
-
-					// -c=0: show offending line of code in context.
-					if Context >= 0 {
-						data, _ := ioutil.ReadFile(posn.Filename)
-						lines := strings.Split(string(data), "\n")
-						for i := posn.Line - Context; i <= posn.Line+Context; i++ {
-							if 1 <= i && i <= len(lines) {
-								fmt.Fprintf(os.Stderr, "%d\t%s\n", i, lines[i-1])
-							}
-						}
-					}
 				}
 			}
 		}

go/analysis/unitchecker/unitchecker.go

@@ -25,7 +25,6 @@ package unitchecker
 //   so we will not get to analyze it. Yet we must in order
 //   to create base facts for, say, the fmt package for the
 //   printf checker.
-// - support JSON output, factored with multichecker.
 
 import (
 	"encoding/gob"
@@ -57,6 +56,7 @@ import (
 // It is provided to the tool in a JSON-encoded file
 // whose name ends with ".cfg".
 type Config struct {
+	ID                        string // e.g. "fmt [fmt.test]"
 	Compiler                  string
 	Dir                       string
 	ImportPath                string
@@ -127,16 +127,37 @@ func Run(configFile string, analyzers []*analysis.Analyzer) {
 	}
 
 	fset := token.NewFileSet()
-	diags, err := run(fset, cfg, analyzers)
+	results, err := run(fset, cfg, analyzers)
 	if err != nil {
 		log.Fatal(err)
 	}
 
-	if !cfg.VetxOnly && len(diags) > 0 {
+	// In VetxOnly mode, the analysis is run only for facts.
-		for _, diag := range diags {
+	if !cfg.VetxOnly {
-			fmt.Fprintf(os.Stderr, "%s: %s\n", fset.Position(diag.Pos), diag.Message)
+		if analysisflags.JSON {
+			// JSON output
+			tree := make(analysisflags.JSONTree)
+			for _, res := range results {
+				tree.Add(fset, cfg.ID, res.a.Name, res.diagnostics, res.err)
+			}
+			tree.Print()
+		} else {
+			// plain text
+			exit := 0
+			for _, res := range results {
+				if res.err != nil {
+					log.Println(res.err)
+					exit = 1
+				}
+			}
+			for _, res := range results {
+				for _, diag := range res.diagnostics {
+					analysisflags.PrintPlain(fset, diag)
+					exit = 1
+				}
+			}
+			os.Exit(exit)
 		}
-		os.Exit(1)
 	}
 
 	os.Exit(0)
@@ -160,7 +181,7 @@ func readConfig(filename string) (*Config, error) {
 	return cfg, nil
 }
 
-func run(fset *token.FileSet, cfg *Config, analyzers []*analysis.Analyzer) ([]analysis.Diagnostic, error) {
+func run(fset *token.FileSet, cfg *Config, analyzers []*analysis.Analyzer) ([]result, error) {
 	// Load, parse, typecheck.
 	var files []*ast.File
 	for _, name := range cfg.GoFiles {
@@ -333,14 +354,13 @@ func run(fset *token.FileSet, cfg *Config, analyzers []*analysis.Analyzer) ([]an
 
 	execAll(analyzers)
 
-	// Return diagnostics from root analyzers.
+	// Return diagnostics and errors from root analyzers.
-	var diags []analysis.Diagnostic
+	results := make([]result, len(analyzers))
-	for _, a := range analyzers {
+	for i, a := range analyzers {
 		act := actions[a]
-		if act.err != nil {
+		results[i].a = a
-			return nil, act.err // some analysis failed
+		results[i].err = act.err
-		}
+		results[i].diagnostics = act.diagnostics
-		diags = append(diags, act.diagnostics...)
 	}
 
 	data := facts.Encode()
@@ -348,7 +368,13 @@ func run(fset *token.FileSet, cfg *Config, analyzers []*analysis.Analyzer) ([]an
 		return nil, fmt.Errorf("failed to write analysis facts: %v", err)
 	}
 
-	return diags, nil
+	return results, nil
+}
+
+type result struct {
+	a           *analysis.Analyzer
+	diagnostics []analysis.Diagnostic
+	err         error
 }
 
 type importerFunc func(path string) (*types.Package, error)

go/analysis/unitchecker/unitchecker_test.go

@@ -59,15 +59,30 @@ testdata/src/a/a.go:4:11: call of MyFunc123(...)
 	const wantB = `# b
 testdata/src/b/b.go:6:13: call of MyFunc123(...)
 testdata/src/b/b.go:7:11: call of MyFunc123(...)
+`
+	const wantAJSON = `# a
+{
+	"a": {
+		"findcall": [
+			{
+				"posn": "$GOPATH/src/a/a.go:4:11",
+				"message": "call of MyFunc123(...)"
+			}
+		]
+	}
+}
 `
 
 	for _, test := range []struct {
 		args     string
-		want string
+		wantOut  string
+		wantExit int
 	}{
-		{args: "a", want: wantA},
+		{args: "a", wantOut: wantA, wantExit: 2},
-		{args: "b", want: wantB},
+		{args: "b", wantOut: wantB, wantExit: 2},
-		{args: "a b", want: wantA + wantB},
+		{args: "a b", wantOut: wantA + wantB, wantExit: 2},
+		{args: "-json a", wantOut: wantAJSON, wantExit: 0},
+		{args: "-c=0 a", wantOut: wantA + "4		MyFunc123()\n", wantExit: 2},
 	} {
 		cmd := exec.Command("go", "vet", "-vettool="+os.Args[0], "-findcall.name=MyFunc123")
 		cmd.Args = append(cmd.Args, strings.Fields(test.args)...)
@@ -77,16 +92,17 @@ testdata/src/b/b.go:7:11: call of MyFunc123(...)
 		)
 
 		out, err := cmd.CombinedOutput()
-		exitcode := -1
+		exitcode := 0
 		if exitErr, ok := err.(*exec.ExitError); ok {
 			exitcode = exitErr.ExitCode()
 		}
-		if exitcode != 2 {
+		if exitcode != test.wantExit {
-			t.Errorf("%s: got exit code %d, want 2", test.args, exitcode)
+			t.Errorf("%s: got exit code %d, want %d", test.args, exitcode, test.wantExit)
 		}
+		got := strings.Replace(string(out), testdata, "$GOPATH", -1)
 
-		if got := string(out); got != test.want {
+		if got != test.wantOut {
-			t.Errorf("%s: got <<%s>>, want <<%s>>", test.args, got, test.want)
+			t.Errorf("%s: got <<%s>>, want <<%s>>", test.args, got, test.wantOut)
 		}
 	}
 }