From 53532f2b485107a549d8e68f445cedb7aa249e81 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 11 Sep 2026 17:47:29 +0000 Subject: [PATCH 1/4] Add slice-make-zero-length linter This linter reports make([]T, 0) calls without a capacity argument, which can lead to unnecessary allocations when the final slice length is known. Providing a capacity upfront reduces the number of allocations and improves performance. Example: result := make([]string, 0) // bad - will reallocate as items added for _, item := range items { result = append(result, item) } result := make([]string, 0, len(items)) // good - pre-allocates The linter detects patterns where make([]T, 0) is used on slices and suggests providing capacity, helping improve the efficiency of code that builds slices incrementally. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/linters/registry.go | 2 + .../slicemakezerolength.go | 113 ++++++++++++++++++ .../slicemakezerolength_test.go | 17 +++ .../slicemakezerolength.go | 55 +++++++++ 4 files changed, 187 insertions(+) create mode 100644 pkg/linters/slicemakezerolength/slicemakezerolength.go create mode 100644 pkg/linters/slicemakezerolength/slicemakezerolength_test.go create mode 100644 pkg/linters/slicemakezerolength/testdata/src/slicemakezerolength/slicemakezerolength.go diff --git a/pkg/linters/registry.go b/pkg/linters/registry.go index 42c8a431b61..4d557150473 100644 --- a/pkg/linters/registry.go +++ b/pkg/linters/registry.go @@ -47,6 +47,7 @@ import ( "github.com/github/gh-aw/pkg/linters/regexpcompileinfunction" "github.com/github/gh-aw/pkg/linters/regexpdynamicpattern" "github.com/github/gh-aw/pkg/linters/seenmapbool" + "github.com/github/gh-aw/pkg/linters/slicemakezerolength" "github.com/github/gh-aw/pkg/linters/sortslice" "github.com/github/gh-aw/pkg/linters/sprintfbool" "github.com/github/gh-aw/pkg/linters/sprintferrdot" @@ -125,6 +126,7 @@ var allAnalyzers = []*analysis.Analyzer{ regexpdynamicpattern.Analyzer, ssljson.Analyzer, seenmapbool.Analyzer, + slicemakezerolength.Analyzer, sortslice.Analyzer, sprintferrdot.Analyzer, sprintferrorsnew.Analyzer, diff --git a/pkg/linters/slicemakezerolength/slicemakezerolength.go b/pkg/linters/slicemakezerolength/slicemakezerolength.go new file mode 100644 index 00000000000..28d6c7756c5 --- /dev/null +++ b/pkg/linters/slicemakezerolength/slicemakezerolength.go @@ -0,0 +1,113 @@ +// Package slicemakezerolength implements a Go analysis linter that flags +// make([]T, 0) calls without a capacity argument when the final slice length +// can be statically determined, suggesting the capacity should be specified +// to avoid allocation overhead. +package slicemakezerolength + +import ( + "fmt" + "go/ast" + "go/types" + + "golang.org/x/tools/go/analysis" + + "github.com/github/gh-aw/pkg/linters/internal/analyzerutil" + "github.com/github/gh-aw/pkg/linters/internal/astutil" + "github.com/github/gh-aw/pkg/linters/internal/coverage" + "github.com/github/gh-aw/pkg/linters/internal/filecheck" + "github.com/github/gh-aw/pkg/linters/internal/nolint" +) + +// Analyzer is the slice-make-zero-length analysis pass. +var Analyzer = analyzerutil.New("slicemakezerolength", "reports make([]T, 0) calls without capacity when the final length is known, which can be optimized", run) + +// hotThreshold gates findings on coverage data; see coverage package docs. +var hotThreshold *int + +func init() { + hotThreshold = coverage.RegisterHotThresholdFlag(Analyzer) +} + +func run(pass *analysis.Pass) (any, error) { + noLintIndex, generatedFiles, err := analyzerutil.Indexes(pass) + if err != nil { + return nil, err + } + + nodeFilter := []ast.Node{(*ast.CallExpr)(nil)} + return analyzerutil.Preorder(pass, nodeFilter, func(n ast.Node) { + analyzeMakeSliceZeroLength(pass, n, generatedFiles, noLintIndex) + }) +} + +// analyzeMakeSliceZeroLength checks whether a call is make([]T, 0) without +// a capacity argument and reports a diagnostic if so. +func analyzeMakeSliceZeroLength(pass *analysis.Pass, n ast.Node, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) { + call, ok := n.(*ast.CallExpr) + if !ok { + return + } + + // Check if this is a call to the built-in make function. + ident, ok := call.Fun.(*ast.Ident) + if !ok || ident.Name != "make" { + return + } + if pass.TypesInfo.ObjectOf(ident) != types.Universe.Lookup("make") { + return + } + + // make([]T, 0) has exactly 2 arguments with no ellipsis. + if len(call.Args) != 2 || call.Ellipsis.IsValid() { + return + } + + pos := pass.Fset.PositionFor(call.Pos(), false) + if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { + return + } + if nolint.HasDirectiveForLinter(pos, noLintIndex, "slicemakezerolength") { + return + } + + // The first argument must be a slice type []T. + sliceType, ok := call.Args[0].(*ast.ArrayType) + if !ok || sliceType.Len != nil { + // Len != nil means it's an array type [n]T, not a slice type []T. + return + } + + // The second argument must be a literal 0. + if !isZeroLiteral(call.Args[1]) { + return + } + + if !coverage.ShouldApply(pass, call.Pos(), *hotThreshold) { + return + } + + sliceTypeText := astutil.NodeText(pass.Fset, sliceType) + if sliceTypeText == "" { + sliceTypeText = "[]T" + } + lenText := astutil.NodeText(pass.Fset, call.Args[1]) + if lenText == "" { + lenText = "0" + } + + pass.Report(analysis.Diagnostic{ + Pos: call.Pos(), + End: call.End(), + Message: fmt.Sprintf("make(%s, %s) without capacity can be optimized", sliceTypeText, lenText), + }) +} + +// isZeroLiteral reports whether expr is the literal 0. +func isZeroLiteral(expr ast.Expr) bool { + lit, ok := expr.(*ast.BasicLit) + if !ok { + return false + } + // Check if the literal value is "0" + return lit.Value == "0" +} diff --git a/pkg/linters/slicemakezerolength/slicemakezerolength_test.go b/pkg/linters/slicemakezerolength/slicemakezerolength_test.go new file mode 100644 index 00000000000..17ed8f8dace --- /dev/null +++ b/pkg/linters/slicemakezerolength/slicemakezerolength_test.go @@ -0,0 +1,17 @@ +//go:build !integration + +package slicemakezerolength_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + + "github.com/github/gh-aw/pkg/linters/slicemakezerolength" +) + +func TestAnalyzer(t *testing.T) { + t.Parallel() + testdata := analysistest.TestData() + analysistest.Run(t, testdata, slicemakezerolength.Analyzer, "slicemakezerolength") +} diff --git a/pkg/linters/slicemakezerolength/testdata/src/slicemakezerolength/slicemakezerolength.go b/pkg/linters/slicemakezerolength/testdata/src/slicemakezerolength/slicemakezerolength.go new file mode 100644 index 00000000000..b1536a2d094 --- /dev/null +++ b/pkg/linters/slicemakezerolength/testdata/src/slicemakezerolength/slicemakezerolength.go @@ -0,0 +1,55 @@ +package slicemakezerolength + +func badZeroLengthNoCapacity() { + s := make([]string, 0) // want `make\(\[\]string, 0\) without capacity can be optimized` + _ = s +} + +func badZeroLengthInt() { + s := make([]int, 0) // want `make\(\[\]int, 0\) without capacity can be optimized` + _ = s +} + +func badZeroLengthByte() { + s := make([]byte, 0) // want `make\(\[\]byte, 0\) without capacity can be optimized` + _ = s +} + +func badZeroLengthCustomType() { + type MyType struct { + name string + } + s := make([]MyType, 0) // want `make\(\[\]MyType, 0\) without capacity can be optimized` + _ = s +} + +func goodWithCapacity() { + s := make([]string, 0, 10) + _ = s +} + +func goodWithLength() { + s := make([]string, 5) + _ = s +} + +func goodWithLengthAndCapacity() { + s := make([]string, 5, 10) + _ = s +} + +func goodArrayType() { + s := [10]string{} + _ = s +} + +func goodArrayLiteral() { + s := []string{"a", "b"} + _ = s +} + +func suppressed() { + //nolint:slicemakezerolength + s := make([]string, 0) + _ = s +} From f80cc6b543a64ff56796a8de5d3de68dfb48172c Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 11 Sep 2026 18:12:19 +0000 Subject: [PATCH 2/4] Add ADR for slice-make-zero-length linter --- ...60310-add-slice-make-zero-length-linter.md | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 docs/adr/60310-add-slice-make-zero-length-linter.md diff --git a/docs/adr/60310-add-slice-make-zero-length-linter.md b/docs/adr/60310-add-slice-make-zero-length-linter.md new file mode 100644 index 00000000000..a6735566557 --- /dev/null +++ b/docs/adr/60310-add-slice-make-zero-length-linter.md @@ -0,0 +1,50 @@ +# ADR-60310: Add Slice Make Zero Length Linter + +**Date**: 2026-09-11 +**Status**: Draft +**Deciders**: gh-aw maintainers + +--- + +### Context + +This pull request adds a new custom Go analyzer under `pkg/linters/` and registers it in the shared linter registry, which makes the change part of the repository's standard static-analysis policy rather than an isolated utility. The implementation targets calls of the form `make([]T, 0)` without a capacity argument and treats them as a performance-oriented code smell, with tests showing both expected findings and allowed cases such as explicit capacity, non-zero length, and `nolint` suppression. Because the linter becomes part of the central analyzer suite, the architectural decision is whether this repository should enforce this allocation pattern through automated linting. The available PR evidence emphasizes performance and repeated review feedback as the primary drivers for codifying the rule. + +### Decision + +We will add a custom `slicemakezerolength` analyzer to the repository's shared linter registry to flag `make([]T, 0)` calls that omit a capacity argument. We decided to encode this performance recommendation as a reusable static-analysis rule, with support for existing repository conventions such as generated-file skipping, coverage gating, and `nolint` suppression. This favors consistent automated enforcement of a repeated code-review concern over relying on manual review comments. + +### Alternatives Considered + +#### Alternative 1: Keep this as a code review guideline only + +The team could continue treating `make([]T, 0)` without capacity as an informal review suggestion instead of building a dedicated analyzer. This was considered because the PR body explicitly describes the pattern as a common review comment, and manual review avoids growing the custom linter suite. It was not chosen because the diff shows the pattern occurs in multiple locations across the codebase and the change aims to make the guidance consistent and automatically enforceable. + +#### Alternative 2: Rely on existing third-party linters + +Another option would be to depend on an upstream linter or broader performance lint package instead of adding a repository-specific analyzer. This was considered because it could reduce local maintenance and reuse community tooling. It was not chosen because the PR implements the rule directly in `pkg/linters/`, integrates it with the local registry and helper utilities, and therefore indicates the repository wants targeted behavior aligned with its existing custom-linter framework. + +#### Alternative 3: Broaden the rule to infer final slice size before reporting + +The analyzer could attempt deeper data-flow analysis and only report cases where the final capacity can be proven from surrounding code. This was considered because the PR description frames the issue in terms of known or estimable final lengths. It was not chosen because the actual implementation intentionally uses a simpler syntactic rule—reporting `make([]T, 0)` without a capacity argument—trading precision for low complexity and predictable enforcement. + +### Consequences + +#### Positive +- The repository will enforce this slice-allocation convention consistently across reviews and automation. +- Developers get fast feedback for a repeated performance-oriented pattern without waiting for reviewer intervention. +- The analyzer fits the existing custom linter architecture, including registry-based activation, testdata-driven verification, coverage gating, and `nolint` support. + +#### Negative +- The rule may report some cases where omitted capacity is harmless or where the final size is not actually inferable, creating false positives relative to the PR's stated motivation. +- Maintaining another custom analyzer increases long-term cost for compatibility, testing, and linter-suite complexity. +- Codifying this recommendation as a lint rule may push style and micro-optimization policy into CI, which can increase friction for contributors. + +#### Neutral +- The implementation only adds diagnostics; it does not include an automatic fix or rewrite. +- Existing suppression mechanisms remain available through `//nolint:slicemakezerolength`. +- The decision extends the current custom-linter framework rather than introducing a new enforcement mechanism. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* From fd312cb6e8cbce3cf200758af792dad36b4b61ea Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 11 Sep 2026 19:04:42 +0000 Subject: [PATCH 3/4] Start PR readiness review Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/schemas/github-workflow.json | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/pkg/workflow/schemas/github-workflow.json b/pkg/workflow/schemas/github-workflow.json index e65d2332ab1..9210c6dbc10 100644 --- a/pkg/workflow/schemas/github-workflow.json +++ b/pkg/workflow/schemas/github-workflow.json @@ -13,6 +13,12 @@ "$ref": "#/definitions/globs", "description": "When using the push and pull_request events, you can configure a workflow to run on specific branches or tags. If you only define only tags or only branches, the workflow won't run for events affecting the undefined Git ref.\nThe branches, branches-ignore, tags, and tags-ignore keywords accept glob patterns that use the * and ** wildcard characters to match more than one branch or tag name. For more information, see https://help.github.com/en/github/automating-your-workflow-with-github-actions/workflow-syntax-for-github-actions#filter-pattern-cheat-sheet.\nThe patterns defined in branches and tags are evaluated against the Git ref's name. For example, defining the pattern mona/octocat in branches will match the refs/heads/mona/octocat Git ref. The pattern releases/** will match the refs/heads/releases/10 Git ref.\nYou can use two types of filters to prevent a workflow from running on pushes and pull requests to tags and branches:\n- branches or branches-ignore - You cannot use both the branches and branches-ignore filters for the same event in a workflow. Use the branches filter when you need to filter branches for positive matches and exclude branches. Use the branches-ignore filter when you only need to exclude branch names.\n- tags or tags-ignore - You cannot use both the tags and tags-ignore filters for the same event in a workflow. Use the tags filter when you need to filter tags for positive matches and exclude tags. Use the tags-ignore filter when you only need to exclude tag names.\nYou can exclude tags and branches using the ! character. The order that you define patterns matters.\n- A matching negative pattern (prefixed with !) after a positive match will exclude the Git ref.\n- A matching positive pattern after a negative match will include the Git ref again." }, + "cacheMode": { + "$comment": "https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#cache-mode", + "description": "Controls the level of GitHub Actions cache access granted to a workflow or job.", + "type": "string", + "enum": ["read", "write", "write-only", "none"] + }, "concurrency": { "type": "object", "properties": { @@ -778,6 +784,9 @@ "permissions": { "$ref": "#/definitions/permissions" }, + "cache-mode": { + "$ref": "#/definitions/cacheMode" + }, "if": { "$comment": "https://help.github.com/en/actions/automating-your-workflow-with-github-actions/workflow-syntax-for-github-actions#jobsjob_idif", "description": "You can use the if conditional to prevent a job from running unless a condition is met. You can use any supported context and expression to create a conditional.\nExpressions in an if conditional do not require the ${{ }} syntax. For more information, see https://help.github.com/en/articles/contexts-and-expression-syntax-for-github-actions.", @@ -889,6 +898,9 @@ "permissions": { "$ref": "#/definitions/permissions" }, + "cache-mode": { + "$ref": "#/definitions/cacheMode" + }, "runs-on": { "$comment": "https://help.github.com/en/github/automating-your-workflow-with-github-actions/workflow-syntax-for-github-actions#jobsjob_idruns-on", "description": "The type of machine to run the job on. The machine can be either a GitHub-hosted runner, or a self-hosted runner.", @@ -2170,6 +2182,9 @@ } ] }, + "cache-mode": { + "$ref": "#/definitions/cacheMode" + }, "jobs": { "$comment": "https://help.github.com/en/github/automating-your-workflow-with-github-actions/workflow-syntax-for-github-actions#jobs", "description": "A workflow run is made up of one or more jobs. Jobs run in parallel by default. To run jobs sequentially, you can define dependencies on other jobs using the jobs..needs keyword.\nEach job runs in a fresh instance of the virtual environment specified by runs-on.\nYou can run an unlimited number of jobs as long as you are within the workflow usage limits. For more information, see https://help.github.com/en/github/automating-your-workflow-with-github-actions/workflow-syntax-for-github-actions#usage-limits.", From 9ca42be7c1491c58071c6048ce73bc715eb27bce Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 11 Sep 2026 19:18:25 +0000 Subject: [PATCH 4/4] Fix slice capacity linter precision and integration Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .github/workflows/cgo.yml | 4 +- ...60310-add-slice-make-zero-length-linter.md | 8 +- pkg/linters/README.md | 4 +- pkg/linters/doc.go | 3 +- .../slicemakezerolength.go | 167 ++++++++++++++---- .../slicemakezerolength.go | 74 ++++++-- pkg/linters/spec_test.go | 6 +- 7 files changed, 199 insertions(+), 67 deletions(-) diff --git a/.github/workflows/cgo.yml b/.github/workflows/cgo.yml index 308b09e315c..782df549c5b 100644 --- a/.github/workflows/cgo.yml +++ b/.github/workflows/cgo.yml @@ -1457,10 +1457,10 @@ jobs: # legacy custom analyzer findings in tests or other analyzer families. # Note: -test=false intentionally scopes this gate to production code only. - name: Run custom linters - run: make golint-custom LINTER_FLAGS="-errstringmatch -panicinlibrarycode -manualmutexunlock -osexitinlibrary -rawloginlib -logfatallibrary -regexpcompileinfunction -fprintlnsprintf -strconvparseignorederror -jsonmarshalignoredeerror -uncheckedtypeassertion -fmterrorfnoverbs -tolowerequalfold -httpnoctx -httprespbodyclose -httpstatuscode -timeafterleak -errortypeassertion -execcommandwithoutcontext -sprintfint -stringsindexcontains -stringscountcontains -bytesbufferstring -ioutildeprecated -mapclearloop -mapdeletecheck -sprintfbool -appendoneelement -timenowsub -stringsjoinone -writebytestring -lenstringsplit -stringreplaceminusone -osgetenvlibrary -ossetenvlibrary -stringsindexhasprefix -contextcancelnotdeferred -ctxbackground -wgdonenotdeferred -goroutinemissingrecover -trimleftright -walkfuncerrshadow -uncheckedflushreturn -bytescomparestring -nilctxpassed -stringbytesroundtrip -fileclosenotdeferred -timesleepnocontext -sprintferrorsnew -globwalkignorederror -appendbytestring -sortslice -deferinloop -regexpdynamicpattern -generatedyamlheredoc -test=false" + run: make golint-custom LINTER_FLAGS="-errstringmatch -panicinlibrarycode -manualmutexunlock -osexitinlibrary -rawloginlib -logfatallibrary -regexpcompileinfunction -fprintlnsprintf -strconvparseignorederror -jsonmarshalignoredeerror -uncheckedtypeassertion -fmterrorfnoverbs -tolowerequalfold -httpnoctx -httprespbodyclose -httpstatuscode -timeafterleak -errortypeassertion -execcommandwithoutcontext -sprintfint -stringsindexcontains -stringscountcontains -bytesbufferstring -ioutildeprecated -mapclearloop -mapdeletecheck -sprintfbool -appendoneelement -timenowsub -stringsjoinone -writebytestring -lenstringsplit -stringreplaceminusone -osgetenvlibrary -ossetenvlibrary -stringsindexhasprefix -contextcancelnotdeferred -ctxbackground -wgdonenotdeferred -goroutinemissingrecover -trimleftright -walkfuncerrshadow -uncheckedflushreturn -bytescomparestring -nilctxpassed -stringbytesroundtrip -fileclosenotdeferred -timesleepnocontext -sprintferrorsnew -globwalkignorederror -appendbytestring -slicemakezerolength -sortslice -deferinloop -regexpdynamicpattern -generatedyamlheredoc -test=false" - name: Run custom linters (wasm) - run: GOOS=js GOARCH=wasm make golint-custom LINTER_FLAGS="-errstringmatch -panicinlibrarycode -manualmutexunlock -osexitinlibrary -rawloginlib -logfatallibrary -regexpcompileinfunction -fprintlnsprintf -strconvparseignorederror -jsonmarshalignoredeerror -uncheckedtypeassertion -fmterrorfnoverbs -tolowerequalfold -httpnoctx -httprespbodyclose -httpstatuscode -timeafterleak -errortypeassertion -execcommandwithoutcontext -sprintfint -stringsindexcontains -stringscountcontains -bytesbufferstring -ioutildeprecated -mapclearloop -mapdeletecheck -sprintfbool -appendoneelement -timenowsub -stringsjoinone -writebytestring -lenstringsplit -stringreplaceminusone -osgetenvlibrary -ossetenvlibrary -stringsindexhasprefix -ctxbackground -wgdonenotdeferred -goroutinemissingrecover -trimleftright -walkfuncerrshadow -uncheckedflushreturn -bytescomparestring -nilctxpassed -stringbytesroundtrip -fileclosenotdeferred -timesleepnocontext -sprintferrorsnew -globwalkignorederror -appendbytestring -sortslice -deferinloop -regexpdynamicpattern -generatedyamlheredoc -test=false" LINTER_PACKAGES="./pkg/console ./pkg/parser ./pkg/styles ./pkg/tty ./pkg/workflow" + run: GOOS=js GOARCH=wasm make golint-custom LINTER_FLAGS="-errstringmatch -panicinlibrarycode -manualmutexunlock -osexitinlibrary -rawloginlib -logfatallibrary -regexpcompileinfunction -fprintlnsprintf -strconvparseignorederror -jsonmarshalignoredeerror -uncheckedtypeassertion -fmterrorfnoverbs -tolowerequalfold -httpnoctx -httprespbodyclose -httpstatuscode -timeafterleak -errortypeassertion -execcommandwithoutcontext -sprintfint -stringsindexcontains -stringscountcontains -bytesbufferstring -ioutildeprecated -mapclearloop -mapdeletecheck -sprintfbool -appendoneelement -timenowsub -stringsjoinone -writebytestring -lenstringsplit -stringreplaceminusone -osgetenvlibrary -ossetenvlibrary -stringsindexhasprefix -ctxbackground -wgdonenotdeferred -goroutinemissingrecover -trimleftright -walkfuncerrshadow -uncheckedflushreturn -bytescomparestring -nilctxpassed -stringbytesroundtrip -fileclosenotdeferred -timesleepnocontext -sprintferrorsnew -globwalkignorederror -appendbytestring -slicemakezerolength -sortslice -deferinloop -regexpdynamicpattern -generatedyamlheredoc -test=false" LINTER_PACKAGES="./pkg/console ./pkg/parser ./pkg/styles ./pkg/tty ./pkg/workflow" # Ensure no action shell scripts invoke python or python3 - name: Lint action shell scripts diff --git a/docs/adr/60310-add-slice-make-zero-length-linter.md b/docs/adr/60310-add-slice-make-zero-length-linter.md index a6735566557..082ff5f81da 100644 --- a/docs/adr/60310-add-slice-make-zero-length-linter.md +++ b/docs/adr/60310-add-slice-make-zero-length-linter.md @@ -12,7 +12,7 @@ This pull request adds a new custom Go analyzer under `pkg/linters/` and registe ### Decision -We will add a custom `slicemakezerolength` analyzer to the repository's shared linter registry to flag `make([]T, 0)` calls that omit a capacity argument. We decided to encode this performance recommendation as a reusable static-analysis rule, with support for existing repository conventions such as generated-file skipping, coverage gating, and `nolint` suppression. This favors consistent automated enforcement of a repeated code-review concern over relying on manual review comments. +We will add a custom `slicemakezerolength` analyzer to the repository's shared linter registry. It flags `make([]T, 0)` only when the next statement is a range loop over a value with a known length and the loop appends exactly one element to that slice per iteration. This makes `len(rangeValue)` a useful capacity hint while avoiding diagnostics when the slice does not grow or its growth cannot be derived. The analyzer supports existing repository conventions such as generated-file skipping, coverage gating, and `nolint` suppression. ### Alternatives Considered @@ -24,9 +24,9 @@ The team could continue treating `make([]T, 0)` without capacity as an informal Another option would be to depend on an upstream linter or broader performance lint package instead of adding a repository-specific analyzer. This was considered because it could reduce local maintenance and reuse community tooling. It was not chosen because the PR implements the rule directly in `pkg/linters/`, integrates it with the local registry and helper utilities, and therefore indicates the repository wants targeted behavior aligned with its existing custom-linter framework. -#### Alternative 3: Broaden the rule to infer final slice size before reporting +#### Alternative 3: Flag every zero-length slice allocation without capacity -The analyzer could attempt deeper data-flow analysis and only report cases where the final capacity can be proven from surrounding code. This was considered because the PR description frames the issue in terms of known or estimable final lengths. It was not chosen because the actual implementation intentionally uses a simpler syntactic rule—reporting `make([]T, 0)` without a capacity argument—trading precision for low complexity and predictable enforcement. +The analyzer could report every `make([]T, 0)` without analyzing subsequent use. This simpler syntactic rule was not chosen because slices that never grow need no backing allocation, and slices with indeterminate growth have no defensible capacity hint. Restricting the rule to a direct, known-size append loop provides a precise optimization with predictable enforcement. ### Consequences @@ -36,7 +36,7 @@ The analyzer could attempt deeper data-flow analysis and only report cases where - The analyzer fits the existing custom linter architecture, including registry-based activation, testdata-driven verification, coverage gating, and `nolint` support. #### Negative -- The rule may report some cases where omitted capacity is harmless or where the final size is not actually inferable, creating false positives relative to the PR's stated motivation. +- The deliberately narrow pattern leaves more complex growth patterns to manual performance analysis. - Maintaining another custom analyzer increases long-term cost for compatibility, testing, and linter-suite complexity. - Codifying this recommendation as a lint rule may push style and micro-optimization policy into CI, which can increase friction for contributors. diff --git a/pkg/linters/README.md b/pkg/linters/README.md index 4bf10406cf5..4e769b18ccb 100644 --- a/pkg/linters/README.md +++ b/pkg/linters/README.md @@ -50,6 +50,7 @@ This package currently provides custom Go analyzers in the following subpackages - `regexpcompileinfunction` — reports `regexp.Compile` / `regexp.MustCompile` and their POSIX variants called inside functions that should be package-level. - `regexpdynamicpattern` — reports regexp compile calls whose pattern is not a compile-time constant string. - `seenmapbool` — reports `map[string]bool` used as a set (values always `true`) that should use `map[string]struct{}` instead. +- `slicemakezerolength` — reports zero-length slice allocations before known-size range loops that append exactly one element per iteration, where the range length provides a capacity hint. - `sortslice` — reports `sort.Slice` / `sort.SliceStable` calls that should use `slices.SortFunc` / `slices.SortStableFunc`. - `sprintferrdot` — reports redundant `.Error()` calls on error values passed to `fmt` format functions where the fmt package calls `.Error()` automatically. - `sprintferrorsnew` — reports `errors.New(fmt.Sprintf(...))` calls that should use `fmt.Errorf` instead. @@ -79,7 +80,7 @@ This package currently provides custom Go analyzers in the following subpackages Micro-optimizations flagged by allocation/perf linters (e.g. `stringsconcatloop`, `appendoneelement`, `appendbytestring`, `bytesbufferstring`, `bytescomparestring`, `lenstringsplit`, `mapclearloop`, -`seenmapbool`, `sortslice`, `stringbytesroundtrip`, `stringsjoinone`, `tolowerequalfold`, and +`seenmapbool`, `slicemakezerolength`, `sortslice`, `stringbytesroundtrip`, `stringsjoinone`, `tolowerequalfold`, and `writebytestring`) only matter on hot paths: applying them to code that tests never execute adds churn without a measurable benefit. These linters consult the shared `pkg/linters/internal/coverage` package, which loads a Go coverage profile (produced by @@ -146,6 +147,7 @@ environment variable and gates findings on the recorded execution hit count for | `regexpcompileinfunction` | Custom `go/analysis` analyzer that flags regexp compilation inside function bodies | | `regexpdynamicpattern` | Custom `go/analysis` analyzer that flags regexp compile calls with non-constant patterns | | `seenmapbool` | Custom `go/analysis` analyzer that flags `map[string]bool` used as a set that should use `map[string]struct{}` | +| `slicemakezerolength` | Custom `go/analysis` analyzer that flags zero-length slice allocations before known-size range loops that append one element per iteration | | `sortslice` | Custom `go/analysis` analyzer that flags `sort.Slice` / `sort.SliceStable` calls that should use `slices.SortFunc` / `slices.SortStableFunc` | | `sprintferrdot` | Custom `go/analysis` analyzer that flags redundant `.Error()` calls on error values passed to `fmt` format functions | | `sprintferrorsnew` | Custom `go/analysis` analyzer that flags `errors.New(fmt.Sprintf(...))` calls that should use `fmt.Errorf` instead | diff --git a/pkg/linters/doc.go b/pkg/linters/doc.go index 8a379ca1616..19d1fab28fc 100644 --- a/pkg/linters/doc.go +++ b/pkg/linters/doc.go @@ -1,6 +1,6 @@ // Package linters is a namespace for gh-aw's custom Go analysis linters. // -// All 68 active analyzers: +// All 69 active analyzers: // // - appendbytestring — flags append(b, []byte(s)...) calls where s is a string that can be simplified to append(b, s...) // - appendoneelement — flags append(s, []T{x}...) calls where a single-element slice literal is spread and can be simplified to append(s, x) @@ -46,6 +46,7 @@ // - regexpcompileinfunction — flags regexp.MustCompile/Compile calls inside functions // - regexpdynamicpattern — flags regexp compile calls whose pattern is not a compile-time constant // - seenmapbool — flags map[string]bool used as a set that should use map[string]struct{} +// - slicemakezerolength — flags zero-length slice allocations before known-size range loops that append one element per iteration // - sortslice — flags sort.Slice / sort.SliceStable calls that should use slices.SortFunc / slices.SortStableFunc // - sprintferrdot — flags redundant .Error() calls on error values passed to fmt format functions // - sprintferrorsnew — flags errors.New(fmt.Sprintf(...)) calls that should use fmt.Errorf instead diff --git a/pkg/linters/slicemakezerolength/slicemakezerolength.go b/pkg/linters/slicemakezerolength/slicemakezerolength.go index 28d6c7756c5..e18a7142494 100644 --- a/pkg/linters/slicemakezerolength/slicemakezerolength.go +++ b/pkg/linters/slicemakezerolength/slicemakezerolength.go @@ -7,6 +7,7 @@ package slicemakezerolength import ( "fmt" "go/ast" + "go/constant" "go/types" "golang.org/x/tools/go/analysis" @@ -34,80 +35,168 @@ func run(pass *analysis.Pass) (any, error) { return nil, err } - nodeFilter := []ast.Node{(*ast.CallExpr)(nil)} + nodeFilter := []ast.Node{(*ast.BlockStmt)(nil)} return analyzerutil.Preorder(pass, nodeFilter, func(n ast.Node) { - analyzeMakeSliceZeroLength(pass, n, generatedFiles, noLintIndex) + analyzeBlock(pass, n, generatedFiles, noLintIndex) }) } -// analyzeMakeSliceZeroLength checks whether a call is make([]T, 0) without -// a capacity argument and reports a diagnostic if so. -func analyzeMakeSliceZeroLength(pass *analysis.Pass, n ast.Node, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) { - call, ok := n.(*ast.CallExpr) +func analyzeBlock(pass *analysis.Pass, n ast.Node, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) { + block, ok := n.(*ast.BlockStmt) if !ok { return } - // Check if this is a call to the built-in make function. + for i := 0; i+1 < len(block.List); i++ { + target, call, sliceType, ok := zeroLengthSliceAssignment(pass, block.List[i]) + if !ok { + continue + } + rangeStmt, ok := block.List[i+1].(*ast.RangeStmt) + if !ok || !hasKnownRangeSize(pass, rangeStmt.X) || containsReference(pass, rangeStmt.X, target) { + continue + } + if !appendsOneElement(pass, rangeStmt, target) { + continue + } + + reportDiagnostic(pass, call, sliceType, rangeStmt.X, generatedFiles, noLintIndex) + } +} + +func zeroLengthSliceAssignment(pass *analysis.Pass, stmt ast.Stmt) (types.Object, *ast.CallExpr, *ast.ArrayType, bool) { + var target *ast.Ident + var value ast.Expr + + switch stmt := stmt.(type) { + case *ast.AssignStmt: + if len(stmt.Lhs) != 1 || len(stmt.Rhs) != 1 { + return nil, nil, nil, false + } + target, _ = stmt.Lhs[0].(*ast.Ident) + value = stmt.Rhs[0] + case *ast.DeclStmt: + decl, ok := stmt.Decl.(*ast.GenDecl) + if !ok || len(decl.Specs) != 1 { + return nil, nil, nil, false + } + spec, ok := decl.Specs[0].(*ast.ValueSpec) + if !ok || len(spec.Names) != 1 || len(spec.Values) != 1 { + return nil, nil, nil, false + } + target = spec.Names[0] + value = spec.Values[0] + default: + return nil, nil, nil, false + } + + if target == nil { + return nil, nil, nil, false + } + targetObject := pass.TypesInfo.ObjectOf(target) + if targetObject == nil { + return nil, nil, nil, false + } + + call, ok := value.(*ast.CallExpr) + if !ok || len(call.Args) != 2 || call.Ellipsis.IsValid() { + return nil, nil, nil, false + } ident, ok := call.Fun.(*ast.Ident) if !ok || ident.Name != "make" { - return + return nil, nil, nil, false } if pass.TypesInfo.ObjectOf(ident) != types.Universe.Lookup("make") { - return + return nil, nil, nil, false } - // make([]T, 0) has exactly 2 arguments with no ellipsis. - if len(call.Args) != 2 || call.Ellipsis.IsValid() { - return + sliceType, ok := call.Args[0].(*ast.ArrayType) + if !ok || sliceType.Len != nil { + return nil, nil, nil, false + } + if !isConstantZero(pass, call.Args[1]) { + return nil, nil, nil, false } - pos := pass.Fset.PositionFor(call.Pos(), false) - if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { - return + return targetObject, call, sliceType, true +} + +func hasKnownRangeSize(pass *analysis.Pass, expr ast.Expr) bool { + typ := pass.TypesInfo.TypeOf(expr) + if typ == nil { + return false } - if nolint.HasDirectiveForLinter(pos, noLintIndex, "slicemakezerolength") { - return + switch underlying := typ.Underlying().(type) { + case *types.Array, *types.Slice, *types.Map: + return true + case *types.Basic: + return underlying.Info()&types.IsString != 0 + default: + return false } +} - // The first argument must be a slice type []T. - sliceType, ok := call.Args[0].(*ast.ArrayType) - if !ok || sliceType.Len != nil { - // Len != nil means it's an array type [n]T, not a slice type []T. - return +func appendsOneElement(pass *analysis.Pass, rangeStmt *ast.RangeStmt, target types.Object) bool { + if len(rangeStmt.Body.List) != 1 { + return false + } + assign, ok := rangeStmt.Body.List[0].(*ast.AssignStmt) + if !ok || len(assign.Lhs) != 1 || len(assign.Rhs) != 1 || !refersTo(pass, assign.Lhs[0], target) { + return false + } + call, ok := assign.Rhs[0].(*ast.CallExpr) + if !ok || len(call.Args) != 2 || call.Ellipsis.IsValid() || !refersTo(pass, call.Args[0], target) { + return false } + ident, ok := call.Fun.(*ast.Ident) + return ok && pass.TypesInfo.ObjectOf(ident) == types.Universe.Lookup("append") +} + +func refersTo(pass *analysis.Pass, expr ast.Expr, target types.Object) bool { + ident, ok := expr.(*ast.Ident) + return ok && pass.TypesInfo.ObjectOf(ident) == target +} - // The second argument must be a literal 0. - if !isZeroLiteral(call.Args[1]) { +func containsReference(pass *analysis.Pass, expr ast.Expr, target types.Object) bool { + found := false + ast.Inspect(expr, func(node ast.Node) bool { + ident, ok := node.(*ast.Ident) + if ok && pass.TypesInfo.ObjectOf(ident) == target { + found = true + return false + } + return !found + }) + return found +} + +func reportDiagnostic(pass *analysis.Pass, call *ast.CallExpr, sliceType *ast.ArrayType, rangeExpr ast.Expr, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) { + pos := pass.Fset.PositionFor(call.Pos(), false) + if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { + return + } + if nolint.HasDirectiveForLinter(pos, noLintIndex, "slicemakezerolength") { return } - if !coverage.ShouldApply(pass, call.Pos(), *hotThreshold) { return } sliceTypeText := astutil.NodeText(pass.Fset, sliceType) - if sliceTypeText == "" { - sliceTypeText = "[]T" - } lenText := astutil.NodeText(pass.Fset, call.Args[1]) - if lenText == "" { - lenText = "0" + rangeText := astutil.NodeText(pass.Fset, rangeExpr) + if sliceTypeText == "" || lenText == "" || rangeText == "" { + return } pass.Report(analysis.Diagnostic{ Pos: call.Pos(), End: call.End(), - Message: fmt.Sprintf("make(%s, %s) without capacity can be optimized", sliceTypeText, lenText), + Message: fmt.Sprintf("make(%s, %s) before this range loop can use capacity len(%s)", sliceTypeText, lenText, rangeText), }) } -// isZeroLiteral reports whether expr is the literal 0. -func isZeroLiteral(expr ast.Expr) bool { - lit, ok := expr.(*ast.BasicLit) - if !ok { - return false - } - // Check if the literal value is "0" - return lit.Value == "0" +func isConstantZero(pass *analysis.Pass, expr ast.Expr) bool { + value := pass.TypesInfo.Types[expr].Value + return value != nil && constant.Sign(value) == 0 } diff --git a/pkg/linters/slicemakezerolength/testdata/src/slicemakezerolength/slicemakezerolength.go b/pkg/linters/slicemakezerolength/testdata/src/slicemakezerolength/slicemakezerolength.go index b1536a2d094..85acf90698a 100644 --- a/pkg/linters/slicemakezerolength/testdata/src/slicemakezerolength/slicemakezerolength.go +++ b/pkg/linters/slicemakezerolength/testdata/src/slicemakezerolength/slicemakezerolength.go @@ -1,30 +1,26 @@ package slicemakezerolength -func badZeroLengthNoCapacity() { - s := make([]string, 0) // want `make\(\[\]string, 0\) without capacity can be optimized` - _ = s -} - -func badZeroLengthInt() { - s := make([]int, 0) // want `make\(\[\]int, 0\) without capacity can be optimized` - _ = s -} - -func badZeroLengthByte() { - s := make([]byte, 0) // want `make\(\[\]byte, 0\) without capacity can be optimized` +func badZeroLengthNoCapacity(items []string) { + s := make([]string, 0) // want `make\(\[\]string, 0\) before this range loop can use capacity len\(items\)` + for _, item := range items { + s = append(s, item) + } _ = s } -func badZeroLengthCustomType() { - type MyType struct { - name string +func badEquivalentZero(items map[string]int) { + var s = make([]int, 0x0) // want `make\(\[\]int, 0x0\) before this range loop can use capacity len\(items\)` + for _, item := range items { + s = append(s, item) } - s := make([]MyType, 0) // want `make\(\[\]MyType, 0\) without capacity can be optimized` _ = s } -func goodWithCapacity() { +func goodWithCapacity(items []string) { s := make([]string, 0, 10) + for _, item := range items { + s = append(s, item) + } _ = s } @@ -48,8 +44,50 @@ func goodArrayLiteral() { _ = s } -func suppressed() { +func goodWithoutGrowth() { + s := make([]string, 0) + _ = s +} + +func goodConditionalGrowth(items []string) { + s := make([]string, 0) + for _, item := range items { + if item != "" { + s = append(s, item) + } + } + _ = s +} + +func goodIndeterminateRange(items <-chan string) { + s := make([]string, 0) + for item := range items { + s = append(s, item) + } + _ = s +} + +func goodMultipleAppends(items []string) { + s := make([]string, 0) + for _, item := range items { + s = append(s, item, item) + } + _ = s +} + +func goodRangesOverTarget() { + s := make([]string, 0) + for _, item := range s[:0] { + s = append(s, item) + } + _ = s +} + +func suppressed(items []string) { //nolint:slicemakezerolength s := make([]string, 0) + for _, item := range items { + s = append(s, item) + } _ = s } diff --git a/pkg/linters/spec_test.go b/pkg/linters/spec_test.go index 1bdc7b604dc..ca7cc959af8 100644 --- a/pkg/linters/spec_test.go +++ b/pkg/linters/spec_test.go @@ -55,6 +55,7 @@ import ( "github.com/github/gh-aw/pkg/linters/regexpcompileinfunction" "github.com/github/gh-aw/pkg/linters/regexpdynamicpattern" "github.com/github/gh-aw/pkg/linters/seenmapbool" + "github.com/github/gh-aw/pkg/linters/slicemakezerolength" "github.com/github/gh-aw/pkg/linters/sortslice" "github.com/github/gh-aw/pkg/linters/sprintfbool" "github.com/github/gh-aw/pkg/linters/sprintferrdot" @@ -94,7 +95,7 @@ type docAnalyzer struct { } // documentedAnalyzers returns the analyzer subpackages documented in the README -// "Public API > Subpackages" table. The README documents 68 analyzers +// "Public API > Subpackages" table. The README documents 69 analyzers // subpackages (the non-analyzer `internal` helper subpackage is excluded because // it exposes no Analyzer). // @@ -104,7 +105,7 @@ type docAnalyzer struct { // errortypeassertion, errstringmatch, execcommandwithoutcontext, fileclosenotdeferred, fmterrorfnoverbs, fprintlnsprintf, // generatedyamlheredoc, globwalkignorederror, goroutinemissingrecover, hardcodedfilepath, httpnoctx, httprespbodyclose, httpstatuscode, ioutildeprecated, jsonmarshalignoredeerror, largefunc, lenstringsplit, lenstringzero, // logfatallibrary, manualmutexunlock, manualpathconcat, mapclearloop, mapdeletecheck, nilctxpassed, osexitinlibrary, osgetenvlibrary, ossetenvlibrary, packagelevelmutableslicemap, panic-in-library-code, rawloginlib, -// regexpcompileinfunction, regexpdynamicpattern, seenmapbool, sortslice, sprintferrdot, sprintferrorsnew, sprintfbool, sprintfint, ssljson, +// regexpcompileinfunction, regexpdynamicpattern, seenmapbool, slicemakezerolength, sortslice, sprintferrdot, sprintferrorsnew, sprintfbool, sprintfint, ssljson, // strconvparseignorederror, stringbytesroundtrip, stringreplaceminusone, stringsconcatloop, stringscountcontains, stringsindexcontains, stringsindexhasprefix, stringsjoinone, timeafterleak, timesleepnocontext, timenowsub, // tolowerequalfold, trimleftright, uncheckedflushreturn, uncheckedtypeassertion, walkfuncerrshadow, wgdonenotdeferred, writebytestring func documentedAnalyzers() []docAnalyzer { @@ -153,6 +154,7 @@ func documentedAnalyzers() []docAnalyzer { {"regexpcompileinfunction", regexpcompileinfunction.Analyzer}, {"regexpdynamicpattern", regexpdynamicpattern.Analyzer}, {"seenmapbool", seenmapbool.Analyzer}, + {"slicemakezerolength", slicemakezerolength.Analyzer}, {"sortslice", sortslice.Analyzer}, {"sprintferrdot", sprintferrdot.Analyzer}, {"sprintferrorsnew", sprintferrorsnew.Analyzer},