From 15a0f8a7f13d32e7ef63241aa7f25369b75ea373 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Fri, 19 Oct 2018 13:38:44 -0400 Subject: [PATCH] go/analysis/internal/unitchecker: a 'go vet'-compatible driver Also: - add cmd/vet-lite, a version of cmd/vet that doesn't depend on go/packages and must be run under "go vet". This will be vendored into $GOROOT/src/cmd/vet. - add an integration test for a unitchecker-based command under "go vet". Change-Id: Id613dac2812816c6d6372fa6d1536c8d4e4c2676 Reviewed-on: https://go-review.googlesource.com/c/143418 Reviewed-by: Ian Cottrell Reviewed-by: Michael Matloob --- go/analysis/cmd/vet-lite/main.go | 83 +++++ go/analysis/internal/checker/checker.go | 13 + go/analysis/internal/facts/facts.go | 11 +- .../internal/unitchecker/testdata/src/a/a.go | 7 + .../internal/unitchecker/testdata/src/b/b.go | 10 + .../internal/unitchecker/unitchecker.go | 306 ++++++++++++++++++ .../internal/unitchecker/unitchecker_test.go | 97 ++++++ go/analysis/multichecker/multichecker.go | 2 +- go/analysis/passes/pkgfact/pkgfact.go | 4 +- 9 files changed, 520 insertions(+), 13 deletions(-) create mode 100644 go/analysis/cmd/vet-lite/main.go create mode 100644 go/analysis/internal/unitchecker/testdata/src/a/a.go create mode 100644 go/analysis/internal/unitchecker/testdata/src/b/b.go create mode 100644 go/analysis/internal/unitchecker/unitchecker.go create mode 100644 go/analysis/internal/unitchecker/unitchecker_test.go diff --git a/go/analysis/cmd/vet-lite/main.go b/go/analysis/cmd/vet-lite/main.go new file mode 100644 index 00000000..ae66a7df --- /dev/null +++ b/go/analysis/cmd/vet-lite/main.go @@ -0,0 +1,83 @@ +// The vet-lite command is a driver for static checkers conforming to +// the golang.org/x/tools/go/analysis API. It must be run by go vet: +// +// $ GOVETTOOL=$(which vet-lite) go vet +// +// For a checker also capable of running standalone, use multichecker. +package main + +import ( + "flag" + "log" + "strings" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/internal/analysisflags" + "golang.org/x/tools/go/analysis/internal/unitchecker" + + "golang.org/x/tools/go/analysis/passes/asmdecl" + "golang.org/x/tools/go/analysis/passes/assign" + "golang.org/x/tools/go/analysis/passes/atomic" + "golang.org/x/tools/go/analysis/passes/bools" + "golang.org/x/tools/go/analysis/passes/buildtag" + "golang.org/x/tools/go/analysis/passes/cgocall" + "golang.org/x/tools/go/analysis/passes/composite" + "golang.org/x/tools/go/analysis/passes/copylock" + "golang.org/x/tools/go/analysis/passes/httpresponse" + "golang.org/x/tools/go/analysis/passes/loopclosure" + "golang.org/x/tools/go/analysis/passes/lostcancel" + "golang.org/x/tools/go/analysis/passes/nilfunc" + "golang.org/x/tools/go/analysis/passes/pkgfact" + "golang.org/x/tools/go/analysis/passes/printf" + "golang.org/x/tools/go/analysis/passes/shift" + "golang.org/x/tools/go/analysis/passes/stdmethods" + "golang.org/x/tools/go/analysis/passes/structtag" + "golang.org/x/tools/go/analysis/passes/tests" + "golang.org/x/tools/go/analysis/passes/unreachable" + "golang.org/x/tools/go/analysis/passes/unsafeptr" + "golang.org/x/tools/go/analysis/passes/unusedresult" +) + +var analyzers = []*analysis.Analyzer{ + // For now, just the traditional vet suite: + asmdecl.Analyzer, + assign.Analyzer, + atomic.Analyzer, + bools.Analyzer, + buildtag.Analyzer, + cgocall.Analyzer, + composite.Analyzer, + copylock.Analyzer, + httpresponse.Analyzer, + loopclosure.Analyzer, + lostcancel.Analyzer, + nilfunc.Analyzer, + pkgfact.Analyzer, + printf.Analyzer, + // shadow.Analyzer, // experimental; not enabled by default + shift.Analyzer, + stdmethods.Analyzer, + structtag.Analyzer, + tests.Analyzer, + unreachable.Analyzer, + unsafeptr.Analyzer, + unusedresult.Analyzer, +} + +func main() { + log.SetFlags(0) + log.SetPrefix("vet: ") + + if err := analysis.Validate(analyzers); err != nil { + log.Fatal(err) + } + + analyzers = analysisflags.Parse(analyzers, true) + + args := flag.Args() + if len(args) != 1 || !strings.HasSuffix(args[0], ".cfg") { + log.Fatalf("invalid command: want .cfg file (this reduced version of vet is intended to be run only by the 'go vet' command)") + } + + unitchecker.Main(args[0], analyzers) +} diff --git a/go/analysis/internal/checker/checker.go b/go/analysis/internal/checker/checker.go index a7c78269..e0c10298 100644 --- a/go/analysis/internal/checker/checker.go +++ b/go/analysis/internal/checker/checker.go @@ -25,6 +25,7 @@ import ( "time" "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/internal/unitchecker" "golang.org/x/tools/go/packages" ) @@ -106,6 +107,18 @@ func Run(args []string, analyzers []*analysis.Analyzer) error { }() } + // The undocumented protocol used by 'go vet' + // is that a vet-like tool must support: + // + // -flags describe flags in JSON + // -V=full describe executable for build caching + // foo.cfg perform separate modular analyze on the single + // unit described by a JSON config file foo.cfg. + if len(args) == 1 && strings.HasSuffix(args[0], ".cfg") { + unitchecker.Main(args[0], analyzers) + panic("unreachable") + } + // Load the packages. if dbg('v') { log.SetPrefix("") diff --git a/go/analysis/internal/facts/facts.go b/go/analysis/internal/facts/facts.go index c1edc903..468f1489 100644 --- a/go/analysis/internal/facts/facts.go +++ b/go/analysis/internal/facts/facts.go @@ -217,7 +217,7 @@ func (s *Set) Encode() []byte { s.mu.Lock() for k, fact := range s.m { if debug { - log.Printf("%#v => %s\n", k, fact) + log.Printf("%v => %s\n", k, fact) } var object objectpath.Path if k.obj != nil { @@ -259,15 +259,6 @@ func (s *Set) Encode() []byte { if len(gobFacts) > 0 { if err := gob.NewEncoder(&buf).Encode(gobFacts); err != nil { // Fact encoding should never fail. Identify the culprit. - // - // TODO(adonovan): what's the right thing to do here? - // The error is clearly a bug, so log.Fatal leads to early - // detection, but it could potentially bring down a big - // job because of an obscure dynamic bug in a fact. - // But perhaps that's fine: other bugs in Analyzers - // have the same potential to cause failures. - // Alternatively we could discard the bad facts with a - // log message, but who reads logs? for _, gf := range gobFacts { if err := gob.NewEncoder(ioutil.Discard).Encode(gf); err != nil { fact := gf.Fact diff --git a/go/analysis/internal/unitchecker/testdata/src/a/a.go b/go/analysis/internal/unitchecker/testdata/src/a/a.go new file mode 100644 index 00000000..9765e19a --- /dev/null +++ b/go/analysis/internal/unitchecker/testdata/src/a/a.go @@ -0,0 +1,7 @@ +package a + +func _() { + MyFunc123() +} + +func MyFunc123() {} diff --git a/go/analysis/internal/unitchecker/testdata/src/b/b.go b/go/analysis/internal/unitchecker/testdata/src/b/b.go new file mode 100644 index 00000000..a7b61254 --- /dev/null +++ b/go/analysis/internal/unitchecker/testdata/src/b/b.go @@ -0,0 +1,10 @@ +package b + +import "a" + +func _() { + a.MyFunc123() + MyFunc123() +} + +func MyFunc123() {} diff --git a/go/analysis/internal/unitchecker/unitchecker.go b/go/analysis/internal/unitchecker/unitchecker.go new file mode 100644 index 00000000..b67c943b --- /dev/null +++ b/go/analysis/internal/unitchecker/unitchecker.go @@ -0,0 +1,306 @@ +// Copyright 2018 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// The unitchecker package defines the main function for an analysis +// driver that analyzes a single compilation unit during a build. +// It is invoked by a build system such as "go vet": +// +// $ GOVETTOOL=$(which vet) go vet +// +// It supports the following command-line protocol: +// +// -V=full describe executable (to the build tool) +// -flags describe flags (to the build tool) +// foo.cfg description of compilation unit (from the build tool) +// +// This package does not depend on go/packages. +// If you need a standalone tool, use multichecker, +// which supports this mode but can also load packages +// from source using go/packages. +package unitchecker + +// TODO(adonovan): +// - with gccgo, go build does not build standard library, +// 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" + "encoding/json" + "fmt" + "go/ast" + "go/build" + "go/importer" + "go/parser" + "go/token" + "go/types" + "io" + "io/ioutil" + "log" + "os" + "sort" + "strings" + "sync" + "time" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/internal/facts" +) + +// A Config describes a compilation unit to be analyzed. +// It is provided to the tool in a JSON-encoded file +// whose name ends with ".cfg". +type Config struct { + Compiler string + Dir string + ImportPath string + GoFiles []string + OtherFiles []string // TODO(adonovan): make go vet populate this (github.com/golang/go/issues/27665) + ImportMap map[string]string + PackageFile map[string]string + Standard map[string]bool + PackageVetx map[string]string + VetxOnly bool + VetxOutput string + SucceedOnTypecheckFailure bool +} + +// Main reads the *.cfg file, runs the analysis, +// and calls os.Exit with an appropriate error code. +func Main(configFile string, analyzers []*analysis.Analyzer) { + cfg, err := readConfig(configFile) + if err != nil { + log.Fatal(err) + } + + fset := token.NewFileSet() + diags, err := run(fset, cfg, analyzers) + if err != nil { + log.Fatal(err) + } + + if len(diags) > 0 { + for _, diag := range diags { + fmt.Fprintf(os.Stderr, "%s: %s\n", fset.Position(diag.Pos), diag.Message) + } + os.Exit(1) + } + + os.Exit(0) +} + +func readConfig(filename string) (*Config, error) { + data, err := ioutil.ReadFile(filename) + if err != nil { + return nil, err + } + cfg := new(Config) + if err := json.Unmarshal(data, cfg); err != nil { + return nil, fmt.Errorf("cannot decode JSON config file %s: %v", filename, err) + } + if len(cfg.GoFiles) == 0 { + // The go command disallows packages with no files. + // The only exception is unsafe, but the go command + // doesn't call vet on it. + return nil, fmt.Errorf("package has no files: %s", cfg.ImportPath) + } + return cfg, nil +} + +func run(fset *token.FileSet, cfg *Config, analyzers []*analysis.Analyzer) ([]analysis.Diagnostic, error) { + // Load, parse, typecheck. + var files []*ast.File + for _, name := range cfg.GoFiles { + f, err := parser.ParseFile(fset, name, nil, parser.ParseComments) + if err != nil { + if cfg.SucceedOnTypecheckFailure { + // Silently succeed; let the compiler + // report parse errors. + err = nil + } + return nil, err + } + files = append(files, f) + } + compilerImporter := importer.For(cfg.Compiler, func(path string) (io.ReadCloser, error) { + // path is a resolved package path, not an import path. + file, ok := cfg.PackageFile[path] + if !ok { + if cfg.Compiler == "gccgo" && cfg.Standard[path] { + return nil, nil // fall back to default gccgo lookup + } + return nil, fmt.Errorf("no package file for %q", path) + } + return os.Open(file) + }) + importer := importerFunc(func(importPath string) (*types.Package, error) { + path, ok := cfg.ImportMap[importPath] // resolve vendoring, etc + if !ok { + return nil, fmt.Errorf("can't resolve import %q", path) + } + return compilerImporter.Import(path) + }) + tc := &types.Config{ + Importer: importer, + Sizes: types.SizesFor("gc", build.Default.GOARCH), // assume gccgo ≡ gc? + } + info := &types.Info{ + Types: make(map[ast.Expr]types.TypeAndValue), + Defs: make(map[*ast.Ident]types.Object), + Uses: make(map[*ast.Ident]types.Object), + Implicits: make(map[ast.Node]types.Object), + Scopes: make(map[ast.Node]*types.Scope), + Selections: make(map[*ast.SelectorExpr]*types.Selection), + } + pkg, err := tc.Check(cfg.ImportPath, fset, files, info) + if err != nil { + if cfg.SucceedOnTypecheckFailure { + // Silently succeed; let the compiler + // report type errors. + err = nil + } + return nil, err + } + + // Register fact types with gob. + // In VetxOnly mode, analyzers are only for their facts, + // so we can skip any analysis that neither produces facts + // nor depends on any analysis that produces facts. + // Also build a map to hold working state and result. + type action struct { + once sync.Once + result interface{} + err error + usesFacts bool // (transitively uses) + diagnostics []analysis.Diagnostic + } + actions := make(map[*analysis.Analyzer]*action) + var registerFacts func(a *analysis.Analyzer) bool + registerFacts = func(a *analysis.Analyzer) bool { + act, ok := actions[a] + if !ok { + act = new(action) + var usesFacts bool + for _, f := range a.FactTypes { + usesFacts = true + gob.Register(f) + } + for _, req := range a.Requires { + if registerFacts(req) { + usesFacts = true + } + } + act.usesFacts = usesFacts + actions[a] = act + } + return act.usesFacts + } + var filtered []*analysis.Analyzer + for _, a := range analyzers { + if registerFacts(a) || !cfg.VetxOnly { + filtered = append(filtered, a) + } + } + analyzers = filtered + + // Read facts from imported packages. + read := func(path string) ([]byte, error) { + if vetx, ok := cfg.PackageVetx[path]; ok { + return ioutil.ReadFile(vetx) + } + return nil, nil // no .vetx file, no facts + } + facts, err := facts.Decode(pkg, read) + if err != nil { + return nil, err + } + + // In parallel, execute the DAG of analyzers. + var exec func(a *analysis.Analyzer) *action + var execAll func(analyzers []*analysis.Analyzer) + exec = func(a *analysis.Analyzer) *action { + act := actions[a] + act.once.Do(func() { + execAll(a.Requires) // prefetch dependencies in parallel + + // The inputs to this analysis are the + // results of its prerequisites. + inputs := make(map[*analysis.Analyzer]interface{}) + var failed []string + for _, req := range a.Requires { + reqact := exec(req) + if reqact.err != nil { + failed = append(failed, req.String()) + continue + } + inputs[req] = reqact.result + } + + // Report an error if any dependency failed. + if failed != nil { + sort.Strings(failed) + act.err = fmt.Errorf("failed prerequisites: %s", strings.Join(failed, ", ")) + return + } + + pass := &analysis.Pass{ + Analyzer: a, + Fset: fset, + Files: files, + OtherFiles: cfg.OtherFiles, + Pkg: pkg, + TypesInfo: info, + ResultOf: inputs, + Report: func(d analysis.Diagnostic) { act.diagnostics = append(act.diagnostics, d) }, + ImportObjectFact: facts.ImportObjectFact, + ExportObjectFact: facts.ExportObjectFact, + ImportPackageFact: facts.ImportPackageFact, + ExportPackageFact: facts.ExportPackageFact, + } + + t0 := time.Now() + act.result, act.err = a.Run(pass) + if false { + log.Printf("analysis %s = %s", pass, time.Since(t0)) + } + }) + return act + } + execAll = func(analyzers []*analysis.Analyzer) { + var wg sync.WaitGroup + for _, a := range analyzers { + wg.Add(1) + go func(a *analysis.Analyzer) { + _ = exec(a) + wg.Done() + }(a) + } + wg.Wait() + } + + execAll(analyzers) + + // Return diagnostics from root analyzers. + var diags []analysis.Diagnostic + for _, a := range analyzers { + act := actions[a] + if act.err != nil { + return nil, act.err // some analysis failed + } + diags = append(diags, act.diagnostics...) + } + + data := facts.Encode() + if err := ioutil.WriteFile(cfg.VetxOutput, data, 0666); err != nil { + return nil, fmt.Errorf("failed to write analysis facts: %v", err) + } + + return diags, nil +} + +type importerFunc func(path string) (*types.Package, error) + +func (f importerFunc) Import(path string) (*types.Package, error) { return f(path) } diff --git a/go/analysis/internal/unitchecker/unitchecker_test.go b/go/analysis/internal/unitchecker/unitchecker_test.go new file mode 100644 index 00000000..872275f1 --- /dev/null +++ b/go/analysis/internal/unitchecker/unitchecker_test.go @@ -0,0 +1,97 @@ +// Copyright 2018 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package unitchecker_test + +import ( + "flag" + "log" + "os" + "os/exec" + "runtime" + "strings" + "testing" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/analysistest" + "golang.org/x/tools/go/analysis/internal/analysisflags" + "golang.org/x/tools/go/analysis/internal/unitchecker" + "golang.org/x/tools/go/analysis/passes/findcall" + "golang.org/x/tools/go/analysis/passes/printf" +) + +func TestMain(m *testing.M) { + if os.Getenv("UNITCHECKER_CHILD") == "1" { + // child process + main() + panic("unreachable") + } + + // test + flag.Parse() + os.Exit(m.Run()) +} + +func main() { + findcall.Analyzer.Flags.Set("name", "MyFunc123") + + var analyzers = []*analysis.Analyzer{ + findcall.Analyzer, + printf.Analyzer, + } + + if err := analysis.Validate(analyzers); err != nil { + log.Fatal(err) + } + analyzers = analysisflags.Parse(analyzers, true) + + args := flag.Args() + if len(args) != 1 || !strings.HasSuffix(args[0], ".cfg") { + log.Fatalf("invalid command: want .cfg file") + } + + unitchecker.Main(args[0], analyzers) +} + +// This is a very basic integration test of modular +// analysis with facts using unitchecker under "go vet". +// It fork/execs the main function above. +func TestIntegration(t *testing.T) { + if runtime.GOOS != "linux" { + t.Skipf("skipping fork/exec test on this platform") + } + + testdata := analysistest.TestData() + + cmd := exec.Command("go", "vet", "b") + cmd.Env = append(os.Environ(), + "GOVETTOOL="+os.Args[0], + "UNITCHECKER_CHILD=1", + "GOPATH="+testdata, + ) + + out, err := cmd.CombinedOutput() + exitcode := -1 + if exitErr, ok := err.(*exec.ExitError); ok { + exitcode = exitErr.ExitCode() + } + if exitcode != 2 { + t.Errorf("got exit code %d, want 2", exitcode) + } + + want := ` +# a +testdata/src/a/a.go:4:11: call of MyFunc123(...) +# b +testdata/src/b/b.go:6:13: call of MyFunc123(...) +testdata/src/b/b.go:7:11: call of MyFunc123(...) +`[1:] + if got := string(out); got != want { + t.Errorf("got <<%s>>, want <<%s>>", got, want) + } + + if t.Failed() { + t.Logf("err=%v stderr=<<%s>", err, cmd.Stderr) + } +} diff --git a/go/analysis/multichecker/multichecker.go b/go/analysis/multichecker/multichecker.go index 27406fa0..9038eac1 100644 --- a/go/analysis/multichecker/multichecker.go +++ b/go/analysis/multichecker/multichecker.go @@ -41,7 +41,7 @@ Usage: PROGNAME [-flag] [package] func Main(analyzers ...*analysis.Analyzer) { progname := filepath.Base(os.Args[0]) log.SetFlags(0) - log.SetPrefix(progname + ": ") + log.SetPrefix(filepath.Base(os.Args[0]) + ": ") // e.g. "vet: " if err := analysis.Validate(analyzers); err != nil { log.Fatal(err) diff --git a/go/analysis/passes/pkgfact/pkgfact.go b/go/analysis/passes/pkgfact/pkgfact.go index eca0f256..e0530867 100644 --- a/go/analysis/passes/pkgfact/pkgfact.go +++ b/go/analysis/passes/pkgfact/pkgfact.go @@ -70,14 +70,14 @@ func run(pass *analysis.Pass) (interface{}, error) { } } - // At each "const _name = value", add a fact into env. + // At each "const _name_ = value", add a fact into env. doConst := func(spec *ast.ValueSpec) { if len(spec.Names) == len(spec.Values) { for i := range spec.Names { name := spec.Names[i].Name if strings.HasPrefix(name, "_") && strings.HasSuffix(name, "_") { - if key := strings.Trim(name[1:], "_"); key != "" { + if key := strings.Trim(name, "_"); key != "" { value := pass.TypesInfo.Types[spec.Values[i]].Value.String() result[key] = value }