diff --git a/README.md b/README.md index 1df58bc..9f0441d 100644 --- a/README.md +++ b/README.md @@ -120,6 +120,39 @@ A move is conservative, because the analyzer must be sure that the result compil A fix writes only files that Git tracks and that have no changes, so `git checkout` recovers a fix that went wrong. Diagnostics that remain after the fixes are printed, and the command exits unsuccessfully. +## Embedded analyzers + +constable runs four analyzers from other modules: `errcheck`, `sumtype`, `testifylint`, and `nilaway`. +Each keeps its upstream name, so its flags keep the upstream prefix, such as `-errcheck.blank`, `-sumtype.include-shared-interfaces`, and `-testifylint.disable-all`. +Run `constable -flags` for the complete list. +Each embedded analyzer skips files that carry the generated-code comment, the same as the constable analyzers. + +### errcheck + +Reports an error that the code discards. + +### sumtype + +Reports a type switch over a sum type that omits a variant. +Mark the interface with `//sumtype:decl` and the switch must cover every variant. + +### testifylint + +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: + +```sh +constable -nilaway_config.include-pkgs=$(go list -m) ./... +``` + +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. + ## Development Run all fixes, diff --git a/cmd/constable/main.go b/cmd/constable/main.go index 00c45b7..dc53b1d 100644 --- a/cmd/constable/main.go +++ b/cmd/constable/main.go @@ -4,6 +4,7 @@ import ( "github.com/nnutter/constable/internal/analysis/complexity" "github.com/nnutter/constable/internal/analysis/errcheck" "github.com/nnutter/constable/internal/analysis/methodical" + "github.com/nnutter/constable/internal/analysis/nilaway" "github.com/nnutter/constable/internal/analysis/nonmutating" "github.com/nnutter/constable/internal/analysis/predicates" "github.com/nnutter/constable/internal/analysis/sumtype" @@ -28,5 +29,6 @@ func main() { errcheck.Analyzer, sumtype.Analyzer, testifylint.Analyzer, + nilaway.Analyzer, ) } diff --git a/cmd/constable/main_test.go b/cmd/constable/main_test.go index 49f77d3..c3d37ac 100644 --- a/cmd/constable/main_test.go +++ b/cmd/constable/main_test.go @@ -301,6 +301,101 @@ func TestCLIComplexityPass(t *testing.T) { assert.Empty(t, output) } +func TestCLIEmbeddedAnalyzerFlags(t *testing.T) { + binary := buildBinary(t) + + command := exec.Command(binary, "-flags") + output, err := command.CombinedOutput() + require.NoError(t, err, string(output)) + + // A flag of a required analyzer must reach the command line, because the + // analyzer that constable embeds reads its configuration from it. + for _, name := range []string{ + "errcheck", + "errcheck.blank", + "sumtype", + "sumtype.include-shared-interfaces", + "testifylint", + "testifylint.disable-all", + "nilaway", + "nilaway_config.include-pkgs", + } { + assert.Contains(t, string(output), `"`+name+`"`) + } +} + +func TestCLIEmbeddedNilawayReadsRequiredAnalyzerFlag(t *testing.T) { + binary := buildBinary(t) + moduleDir := writeModule(t, module{ + GoMod: []string{ + "module example.com/nil", + "", + "go 1.26", + }, + Files: map[string][]string{"a.go": { + "package nil", + "", + "type T struct{ X int }", + "", + "func F() {", + " var t *T", + " _ = t.X", + "}", + }}, + }) + + // The package is out of scope, so nilaway must stay silent. This holds + // only when the flag of the required configuration analyzer is parsed. + output, err := runConstable(t, binary, moduleDir, "-nilaway_config.include-pkgs=example.com/other") + 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") +} + +func TestCLIEmbeddedAnalyzersSkipGeneratedFiles(t *testing.T) { + binary := buildBinary(t) + moduleDir := writeModule(t, module{ + GoMod: []string{ + "module example.com/generated", + "", + "go 1.26", + }, + Files: map[string][]string{"gen.go": { + "// Code generated by hand. DO NOT EDIT.", + "", + "package generated", + "", + "import \"os\"", + "", + "//sumtype:decl", + "type Shape interface{ shape() }", + "", + "type Circle struct{}", + "", + "func (*Circle) shape() {}", + "", + "type Square struct{}", + "", + "func (*Square) shape() {}", + "", + "func F(s Shape) {", + "\t_ = os.Remove(\"x\")", + "\tswitch s.(type) {", + "\tcase *Circle:", + "\t}", + "}", + }}, + }) + + output, err := runConstable(t, binary, moduleDir) + + require.NoError(t, err, output) + assert.Empty(t, output) +} + func buildBinary(t *testing.T) string { t.Helper() diff --git a/go.mod b/go.mod index 67a2278..f369518 100644 --- a/go.mod +++ b/go.mod @@ -7,12 +7,14 @@ require ( github.com/kisielk/errcheck v1.30.0 github.com/nnutter/testifylint v0.0.1 github.com/stretchr/testify v1.12.1 + go.uber.org/nilaway v0.0.0-20260918162853-acb8859b9031 golang.org/x/tools v0.52.0 ) require ( - github.com/google/go-cmp v0.7.0 // indirect + github.com/klauspost/compress v1.20.0 // indirect go.yaml.in/yaml/v3 v3.0.5 // indirect + golang.org/x/exp/typeparams v0.0.0-20260908205506-85c1c2202aba // indirect golang.org/x/mod v0.42.0 // indirect golang.org/x/sync v0.24.0 // indirect ) diff --git a/go.sum b/go.sum index 642c552..ea37d68 100644 --- a/go.sum +++ b/go.sum @@ -4,12 +4,22 @@ github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8= github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX3N/iU= github.com/kisielk/errcheck v1.30.0 h1:ZIxNDAfAd4larXHTceoZP/dYPAB90dHg1OYhA2sVh5k= github.com/kisielk/errcheck v1.30.0/go.mod h1:uiCAInmFuyOje4VDdbQSP9mgqf5gORYSXrDf19XilsI= +github.com/klauspost/compress v1.20.0 h1:a3C1ke2ohxFymNlb2HWAHjDeKCI90scRskErZkR0ezA= +github.com/klauspost/compress v1.20.0/go.mod h1:LUdAzn7YLVvxLpc7y3V1m40wESHTgc1422pwwBSKYuI= github.com/nnutter/testifylint v0.0.1 h1:QhFq2HgC7LxnaG5Z1Gi4aYN42y+fywfMY+e9xTYe0WM= github.com/nnutter/testifylint v0.0.1/go.mod h1:DvNLVJQrA3YfswHJxaGzitfYt2gxFzlIs29buaH8sP4= +github.com/stretchr/objx v0.5.3 h1:jmXUvGomnU1o3W/V5h2VEradbpJDwGrzugQQvL0POH4= +github.com/stretchr/objx v0.5.3/go.mod h1:rDQraq+vQZU7Fde9LOZLr8Tax6zZvy4kuNKF+QYS+U0= github.com/stretchr/testify v1.12.1 h1:EuwCh5fleGS7H32xRwO3wRGT7DxrDhLAT6FF8MpWDWE= github.com/stretchr/testify v1.12.1/go.mod h1:MDEgiDPPsNp5cuIrHPPCyornHKgEVbtFUmoNlxoYthg= +go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto= +go.uber.org/goleak v1.3.0/go.mod h1:CoHD4mav9JJNrW/WLlf7HGZPjdw8EucARQHekz1X6bE= +go.uber.org/nilaway v0.0.0-20260918162853-acb8859b9031 h1:WRbdrL7q1+PV8w97QZDQUlCnyM0aKHrpMizqHM+reWc= +go.uber.org/nilaway v0.0.0-20260918162853-acb8859b9031/go.mod h1:h/Z4MnNYOaqAjgJglu/yah/YzvCdcJDvDth5TWUqIJc= go.yaml.in/yaml/v3 v3.0.5 h1:N6y/pJk8buWs9NY5ERU2HSMfm+IuD/OtfdAnq6kESPw= go.yaml.in/yaml/v3 v3.0.5/go.mod h1:HVTZu1O7/Vkt2N+BFy8Zza+lnLsABggaTM2ZpNIGuKg= +golang.org/x/exp/typeparams v0.0.0-20260908205506-85c1c2202aba h1:ZoZmFkcuQ289xJilBm9Ck3W+/okrhXc4LUpbtKFQE4k= +golang.org/x/exp/typeparams v0.0.0-20260908205506-85c1c2202aba/go.mod h1:qkMnvgP8C7+xkci04txL2SJFc/0PGqnkf6gs5JUC43E= golang.org/x/mod v0.42.0 h1:ICeL3t2L30ljSPHHxDnR4fqVcBt/zR2zJAb9juavmQg= golang.org/x/mod v0.42.0/go.mod h1:u5fzIrRInkL8MaO1cbRkUHT64v303YIT5KEqpNr7jRs= golang.org/x/sync v0.24.0 h1:iqiDrGoDgyuvAKrhLQthUEzU/R378N78gXbBjxed7oA= diff --git a/hk.pkl b/hk.pkl index bd3cd35..69f903d 100644 --- a/hk.pkl +++ b/hk.pkl @@ -16,9 +16,12 @@ 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 ./..." - fix = "go run ./cmd/constable -fix ./..." + 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 ./..." } // 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 new file mode 100644 index 0000000..c6cc35e --- /dev/null +++ b/internal/analysis/nilaway/nilaway.go @@ -0,0 +1,26 @@ +// Package nilaway adapts the nilaway analyzer to constable. +// +// constable embeds the analyzer from another module instead of reimplementing +// it, so this package holds the places where constable's own policy differs +// from the upstream default. +package nilaway + +import ( + "go.uber.org/nilaway" + nilawayconfig "go.uber.org/nilaway/config" + + "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. +var Analyzer = generated.Skip(nilaway.Analyzer) + +func init() { + // 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. + if flag := nilawayconfig.Analyzer.Flags.Lookup(nilawayconfig.PrettyPrintFlag); flag != nil { + _ = flag.Value.Set("false") + } +} diff --git a/internal/analysis/nilaway/nilaway_test.go b/internal/analysis/nilaway/nilaway_test.go new file mode 100644 index 0000000..bbec83d --- /dev/null +++ b/internal/analysis/nilaway/nilaway_test.go @@ -0,0 +1,13 @@ +package nilaway_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + + "github.com/nnutter/constable/internal/analysis/nilaway" +) + +func TestAnalyzer(t *testing.T) { + analysistest.Run(t, analysistest.TestData(), nilaway.Analyzer, "na") +} diff --git a/internal/analysis/nilaway/testdata/src/na/na.go b/internal/analysis/nilaway/testdata/src/na/na.go new file mode 100644 index 0000000..d81fd57 --- /dev/null +++ b/internal/analysis/nilaway/testdata/src/na/na.go @@ -0,0 +1,8 @@ +package na + +type T struct{ X int } + +func F() { + var t *T + _ = t.X // want `Potential nil panic detected` +} diff --git a/internal/analysis/nilaway/testdata/src/na/zz_generated.go b/internal/analysis/nilaway/testdata/src/na/zz_generated.go new file mode 100644 index 0000000..e1cf6f1 --- /dev/null +++ b/internal/analysis/nilaway/testdata/src/na/zz_generated.go @@ -0,0 +1,8 @@ +// Code generated by hand. DO NOT EDIT. + +package na + +func Generated() { + var t *T + _ = t.X +} diff --git a/internal/driver/driver.go b/internal/driver/driver.go index 229c8e2..47a4c24 100644 --- a/internal/driver/driver.go +++ b/internal/driver/driver.go @@ -124,15 +124,10 @@ func Main(version string, analyzers ...*analysis.Analyzer) { enable := new(triState) flag.Var(enable, a.Name, fmt.Sprintf("enable %q analysis", a.Name)) enabled[a] = enable - - a.Flags.VisitAll(func(f *flag.Flag) { - name := a.Name + "." + f.Name - if flag.Lookup(name) == nil { - flag.Var(f.Value, name, f.Usage) - } - }) } + registerFlags(flag.CommandLine, analyzers) + printFlags := flag.Bool("flags", false, "print analyzer flags in JSON") var showVersion versionValue flag.Var(&showVersion, "V", "print version and exit") @@ -174,6 +169,38 @@ func Main(version string, analyzers ...*analysis.Analyzer) { os.Exit(Run(args, analyzers, opts)) } +// 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 +// analyzer must reach the command line even though only the roots have an +// enable flag. +func registerFlags(fs *flag.FlagSet, analyzers []*analysis.Analyzer) { + seen := make(map[*analysis.Analyzer]bool) + for _, a := range analyzers { + registerAnalyzerFlags(fs, a, seen) + } +} + +// registerAnalyzerFlags registers the flags of a and of the analyzers it +// requires. +func registerAnalyzerFlags(fs *flag.FlagSet, a *analysis.Analyzer, seen map[*analysis.Analyzer]bool) { + if seen[a] { + return + } + seen[a] = true + + a.Flags.VisitAll(func(f *flag.Flag) { + name := a.Name + "." + f.Name + if fs.Lookup(name) == nil { + fs.Var(f.Value, name, f.Usage) + } + }) + + for _, req := range a.Requires { + registerAnalyzerFlags(fs, req, seen) + } +} + func succeeded(ok bool) bool { return ok } @@ -389,9 +416,7 @@ func writeHelp(progname string, analyzers []*analysis.Analyzer, args []string) { for _, a := range analyzers { if a.Name == args[0] { _, _ = fmt.Fprintf(os.Stdout, "%s: %s\n", a.Name, a.Doc) - a.Flags.VisitAll(func(f *flag.Flag) { - _, _ = fmt.Fprintf(os.Stdout, "-%s.%s: %s\n", a.Name, f.Name, f.Usage) - }) + writeAnalyzerFlags(a, make(map[*analysis.Analyzer]bool)) return } } @@ -399,6 +424,23 @@ func writeHelp(progname string, analyzers []*analysis.Analyzer, args []string) { os.Exit(1) } +// writeAnalyzerFlags writes the flags of a and of the analyzers it requires, +// because a required analyzer carries configuration that a reads. +func writeAnalyzerFlags(a *analysis.Analyzer, seen map[*analysis.Analyzer]bool) { + if seen[a] { + return + } + seen[a] = true + + a.Flags.VisitAll(func(f *flag.Flag) { + _, _ = fmt.Fprintf(os.Stdout, "-%s.%s: %s\n", a.Name, f.Name, f.Usage) + }) + + for _, req := range a.Requires { + writeAnalyzerFlags(req, seen) + } +} + // Run loads the packages and applies the analyzers, printing // diagnostics with paths relative to the current working directory. // It returns the appropriate exit code. diff --git a/internal/driver/flags_test.go b/internal/driver/flags_test.go new file mode 100644 index 0000000..648fda3 --- /dev/null +++ b/internal/driver/flags_test.go @@ -0,0 +1,66 @@ +package driver + +import ( + "flag" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/tools/go/analysis" +) + +func newAnalyzer(name string, requires ...*analysis.Analyzer) *analysis.Analyzer { + return &analysis.Analyzer{ + Name: name, + Doc: name, + Run: func(*analysis.Pass) (any, error) { return nil, nil }, + Requires: requires, + } +} + +func TestRegisterFlagsRegistersRequiredAnalyzerFlags(t *testing.T) { + required := newAnalyzer("required") + required.Flags.Bool("on", false, "turn the required analyzer on") + required.Flags.String("scope", "", "limit the packages that the required analyzer reads") + + root := newAnalyzer("root", required) + root.Flags.String("limit", "10", "limit of the root analyzer") + + fs := flag.NewFlagSet("test", flag.ContinueOnError) + registerFlags(fs, []*analysis.Analyzer{root}) + + assert.NotNil(t, fs.Lookup("root.limit")) + assert.NotNil(t, fs.Lookup("required.on")) + assert.NotNil(t, fs.Lookup("required.scope")) + assert.Nil(t, fs.Lookup("root.on"), "a flag of a required analyzer does not belong to the root") + 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()) +} + +func TestRegisterFlagsVisitsAnAnalyzerOnce(t *testing.T) { + shared := newAnalyzer("shared") + shared.Flags.Bool("on", false, "turn the shared analyzer on") + + first := newAnalyzer("first", shared) + second := newAnalyzer("second", shared) + + fs := flag.NewFlagSet("test", flag.ContinueOnError) + registerFlags(fs, []*analysis.Analyzer{first, second}) + + assert.NotNil(t, fs.Lookup("shared.on")) + assert.Nil(t, fs.Lookup("second.on")) +} + +func TestRegisterFlagsKeepsAnExistingFlag(t *testing.T) { + analyzer := newAnalyzer("analyzer") + analyzer.Flags.Bool("on", false, "turn the analyzer on") + + fs := flag.NewFlagSet("test", flag.ContinueOnError) + 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) +}