From c93053c99d757c340c6a506bcbc74385d6068653 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Wed, 4 Sep 2024 16:47:45 +0200 Subject: [PATCH] Do not report nogo diagnostics for cgo generated files (#4081) **What type of PR is this?** Bug fix **What does this PR do? Why is it needed?** **Which issues(s) does this PR fix?** Fixes #4079 **Other notes for review** --- go/private/actions/compilepkg.bzl | 2 +- go/tools/builders/nogo.go | 14 ++++--- go/tools/builders/nogo_main.go | 20 ++++++++- tests/core/nogo/custom/custom_test.go | 60 +++++++++++++++++++++++++-- 4 files changed, 85 insertions(+), 11 deletions(-) diff --git a/go/private/actions/compilepkg.bzl b/go/private/actions/compilepkg.bzl index 43872cf3c..a7d26b616 100644 --- a/go/private/actions/compilepkg.bzl +++ b/go/private/actions/compilepkg.bzl @@ -246,7 +246,7 @@ def _run_nogo( args.add_all(sources, before_each = "-src") if cgo_go_srcs: inputs.append(cgo_go_srcs) - args.add_all([cgo_go_srcs], before_each = "-src") + args.add_all([cgo_go_srcs], before_each = "-ignore_src") if cover_mode: args.add("-cover_mode", cover_mode) if recompile_internal_deps: diff --git a/go/tools/builders/nogo.go b/go/tools/builders/nogo.go index 9747942f8..44222013a 100644 --- a/go/tools/builders/nogo.go +++ b/go/tools/builders/nogo.go @@ -20,13 +20,14 @@ func nogo(args []string) error { fs := flag.NewFlagSet("GoNogo", flag.ExitOnError) goenv := envFlags(fs) - var unfilteredSrcs, recompileInternalDeps multiFlag + var unfilteredSrcs, ignoreSrcs, recompileInternalDeps multiFlag var deps, facts archiveMultiFlag var importPath, packagePath, nogoPath, packageListPath string var testFilter string var outFactsPath, outLogPath string var coverMode string - fs.Var(&unfilteredSrcs, "src", ".go, .c, .cc, .m, .mm, .s, or .S file to be filtered and compiled") + fs.Var(&unfilteredSrcs, "src", ".go, .c, .cc, .m, .mm, .s, or .S file to be filtered and checked") + fs.Var(&ignoreSrcs, "ignore_src", ".go, .c, .cc, .m, .mm, .s, or .S file to be filtered and checked, but with its diagnostics ignored") fs.Var(&deps, "arc", "Import path, package path, and file name of a direct dependency, separated by '='") fs.Var(&facts, "facts", "Import path, package path, and file name of a direct dependency's nogo facts file, separated by '='") fs.StringVar(&importPath, "importpath", "", "The import path of the package being compiled. Not passed to the compiler, but may be displayed in debug data.") @@ -49,7 +50,7 @@ func nogo(args []string) error { } // Filter sources. - srcs, err := filterAndSplitFiles(unfilteredSrcs) + srcs, err := filterAndSplitFiles(append(unfilteredSrcs, ignoreSrcs...)) if err != nil { return err } @@ -81,10 +82,10 @@ func nogo(args []string) error { return err } - return runNogo(workDir, nogoPath, goSrcs, facts, importPath, importcfgPath, outFactsPath, outLogPath) + return runNogo(workDir, nogoPath, goSrcs, ignoreSrcs, facts, importPath, importcfgPath, outFactsPath, outLogPath) } -func runNogo(workDir string, nogoPath string, srcs []string, facts []archive, packagePath, importcfgPath, outFactsPath string, outLogPath string) error { +func runNogo(workDir string, nogoPath string, srcs, ignores []string, facts []archive, packagePath, importcfgPath, outFactsPath string, outLogPath string) error { if len(srcs) == 0 { // emit_compilepkg expects a nogo facts file, even if it's empty. // We also need to write the validation output log. @@ -105,6 +106,9 @@ func runNogo(workDir string, nogoPath string, srcs []string, facts []archive, pa args = append(args, "-fact", fmt.Sprintf("%s=%s", fact.importPath, fact.file)) } args = append(args, "-x", outFactsPath) + for _, ignore := range ignores { + args = append(args, "-ignore", ignore) + } args = append(args, srcs...) paramsFile := filepath.Join(workDir, "nogo.param") diff --git a/go/tools/builders/nogo_main.go b/go/tools/builders/nogo_main.go index 9cf558ba1..e9aff5ef5 100644 --- a/go/tools/builders/nogo_main.go +++ b/go/tools/builders/nogo_main.go @@ -77,6 +77,8 @@ func run(args []string) (error, int) { importcfg := flags.String("importcfg", "", "The import configuration file") packagePath := flags.String("p", "", "The package path (importmap) of the package being compiled") xPath := flags.String("x", "", "The archive file where serialized facts should be written") + var ignores multiFlag + flags.Var(&ignores, "ignore", "Names of files to ignore") flags.Parse(args) srcs := flags.Args() @@ -85,7 +87,7 @@ func run(args []string) (error, int) { return fmt.Errorf("error parsing importcfg: %v", err), nogoError } - diagnostics, facts, err := checkPackage(analyzers, *packagePath, packageFile, importMap, factMap, srcs) + diagnostics, facts, err := checkPackage(analyzers, *packagePath, packageFile, importMap, factMap, srcs, ignores) if err != nil { return fmt.Errorf("error running analyzers: %v", err), nogoError } @@ -156,7 +158,7 @@ func readImportCfg(file string) (packageFile map[string]string, importMap map[st // It returns an empty string if no source code diagnostics need to be printed. // // This implementation was adapted from that of golang.org/x/tools/go/checker/internal/checker. -func checkPackage(analyzers []*analysis.Analyzer, packagePath string, packageFile, importMap map[string]string, factMap map[string]string, filenames []string) (string, []byte, error) { +func checkPackage(analyzers []*analysis.Analyzer, packagePath string, packageFile, importMap map[string]string, factMap map[string]string, filenames, ignoreFiles []string) (string, []byte, error) { // Register fact types and establish dependencies between analyzers. actions := make(map[*analysis.Analyzer]*action) var visit func(a *analysis.Analyzer) *action @@ -211,8 +213,22 @@ func checkPackage(analyzers []*analysis.Analyzer, packagePath string, packageFil act.pkg = pkg } + ignoreFilesSet := map[string]struct{}{} + for _, ignore := range ignoreFiles { + ignoreFilesSet[ignore] = struct{}{} + } // Process nolint directives similar to golangci-lint. + // Also skip over fully ignored files. for _, f := range pkg.syntax { + if _, ok := ignoreFilesSet[pkg.fset.Position(f.Pos()).Filename]; ok { + for _, act := range actions { + act.nolint = append(act.nolint, &Range{ + from: pkg.fset.Position(f.FileStart), + to: pkg.fset.Position(f.End()).Line, + }) + } + continue + } // CommentMap will correctly associate comments to the largest node group // applicable. This handles inline comments that might trail a large // assignment and will apply the comment to the entire assignment. diff --git a/tests/core/nogo/custom/custom_test.go b/tests/core/nogo/custom/custom_test.go index a7a6786b3..b23a20f00 100644 --- a/tests/core/nogo/custom/custom_test.go +++ b/tests/core/nogo/custom/custom_test.go @@ -39,6 +39,7 @@ nogo( ":foofuncname", ":importfmt", ":visibility", + ":cgogen", ], # config = "", visibility = ["//visibility:public"], @@ -71,6 +72,14 @@ go_library( visibility = ["//visibility:public"], ) +go_library( + name = "cgogen", + srcs = ["cgogen.go"], + importpath = "cgogenanalyzer", + deps = ["@org_golang_x_tools//go/analysis"], + visibility = ["//visibility:public"], +) + go_library( name = "has_errors", srcs = ["has_errors.go"], @@ -85,6 +94,13 @@ go_library( deps = [":dep"], ) +go_library( + name = "uses_cgo_clean", + srcs = ["examplepkg/uses_cgo_clean.go"], + importpath = "uses_cgo_clean", + cgo = True, +) + go_library( name = "uses_cgo_with_errors", srcs = [ @@ -109,7 +125,7 @@ go_library( ) -- foofuncname.go -- -// importfmt checks for functions named "Foo". +// foofuncname checks for functions named "Foo". // It has the same package name as another check to test the checks with // the same package name do not conflict. package importfmt @@ -279,6 +295,40 @@ func run(pass *analysis.Pass) (interface{}, error) { return nil, nil } +-- cgogen.go -- +// cgogen reports diagnostics on files generated by cgo. +package cgogen + +import ( + "go/ast" + "strings" + + "golang.org/x/tools/go/analysis" +) + +const doc = "report synthetic diagnostics on files generated by cgo" + +var Analyzer = &analysis.Analyzer{ + Name: "cgogen", + Run: run, + Doc: doc, +} + +func run(pass *analysis.Pass) (interface{}, error) { + for _, f := range pass.Files { + ast.Inspect(f, func(n ast.Node) bool { + switch n := n.(type) { + case *ast.Comment: + if strings.HasPrefix(n.Text, "//go:linkname") || strings.HasPrefix(n.Text, "//go:cgo_import") { + pass.Reportf(n.Pos(), "nogo must not run on code generated by cgo") + } + return true + } + return true + }) + } + return nil, nil +} -- config.json -- { @@ -462,6 +512,10 @@ func Test(t *testing.T) { target: "//:no_errors", wantSuccess: true, excludes: []string{"no_errors.go"}, + }, { + desc: "uses_cgo_clean", + target: "//:uses_cgo_clean", + wantSuccess: true, }, } { t.Run(test.desc, func(t *testing.T) { @@ -477,9 +531,9 @@ func Test(t *testing.T) { stderr := &bytes.Buffer{} cmd.Stderr = stderr if err := cmd.Run(); err == nil && !test.wantSuccess { - t.Fatal("unexpected success") + t.Fatalf("unexpected success\n%s", stderr) } else if err != nil && test.wantSuccess { - t.Fatalf("unexpected error: %v", err) + t.Fatalf("unexpected error: %v\n%s", err, stderr) } for _, pattern := range test.includes {