From 04cadf3f9496811db29d115b90088e871567201e Mon Sep 17 00:00:00 2001 From: Samuel Tan Date: Wed, 22 Aug 2018 10:17:46 -0700 Subject: [PATCH] Allow the checker to run vet (#1673) Add a go_checker attribute that causes the generated checker binary to run 'go tool vet'. Just like in the official Go build toolchain, only a subset of vet checks are run. Any vet findings will fail compilation. To avoid breaking users, vet is disabled by default. While there: - Fix issues with the importer used to load analysis packages. - Silence type-checking errors generated by the checker binary. It will now fail silently if type-checking errors occur. - Refactor the checker binary and unit tests to improve readability. --- BUILD.bazel | 1 - go/checks.rst | 37 ++- go/private/actions/compile.bzl | 1 + go/private/rules/builders.bzl | 1 + go/private/rules/checker.bzl | 12 +- go/tools/builders/BUILD.bazel | 2 + go/tools/builders/checker_main.go | 252 +++++++++++++-------- go/tools/builders/compile.go | 22 +- go/tools/builders/generate_checker_main.go | 11 +- go/tools/builders/vet.go | 147 ++++++++++++ tests/core/checks/BUILD.bazel | 5 + tests/core/checks/common.bzl | 34 +++ tests/core/checks/custom/BUILD.bazel | 46 +--- tests/core/checks/custom/README.rst | 2 +- tests/core/checks/default/BUILD.bazel | 49 ---- tests/core/checks/default/README.rst | 15 -- tests/core/checks/default/lib.go | 8 - tests/core/checks/vet/BUILD.bazel | 72 ++++++ tests/core/checks/vet/README.rst | 21 ++ tests/core/checks/vet/has_errors.go | 20 ++ tests/core/checks/vet/no_errors.go | 5 + 21 files changed, 547 insertions(+), 216 deletions(-) create mode 100644 go/tools/builders/vet.go create mode 100644 tests/core/checks/BUILD.bazel create mode 100644 tests/core/checks/common.bzl delete mode 100644 tests/core/checks/default/BUILD.bazel delete mode 100644 tests/core/checks/default/README.rst delete mode 100644 tests/core/checks/default/lib.go create mode 100644 tests/core/checks/vet/BUILD.bazel create mode 100644 tests/core/checks/vet/README.rst create mode 100644 tests/core/checks/vet/has_errors.go create mode 100644 tests/core/checks/vet/no_errors.go diff --git a/BUILD.bazel b/BUILD.bazel index 738296537e..a42343d5f2 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -16,7 +16,6 @@ builders( visibility = ["//visibility:public"], ) -# TODO(samueltan): add checks. go_checker( name = "default_checker", visibility = ["//visibility:public"], diff --git a/go/checks.rst b/go/checks.rst index 7494141b21..273b85289e 100644 --- a/go/checks.rst +++ b/go/checks.rst @@ -8,6 +8,7 @@ Go build-time code analysis .. _GoLibrary: providers.rst#GoLibrary .. _GoSource: providers.rst#GoSource .. _GoArchive: providers.rst#GoArchive +.. _vet: https://golang.org/cmd/vet/ .. role:: param(kbd) .. role:: type(emphasis) @@ -19,11 +20,9 @@ Please do not rely on it for production use, but feel free to use it and file issues. rules_go allows you to define custom source-code-level checks that are executed -alongside the Go compiler. These checks print error messages by default, but can -be configured to also fail compilation. Checks can be used to catch bug and -anti-patterns early in the development process. - -**TODO**: make vet run by default. +alongside the Go compiler. These checks will print error messages and fail +compilation if they find issues in source code. Checks can be used to catch +bugs and coding anti-patterns early in the development process. .. contents:: :depth: 2 @@ -73,6 +72,9 @@ syntax trees (ASTs) and type information for that package. For example: return &analysis.Result{Findings: findings}, nil } +Any findings returned by the check will fail compilation. Do not emit findings +unless they are severe enough to warrant interrupting the compiler. + Each check must be written as a `go_tool_library`_ rule. This rule is identical to `go_library`_ but avoids a bootstrapping problem, which we will explain later. For example: @@ -213,6 +215,27 @@ This label referencing this configuration file must be provided as the visibility = ["//visibility:public"], ) +Running vet +~~~~~~~~~~~ + +You can choose to run the `vet`_ tool alongside the Go compiler and custom +checks by setting the ``vet`` attribute of your `go_checker`_ rule: + +.. code:: bzl + + go_checker( + name = "my_checker", + vet = True, + visibility = ["//visibility:public"], + ) + +`vet`_ will print error messages and fail compilation if it finds any issues in +the source code being built. Just like in the upstream Go build toolchain, only +a subset of `vet`_ checks which are 100% accurate will be run. + +In the above example, `vet`_ will run alone. It can also run alongside custom +checks given by the ``deps`` attribute. + API --- @@ -243,6 +266,10 @@ Attributes +----------------------------+-----------------------------+---------------------------------------+ | JSON configuration file that configures one or more of the checks in `deps`. | +----------------------------+-----------------------------+---------------------------------------+ +| :param:`vet` | :type:`bool` | :value:`False` | ++----------------------------+-----------------------------+---------------------------------------+ +| Whether to run the `vet` tool. | ++----------------------------+-----------------------------+---------------------------------------+ Example ^^^^^^^ diff --git a/go/private/actions/compile.bzl b/go/private/actions/compile.bzl index 6d519f2551..60d03cfb67 100644 --- a/go/private/actions/compile.bzl +++ b/go/private/actions/compile.bzl @@ -67,6 +67,7 @@ def emit_compile( builder_args.add_all(archives, before_each = "-importmap", map_each = _importmap) builder_args.add_all(archives, before_each = "-archivefile", map_each = _archivefile) builder_args.add_all(["-o", out_lib]) + builder_args.add_all(["-stdlib", go.toolchain.sdk.root_file.dirname+"/pkg/"+go.toolchain.sdk.goos+"_"+go.toolchain.sdk.goarch]) builder_args.add_all(["-package_list", go.package_list]) if testfilter: builder_args.add_all(["-testfilter", testfilter]) diff --git a/go/private/rules/builders.bzl b/go/private/rules/builders.bzl index e44836d803..e18527be8b 100644 --- a/go/private/rules/builders.bzl +++ b/go/private/rules/builders.bzl @@ -33,6 +33,7 @@ def _builders_impl(ctx): ctx.executable._pack, ctx.executable._link, ctx.executable._cgo, + ctx.executable._checker_generator, ctx.executable._test_generator, ctx.executable._cover, ]), diff --git a/go/private/rules/checker.bzl b/go/private/rules/checker.bzl index e30555bdb4..3cf10ab483 100644 --- a/go/private/rules/checker.bzl +++ b/go/private/rules/checker.bzl @@ -33,12 +33,14 @@ def _go_checker_impl(ctx): go = go_context(ctx) checker_main = go.declare_file(go, "checker_main.go") checker_args = ctx.actions.args() - checker_args.add(["-output", checker_main]) + checker_args.add("-output", checker_main) check_archives = [get_archive(dep) for dep in ctx.attr.deps] check_importpaths = [archive.data.importpath for archive in check_archives] - checker_args.add(check_importpaths, before_each = "-check_importpath") + checker_args.add_all(check_importpaths, before_each = "-check_importpath") + if ctx.attr.vet: + checker_args.add("-vet") if ctx.file.config: - checker_args.add(["-config", ctx.file.config]) + checker_args.add("-config", ctx.file.config.path) ctx.actions.run( outputs = [checker_main], mnemonic = "GoGenChecker", @@ -81,11 +83,13 @@ go_checker = go_rule( attrs = { "deps": attr.label_list( providers = [GoArchive], - # TODO(samueltan): make this attribute mandatory. ), "config": attr.label( allow_single_file = True, ), + "vet": attr.bool( + default = False, + ), "_analysis": attr.label( default = "@io_bazel_rules_go//go/tools/analysis:analysis", ), diff --git a/go/tools/builders/BUILD.bazel b/go/tools/builders/BUILD.bazel index c0b2fda9ea..502bc58dbd 100644 --- a/go/tools/builders/BUILD.bazel +++ b/go/tools/builders/BUILD.bazel @@ -37,6 +37,7 @@ go_tool_binary( "env.go", "filter.go", "flags.go", + "vet.go", ], visibility = ["//visibility:public"], ) @@ -71,6 +72,7 @@ go_source( srcs = [ "checker_main.go", "flags.go", + "vet.go", ], deps = [ "@io_bazel_rules_go//go/tools/analysis:analysis", diff --git a/go/tools/builders/checker_main.go b/go/tools/builders/checker_main.go index eebde1d442..020c76d87a 100644 --- a/go/tools/builders/checker_main.go +++ b/go/tools/builders/checker_main.go @@ -20,11 +20,11 @@ limitations under the License. package main import ( + "bytes" "errors" "flag" "fmt" "go/ast" - "go/build" "go/parser" "go/token" "go/types" @@ -44,39 +44,94 @@ import ( // do not return an error so as not to unnecessarily interrupt builds. func run(args []string) error { archiveFiles := multiFlag{} + importMapIn := multiFlag{} + srcs := multiFlag{} flags := flag.NewFlagSet("checker", flag.ContinueOnError) flags.Var(&archiveFiles, "archivefile", "Archive file of a direct dependency") + flags.Var(&importMapIn, "importmap", "Import maps of a direct dependency") + flags.Var(&srcs, "src", "A source file being compiled") + vetTool := flags.String("vet_tool", "", "The vet tool") + stdLib := flags.String("stdlib", "", "The directory containing stdlib packages") + packageList := flags.String("package_list", "", "The file containing the list of standard library packages") if err := flags.Parse(args); err != nil { log.Println(err) return nil } - goroot, ok := os.LookupEnv("GOROOT") - if !ok { - log.Fatalf("GOROOT not set") + // TODO(samueltan): derive this information from the importcfg file built by + // compile.go instead of using multiple flags. + packageFile, importMap, err := parseArchivesAndImportMap(archiveFiles, importMapIn) + if err != nil { + log.Printf("error parsing arguments: %v", err) + return nil + } + if enableVet { + vcfgFile, err := makeVetCfg(*packageList, *stdLib, packageFile, importMap, srcs) + if err != nil { + log.Printf("error creating vet config: %v", err) + return nil + } + defer os.Remove(vcfgFile) + findings, err := runVet(*vetTool, vcfgFile) + if err != nil { + return fmt.Errorf("error running vet:\n%v\n", err) + } else if findings != "" { + return fmt.Errorf("errors found by vet:\n%s\n", findings) + } + } + c := make(chan result) + fset := token.NewFileSet() + if err := runAnalyses(c, fset, packageFile, importMap, flags.Args(), *stdLib); err != nil { + log.Printf("error running analyses: %s\n", err) + return nil + } + if err := checkAnalysisResults(c, fset); err != nil { + return fmt.Errorf("errors found during build-time code analysis:\n%s\n", err) + } + return nil +} + +// parseArchivesAndImportMap parses the -archivefile and -importmap arguments +// created by the compile rule into the packageFiles and importMap maps used +// by the vet and checker importers. +func parseArchivesAndImportMap(archiveFiles, importMapIn []string) (packageFiles map[string]string, importMap map[string]string, err error) { + packageFiles, importMap = make(map[string]string), make(map[string]string) + for _, mapping := range importMapIn { + i := strings.Index(mapping, "=") + if i < 0 { + return nil, nil, fmt.Errorf("invalid importmap %v: no = separator", mapping) + } + source := mapping[:i] + actual := mapping[i+1:] + if source == "" || actual == "" { + continue + } + importMap[source] = actual } - importsToArchives := make(map[string]string) for _, a := range archiveFiles { kv := strings.Split(a, "=") if len(kv) != 2 { - continue // sanity check + return nil, nil, fmt.Errorf("invalid archivefile %v: no = separator", a) + } + path := kv[0] + if p, ok := importMap[path]; ok { + path = p } - importsToArchives[kv[0]] = kv[1] + packageFiles[path] = kv[1] } - fset := token.NewFileSet() - imp := &importer{ - fset: fset, - packages: make(map[string]*types.Package), - importsToArchives: importsToArchives, - stdlib: goroot, + return +} + +// runAnalyses runs all analyses, filters results, and writes findings to the +// given channel. +func runAnalyses(c chan result, fset *token.FileSet, packageFile, importMap map[string]string, srcFiles []string, stdLib string) error { + if len(analysis.Analyses()) == 0 { + return nil } - apkg, err := load(fset, imp, flags.Args()) + apkg, err := newAnalysisPackage(fset, packageFile, importMap, srcFiles, stdLib) if err != nil { - log.Printf("error loading package: %v\n", err) - return nil + return fmt.Errorf("error building analysis package: %s\n", err) } - - c := make(chan result) - // Perform analyses in parallel. + // Run all other analyses. for _, a := range analysis.Analyses() { go func(a *analysis.Analysis) { defer func() { @@ -88,7 +143,7 @@ func run(args []string) error { res, err := a.Run(apkg) switch err { case nil: - c <- result{name: a.Name, findings: res.Findings} + c <- result{analysis.Result{res.Findings}, a.Name, nil} case analysis.ErrSkipped: c <- result{name: a.Name, err: fmt.Errorf("skipped : %v", err)} default: @@ -96,26 +151,74 @@ func run(args []string) error { } }(a) } - // Collate analysis results. - var allFindings []*analysis.Finding + return nil +} + +func newAnalysisPackage(fset *token.FileSet, packageFile, importMap map[string]string, srcFiles []string, stdLib string) (*analysis.Package, error) { + imp := &importer{ + fset: fset, + importMap: importMap, + packageCache: make(map[string]*types.Package), + packageFile: packageFile, + stdLib: stdLib, + } + apkg, err := load(fset, imp, srcFiles) + if err != nil { + return nil, fmt.Errorf("error loading package: %v\n", err) + } + return apkg, nil +} + +// load parses and type checks the source code in each file in filenames. +func load(fset *token.FileSet, imp types.Importer, filenames []string) (*analysis.Package, error) { + if len(filenames) == 0 { + return nil, errors.New("no filenames") + } + var files []*ast.File + for _, file := range filenames { + f, err := parser.ParseFile(fset, file, nil, parser.ParseComments) + if err != nil { + return nil, err + } + files = append(files, f) + } + config := types.Config{Importer: imp} + info := &types.Info{ + Types: make(map[ast.Expr]types.TypeAndValue), + Uses: make(map[*ast.Ident]types.Object), + Defs: make(map[*ast.Ident]types.Object), + Implicits: make(map[ast.Node]types.Object), + } + pkg, err := config.Check(files[0].Name.Name, fset, files, info) + if err != nil { + return nil, errors.New("type-checking failed") + } + return &analysis.Package{Fset: fset, Files: files, Types: pkg, Info: info}, nil +} + +// checkAnalysisResults checks the analysis results written to the given channel +// and returns an error if the analysis finds errors that should fail +// compilation. +func checkAnalysisResults(c chan result, fset *token.FileSet) error { + var analysisFindings []*analysis.Finding for i := 0; i < len(analysis.Analyses()); i++ { result := <-c if result.err != nil { // Analysis failed or skipped. - log.Printf("analysis %q %v", result.name, result.err) + log.Printf("analysis %q: %v", result.name, result.err) continue } - if len(result.findings) == 0 { + if len(result.Findings) == 0 { continue } config, ok := configs[result.name] if !ok { // If the check is not explicitly configured, it applies to all files. - allFindings = append(allFindings, result.findings...) + analysisFindings = append(analysisFindings, result.Findings...) continue } // Discard findings based on the check configuration. - for _, finding := range result.findings { + for _, finding := range result.Findings { filename := fset.File(finding.Pos).Name() include := true if len(config.applyTo) > 0 { @@ -133,22 +236,24 @@ func run(args []string) error { } } if include { - allFindings = append(allFindings, finding) + analysisFindings = append(analysisFindings, finding) } } } - // Return an error that fails the build if we have any findings. - if len(allFindings) == 0 { + if len(analysisFindings) == 0 { return nil } - sort.Slice(allFindings, func(i, j int) bool { - return allFindings[i].Pos < allFindings[j].Pos + sort.Slice(analysisFindings, func(i, j int) bool { + return analysisFindings[i].Pos < analysisFindings[j].Pos }) - errMsg := "errors found during build-time code analysis:\n" - for _, f := range allFindings { - errMsg += fmt.Sprintf("%s: %s\n", fset.Position(f.Pos), f.Message) + var errMsg bytes.Buffer + for i, f := range analysisFindings { + if i > 0 { + errMsg.WriteByte('\n') + } + errMsg.WriteString(fmt.Sprintf("%s: %s\n", fset.Position(f.Pos), f.Message)) } - return errors.New(errMsg) + return errors.New(errMsg.String()) } type config struct { @@ -163,69 +268,38 @@ func main() { } } +// result is used to collate all the findings and errors returned +// by analyses run in parallel. type result struct { - name string - findings []*analysis.Finding - err error - failBuild bool + analysis.Result + name string + err error } -// load parses and type checks the source code in each file in filenames. -func load(fset *token.FileSet, imp types.Importer, filenames []string) (*analysis.Package, error) { - if len(filenames) == 0 { - return nil, errors.New("no filenames") - } - var files []*ast.File - for _, file := range filenames { - f, err := parser.ParseFile(fset, file, nil, parser.ParseComments) - if err != nil { - return nil, err - } - files = append(files, f) - } +type importer struct { + fset *token.FileSet + importMap map[string]string // map import path in source code to package path + packageCache map[string]*types.Package // cache of previously imported packages + packageFile map[string]string // map non-stdlib package path to .a file with export data + stdLib string // the directory containing stdlib packages +} - config := types.Config{ - Importer: imp, - Error: func(err error) { - e := err.(types.Error) - msg := fmt.Sprintf("%s", e.Msg) - posn := e.Fset.Position(e.Pos) - if posn.Filename != "" { - msg = fmt.Sprintf("%s: %s", posn, msg) - } - fmt.Fprintln(os.Stderr, msg) - }, +func (i *importer) Import(path string) (*types.Package, error) { + if imp, ok := i.importMap[path]; ok { + // Translate import path if necessary. + path = imp } - info := &types.Info{ - Types: make(map[ast.Expr]types.TypeAndValue), - Uses: make(map[*ast.Ident]types.Object), - Defs: make(map[*ast.Ident]types.Object), - Implicits: make(map[ast.Node]types.Object), + if path == "unsafe" { + return types.Unsafe, nil } - pkg, err := config.Check(files[0].Name.Name, fset, files, info) - if err != nil { - // Errors were already reported through config.Error. - return nil, nil + if pkg, ok := i.packageCache[path]; ok && pkg.Complete() { + return pkg, nil // cache hit } - return &analysis.Package{Fset: fset, Files: files, Types: pkg, Info: info}, nil -} -type importer struct { - fset *token.FileSet - packages map[string]*types.Package - // importsToArchives maps import paths to the path to the archive file representing the - // corresponding library. - importsToArchives map[string]string - // stdlib is the root directory containing standard library package archive files. - stdlib string -} - -func (i *importer) Import(path string) (*types.Package, error) { - archive, ok := i.importsToArchives[path] + archive, ok := i.packageFile[path] if !ok { // stdlib package. - ctxt := build.Default - archive = filepath.Join(i.stdlib, "pkg", ctxt.GOOS+"_"+ctxt.GOARCH, path+".a") + archive = filepath.Join(i.stdLib, path+".a") } // open file f, err := os.Open(archive) @@ -245,5 +319,5 @@ func (i *importer) Import(path string) (*types.Package, error) { return nil, err } - return gcexportdata.Read(r, i.fset, i.packages, path) + return gcexportdata.Read(r, i.fset, i.packageCache, path) } diff --git a/go/tools/builders/compile.go b/go/tools/builders/compile.go index 894744935c..2805f08566 100644 --- a/go/tools/builders/compile.go +++ b/go/tools/builders/compile.go @@ -36,13 +36,14 @@ func run(args []string) error { unfiltered := multiFlag{} deps := multiFlag{} archiveFiles := multiFlag{} - importmap := multiFlag{} + importMap := multiFlag{} goenv := envFlags(flags) flags.Var(&unfiltered, "src", "A source file to be filtered and compiled") flags.Var(&deps, "dep", "Import path of a direct dependency") - flags.Var(&importmap, "importmap", "Import maps of a direct dependency") + flags.Var(&importMap, "importmap", "Import maps of a direct dependency") flags.Var(&archiveFiles, "archivefile", "Archive file of a direct dependency") checker := flags.String("checker", "", "The checker binary") + stdLib := flags.String("stdlib", "", "The directory containing stdlib packages") output := flags.String("o", "", "The output object file to write") packageList := flags.String("package_list", "", "The file containing the list of standard library packages") testfilter := flags.String("testfilter", "off", "Controls test package filtering") @@ -95,8 +96,8 @@ func run(args []string) error { // Check that the filtered sources don't import anything outside of deps. strictdeps := deps - var importmapArgs []string - for _, mapping := range importmap { + var importMapArgs []string + for _, mapping := range importMap { i := strings.Index(mapping, "=") if i < 0 { return fmt.Errorf("Invalid importmap %v: no = separator", mapping) @@ -106,7 +107,7 @@ func run(args []string) error { if source == "" || actual == "" || source == actual { continue } - importmapArgs = append(importmapArgs, "-importmap", mapping) + importMapArgs = append(importMapArgs, "-importmap", mapping) strictdeps = append(strictdeps, source) } if err := checkDirectDeps(build.Default, files, strictdeps, *packageList); err != nil { @@ -115,7 +116,7 @@ func run(args []string) error { // Compile the filtered files. goargs := goenv.goTool("compile") - goargs = append(goargs, importmapArgs...) + goargs = append(goargs, importMapArgs...) goargs = append(goargs, "-pack", "-o", *output) goargs = append(goargs, toolArgs...) goargs = append(goargs, "--") @@ -140,6 +141,15 @@ func run(args []string) error { for _, a := range archiveFiles { checkerargs = append(checkerargs, "-archivefile", a) } + checkerargs = append(checkerargs, "-vet_tool", goenv.goTool("vet")[0]) + checkerargs = append(checkerargs, "-package_list", *packageList) + checkerargs = append(checkerargs, "-stdlib", *stdLib) + for _, im := range importMap { + checkerargs = append(checkerargs, "-importmap", im) + } + for _, f := range filenames { + checkerargs = append(checkerargs, "-src", f) + } checkerargs = append(checkerargs, filenames...) checkerCmd := exec.Command(*checker, checkerargs...) checkerCmd.Stdout, checkerCmd.Stderr = &checkerOutput, &checkerOutput diff --git a/go/tools/builders/generate_checker_main.go b/go/tools/builders/generate_checker_main.go index 3be6445f0c..e2a01b17cc 100644 --- a/go/tools/builders/generate_checker_main.go +++ b/go/tools/builders/generate_checker_main.go @@ -28,7 +28,7 @@ import ( "text/template" ) -var codeTpl = ` +const codeTpl = ` package main import ( @@ -37,6 +37,8 @@ import ( {{- end}} ) +const enableVet = {{.EnableVet}} + // configs maps analysis names to configurations. var configs = map[string]config{ {{- range $name, $config := .Configs}} @@ -44,7 +46,7 @@ var configs = map[string]config{ {{- if $config.ApplyTo -}} applyTo: map[string]bool{ {{range $path, $comment := $config.ApplyTo -}} - // {{$comment}} + {{if $comment -}} // {{$comment}} {{- end}} {{printf "%q" $path}}: true, {{- end}} }, @@ -52,7 +54,7 @@ var configs = map[string]config{ {{if $config.Whitelist -}} whitelist: map[string]bool{ {{range $path, $comment := $config.Whitelist -}} - // {{$comment}} + {{if $comment -}} // {{$comment}} {{- end}} {{printf "%q" $path}}: true, {{- end}} }, @@ -68,6 +70,7 @@ func run(args []string) error { out := flags.String("output", "", "output file to write (defaults to stdout)") flags.Var(&checkImportPaths, "check_importpath", "import path of a check library") configFile := flags.String("config", "", "checker config file") + enableVet := flags.Bool("vet", false, "whether to run vet") if err := flags.Parse(args); err != nil { return err } @@ -94,9 +97,11 @@ func run(args []string) error { data := struct { ImportPaths []string Configs Configs + EnableVet bool }{ ImportPaths: checkImportPaths, Configs: config, + EnableVet: *enableVet, } tpl := template.Must(template.New("source").Parse(codeTpl)) if err := tpl.Execute(outFile, data); err != nil { diff --git a/go/tools/builders/vet.go b/go/tools/builders/vet.go new file mode 100644 index 0000000000..b49330e17a --- /dev/null +++ b/go/tools/builders/vet.go @@ -0,0 +1,147 @@ +// Copyright 2018 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package main + +import ( + "encoding/json" + "errors" + "fmt" + "io/ioutil" + "os" + "os/exec" + "path/filepath" + "regexp" + "strings" +) + +// vet runs the 'go vet tool' command with the given vet configuration file and +// returns the emitted findings. It returns an error only if vet emits no +// findings and at least one error message. +func runVet(vetTool, vcfg string) (string, error) { + args := append(vetFlags, vcfg) + cmd := exec.Command(vetTool, args...) + out, err := cmd.CombinedOutput() + // Note: 'go tool vet' emits a non-zero return code both when vet encounters + // an internal error and when vet finds legitimate issues. + if err == nil { + return "", nil + } + vetFindings, vetErrors := []string{}, []string{} + errMsgs := splitOutput(string(out)) + for _, m := range errMsgs { + if !vetErrorMsgPattern.MatchString(m) { + vetErrors = append(vetErrors, m) + continue + } + vetFindings = append(vetFindings, m) + } + if len(vetFindings) == 0 && len(vetErrors) != 0 { + return "", errors.New(strings.Join(vetErrors, "\n")) + } + return strings.Join(vetFindings, "\n"), nil +} + +var vetFlags = []string{ + // NOTE: Keep in sync with github.com/golang/go/src/cmd/go/internal/test/test.go + "-atomic", + "-bool", + "-buildtags", + "-nilfunc", + "-printf", +} + +// vetErrorMsgPattern matches an error message emitted by vet. +// +// This regexp should be strict enough to exclude internal errors (prefixed with +// "vet") and type-check errors (which additionally include a column number). +// +// NOTE: this might break if the formating of vet error messages changes. +var vetErrorMsgPattern = regexp.MustCompile(`^([^:]+?):([0-9]+): (.*)`) + +// splitOutput was adapted from go/test/run.go. +func splitOutput(out string) []string { + // gc error messages continue onto additional lines with leading tabs. + // Split the output at the beginning of each line that doesn't begin with a tab. + // lines are impossible to match so those are filtered out. + var res []string + for _, line := range strings.Split(out, "\n") { + line = strings.TrimSuffix(line, "\r") // normalize Windows output + if strings.HasPrefix(line, "\t") { + res[len(res)-1] += "\n" + line + } else if strings.HasPrefix(line, "go tool") || strings.HasPrefix(line, "#") { + continue + } else if strings.TrimSpace(line) != "" { + res = append(res, line) + } + } + return res +} + +// makeVetCfg creates a vet.cfg file and returns its file path. It is the +// caller's responsibility to remove this file when it is no longer needed. +func makeVetCfg(packageList, stdLib string, packageFile, importMap map[string]string, files []string) (string, error) { + vcfg := &vetConfig{ + Compiler: "gc", // gccgo is currently not supported + GoFiles: files, + ImportMap: importMap, + PackageFile: packageFile, + Standard: make(map[string]bool), + SucceedOnTypecheckFailure: false, + } + packagesTxt, err := ioutil.ReadFile(packageList) + if err != nil { + return "", err + } + for _, line := range strings.Split(string(packagesTxt), "\n") { + line = strings.TrimSpace(line) + if line == "" { + continue + } + vcfg.Standard[line] = true + // vet expects all packages, including those in the stdlib, to be in + // ImportMap and PackageFile. + vcfg.ImportMap[line] = line // stdlib package names have no aliases. + vcfg.PackageFile[line] = filepath.Join(stdLib, line+".a") + } + + js, err := json.MarshalIndent(vcfg, "", "\t") + if err != nil { + return "", fmt.Errorf("internal error marshaling vet config: %v", err) + } + js = append(js, '\n') + vcfgFile := filepath.Join(os.TempDir(), "vet.cfg") + if err := ioutil.WriteFile(vcfgFile, js, 0666); err != nil { + return "", err + } + return vcfgFile, nil +} + +// vetConfig is the configuration passed to vet describing a single package. +// NOTE: Keep in sync with github.com/golang/go/internal/work/exec.go +type vetConfig struct { + Compiler string // compiler name (gc, gccgo) + Dir string // directory containing package + ImportPath string // canonical import path ("package path") + GoFiles []string // absolute paths to package source files + + ImportMap map[string]string // map import path in source code to package path + PackageFile map[string]string // map package path to .a file with export data + Standard map[string]bool // map package path to whether it's in the standard library + PackageVetx map[string]string // map package path to vetx data from earlier vet run + VetxOnly bool // only compute vetx data; don't report detected problems + VetxOutput string // write vetx data to this output file + + SucceedOnTypecheckFailure bool // awful hack; see #18395 and below +} diff --git a/tests/core/checks/BUILD.bazel b/tests/core/checks/BUILD.bazel new file mode 100644 index 0000000000..46c7ee89e2 --- /dev/null +++ b/tests/core/checks/BUILD.bazel @@ -0,0 +1,5 @@ +filegroup( + name = "rules_go_deps", + srcs = [":common.bzl"], + visibility = ["//visibility:public"], +) diff --git a/tests/core/checks/common.bzl b/tests/core/checks/common.bzl new file mode 100644 index 0000000000..0f84758fdc --- /dev/null +++ b/tests/core/checks/common.bzl @@ -0,0 +1,34 @@ +# Macros used by all build-time code analysis integration tests. + +BUILD_FAILED_TMPL = """ +if [[ result -eq 0 ]]; then + echo "TEST FAILED: expected build error" >&2 + result=1 +else + result=0 + {check_err} +fi +""" + +BUILD_PASSED_TMPL = """ +if [[ result -ne 0 ]]; then + echo "TEST FAILED: unexpected build error" >&2 + result=1 +else + {check_err} +fi +""" + +CONTAINS_ERR_TMPL = """ + if ! grep -q '{err}' bazel-output.txt; then + echo "TEST FAILED: expected error message containing: '{err}'" >&2 + result=1 + fi +""" + +DOES_NOT_CONTAIN_ERR_TMPL = """ + if grep -q '{err}' bazel-output.txt; then + echo "TEST FAILED: received error message containing: '{err}'" >&2 + result=1 + fi +""" diff --git a/tests/core/checks/custom/BUILD.bazel b/tests/core/checks/custom/BUILD.bazel index bb69a0ce94..9380281e78 100644 --- a/tests/core/checks/custom/BUILD.bazel +++ b/tests/core/checks/custom/BUILD.bazel @@ -1,5 +1,12 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library") load("@io_bazel_rules_go//tests:bazel_tests.bzl", "bazel_test") +load( + "@io_bazel_rules_go//tests/core/checks:common.bzl", + "BUILD_FAILED_TMPL", + "BUILD_PASSED_TMPL", + "CONTAINS_ERR_TMPL", + "DOES_NOT_CONTAIN_ERR_TMPL", +) BUILD_TMPL = """ load("@io_bazel_rules_go//go:def.bzl", "go_checker", "go_tool_library") @@ -47,43 +54,12 @@ EXTRA_FILES = [ ":config.json", ] -BUILD_FAILED_TMPL = """ -if [[ result -eq 0 ]]; then - echo "TEST FAILED: expected build error" >&2 - result=1 -else - result=0 - {check_err} -fi -""" - -BUILD_PASSED_TMPL = """ -if [[ result -ne 0 ]]; then - echo "TEST FAILED: unexpected build error" >&2 - result=1 -else - {check_err} -fi -""" - -CONTAINS_ERR_TMPL = """ - if ! grep -q '{err}' bazel-output.txt; then - echo "TEST FAILED: expected error message containing: '{err}'" >&2 - result=1 - fi -""" - -DOES_NOT_CONTAIN_ERR_TMPL = """ - if grep -q '{err}' bazel-output.txt; then - echo "TEST FAILED: received error message containing: '{err}'" >&2 - result=1 - fi -""" +CHECKER = "@//:my_checker" bazel_test( name = "custom_checks_default_config", build = BUILD_TMPL.format(config = ""), - go_checker = "@//:my_checker", + go_checker = CHECKER, check = BUILD_FAILED_TMPL.format( check_err = CONTAINS_ERR_TMPL.format(err = "custom/has_errors.go:4:2: package fmt must not be imported") + @@ -98,7 +74,7 @@ bazel_test( bazel_test( name = "custom_checks_custom_config", build = BUILD_TMPL.format(config = "config = \":config.json\","), - go_checker = "@//:my_checker", + go_checker = CHECKER, check = BUILD_FAILED_TMPL.format( check_err = CONTAINS_ERR_TMPL.format(err = "custom/has_errors.go:4:2: package fmt must not be imported") + @@ -113,7 +89,7 @@ bazel_test( bazel_test( name = "custom_checks_no_errors", build = BUILD_TMPL.format(config = ""), - go_checker = "@//:my_checker", + go_checker = CHECKER, check = BUILD_PASSED_TMPL.format( check_err = DOES_NOT_CONTAIN_ERR_TMPL.format(err = "no_errors.go:") diff --git a/tests/core/checks/custom/README.rst b/tests/core/checks/custom/README.rst index b872c286d5..38c24501f2 100644 --- a/tests/core/checks/custom/README.rst +++ b/tests/core/checks/custom/README.rst @@ -10,7 +10,7 @@ errors. custom_checks_default_config ---------------------------- -Verifies that custom checks print errors and fail a go_library build when a +Verifies that custom checks print errors and fail a `go_library`_ build when a configuration file is not provided. custom_checks_custom_config diff --git a/tests/core/checks/default/BUILD.bazel b/tests/core/checks/default/BUILD.bazel deleted file mode 100644 index 4e59c171be..0000000000 --- a/tests/core/checks/default/BUILD.bazel +++ /dev/null @@ -1,49 +0,0 @@ -# Add this test back after support for vet has been added. - -# load("@io_bazel_rules_go//go:def.bzl", "go_library") -# load("@io_bazel_rules_go//tests:bazel_tests.bzl", "bazel_test") - -# default_checker_error = """ -# if [[ result -eq 0 ]]; then -# echo "error: expected build error" >&2 -# result=1 -# else -# grep -q 'TODO: fill with default check failure' bazel-output.txt -# if [[ $? -ne 0 ]]; then -# echo "error: expected error message was not emitted" >&2 -# result=1 -# else -# result=0 -# fi -# fi -# """ - -# bazel_test( -# name = "checker_error", -# check = default_checker_error, -# command = "build", -# targets = [":lib"], -# ) - -# go_library( -# name = "lib", -# srcs = ["lib.go"], -# importpath = "lib", -# ) - -_build = """ -load("@io_bazel_rules_go//go:def.bzl", "go_checker", "go_binary", "go_library", "go_test") - -go_checker( - name = "my_checker", - visibility = ["//visibility:public"], -) -""" - - -_workspace = """ -load("@io_bazel_rules_go//go:def.bzl", "go_rules_dependencies", "go_register_toolchains") │············· - │············· -go_rules_dependencies() │············· -go_register_toolchains(go_checker="@//:my_checker") -""" diff --git a/tests/core/checks/default/README.rst b/tests/core/checks/default/README.rst deleted file mode 100644 index 92c00c6917..0000000000 --- a/tests/core/checks/default/README.rst +++ /dev/null @@ -1,15 +0,0 @@ -Default checker -=============== - -.. _go_library: /go/core.rst#_go_library - -Tests to ensure that the default checker is working is generated when using -the core Go rules. - -.. contents:: - -checker_error -------------- -Verifies that the default checker binary runs during bazel build by ensuring -that a build failure and error message are emitted when building a go_library_ -target containing erroneous source code. diff --git a/tests/core/checks/default/lib.go b/tests/core/checks/default/lib.go deleted file mode 100644 index 8dd5a8aee2..0000000000 --- a/tests/core/checks/default/lib.go +++ /dev/null @@ -1,8 +0,0 @@ -package lib - -func Foo() bool { - // This redundant boolean expression should trigger a vet error, which is - // turned on by default. - a := true - return a || a -} diff --git a/tests/core/checks/vet/BUILD.bazel b/tests/core/checks/vet/BUILD.bazel new file mode 100644 index 0000000000..1c049bf386 --- /dev/null +++ b/tests/core/checks/vet/BUILD.bazel @@ -0,0 +1,72 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_library") +load("@io_bazel_rules_go//tests:bazel_tests.bzl", "bazel_test") +load( + "@io_bazel_rules_go//tests/core/checks:common.bzl", + "BUILD_FAILED_TMPL", + "BUILD_PASSED_TMPL", + "CONTAINS_ERR_TMPL", + "DOES_NOT_CONTAIN_ERR_TMPL", +) + +BUILD_ENABLE_VET = """ +load("@io_bazel_rules_go//go:def.bzl", "go_checker", "go_tool_library") + +go_checker( + name = "my_checker", + vet = True, + visibility = ["//visibility:public"], +) +""" + +CHECKER = "@//:my_checker" + +bazel_test( + name = "vet_enabled_no_errors", + build = BUILD_ENABLE_VET, + go_checker = CHECKER, + check = BUILD_PASSED_TMPL.format( + check_err = ":" # no-op + ), + command = "build", + targets = [":no_errors"], +) + +bazel_test( + name = "vet_enabled_has_errors", + build = BUILD_ENABLE_VET, + go_checker = CHECKER, + check = BUILD_FAILED_TMPL.format( + check_err = + CONTAINS_ERR_TMPL.format(err = "has_errors.go:3: +build comment must appear before package clause and be followed by a blank line") + + CONTAINS_ERR_TMPL.format(err = "has_errors.go:15: comparison of function F == nil is always false") + + CONTAINS_ERR_TMPL.format(err = 'has_errors.go:18: Printf format %b has arg "hi" of wrong type strin') + + CONTAINS_ERR_TMPL.format(err = "has_errors.go:19: redundant or: true || true") + ), + command = "build", + targets = [":has_errors"], +) + +bazel_test( + name = "vet_default", + check = BUILD_PASSED_TMPL.format( + check_err = + DOES_NOT_CONTAIN_ERR_TMPL.format(err = "+build comment must appear before package clause and be followed by a blank line") + + DOES_NOT_CONTAIN_ERR_TMPL.format(err = "comparison of function F == nil is always false") + + DOES_NOT_CONTAIN_ERR_TMPL.format(err = 'Printf format %b has arg "hi" of wrong type strin') + + DOES_NOT_CONTAIN_ERR_TMPL.format(err = "redundant or: true || true") + ), + command = "build", + targets = [":has_errors"], +) + +go_library( + name = "has_errors", + srcs = ["has_errors.go"], + importpath = "haserrors", +) + +go_library( + name = "no_errors", + srcs = ["no_errors.go"], + importpath = "noerrors", +) diff --git a/tests/core/checks/vet/README.rst b/tests/core/checks/vet/README.rst new file mode 100644 index 0000000000..d49948942c --- /dev/null +++ b/tests/core/checks/vet/README.rst @@ -0,0 +1,21 @@ +Vet check +========= + +.. _go_library: /go/core.rst#_go_library + +Tests to ensure that vet run and detects errors. + +.. contents:: + +vet_enabled_no_errors +--------------------- +Verifies that vet does not fail the build when analyzing error-free source code. + +vet_enabled_has_errors +---------------------- +Verifies that vet emits findings and fails a `go_library`_ build when analyzing +erroneous source code. + +vet_default +----------- +Verifies that vet is disabled by default. diff --git a/tests/core/checks/vet/has_errors.go b/tests/core/checks/vet/has_errors.go new file mode 100644 index 0000000000..84b4a4608a --- /dev/null +++ b/tests/core/checks/vet/has_errors.go @@ -0,0 +1,20 @@ +package haserrors + +// +build build_tags_error + +import ( + "fmt" + "sync/atomic" +) + +func F() {} + +func Foo() bool { + x := uint64(1) + _ = atomic.AddUint64(&x, 1) + if F == nil { // nilfunc error. + return false + } + fmt.Printf("%b", "hi") // printf error. + return true || true // redundant boolean error. +} diff --git a/tests/core/checks/vet/no_errors.go b/tests/core/checks/vet/no_errors.go new file mode 100644 index 0000000000..9a9b6eb4df --- /dev/null +++ b/tests/core/checks/vet/no_errors.go @@ -0,0 +1,5 @@ +package noerrors + +func Foo() bool { + return true +}