[linter-miner] feat(linters): add appendoneelement linter#46289
Conversation
Adds a new linter that flags append(s, []T{x}...) patterns where
a single-element slice literal is spread unnecessarily. These can
be simplified to append(s, x), which is more idiomatic Go.
The linter:
- Detects append calls with exactly 2 args + ellipsis
- Checks when the second arg is a composite slice literal with 1 element
- Provides a suggested fix rewriting the call to the simpler form
- Skips generated files and respects //nolint:appendoneelement directives
Registers the analyzer in cmd/linters/main.go.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
automation
cookie
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
Adds the appendoneelement Go analyzer to detect and simplify unnecessary single-element slice expansion.
Changes:
- Implements and registers the analyzer.
- Adds analyzer fixtures and suggested-fix tests.
- Handles generated files and
nolintdirectives.
Show a summary per file
| File | Description |
|---|---|
cmd/linters/main.go |
Registers the analyzer. |
pkg/linters/appendoneelement/appendoneelement.go |
Implements detection and fixes. |
pkg/linters/appendoneelement/appendoneelement_test.go |
Runs analyzer tests. |
pkg/linters/appendoneelement/testdata/src/appendoneelement/appendoneelement.go |
Provides test cases. |
pkg/linters/appendoneelement/testdata/src/appendoneelement/appendoneelement.go.golden |
Defines expected fixes. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
pkg/linters/appendoneelement/appendoneelement.go:106
- Replacing the entire call with printer-generated text silently drops comments and source structure outside the redundant literal. For example,
append(s /* why this destination */, []int{3}...)is fixed toappend(s, 3), deleting the comment. Apply narrow deletions to the literal type/braces and ellipsis instead so comments on the destination and around the element survive.
Pos: call.Pos(),
End: call.End(),
NewText: []byte(fmt.Sprintf("append(%s, %s)", sliceText, elemText)),
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Medium
| if len(lit.Elts) != 1 { | ||
| return | ||
| } | ||
|
|
||
| elem := lit.Elts[0] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The implementation is correct and follows existing linter conventions. Logic, tests, and golden fixtures all look good.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 19.5 AIC · ⌖ 4.91 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /grill-with-docs — requesting changes on two small issues.
📋 Key Themes & Highlights
Issues Found
- Misleading
*ast.ArrayTypecomment (line 68): the comment says "not a map/struct/array" but*ast.ArrayTypematches both slices (Len == nil) and fixed-size arrays (Len != nil). ALen != nilguard should be added to make the intent explicit and future-proof. - Missing suppression regression test: no fixture tests the
//nolint:appendoneelementpath, leaving a silent regression surface.
Positive Highlights
- ✅ Clean implementation following the established
pkg/linters/pattern - ✅ Golden-file test with
analysistest.RunWithSuggestedFixesis the right approach - ✅ Good test coverage of the happy path (int, string, variable element)
- ✅ Correct use of
filecheck.ShouldSkipFilenameandnolint.HasDirectiveForLinter - ✅ Zero-false-positive design: only triggers on single-element composite literals
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 25.7 AIC · ⌖ 4.58 AIC · ⊞ 6.7K
Comment /matt to run again
| return | ||
| } | ||
|
|
||
| // The second argument must be a composite literal with exactly one element. |
There was a problem hiding this comment.
[/tdd] The comment says "Must be a slice type, not a map/struct/array" but *ast.ArrayType in Go's AST represents both []T (slice, Len == nil) and [N]T (fixed-size array, Len != nil). The guard works in practice because a fixed-size array literal cannot be spread in append, but the comment is misleading.
💡 Suggested fix
Add a Len == nil check and update the comment to be precise:
// Must be a slice type ([]T). *ast.ArrayType covers both slices and
// fixed-size arrays; only slices have Len == nil.
arrType, ok := lit.Type.(*ast.ArrayType)
if !ok || arrType.Len != nil {
return
}This makes the intent explicit and guards against any future compiler relaxation.
@copilot please address this.
|
|
||
| func TestAnalyzer(t *testing.T) { | ||
| testdata := analysistest.TestData() | ||
| analysistest.RunWithSuggestedFixes(t, testdata, appendoneelement.Analyzer, "appendoneelement") |
There was a problem hiding this comment.
[/tdd] The test suite covers happy-path cases but has no negative test for //nolint:appendoneelement suppression. This code path exists in the implementation and can silently regress without a fixture.
💡 Suggested addition to testdata
Add to testdata/src/appendoneelement/appendoneelement.go:
func nolintCase() {
s := []int{1}
s = append(s, []int{2}...) (nolint/redacted):appendoneelement
_ = s
}This should produce no diagnostic. Without this, a regression in nolint.HasDirectiveForLinter goes undetected.
@copilot please address this.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 95/100 — Excellent
📊 Metrics (1 test)
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
There was a problem hiding this comment.
Two correctness issues must be fixed before merge
1. Built-in name check is purely syntactic — The ident.Name == "append" guard at line 53 does not verify the identifier resolves to the built-in. Code that shadows append with a local variadic function will be incorrectly flagged and have its call site rewritten by the suggested fix, breaking the program silently. Use pass.TypesInfo.ObjectOf(ident) and check obj.Pkg() == nil.
2. *ast.ArrayType includes fixed-size arrays — The slice-type guard at line 74 accepts both []T and [N]T because both are *ast.ArrayType. Add arrayType.Len != nil check to filter out fixed-size arrays.
🔎 Code quality review by PR Code Quality Reviewer · 41 AIC · ⌖ 4.53 AIC · ⊞ 5.6K
Comment /review to run again
|
|
||
| // Must be append(x, y...) with exactly 2 arguments and ellipsis. | ||
| ident, ok := call.Fun.(*ast.Ident) | ||
| if !ok || ident.Name != "append" { |
There was a problem hiding this comment.
Shadowed built-in not detected: checking ident.Name == "append" is purely syntactic — if a package defines a local function named append, this linter will fire on calls to it and emit an incorrect suggested fix.
💡 Suggested fix
Use the type checker to verify the callee is the built-in:
obj := pass.TypesInfo.ObjectOf(ident)
if obj == nil {
return // unresolved
}
if obj.Pkg() != nil {
return // not a built-in
}Or equivalently, use types.Universe.Lookup("append") and confirm obj == types.Universe.Lookup("append").
Without this guard the analyzer's suggested fix silently corrupts calls to user-defined variadic functions that happen to be named append.
| return | ||
| } | ||
| // Must be a slice type, not a map/struct/array. | ||
| if _, ok := lit.Type.(*ast.ArrayType); !ok { |
There was a problem hiding this comment.
*ast.ArrayType matches fixed-size arrays, not just slices: the comment says "not a map/struct/array" but [N]T is also an *ast.ArrayType — the linter will flag append(s, [1]int{x}...) even though spreading a fixed-size array is semantically different and cannot be simplified the same way.
💡 Suggested fix
Check that Len is nil (which distinguishes a slice type from a fixed-size array):
arrayType, ok := lit.Type.(*ast.ArrayType)
if !ok || arrayType.Len != nil {
return // not a slice literal
}Spreading [1]int{x}... is already a compile error in real Go code (arrays are not variadic-spreadable), so this guard is defensive but makes the intent explicit and future-proof against unexpected AST shapes.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (214 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
|
@copilot run or-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Ran the finisher flow and pushed fixes in |
|
@copilot please run the
|
test