diff --git a/README.md b/README.md index 84ea290..ccfc7c5 100644 --- a/README.md +++ b/README.md @@ -21,6 +21,9 @@ Run `constable` on your Go packages, constable ./... ``` +constable skips files that carry the standard generated-code comment, `// Code generated ... DO NOT EDIT.`. +A diagnostic in such a file is not actionable because the next run of the generator overwrites the file. + ## Example This function is reported because it mutates through a pointer parameter, diff --git a/internal/analysis/complexity/complexity.go b/internal/analysis/complexity/complexity.go index 2b7c0e8..79b08eb 100644 --- a/internal/analysis/complexity/complexity.go +++ b/internal/analysis/complexity/complexity.go @@ -6,6 +6,7 @@ import ( "golang.org/x/tools/go/analysis" + "github.com/nnutter/constable/internal/generated" "github.com/nnutter/constable/internal/report" ) @@ -41,6 +42,10 @@ func isLogicalOr(op token.Token) bool { func run(pass *analysis.Pass) (any, error) { for _, file := range pass.Files { + if generated.IsGenerated(file) { + continue + } + for _, decl := range file.Decls { funcDecl, ok := decl.(*ast.FuncDecl) if !isFuncDecl(ok) || isNilBody(funcDecl.Body) { diff --git a/internal/analysis/complexity/testdata/src/c/zz_generated.go b/internal/analysis/complexity/testdata/src/c/zz_generated.go new file mode 100644 index 0000000..d2cba90 --- /dev/null +++ b/internal/analysis/complexity/testdata/src/c/zz_generated.go @@ -0,0 +1,40 @@ +// Code generated by hand for the test. DO NOT EDIT. + +package c + +// GeneratedOverLimit has a complexity over the limit, but a person cannot +// fix a generated file, so the analyzer ignores it. +func GeneratedOverLimit(x int) int { + n := 0 + if x > 0 { + n++ + } + if x > 1 { + n++ + } + if x > 2 { + n++ + } + if x > 3 { + n++ + } + if x > 4 { + n++ + } + if x > 5 { + n++ + } + if x > 6 { + n++ + } + if x > 7 { + n++ + } + if x > 8 { + n++ + } + if x > 9 { + n++ + } + return n +} diff --git a/internal/analysis/methodical/fix.go b/internal/analysis/methodical/fix.go index 999f1f3..fc2d5cc 100644 --- a/internal/analysis/methodical/fix.go +++ b/internal/analysis/methodical/fix.go @@ -11,6 +11,8 @@ import ( "strings" "golang.org/x/tools/go/analysis" + + "github.com/nnutter/constable/internal/generated" ) type diagnosticReporter struct { @@ -82,7 +84,7 @@ func readFixFile(pass *analysis.Pass, file *ast.File) *fixFile { return nil } src, err := pass.ReadFile(tf.Name()) - if err != nil || ast.IsGenerated(file) || hasStandaloneComments(pass.Fset, file) { + if err != nil || generated.IsGenerated(file) || hasStandaloneComments(pass.Fset, file) { return nil } base := strings.TrimSuffix(filepath.Base(tf.Name()), ".go") diff --git a/internal/analysis/methodical/methodical.go b/internal/analysis/methodical/methodical.go index e735c75..7517f7d 100644 --- a/internal/analysis/methodical/methodical.go +++ b/internal/analysis/methodical/methodical.go @@ -10,6 +10,7 @@ import ( "golang.org/x/tools/go/analysis" + "github.com/nnutter/constable/internal/generated" "github.com/nnutter/constable/internal/report" ) @@ -63,6 +64,10 @@ 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) { + continue + } + for _, decl := range file.Decls { genDecl, ok := decl.(*ast.GenDecl) if !ok { @@ -102,6 +107,10 @@ 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) { + continue + } + for _, decl := range file.Decls { funcDecl, ok := decl.(*ast.FuncDecl) if !succeeded(ok) || isNilRecv(funcDecl.Recv) { @@ -233,6 +242,10 @@ 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) { + return + } + states := make(map[types.Object]*groupState) for _, decl := range file.Decls { funcDecl, ok := decl.(*ast.FuncDecl) diff --git a/internal/analysis/methodical/methodical_test.go b/internal/analysis/methodical/methodical_test.go index 22a12a5..0801107 100644 --- a/internal/analysis/methodical/methodical_test.go +++ b/internal/analysis/methodical/methodical_test.go @@ -15,3 +15,7 @@ func TestAnalyzer(t *testing.T) { func TestSuggestedFixes(t *testing.T) { analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), methodical.Analyzer, "fix", "generic") } + +func TestAnalyzerSkipsGeneratedFiles(t *testing.T) { + analysistest.Run(t, analysistest.TestData(), methodical.Analyzer, "g") +} diff --git a/internal/analysis/methodical/testdata/src/g/g.go b/internal/analysis/methodical/testdata/src/g/g.go new file mode 100644 index 0000000..2975bdc --- /dev/null +++ b/internal/analysis/methodical/testdata/src/g/g.go @@ -0,0 +1,6 @@ +package g + +// Hand is a type that a person owns in this file. +type Hand struct{} + +func (h Hand) A() {} diff --git a/internal/analysis/methodical/testdata/src/g/handwritten.go b/internal/analysis/methodical/testdata/src/g/handwritten.go new file mode 100644 index 0000000..1662fc6 --- /dev/null +++ b/internal/analysis/methodical/testdata/src/g/handwritten.go @@ -0,0 +1,7 @@ +package g + +// Methods on a generated type cannot move next to the type, so the analyzer +// leaves them alone even though they are in a handwritten file. +func (g Gen) D() {} + +func (g Gen) C() {} diff --git a/internal/analysis/methodical/testdata/src/g/zz_generated.go b/internal/analysis/methodical/testdata/src/g/zz_generated.go new file mode 100644 index 0000000..dc5f87a --- /dev/null +++ b/internal/analysis/methodical/testdata/src/g/zz_generated.go @@ -0,0 +1,18 @@ +// Code generated by hand for the test. DO NOT EDIT. + +package g + +// Gen is a type that a generator writes. Its methods are unsorted, and the +// methods of Hand sit in the wrong file, but a person cannot edit a generated +// file, so the analyzer ignores everything in it. +type Gen struct{} + +func (g Gen) B() {} + +func (g Gen) A() {} + +func (h Hand) Z() {} + +func generatedHelper() {} + +func (h Hand) Y() {} diff --git a/internal/analysis/nonmutating/nonmutating.go b/internal/analysis/nonmutating/nonmutating.go index 1f7d1e8..e34a0d4 100644 --- a/internal/analysis/nonmutating/nonmutating.go +++ b/internal/analysis/nonmutating/nonmutating.go @@ -10,6 +10,7 @@ import ( "github.com/nnutter/constable/internal/directive" "github.com/nnutter/constable/internal/directive/enum" + "github.com/nnutter/constable/internal/generated" "github.com/nnutter/constable/internal/report" ) @@ -59,6 +60,10 @@ func hasNoArgs(call *ast.CallExpr) bool { func run(pass *analysis.Pass) (any, error) { for _, file := range pass.Files { + if generated.IsGenerated(file) { + continue + } + for _, decl := range file.Decls { funcDecl, ok := decl.(*ast.FuncDecl) if !succeeded(ok) || !hasNonmutatingDirective(funcDecl) || isNilBody(funcDecl.Body) { diff --git a/internal/analysis/nonmutating/testdata/src/a/zz_generated.go b/internal/analysis/nonmutating/testdata/src/a/zz_generated.go new file mode 100644 index 0000000..b7a5527 --- /dev/null +++ b/internal/analysis/nonmutating/testdata/src/a/zz_generated.go @@ -0,0 +1,11 @@ +// Code generated by hand for the test. DO NOT EDIT. + +package a + +// GeneratedMutatesPointer is a violation, but a person cannot fix a generated +// file, so the analyzer ignores it. +// +//constable:nonmutating +func GeneratedMutatesPointer(p *int) { + *p = 1 +} diff --git a/internal/analysis/predicates/predicates.go b/internal/analysis/predicates/predicates.go index b713921..81d58d2 100644 --- a/internal/analysis/predicates/predicates.go +++ b/internal/analysis/predicates/predicates.go @@ -8,6 +8,7 @@ import ( "golang.org/x/tools/go/analysis" + "github.com/nnutter/constable/internal/generated" "github.com/nnutter/constable/internal/report" ) @@ -54,6 +55,10 @@ func isBoolName(ident *ast.Ident) bool { func run(pass *analysis.Pass) (any, error) { for _, file := range pass.Files { + if generated.IsGenerated(file) { + continue + } + ast.Inspect(file, func(node ast.Node) bool { expr, ok := node.(*ast.BinaryExpr) if !succeeded(ok) || !isLogicalExpr(expr) { diff --git a/internal/analysis/predicates/testdata/src/p/zz_generated.go b/internal/analysis/predicates/testdata/src/p/zz_generated.go new file mode 100644 index 0000000..bd8602e --- /dev/null +++ b/internal/analysis/predicates/testdata/src/p/zz_generated.go @@ -0,0 +1,9 @@ +// Code generated by hand for the test. DO NOT EDIT. + +package p + +// GeneratedCompoundConditional is a violation, but a person cannot fix a +// generated file, so the analyzer ignores it. +func GeneratedCompoundConditional(a, b bool) bool { + return a && b +} diff --git a/internal/analysis/testify/testdata/src/t/zz_generated.go b/internal/analysis/testify/testdata/src/t/zz_generated.go new file mode 100644 index 0000000..386e54a --- /dev/null +++ b/internal/analysis/testify/testdata/src/t/zz_generated.go @@ -0,0 +1,11 @@ +// Code generated by hand for the test. DO NOT EDIT. + +package t + +import "testing" + +// GeneratedUsesError is a violation, but a person cannot fix a generated +// file, so the analyzer ignores it. +func GeneratedUsesError(t *testing.T) { + t.Error("boom") +} diff --git a/internal/analysis/testify/testify.go b/internal/analysis/testify/testify.go index 48623a5..107636b 100644 --- a/internal/analysis/testify/testify.go +++ b/internal/analysis/testify/testify.go @@ -6,6 +6,7 @@ import ( "golang.org/x/tools/go/analysis" + "github.com/nnutter/constable/internal/generated" "github.com/nnutter/constable/internal/report" ) @@ -46,6 +47,10 @@ var testifyPackages = map[string]string{ func run(pass *analysis.Pass) (any, error) { for _, file := range pass.Files { + if generated.IsGenerated(file) { + continue + } + ast.Inspect(file, func(node ast.Node) bool { call, ok := node.(*ast.CallExpr) if !ok { diff --git a/internal/generated/generated.go b/internal/generated/generated.go new file mode 100644 index 0000000..d716071 --- /dev/null +++ b/internal/generated/generated.go @@ -0,0 +1,15 @@ +// Package generated recognizes Go files that a tool writes and that a person +// must not edit. +package generated + +import "go/ast" + +// IsGenerated reports whether file carries the standard generated-code +// comment, "// Code generated DO NOT EDIT.". +// +// 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 { + return ast.IsGenerated(file) +} diff --git a/internal/generated/generated_test.go b/internal/generated/generated_test.go new file mode 100644 index 0000000..42fd15f --- /dev/null +++ b/internal/generated/generated_test.go @@ -0,0 +1,68 @@ +package generated_test + +import ( + "go/ast" + "go/parser" + "go/token" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/nnutter/constable/internal/generated" +) + +func parse(t *testing.T, src string) *ast.File { + t.Helper() + + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, "src.go", src, parser.ParseComments) + require.NoError(t, err) + + return file +} + +func TestIsGenerated(t *testing.T) { + tests := []struct { + name string + src string + want bool + }{ + { + name: "standard comment before package clause", + src: "// Code generated by mockgen. DO NOT EDIT.\n\npackage p\n", + want: true, + }, + { + name: "comment in a block with other comments", + src: "// Package doc.\n\n// Code generated by stringer. DO NOT EDIT.\n\npackage p\n", + want: true, + }, + { + name: "no comment", + src: "package p\n", + want: false, + }, + { + name: "comment without the marker", + src: "// Code generated by hand.\n\npackage p\n", + want: false, + }, + { + name: "marker without the trailing period", + src: "// Code generated by hand. DO NOT EDIT\n\npackage p\n", + want: false, + }, + { + name: "marker after the package clause", + src: "package p\n\n// Code generated by hand. DO NOT EDIT.\n", + want: false, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + assert.Equal(t, test.want, generated.IsGenerated(parse(t, test.src))) + }) + } +} diff --git a/skills/constable/SKILL.md b/skills/constable/SKILL.md index fd45b90..c415843 100644 --- a/skills/constable/SKILL.md +++ b/skills/constable/SKILL.md @@ -152,4 +152,7 @@ It does not by itself guarantee that a function has no I/O, global state changes Calls to helper functions, interface methods, function values, and other indirect mutation paths may not be modeled. The analyzer currently recognizes only the `nonmutating` directive. +The analyzers skip files that carry the standard generated-code comment, `// Code generated ... DO NOT EDIT.`. +Edit such a file through its generator, not by hand. + When behavior is uncertain, inspect the diagnostic and the implementation rather than assuming that a clean run proves complete immutability.