diff --git a/docs/community/contributing/index.md b/docs/community/contributing/index.md index ec8f935ec..8d1070305 100644 --- a/docs/community/contributing/index.md +++ b/docs/community/contributing/index.md @@ -105,10 +105,47 @@ Key conventions: ## Lint rules `task lint` runs the shared and project-specific cops selected in `lint/main.go`. -Reusable checks come from [rubocop-go](https://github.com/dgageot/rubocop-go/blob/92be797454c8ebde41b9f1eb084be3535cfef668/docs/shared-cops.md) +Reusable checks come from [rubocop-go](https://github.com/dgageot/rubocop-go/blob/202b67b66167808e75285d0666f880a2bd4b1144/docs/shared-cops.md) (pinned in `go.mod`); project-specific checks and frozen-config exclusions stay in `lint/`. Cop IDs and `//rubocop:disable` annotations are unchanged. Add shared checks by their -constructors, not by enabling the entire upstream catalog. +constructors, not by enabling the entire upstream catalog. 34 of the 36 shared +opt-in cops are currently selected. `Lint/ContextFirstParameter` and +`Lint/NoContextField` remain disabled: existing public constructor signatures and +intentionally owned lifecycle or telemetry contexts would require suppressions or +unrelated refactoring. + +`Lint/HTTPRequestWithContext` enforces contextual HTTP request construction outside +frozen config versions. It inspects production files only; matching is syntactic, +so aliases and shadowing require care. `Lint/NoFatalOutsideMain` reserves +`log.Fatal*` for package main; tests are exempt. + +The following modernization cops inspect resolved production, internal/external +tests, and test-only packages, excluding generated files and frozen configs: + +- `Lint/MapsCopy` and `Lint/MapsClone` suggest plain entry-copy loops and nil-safe + shallow-copy helpers. Preserve merge order, destination initialization, named + types, and nil results; the two cops can report overlapping suggestions. +- `Lint/SlicesContains` and `Lint/SlicesEqual` suggest simple comparable-element + membership and equality helpers. Preserve nil-sensitive guards and comparison + behavior, including NaNs and interface-comparison panics. +- `Lint/SplitSeq` suggests lazy `strings.SplitSeq` or `SplitAfterSeq` for direct + value-only ranges. Preserve input/separator evaluation and empty/trailing fields. +- `Lint/SortedMapKeys` suggests `slices.Sorted(maps.Keys(m))` for adjacent ascending + integer/string key collection. Review nil/empty results and downstream capacity + contracts; this is not an allocation-performance guarantee. +- `Lint/WaitGroupGo` suggests reviewing adjacent `Add(1)`/`go`/`defer Done()`. + The callback must not let a panic escape; preserve captures and completion timing. +- `Lint/ErrorsAsType` suggests fresh targets consumed only on successful matches. + Custom `As` methods can retain target pointers; do not mechanically replace them. +- `Lint/HTTPTestRequestWithContext` suggests immediate test-request context + attachment with constant method/target and nil body. Keep non-nil contexts and + preserve context evaluation versus request validation order. + +These checks follow active build constraints, skip ill-typed candidate packages, +and gate suggestions on the target module/file's Go and stdlib versions, not the +linter toolchain. Suggestions require behavior review, not automatic rewriting. +Rubocop exclusions and suppressions do not configure golangci-lint's overlapping +`modernize` checks. `Lint/FieldsSeq` flags `strings.Fields` slices used only for one value-only range, including loops @@ -151,6 +188,9 @@ Generated files and frozen config versions are excluded. Only constant or local identifier inputs are matched; compound conditions, intervening work, and effectful expressions are excluded. Preserve original values, assignment scope, and evaluation order when introducing the cut result. +Both cut cops also cover immediate `bytes.HasPrefix`/`HasSuffix` plus matching +trimming or slicing on local unnamed byte slices. Preserve nilness, capacity, and +backing-array aliasing. `Lint/NewExpr` flags a fresh local declared only to return its address, recommending `new(expr)` instead. It requires the declaration and `return &x` to be adjacent, the diff --git a/go.mod b/go.mod index 2a912d09e..dc7ac98e1 100644 --- a/go.mod +++ b/go.mod @@ -33,7 +33,7 @@ require ( github.com/clipperhouse/uax29/v2 v2.7.0 github.com/coder/acp-go-sdk v0.13.5 github.com/creack/pty v1.1.24 - github.com/dgageot/rubocop-go v1.0.1-0.20260925155715-92be797454c8 + github.com/dgageot/rubocop-go v1.0.1-0.20261009115150-202b67b66167 github.com/docker/aijson v0.1.0 github.com/docker/cli v29.8.1+incompatible github.com/docker/go-units v0.5.0 diff --git a/go.sum b/go.sum index 3394efbec..a60c544ca 100644 --- a/go.sum +++ b/go.sum @@ -154,8 +154,8 @@ github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSs github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc h1:U9qPSI2PIWSS1VwoXQT9A3Wy9MM3WgvqSxFWenqJduM= github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= -github.com/dgageot/rubocop-go v1.0.1-0.20260925155715-92be797454c8 h1:tgXLZWkc5PKG38zmUM2+bY0OU8BSQ3v2rr8XdRNd6l8= -github.com/dgageot/rubocop-go v1.0.1-0.20260925155715-92be797454c8/go.mod h1:szP4Puq4DW5fbuTd3EZQH/NsN1VqMHPSI5JcKKZN21Y= +github.com/dgageot/rubocop-go v1.0.1-0.20261009115150-202b67b66167 h1:rdZvdv690da37V6udEtybQNTrJ7eZ8jercvtrlOBLxc= +github.com/dgageot/rubocop-go v1.0.1-0.20261009115150-202b67b66167/go.mod h1:szP4Puq4DW5fbuTd3EZQH/NsN1VqMHPSI5JcKKZN21Y= github.com/dgageot/ultraviolet v0.0.0-20260313154905-9451997d56b6 h1:88fWkkjwzuI4tRTqadbJIbA9O+gO67oyu+2OpHHuuT8= github.com/dgageot/ultraviolet v0.0.0-20260313154905-9451997d56b6/go.mod h1:SQpCTRNBtzJkwku5ye4S3HEuthAlGy2n9VXZnWkEW98= github.com/distribution/reference v0.6.0 h1:0IXCQ5g4/QMHHkarYzh5l+u8T3t73zM5QvfrDyIgxBk= diff --git a/lint/additional_shared_cops_test.go b/lint/additional_shared_cops_test.go new file mode 100644 index 000000000..e8f02c551 --- /dev/null +++ b/lint/additional_shared_cops_test.go @@ -0,0 +1,149 @@ +package main + +import ( + "bytes" + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/dgageot/rubocop-go/config" + "github.com/dgageot/rubocop-go/coptest" + "github.com/dgageot/rubocop-go/prog" + "github.com/dgageot/rubocop-go/runner" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestAdditionalSharedFileCopScopes(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name, src string + frozen bool + }{ + {"HTTPRequestWithContext", `import "net/http" +func f() { _, _ = http.NewRequest("GET", "/", nil) }`, false}, + {"NoFatalOutsideMain", `import "log" +func f() { log.Fatal("failed") }`, true}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + c := registeredSharedCop(t, cops, "Lint/"+tc.name) + for _, path := range []string{"pkg/config/v0/sample.go", "pkg/config/v15/sample.go", "pkg/config/latest/sample.go", "notpkg/config/v0/sample.go", "pkg/generated/sample.go", "pkg/sample_test.go"} { + src := "package p\n" + tc.src + if strings.Contains(path, "generated") { + src = "// Code generated by fixture; DO NOT EDIT.\n" + src + } + want := 1 + if strings.HasSuffix(path, "_test.go") || (!tc.frozen && frozenConfigPath.MatchString(path)) { + want = 0 + } + assert.Len(t, coptest.RunNamed(t, c, path, src), want, path) + } + if tc.name == "NoFatalOutsideMain" { + assert.Empty(t, coptest.Run(t, c, "package main\n"+tc.src)) + } + }) + } +} + +func TestAdditionalSharedProgramCopsRunner(t *testing.T) { + for _, tc := range []struct { + name, minimum, older, src string + }{ + {"ErrorsAsType", "1.26", "1.25", `import "errors" +type E struct{} +func (*E) Error() string { return "e" } +func f(err *E) { var e *E; if errors.As(err, &e) { println(e) } }`}, + {"MapsCopy", "1.21", "1.20", `func f(dst, src map[string]int) { for k, v := range src { dst[k] = v } }`}, + {"MapsClone", "1.21", "1.20", `func f(src map[string]int) map[string]int { + if src == nil { return nil }; dst := make(map[string]int, len(src)) + for k, v := range src { dst[k] = v }; return dst +}`}, + {"SlicesContains", "1.21", "1.20", `func f(xs []int, n int) bool { + for _, v := range xs { if v == n { return true } }; return false +}`}, + {"SlicesEqual", "1.21", "1.20", `func f(a, b []int) bool { + if len(a) != len(b) { return false } + for i := range a { if a[i] != b[i] { return false } }; return true +}`}, + {"SplitSeq", "1.24", "1.23", `import "strings" +func f(s string) { for _, part := range strings.Split(s, "/") { println(part) } }`}, + {"SortedMapKeys", "1.23", "1.22", `import "sort" +func f(src map[string]int) []string { + var keys []string; for k := range src { keys = append(keys, k) }; sort.Strings(keys); return keys +}`}, + {"WaitGroupGo", "1.25", "1.24", `import "sync" +func f() { var wg sync.WaitGroup; wg.Add(1); go func() { defer wg.Done(); println("work") }(); wg.Wait() }`}, + {"HTTPTestRequestWithContext", "1.23", "1.22", `import ("context"; "net/http"; "net/http/httptest") +func f(ctx context.Context) *http.Request { return httptest.NewRequest("GET", "/", nil).WithContext(ctx) }`}, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + t.Chdir(dir) + id := "Lint/" + tc.name + var want []string + for _, path := range []string{ + "pkg/config/v0/sample.go", "pkg/config/v15/sample.go", "pkg/config/latest/sample.go", + "pkg/config/version/sample.go", "notpkg/config/v0/sample.go", "generated/sample.go", + "internal/sample_test.go", "external/sample_test.go", "testonly/sample_test.go", + "inactive/sample.go", "oldlanguage/sample.go", "illtyped/sample_test.go", + } { + src := "package p\n" + tc.src + switch { + case strings.HasPrefix(path, "generated/"): + src = "// Code generated by fixture; DO NOT EDIT.\n" + src + case strings.HasPrefix(path, "external/"): + src = strings.Replace(src, "package p", "package p_test", 1) + case strings.HasPrefix(path, "inactive/"): + src = "//go:build lint_fixture_disabled\n\n" + src + case strings.HasPrefix(path, "oldlanguage/"): + if tc.name != "SplitSeq" { + continue + } + src = "//go:build go1.22\n\n" + src + case strings.HasPrefix(path, "illtyped/"): + src += "\nvar _ = missing\n" + } + require.NoError(t, os.MkdirAll(filepath.Dir(path), 0o700)) + require.NoError(t, os.WriteFile(path, []byte(src), 0o600)) + if strings.HasPrefix(path, "internal/") || strings.HasPrefix(path, "external/") || strings.HasPrefix(path, "illtyped/") { + require.NoError(t, os.WriteFile(filepath.Join(filepath.Dir(path), "empty.go"), []byte("package p\n"), 0o600)) + } + if strings.HasPrefix(path, "pkg/config/latest/") || strings.HasPrefix(path, "pkg/config/version/") || strings.HasPrefix(path, "notpkg/") || (strings.HasSuffix(path, "_test.go") && !strings.HasPrefix(path, "illtyped/")) { + want = append(want, path) + } + } + cfg := config.DefaultConfig() + cfg.Cops[id] = config.CopConfig{Severity: "warning"} + c := registeredSharedCop(t, programCops, id) + for _, version := range []string{tc.older, tc.minimum, "1.27"} { + require.NoError(t, os.WriteFile("go.mod", []byte("module example.test\n\ngo "+version+"\n"), 0o600)) + var output bytes.Buffer + r := runner.New(nil, cfg, &output).WithProgramCops([]prog.Cop{c}) + r.Reporter = runner.NewJSONReporter(&output) + count, err := r.Run([]string{"."}) + require.NoError(t, err) + if version == tc.older { + assert.Zero(t, count, output.String()) + continue + } + assert.Equal(t, len(want), count, output.String()) + var report struct { + Offenses []struct{ Cop, File, Severity string } + } + require.NoError(t, json.Unmarshal(output.Bytes(), &report)) + var got []string + for _, offense := range report.Offenses { + assert.Equal(t, id, offense.Cop) + assert.Equal(t, "warning", offense.Severity) + file, err := filepath.Rel(dir, offense.File) + require.NoError(t, err) + got = append(got, filepath.ToSlash(file)) + } + assert.ElementsMatch(t, want, got, version) + } + }) + } +} diff --git a/lint/main.go b/lint/main.go index 1a64224df..dc9fa844e 100644 --- a/lint/main.go +++ b/lint/main.go @@ -30,6 +30,8 @@ var cops = []cop.Cop{ HookConfigSync, HookBuiltinsRegistered, HookBuiltinsDocumented, + rubocops.NewLintHTTPRequestWithContext(cop.WithScope(outsideFrozenConfig)), + rubocops.NewLintNoFatalOutsideMain(), rubocops.NewLintSlogContextual(), ToolArgumentsViaAIJSON, rubocops.NewLintConstructorPurity(), @@ -51,6 +53,15 @@ var cops = []cop.Cop{ // programCops lists whole-program, inter-procedural cops. These run once over // the entire loaded program rather than once per file. var programCops = []prog.Cop{ + rubocops.NewLintErrorsAsType(cop.WithScope(outsideFrozenConfig)), + rubocops.NewLintMapsCopy(cop.WithScope(outsideFrozenConfig)), + rubocops.NewLintMapsClone(cop.WithScope(outsideFrozenConfig)), + rubocops.NewLintSlicesContains(cop.WithScope(outsideFrozenConfig)), + rubocops.NewLintSlicesEqual(cop.WithScope(outsideFrozenConfig)), + rubocops.NewLintSplitSeq(cop.WithScope(outsideFrozenConfig)), + rubocops.NewLintSortedMapKeys(cop.WithScope(outsideFrozenConfig)), + rubocops.NewLintWaitGroupGo(cop.WithScope(outsideFrozenConfig)), + rubocops.NewLintHTTPTestRequestWithContext(cop.WithScope(outsideFrozenConfig)), withTypedFiles(rubocops.NewLintSlicesClone, cop.WithScope(outsideFrozenConfig)), rubocops.NewLintSortStableFunc(cop.WithScope(outsideFrozenConfig)), rubocops.NewLintPointerHelper(cop.WithScope(outsideFrozenConfig)), diff --git a/lint/shared_cops_test.go b/lint/shared_cops_test.go index 012040c22..edf034062 100644 --- a/lint/shared_cops_test.go +++ b/lint/shared_cops_test.go @@ -22,8 +22,8 @@ import ( func TestSharedCopRegistrations(t *testing.T) { t.Parallel() - require.Len(t, cops, 29) - require.Len(t, programCops, 19) + require.Len(t, cops, 31) + require.Len(t, programCops, 28) counts := make(map[string]int) for _, c := range cops { counts[c.Name()]++ @@ -31,11 +31,15 @@ func TestSharedCopRegistrations(t *testing.T) { for _, c := range programCops { counts[c.Name()]++ } - assert.Len(t, counts, 48) + assert.Len(t, counts, 59) + for _, name := range []string{"Lint/ContextFirstParameter", "Lint/NoContextField"} { + assert.NotContains(t, counts, name) + } for name, count := range counts { assert.Equal(t, 1, count, name) } for _, name := range []string{ + "HTTPRequestWithContext", "NoFatalOutsideMain", "SlogContextual", "ConstructorPurity", "ConstructorNetworkIO", "WrapErrors", "ErrorStringMatching", "DeferMutexUnlock", "NewExpr", "NoStdoutInLibraries", "ConstructorCommandExec", } { @@ -45,6 +49,8 @@ func TestSharedCopRegistrations(t *testing.T) { "PointerHelper", "ReflectFields", "StdlibUUID", "URLClone", "JSONMarshalWrite", "BenchmarkLoop", "SplitTrimJoin", "FieldsSeq", "StreamCloseSafety", "SlicesClone", "CutPrefix", "CutSuffix", "FieldsSeqLookup", "SortStableFunc", + "ErrorsAsType", "MapsCopy", "MapsClone", "SlicesContains", "SlicesEqual", + "SplitSeq", "SortedMapKeys", "WaitGroupGo", "HTTPTestRequestWithContext", } { registeredSharedCop(t, programCops, "Lint/"+name) }