]> Cypherpunks repositories - gostls13.git/commitdiff
cmd/go: pass package config to vet during "go vet"
authorRuss Cox <rsc@golang.org>
Tue, 31 Oct 2017 16:59:47 +0000 (12:59 -0400)
committerRuss Cox <rsc@golang.org>
Wed, 1 Nov 2017 13:47:48 +0000 (13:47 +0000)
After this CL, "go vet" can be guaranteed to have complete type information
about the packages being checked, even if cgo or swig is in use,
which will in turn make it reasonable for vet checks to insist on type
information. It also fixes vet's understanding of unusual import paths
like relative paths and vendored packages.

For now "go tool vet" will continue to cope without type information,
but the eventual plan is for "go tool vet" to query the go command for
what it needs, and also to be able to query alternate build systems
like bazel. But that's future work.

Fixes #4889.
Fixes #12556 (if not already fixed).
Fixes #15182.
Fixes #16086.
Fixes #17571.

Change-Id: I932626ee7da649b302cd269b82eb6fe5d7b9f0f2
Reviewed-on: https://go-review.googlesource.com/74750
Reviewed-by: Ian Lance Taylor <iant@golang.org>
src/cmd/dist/deps.go
src/cmd/go/go_test.go
src/cmd/go/internal/load/pkg.go
src/cmd/go/internal/vet/vet.go
src/cmd/go/internal/work/action.go
src/cmd/go/internal/work/exec.go
src/cmd/vet/all/whitelist/all.txt

index 5bdb45dc4e1d88bdf020b1d4f3409392c6bfd3ef..44681f99952bdfe1f0d47ff900f526477b7f5b22 100644 (file)
@@ -311,15 +311,12 @@ var builddeps = map[string][]string{
 
        "cmd/go/internal/vet": {
                "cmd/go/internal/base",    // cmd/go/internal/vet
-               "cmd/go/internal/cfg",     // cmd/go/internal/vet
                "cmd/go/internal/cmdflag", // cmd/go/internal/vet
                "cmd/go/internal/load",    // cmd/go/internal/vet
-               "cmd/go/internal/str",     // cmd/go/internal/vet
                "cmd/go/internal/work",    // cmd/go/internal/vet
                "flag",                    // cmd/go/internal/vet
                "fmt",                     // cmd/go/internal/vet
                "os",                      // cmd/go/internal/vet
-               "path/filepath",           // cmd/go/internal/vet
                "strings",                 // cmd/go/internal/vet
        },
 
index 56a3c9c02b62d608b3caa938b281d7916121ee6d..a4f6452de5af78406304bb5542a0ea8d76f3719d 100644 (file)
@@ -3763,9 +3763,11 @@ func TestBinaryOnlyPackages(t *testing.T) {
        tg.grepStdout("hello from p1", "did not see message from p1")
 
        tg.tempFile("src/p4/p4.go", `package main`)
+       // The odd string split below avoids vet complaining about
+       // a // +build line appearing too late in this source file.
        tg.tempFile("src/p4/p4not.go", `//go:binary-only-package
 
-               // +build asdf
+               /`+`/ +build asdf
 
                package main
        `)
index 0f76975f56abd7c3467d7ac1793edabc5a50902a..a2c3d8e89366e917512c44093d3b4849c0ec0cc3 100644 (file)
@@ -95,6 +95,7 @@ type PackageInternal struct {
        // Unexported fields are not part of the public API.
        Build        *build.Package
        Imports      []*Package           // this package's direct imports
+       RawImports   []string             // this package's original imports as they appear in the text of the program
        ForceLibrary bool                 // this package is a library (even if named "main")
        Cmdline      bool                 // defined by files listed on command line
        Local        bool                 // imported via local path (./ or ../)
@@ -208,6 +209,7 @@ func (p *Package) copyBuild(pp *build.Package) {
        // We modify p.Imports in place, so make copy now.
        p.Imports = make([]string, len(pp.Imports))
        copy(p.Imports, pp.Imports)
+       p.Internal.RawImports = pp.Imports
        p.TestGoFiles = pp.TestGoFiles
        p.TestImports = pp.TestImports
        p.XTestGoFiles = pp.XTestGoFiles
index f22dd29286e5f484678bc471f6925a494028a7f9..ff129a62f04cbb33edf6d9cccc96c06c8c42c185 100644 (file)
@@ -6,12 +6,9 @@
 package vet
 
 import (
-       "path/filepath"
-
        "cmd/go/internal/base"
-       "cmd/go/internal/cfg"
        "cmd/go/internal/load"
-       "cmd/go/internal/str"
+       "cmd/go/internal/work"
 )
 
 var CmdVet = &base.Command{
@@ -37,22 +34,23 @@ See also: go fmt, go fix.
 }
 
 func runVet(cmd *base.Command, args []string) {
-       vetFlags, packages := vetFlags(args)
-       for _, p := range load.Packages(packages) {
-               // Vet expects to be given a set of files all from the same package.
-               // Run once for package p and once for package p_test.
-               if len(p.GoFiles)+len(p.CgoFiles)+len(p.TestGoFiles) > 0 {
-                       runVetFiles(p, vetFlags, str.StringList(p.GoFiles, p.CgoFiles, p.TestGoFiles, p.SFiles))
-               }
-               if len(p.XTestGoFiles) > 0 {
-                       runVetFiles(p, vetFlags, str.StringList(p.XTestGoFiles))
-               }
+       vetFlags, pkgArgs := vetFlags(args)
+
+       work.InstrumentInit()
+       work.BuildModeInit()
+       work.VetFlags = vetFlags
+
+       pkgs := load.PackagesForBuild(pkgArgs)
+       if len(pkgs) == 0 {
+               base.Fatalf("no packages to vet")
        }
-}
 
-func runVetFiles(p *load.Package, flags, files []string) {
-       for i := range files {
-               files[i] = filepath.Join(p.Dir, files[i])
+       var b work.Builder
+       b.Init()
+
+       root := &work.Action{Mode: "go vet"}
+       for _, p := range pkgs {
+               root.Deps = append(root.Deps, b.VetAction(work.ModeBuild, work.ModeBuild, p))
        }
-       base.Run(cfg.BuildToolexec, base.Tool("vet"), flags, base.RelPaths(files))
+       b.Do(root)
 }
index c01e266e97e03fda95bffb51558698cec1abcacc..413e950d6e71f488558fcb316050ab96410113ab 100644 (file)
@@ -74,6 +74,9 @@ type Action struct {
        built   string // the actual created package or executable
        buildID string // build ID of action output
 
+       needVet bool       // Mode=="build": need to fill in vet config
+       vetCfg  *vetConfig // vet config
+
        // Execution state.
        pending  int  // number of deps yet to complete
        priority int  // relative execution priority
@@ -349,6 +352,51 @@ func (b *Builder) CompileAction(mode, depMode BuildMode, p *load.Package) *Actio
        return a
 }
 
+// VetAction returns the action for running go vet on package p.
+// It depends on the action for compiling p.
+// If the caller may be causing p to be installed, it is up to the caller
+// to make sure that the install depends on (runs after) vet.
+func (b *Builder) VetAction(mode, depMode BuildMode, p *load.Package) *Action {
+       // Construct vet action.
+       a := b.cacheAction("vet", p, func() *Action {
+               a1 := b.CompileAction(mode, depMode, p)
+
+               // vet expects to be able to import "fmt".
+               var stk load.ImportStack
+               stk.Push("vet")
+               p1 := load.LoadPackage("fmt", &stk)
+               stk.Pop()
+               aFmt := b.CompileAction(ModeBuild, depMode, p1)
+
+               a := &Action{
+                       Mode:    "vet",
+                       Package: p,
+                       Deps:    []*Action{a1, aFmt},
+                       Objdir:  a1.Objdir,
+               }
+               if a1.Func == nil {
+                       // Built-in packages like unsafe.
+                       return a
+               }
+               a1.needVet = true
+               a.Func = (*Builder).vet
+
+               // If there might be an install action, make it depend on vet,
+               // so that the temporary files generated by the build step
+               // are not deleted before vet can use them.
+               // If nothing was going to install p, calling b.CompileAction with
+               // ModeInstall here creates the action, but nothing links it into the
+               // graph, so it will still not be installed.
+               install := b.CompileAction(ModeInstall, depMode, p)
+               if install != a1 {
+                       install.Deps = append(install.Deps, a)
+               }
+
+               return a
+       })
+       return a
+}
+
 // LinkAction returns the action for linking p into an executable
 // and possibly installing the result (according to mode).
 // depMode is the action (build or install) to use when compiling dependencies.
index 3ca26881d0bc4ec8500da60748292778b022b6aa..680a756bb6d59f4ebff5a8fc6ead79dca517df33 100644 (file)
@@ -8,6 +8,7 @@ package work
 
 import (
        "bytes"
+       "encoding/json"
        "errors"
        "fmt"
        "io"
@@ -254,9 +255,13 @@ func (b *Builder) buildActionID(a *Action) cache.ActionID {
 // Note that any new influence on this logic must be reported in b.buildActionID above as well.
 func (b *Builder) build(a *Action) (err error) {
        p := a.Package
+       cached := false
        if !p.BinaryOnly {
                if b.useCache(a, p, b.buildActionID(a), p.Target) {
-                       return nil
+                       if !a.needVet {
+                               return nil
+                       }
+                       cached = true
                }
        }
 
@@ -417,6 +422,34 @@ func (b *Builder) build(a *Action) (err error) {
                }
        }
 
+       // Prepare Go vet config if needed.
+       var vcfg *vetConfig
+       if a.needVet {
+               // Pass list of absolute paths to vet,
+               // so that vet's error messages will use absolute paths,
+               // so that we can reformat them relative to the directory
+               // in which the go command is invoked.
+               absfiles := make([]string, len(gofiles))
+               for i, f := range gofiles {
+                       if !filepath.IsAbs(f) {
+                               f = filepath.Join(a.Package.Dir, f)
+                       }
+                       absfiles[i] = f
+               }
+               vcfg = &vetConfig{
+                       Compiler:    cfg.BuildToolchainName,
+                       Dir:         a.Package.Dir,
+                       GoFiles:     absfiles,
+                       ImportMap:   make(map[string]string),
+                       PackageFile: make(map[string]string),
+               }
+               a.vetCfg = vcfg
+               for i, raw := range a.Package.Internal.RawImports {
+                       final := a.Package.Imports[i]
+                       vcfg.ImportMap[raw] = final
+               }
+       }
+
        // Prepare Go import config.
        var icfg bytes.Buffer
        for _, a1 := range a.Deps {
@@ -434,13 +467,42 @@ func (b *Builder) build(a *Action) (err error) {
                        continue
                }
                fmt.Fprintf(&icfg, "importmap %s=%s\n", path[i:], path)
+               if vcfg != nil {
+                       vcfg.ImportMap[path[i:]] = path
+               }
        }
+
+       // Compute the list of mapped imports in the vet config
+       // so that we can add any missing mappings below.
+       var vcfgMapped map[string]bool
+       if vcfg != nil {
+               vcfgMapped = make(map[string]bool)
+               for _, p := range vcfg.ImportMap {
+                       vcfgMapped[p] = true
+               }
+       }
+
        for _, a1 := range a.Deps {
                p1 := a1.Package
                if p1 == nil || p1.ImportPath == "" || a1.built == "" {
                        continue
                }
                fmt.Fprintf(&icfg, "packagefile %s=%s\n", p1.ImportPath, a1.built)
+               if vcfg != nil {
+                       // Add import mapping if needed
+                       // (for imports like "runtime/cgo" that appear only in generated code).
+                       if !vcfgMapped[p1.ImportPath] {
+                               vcfg.ImportMap[p1.ImportPath] = p1.ImportPath
+                       }
+                       vcfg.PackageFile[p1.ImportPath] = a1.built
+               }
+       }
+
+       if cached {
+               // The cached package file is OK, so we don't need to run the compile.
+               // We've only going through the motions to prepare the vet configuration,
+               // which is now complete.
+               return nil
        }
 
        // Compile Go.
@@ -532,6 +594,50 @@ func (b *Builder) build(a *Action) (err error) {
        return nil
 }
 
+type vetConfig struct {
+       Compiler    string
+       Dir         string
+       GoFiles     []string
+       ImportMap   map[string]string
+       PackageFile map[string]string
+}
+
+// VetFlags are the flags to pass to vet.
+// The caller is expected to set them before executing any vet actions.
+var VetFlags []string
+
+func (b *Builder) vet(a *Action) error {
+       // a.Deps[0] is the build of the package being vetted.
+       // a.Deps[1] is the build of the "fmt" package.
+
+       vcfg := a.Deps[0].vetCfg
+       if vcfg == nil {
+               // Vet config should only be missing if the build failed.
+               if !a.Deps[0].Failed {
+                       return fmt.Errorf("vet config not found")
+               }
+               return nil
+       }
+
+       if vcfg.ImportMap["fmt"] == "" {
+               a1 := a.Deps[1]
+               vcfg.ImportMap["fmt"] = "fmt"
+               vcfg.PackageFile["fmt"] = a1.built
+       }
+
+       js, err := json.MarshalIndent(vcfg, "", "\t")
+       if err != nil {
+               return fmt.Errorf("internal error marshaling vet config: %v", err)
+       }
+       js = append(js, '\n')
+       if err := b.writeFile(a.Objdir+"vet.cfg", js); err != nil {
+               return err
+       }
+
+       p := a.Package
+       return b.run(p.Dir, p.ImportPath, nil, cfg.BuildToolexec, base.Tool("vet"), VetFlags, a.Objdir+"vet.cfg")
+}
+
 // linkActionID computes the action ID for a link action.
 func (b *Builder) linkActionID(a *Action) cache.ActionID {
        h := cache.NewHash("link")
index 98415ef056832bc20606ea867571b974cd932ae3..6792d263a5e76c680e342481bf71bd39ec520561 100644 (file)
@@ -1,9 +1,5 @@
 // Non-platform-specific vet whitelist. See readme.txt for details.
 
-// Issue 17580 (remove when fixed)
-cmd/go/go_test.go: +build comment must appear before package clause and be followed by a blank line
-
-
 // Real problems that we can't fix.
 
 // This is a bad WriteTo signature. Errors are being ignored!