Skip to content

Commit

Permalink
Do not report nogo diagnostics for cgo generated files (#4081)
Browse files Browse the repository at this point in the history
**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**
  • Loading branch information
fmeum authored and tyler-french committed Sep 4, 2024
1 parent 6df867c commit 503d471
Show file tree
Hide file tree
Showing 4 changed files with 86 additions and 12 deletions.
4 changes: 2 additions & 2 deletions go/private/actions/compilepkg.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -245,8 +245,8 @@ def _run_nogo(
args = go.builder_args(go, "nogo", use_path_mapping = True)
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")
inputs_direct.append(cgo_go_srcs)
args.add_all([cgo_go_srcs], before_each = "-ignore_src")
if cover_mode:
args.add("-cover_mode", cover_mode)
if recompile_internal_deps:
Expand Down
14 changes: 9 additions & 5 deletions go/tools/builders/nogo.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.")
Expand All @@ -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
}
Expand Down Expand Up @@ -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.
Expand All @@ -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")
Expand Down
20 changes: 18 additions & 2 deletions go/tools/builders/nogo_main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand All @@ -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
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
60 changes: 57 additions & 3 deletions tests/core/nogo/custom/custom_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ nogo(
":foofuncname",
":importfmt",
":visibility",
":cgogen",
],
# config = "",
visibility = ["//visibility:public"],
Expand Down Expand Up @@ -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"],
Expand All @@ -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 = [
Expand All @@ -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
Expand Down Expand Up @@ -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 --
{
Expand Down Expand Up @@ -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) {
Expand All @@ -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 {
Expand Down

0 comments on commit 503d471

Please sign in to comment.