Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not report nogo diagnostics for cgo generated files #4081

Merged
merged 1 commit into from
Sep 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion go/private/actions/compilepkg.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ def _run_nogo(
args.add_all(sources, before_each = "-src")
if cgo_go_srcs:
inputs_direct.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:
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