From 4558d4c5e600c3c921b4e5ad3081ed4ef350408a Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Thu, 5 Nov 2026 01:06:14 -0600 Subject: [PATCH 1/3] Refactor: Let the driver call a hook after it loads packages A flag default can name a package, and only the loaded packages say whether the default is right. nilaway's -include-pkgs wants the main module of the run, which neither the working directory nor a flag parse reveals: a workspace holds several main modules, and a pattern can point at a module that is not the current one. Add a hook that the command supplies and the driver calls after loading, in the analysis path and in the fix path. No command supplies a hook yet, so no flag changes. The test pins the order: the hook runs before the analyzers, and it sees the loaded roots and their modules. Change-Id: If8fdf9c1deeca15f4f8d6919e6060541a040dbfd --- cmd/constable/main.go | 1 + internal/driver/driver.go | 36 ++++++++++++++++--- internal/driver/hooks_test.go | 66 +++++++++++++++++++++++++++++++++++ 3 files changed, 99 insertions(+), 4 deletions(-) create mode 100644 internal/driver/hooks_test.go diff --git a/cmd/constable/main.go b/cmd/constable/main.go index dc53b1d..116b9e8 100644 --- a/cmd/constable/main.go +++ b/cmd/constable/main.go @@ -21,6 +21,7 @@ var version string func main() { driver.Main( version, + driver.Hooks{}, nonmutating.Analyzer, methodical.Analyzer, predicates.Analyzer, diff --git a/internal/driver/driver.go b/internal/driver/driver.go index 47a4c24..b5fcc3d 100644 --- a/internal/driver/driver.go +++ b/internal/driver/driver.go @@ -34,6 +34,21 @@ type Options struct { IncludeTests bool Fix bool Diff bool + + // AfterLoad runs with the packages that the driver loaded, before any + // analyzer runs. A flag whose default names a package is set here, because + // only the loaded packages know which packages the run covers. + // + // Main copies Hooks.AfterLoad here, so Run and its fix passes call the + // same hook. + AfterLoad func(roots []*packages.Package) +} + +// Hooks are the steps that a command supplies to the driver. +type Hooks struct { + // AfterLoad runs with the packages that the driver loaded, before any + // analyzer runs. + AfterLoad func(roots []*packages.Package) } type triState int @@ -106,7 +121,7 @@ func isShortVersion(v versionValue) bool { // Main is the main function for the constable command. // version is the release version injected at build time via // -ldflags "-X main.version=..."; when empty, the embedded build info is used. -func Main(version string, analyzers ...*analysis.Analyzer) { +func Main(version string, hooks Hooks, analyzers ...*analysis.Analyzer) { progname := filepath.Base(os.Args[0]) log.SetFlags(0) log.SetPrefix(progname + ": ") @@ -118,6 +133,7 @@ func Main(version string, analyzers ...*analysis.Analyzer) { var opts Options opts.IncludeTests = true opts.Context = -1 + opts.AfterLoad = hooks.AfterLoad enabled := make(map[*analysis.Analyzer]*triState) for _, a := range analyzers { @@ -169,6 +185,13 @@ func Main(version string, analyzers ...*analysis.Analyzer) { os.Exit(Run(args, analyzers, opts)) } +// afterLoad runs the hook that the command supplied, if any. +func (opts Options) afterLoad(roots []*packages.Package) { + if opts.AfterLoad != nil { + opts.AfterLoad(roots) + } +} + // registerFlags registers the flags of each analyzer in the requirement graph // of analyzers under the "." names. A required analyzer // carries configuration that its dependents read, so the flags of a required @@ -466,6 +489,8 @@ func Run(args []string, analyzers []*analysis.Analyzer, opts Options) (exitcode return exitcode } + opts.afterLoad(initial) + if n := packages.PrintErrors(initial); n > 0 { exitAtLeast(1) } @@ -531,7 +556,7 @@ func runFixes(args []string, analyzers []*analysis.Analyzer, opts Options, cwd s overlay := state var sources map[*token.File][]byte var err error - graph, sources, err = analyzeFixPass(args, analyzers, opts.IncludeTests, overlay) + graph, sources, err = analyzeFixPass(args, analyzers, opts, overlay) if err != nil { log.Print(err) return 1 @@ -579,14 +604,17 @@ func fixPassLimit(sources map[*token.File][]byte) int { return max(1000, size) } -func analyzeFixPass(args []string, analyzers []*analysis.Analyzer, includeTests bool, overlay map[string][]byte) (*checker.Graph, map[*token.File][]byte, error) { - initial, sources, err := load(args, includeTests, needFacts(analyzers), overlay) +func analyzeFixPass(args []string, analyzers []*analysis.Analyzer, opts Options, overlay map[string][]byte) (*checker.Graph, map[*token.File][]byte, error) { + initial, sources, err := load(args, opts.IncludeTests, needFacts(analyzers), overlay) if err != nil { return nil, nil, err } if packages.PrintErrors(initial) > 0 { return nil, nil, fmt.Errorf("package loading failed") } + + opts.afterLoad(initial) + graph, err := checker.Analyze(overlayAnalyzers(analyzers, overlay), initial, &checker.Options{}) return graph, sources, err } diff --git a/internal/driver/hooks_test.go b/internal/driver/hooks_test.go new file mode 100644 index 0000000..734b20c --- /dev/null +++ b/internal/driver/hooks_test.go @@ -0,0 +1,66 @@ +package driver_test + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/packages" + + "github.com/nnutter/constable/internal/driver" +) + +func TestRunCallsAfterLoadBeforeAnalysis(t *testing.T) { + dir := t.TempDir() + cwd, err := os.Getwd() + require.NoError(t, err) + require.NoError(t, os.Chdir(dir)) + t.Cleanup(func() { require.NoError(t, os.Chdir(cwd)) }) + require.NoError(t, os.WriteFile(filepath.Join(dir, "go.mod"), []byte("module example.com/hook\n\ngo 1.25\n"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "hook.go"), []byte("package hook\n"), 0o644)) + + var order []string + var roots []*packages.Package + analyzer := &analysis.Analyzer{ + Name: "noop", + Doc: "reports nothing", + Run: func(*analysis.Pass) (any, error) { + order = append(order, "analyze") + return nil, nil + }, + } + + code := driver.Run([]string{"."}, []*analysis.Analyzer{analyzer}, driver.Options{ + Context: -1, + IncludeTests: true, + AfterLoad: func(pkgs []*packages.Package) { + order = append(order, "load") + roots = pkgs + }, + }) + + require.Equal(t, 0, code) + require.Equal(t, []string{"load", "analyze"}, order) + require.Len(t, roots, 1) + require.Equal(t, "example.com/hook", roots[0].PkgPath) + require.NotNil(t, roots[0].Module) + require.Equal(t, "example.com/hook", roots[0].Module.Path) + require.True(t, roots[0].Module.Main) +} + +func TestRunWithoutAfterLoad(t *testing.T) { + dir := t.TempDir() + cwd, err := os.Getwd() + require.NoError(t, err) + require.NoError(t, os.Chdir(dir)) + t.Cleanup(func() { require.NoError(t, os.Chdir(cwd)) }) + require.NoError(t, os.WriteFile(filepath.Join(dir, "go.mod"), []byte("module example.com/nohook\n\ngo 1.25\n"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "hook.go"), []byte("package nohook\n"), 0o644)) + + analyzer := &analysis.Analyzer{Name: "noop", Doc: "reports nothing", Run: func(*analysis.Pass) (any, error) { return nil, nil }} + code := driver.Run([]string{"."}, []*analysis.Analyzer{analyzer}, driver.Options{Context: -1, IncludeTests: true}) + + require.Equal(t, 0, code) +} From 6dd56d34b334bf1c67c3c5cf9cfa8d92cb60267c Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Thu, 5 Nov 2026 01:06:38 -0600 Subject: [PATCH 2/3] Refactor: Name the generated-file predicate for what it decides The analyzers asked whether a file is generated, but what they do with the answer is skip its diagnostics. Rename the predicate in the signature change, not in a behavior change. The next commit adds a flag that turns the skip off, and a name that states the decision keeps the call site honest about which question the flag answers. Change-Id: Id1f81ecea1ae760033b52724574db7f9c2a0d44d --- internal/analysis/complexity/complexity.go | 2 +- internal/analysis/methodical/fix.go | 2 +- internal/analysis/methodical/methodical.go | 6 +++--- internal/analysis/nonmutating/nonmutating.go | 2 +- internal/analysis/predicates/predicates.go | 2 +- internal/analysis/testify/testify.go | 2 +- internal/generated/generated.go | 12 ++++++------ internal/generated/generated_test.go | 4 ++-- internal/generated/skip.go | 2 +- 9 files changed, 17 insertions(+), 17 deletions(-) diff --git a/internal/analysis/complexity/complexity.go b/internal/analysis/complexity/complexity.go index 79b08eb..b753cc2 100644 --- a/internal/analysis/complexity/complexity.go +++ b/internal/analysis/complexity/complexity.go @@ -42,7 +42,7 @@ func isLogicalOr(op token.Token) bool { func run(pass *analysis.Pass) (any, error) { for _, file := range pass.Files { - if generated.IsGenerated(file) { + if generated.Skipped(file) { continue } diff --git a/internal/analysis/methodical/fix.go b/internal/analysis/methodical/fix.go index 9c22f56..34fd5e3 100644 --- a/internal/analysis/methodical/fix.go +++ b/internal/analysis/methodical/fix.go @@ -85,7 +85,7 @@ func readFixFile(pass *analysis.Pass, file *ast.File) *fixFile { return nil } src, err := pass.ReadFile(tf.Name()) - if err != nil || generated.IsGenerated(file) || hasStandaloneComments(pass.Fset, file) { + if err != nil || generated.Skipped(file) || hasStandaloneComments(pass.Fset, file) { return nil } return &fixFile{file: file, token: tf, source: src} diff --git a/internal/analysis/methodical/methodical.go b/internal/analysis/methodical/methodical.go index 7517f7d..5cd0d31 100644 --- a/internal/analysis/methodical/methodical.go +++ b/internal/analysis/methodical/methodical.go @@ -64,7 +64,7 @@ func isNilVar(v *types.Var) bool { func collectTypeFiles(pass *analysis.Pass) map[types.Object]string { typeFiles := make(map[types.Object]string) for _, file := range pass.Files { - if generated.IsGenerated(file) { + if generated.Skipped(file) { continue } @@ -107,7 +107,7 @@ type groupKey struct { func collectMethods(pass *analysis.Pass, typeFiles map[types.Object]string) []methodInfo { var methods []methodInfo for _, file := range pass.Files { - if generated.IsGenerated(file) { + if generated.Skipped(file) { continue } @@ -242,7 +242,7 @@ func checkGrouped(reporter *diagnosticReporter, typeFiles map[types.Object]strin } func checkFileGrouped(reporter *diagnosticReporter, file *ast.File, typeFiles map[types.Object]string) { - if generated.IsGenerated(file) { + if generated.Skipped(file) { return } diff --git a/internal/analysis/nonmutating/nonmutating.go b/internal/analysis/nonmutating/nonmutating.go index e34a0d4..0f15fb0 100644 --- a/internal/analysis/nonmutating/nonmutating.go +++ b/internal/analysis/nonmutating/nonmutating.go @@ -60,7 +60,7 @@ func hasNoArgs(call *ast.CallExpr) bool { func run(pass *analysis.Pass) (any, error) { for _, file := range pass.Files { - if generated.IsGenerated(file) { + if generated.Skipped(file) { continue } diff --git a/internal/analysis/predicates/predicates.go b/internal/analysis/predicates/predicates.go index 81d58d2..06338d9 100644 --- a/internal/analysis/predicates/predicates.go +++ b/internal/analysis/predicates/predicates.go @@ -55,7 +55,7 @@ func isBoolName(ident *ast.Ident) bool { func run(pass *analysis.Pass) (any, error) { for _, file := range pass.Files { - if generated.IsGenerated(file) { + if generated.Skipped(file) { continue } diff --git a/internal/analysis/testify/testify.go b/internal/analysis/testify/testify.go index 107636b..12458c7 100644 --- a/internal/analysis/testify/testify.go +++ b/internal/analysis/testify/testify.go @@ -47,7 +47,7 @@ var testifyPackages = map[string]string{ func run(pass *analysis.Pass) (any, error) { for _, file := range pass.Files { - if generated.IsGenerated(file) { + if generated.Skipped(file) { continue } diff --git a/internal/generated/generated.go b/internal/generated/generated.go index d716071..a41d941 100644 --- a/internal/generated/generated.go +++ b/internal/generated/generated.go @@ -4,12 +4,12 @@ package generated import "go/ast" -// IsGenerated reports whether file carries the standard generated-code -// comment, "// Code generated DO NOT EDIT.". +// Skipped reports whether analyzers omit the diagnostics of file. // -// Analyzers skip generated files. A diagnostic there is not actionable -// because the next run of the generating tool overwrites the file. The check -// is the Go convention, so an author controls it with the comment alone. -func IsGenerated(file *ast.File) bool { +// A diagnostic in a generated file is not actionable because the next run of +// the generating tool overwrites the file. The mark is the Go convention, +// "// Code generated DO NOT EDIT.", so an author controls it with the +// comment alone. +func Skipped(file *ast.File) bool { return ast.IsGenerated(file) } diff --git a/internal/generated/generated_test.go b/internal/generated/generated_test.go index 42fd15f..d73bf9f 100644 --- a/internal/generated/generated_test.go +++ b/internal/generated/generated_test.go @@ -22,7 +22,7 @@ func parse(t *testing.T, src string) *ast.File { return file } -func TestIsGenerated(t *testing.T) { +func TestSkipped(t *testing.T) { tests := []struct { name string src string @@ -62,7 +62,7 @@ func TestIsGenerated(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - assert.Equal(t, test.want, generated.IsGenerated(parse(t, test.src))) + assert.Equal(t, test.want, generated.Skipped(parse(t, test.src))) }) } } diff --git a/internal/generated/skip.go b/internal/generated/skip.go index 875e4fd..320217e 100644 --- a/internal/generated/skip.go +++ b/internal/generated/skip.go @@ -38,7 +38,7 @@ func Skip(a *analysis.Analyzer) *analysis.Analyzer { func generatedFiles(pass *analysis.Pass, files []*ast.File) map[string]bool { var names map[string]bool for _, file := range files { - if !IsGenerated(file) { + if !Skipped(file) { continue } if names == nil { From 492a6583318964def4ee89983d0cb003773c566f Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Thu, 5 Nov 2026 01:09:23 -0600 Subject: [PATCH 3/3] Default nilaway to the main module NilAway reads every package that it loads, so a run over one module also builds the SSA form of the standard library and of every dependency. That costs seconds and gigabytes, and it reports flows that the analyzed repository cannot fix: a nil value inside the standard library belongs to Go, not to the code under review. Set -include-pkgs to the main modules of the loaded packages. The driver hook supplies them, because a workspace has several main modules and a pattern can name a module other than the current one. A reader who wants the old scope sets -nilaway.include-all-pkgs, and an explicit -nilaway_config.include-pkgs keeps the packages that it names. Widening the scope exposes two code sites that the narrow run never reaches, because it needs a fact about a standard library package. The driver test now guards each flag lookup, and the fix pass builds its map instead of cloning one that can be nil. The hook no longer needs the flag on its command line, so the command returns to the plain form. Change-Id: I1ef0782ba71084540c610bc5064ba595167d6f62 --- README.md | 15 +++- cmd/constable/main.go | 2 +- cmd/constable/main_test.go | 12 ++- hk.pkl | 7 +- internal/analysis/nilaway/nilaway.go | 82 ++++++++++++++++++++- internal/analysis/nilaway/nilaway_test.go | 90 +++++++++++++++++++++++ internal/driver/driver.go | 3 +- internal/driver/flags_test.go | 36 ++++++++- 8 files changed, 228 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index 9f0441d..6396a1a 100644 --- a/README.md +++ b/README.md @@ -143,15 +143,22 @@ Reports a test that misuses testify. ### nilaway Reports a nil value that flows to a dereference. -NilAway reads every dependency by default, which costs seconds and gigabytes. -Give it the module of the code that constable analyzes: + +NilAway reads every package that it loads by default, which costs seconds and gigabytes and reports flows that the code under analysis cannot fix. +constable limits it to the main modules of the packages that it analyzes. +Two flags change the choice, ```sh -constable -nilaway_config.include-pkgs=$(go list -m) ./... +# Analyze the packages in this module, and no other package. +constable -nilaway_config.include-pkgs=example.com/my/module ./... + +# Analyze every package, including dependencies and the standard library. +constable -nilaway.include-all-pkgs ./... ``` +An explicit `-nilaway_config.include-pkgs` wins over `-nilaway.include-all-pkgs`, and both win over the default. The `nilaway_config` prefix is the name of the analyzer that holds the configuration, not the name of the analyzer that reports the diagnostic. -The driver registers the flags of a required analyzer, so this flag reaches the analyzer that reads it. +The driver registers the flags of a required analyzer, so that flag reaches the analyzer that reads it. ## Development diff --git a/cmd/constable/main.go b/cmd/constable/main.go index 116b9e8..8469b5e 100644 --- a/cmd/constable/main.go +++ b/cmd/constable/main.go @@ -21,7 +21,7 @@ var version string func main() { driver.Main( version, - driver.Hooks{}, + driver.Hooks{AfterLoad: nilaway.UseMainModules}, nonmutating.Analyzer, methodical.Analyzer, predicates.Analyzer, diff --git a/cmd/constable/main_test.go b/cmd/constable/main_test.go index c3d37ac..fd60f1d 100644 --- a/cmd/constable/main_test.go +++ b/cmd/constable/main_test.go @@ -318,13 +318,14 @@ func TestCLIEmbeddedAnalyzerFlags(t *testing.T) { "testifylint", "testifylint.disable-all", "nilaway", + "nilaway.include-all-pkgs", "nilaway_config.include-pkgs", } { assert.Contains(t, string(output), `"`+name+`"`) } } -func TestCLIEmbeddedNilawayReadsRequiredAnalyzerFlag(t *testing.T) { +func TestCLIEmbeddedNilawayPackages(t *testing.T) { binary := buildBinary(t) moduleDir := writeModule(t, module{ GoMod: []string{ @@ -350,9 +351,12 @@ func TestCLIEmbeddedNilawayReadsRequiredAnalyzerFlag(t *testing.T) { require.NoError(t, err, output) assert.Empty(t, output) - output, err = runConstable(t, binary, moduleDir) - require.Error(t, err) - assert.Contains(t, output, "Potential nil panic detected") + // The default covers the main module, and every package keeps the report. + for _, args := range [][]string{{}, {"-nilaway.include-all-pkgs"}} { + output, err = runConstable(t, binary, moduleDir, args...) + require.Error(t, err, "args=%v output=%s", args, output) + assert.Contains(t, output, "Potential nil panic detected", "args=%v", args) + } } func TestCLIEmbeddedAnalyzersSkipGeneratedFiles(t *testing.T) { diff --git a/hk.pkl b/hk.pkl index 69f903d..bd3cd35 100644 --- a/hk.pkl +++ b/hk.pkl @@ -16,12 +16,9 @@ exclude = List("**/testdata/**") // This repository develops constable, so every hook runs the working tree's // build instead of the installed release. -// nilaway is part of constable now, and it reads every dependency unless it is -// told which module to analyze. Without the flag, a hook spends seconds and -// gigabytes on the standard library. local const constableDev = (Extra.constable) { - check = "go run ./cmd/constable -nilaway_config.include-pkgs=$(go list -m) ./..." - fix = "go run ./cmd/constable -nilaway_config.include-pkgs=$(go list -m) -fix ./..." + check = "go run ./cmd/constable ./..." + fix = "go run ./cmd/constable -fix ./..." } // The templates build the check, fix, pre-commit, and commit-msg hooks, and diff --git a/internal/analysis/nilaway/nilaway.go b/internal/analysis/nilaway/nilaway.go index c6cc35e..04422b1 100644 --- a/internal/analysis/nilaway/nilaway.go +++ b/internal/analysis/nilaway/nilaway.go @@ -6,17 +6,29 @@ package nilaway import ( + "flag" + "sort" + "strings" + "go.uber.org/nilaway" nilawayconfig "go.uber.org/nilaway/config" + "golang.org/x/tools/go/packages" "github.com/nnutter/constable/internal/generated" ) // Analyzer reports a nil value that flows to a dereference. It is the upstream -// analyzer with constable's generated-file policy applied. +// analyzer with constable's generated-file policy applied and with the +// analyzed packages limited to the module that constable runs on. var Analyzer = generated.Skip(nilaway.Analyzer) +// includeAllPkgs keeps nilaway's own default, every package that it loads. +var includeAllPkgs bool + func init() { + Analyzer.Flags.BoolVar(&includeAllPkgs, "include-all-pkgs", false, + "analyze every package that nilaway loads, including dependencies and the standard library") + // nilaway pretty-prints a multi-line explanation by default, with color // when the terminal supports it. constable prints one diagnostic per line // for editors and hooks, so the default is off. A flag still turns it on. @@ -24,3 +36,71 @@ func init() { _ = flag.Value.Set("false") } } + +// UseMainModules limits nilaway to the main modules of the packages that the +// driver loaded, unless the command line chose the packages itself. +// +// NilAway reads every package that it loads by default, which costs seconds +// and gigabytes and reports flows that the analyzed repository cannot fix. Its +// -include-pkgs flag takes package path prefixes, and the module path of a +// root covers every package of that module. +// +// UseMainModules is a driver.Hooks.AfterLoad step: the packages and their +// modules exist only after the driver loads them. +func UseMainModules(roots []*packages.Package) { + if includeAllPkgs { + return + } + + chosen := nilawayconfig.Analyzer.Flags.Lookup(nilawayconfig.IncludePkgsFlag) + if chosen == nil { + return + } + if isChosen(chosen) { + return + } + + modules := mainModules(roots) + if len(modules) == 0 { + return + } + _ = chosen.Value.Set(strings.Join(modules, ",")) +} + +// mainModules returns the sorted paths of the main modules of roots. +// +// A root outside a module, such as a package of the standard library that a +// pattern names, contributes nothing. +func mainModules(roots []*packages.Package) []string { + var paths []string + seen := make(map[string]bool) + for _, root := range roots { + if isNilRoot(root) || isNilModule(root.Module) || isDependencyModule(root.Module) || isSeen(seen, root.Module.Path) { + continue + } + seen[root.Module.Path] = true + paths = append(paths, root.Module.Path) + } + sort.Strings(paths) + return paths +} + +func isChosen(f *flag.Flag) bool { + return f.Value.String() != "" +} + +func isNilRoot(root *packages.Package) bool { + return root == nil +} + +func isNilModule(module *packages.Module) bool { + return module == nil +} + +func isDependencyModule(module *packages.Module) bool { + return !module.Main +} + +func isSeen(seen map[string]bool, path string) bool { + return seen[path] +} diff --git a/internal/analysis/nilaway/nilaway_test.go b/internal/analysis/nilaway/nilaway_test.go index bbec83d..f7b462d 100644 --- a/internal/analysis/nilaway/nilaway_test.go +++ b/internal/analysis/nilaway/nilaway_test.go @@ -3,7 +3,11 @@ package nilaway_test import ( "testing" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/nilaway/config" "golang.org/x/tools/go/analysis/analysistest" + "golang.org/x/tools/go/packages" "github.com/nnutter/constable/internal/analysis/nilaway" ) @@ -11,3 +15,89 @@ import ( func TestAnalyzer(t *testing.T) { analysistest.Run(t, analysistest.TestData(), nilaway.Analyzer, "na") } + +// includePkgs returns the package list that the configuration analyzer read. +func includePkgs(t *testing.T) string { + t.Helper() + + flag := config.Analyzer.Flags.Lookup(config.IncludePkgsFlag) + require.NotNil(t, flag) + return flag.Value.String() +} + +// setIncludePkgs records the value that a test gives the flag, and clears it +// afterwards so that the flag does not leak into another test. +func setIncludePkgs(t *testing.T, value string) { + t.Helper() + + flag := config.Analyzer.Flags.Lookup(config.IncludePkgsFlag) + require.NotNil(t, flag) + require.NoError(t, flag.Value.Set(value)) + t.Cleanup(func() { + require.NoError(t, flag.Value.Set("")) + }) +} + +// setIncludeAllPkgs sets the adapter flag, and restores it afterwards. +func setIncludeAllPkgs(t *testing.T, value string) { + t.Helper() + + require.NoError(t, nilaway.Analyzer.Flags.Set("include-all-pkgs", value)) + t.Cleanup(func() { + require.NoError(t, nilaway.Analyzer.Flags.Set("include-all-pkgs", "false")) + }) +} + +func moduleRoot(path string, main bool) *packages.Package { + return &packages.Package{PkgPath: path + "/pkg", Module: &packages.Module{Path: path, Main: main}} +} + +func TestUseMainModules(t *testing.T) { + setIncludePkgs(t, "") + + nilaway.UseMainModules([]*packages.Package{ + moduleRoot("example.com/m", true), + moduleRoot("example.com/m", true), + moduleRoot("example.com/dep", false), + {PkgPath: "fmt", Module: nil}, + nil, + }) + + assert.Equal(t, "example.com/m", includePkgs(t)) +} + +func TestUseMainModulesCoversEveryMainModule(t *testing.T) { + setIncludePkgs(t, "") + + nilaway.UseMainModules([]*packages.Package{ + moduleRoot("example.com/one", true), + moduleRoot("example.com/two", true), + }) + + assert.Equal(t, "example.com/one,example.com/two", includePkgs(t)) +} + +func TestUseMainModulesKeepsChosenPackages(t *testing.T) { + setIncludePkgs(t, "example.com/chosen") + + nilaway.UseMainModules([]*packages.Package{moduleRoot("example.com/m", true)}) + + assert.Equal(t, "example.com/chosen", includePkgs(t)) +} + +func TestUseMainModulesWithAllPackages(t *testing.T) { + setIncludePkgs(t, "") + setIncludeAllPkgs(t, "true") + + nilaway.UseMainModules([]*packages.Package{moduleRoot("example.com/m", true)}) + + assert.Empty(t, includePkgs(t)) +} + +func TestUseMainModulesWithoutModule(t *testing.T) { + setIncludePkgs(t, "") + + nilaway.UseMainModules([]*packages.Package{{PkgPath: "fmt", Module: nil}}) + + assert.Empty(t, includePkgs(t)) +} diff --git a/internal/driver/driver.go b/internal/driver/driver.go index b5fcc3d..8030248 100644 --- a/internal/driver/driver.go +++ b/internal/driver/driver.go @@ -628,7 +628,8 @@ func outputFinalFixes(original, state map[string][]byte, cwd string, diff bool) } func prepareFixPass(files map[string]*fixFile, original, state map[string][]byte) (map[string][]byte, bool, error) { - next := maps.Clone(state) + next := make(map[string][]byte, len(state)) + maps.Copy(next, state) changed := false for name, file := range files { if err := prepareFixFile(name, file); err != nil { diff --git a/internal/driver/flags_test.go b/internal/driver/flags_test.go index 648fda3..61bf251 100644 --- a/internal/driver/flags_test.go +++ b/internal/driver/flags_test.go @@ -9,6 +9,36 @@ import ( "golang.org/x/tools/go/analysis" ) +// flagUsage returns the usage of a registered flag, or an empty string so that +// the test reports the missing flag instead of a panic. +func flagUsage(t *testing.T, fs *flag.FlagSet, name string) string { + t.Helper() + + f := fs.Lookup(name) + require.NotNil(t, f, "flag %s is not registered", name) + // The nil check states that a flag can be missing, so the analyzer that + // looks for nil dereferences sees the guard that require makes. + if f == nil { + return "" + } + return f.Usage +} + +// flagValue returns the value of a registered flag, or an empty string so that +// the test reports the missing flag instead of a panic. +func flagValue(t *testing.T, fs *flag.FlagSet, name string) string { + t.Helper() + + f := fs.Lookup(name) + require.NotNil(t, f, "flag %s is not registered", name) + // The nil check states that a flag can be missing, so the analyzer that + // looks for nil dereferences sees the guard that require makes. + if f == nil { + return "" + } + return f.Value.String() +} + func newAnalyzer(name string, requires ...*analysis.Analyzer) *analysis.Analyzer { return &analysis.Analyzer{ Name: name, @@ -36,8 +66,8 @@ func TestRegisterFlagsRegistersRequiredAnalyzerFlags(t *testing.T) { assert.Nil(t, fs.Lookup("required"), "a required analyzer has no enable flag") require.NoError(t, fs.Parse([]string{"-required.on", "-required.scope=example.com/m"})) - assert.Equal(t, "true", fs.Lookup("required.on").Value.String()) - assert.Equal(t, "example.com/m", fs.Lookup("required.scope").Value.String()) + assert.Equal(t, "true", flagValue(t, fs, "required.on")) + assert.Equal(t, "example.com/m", flagValue(t, fs, "required.scope")) } func TestRegisterFlagsVisitsAnAnalyzerOnce(t *testing.T) { @@ -62,5 +92,5 @@ func TestRegisterFlagsKeepsAnExistingFlag(t *testing.T) { fs.Bool("analyzer.on", false, "a flag that the driver already defines") registerFlags(fs, []*analysis.Analyzer{analyzer}) - assert.Equal(t, "a flag that the driver already defines", fs.Lookup("analyzer.on").Usage) + assert.Equal(t, "a flag that the driver already defines", flagUsage(t, fs, "analyzer.on")) }