feat(linters): add stringsjoinone analyzer#47288
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds and registers the stringsjoinone Go analyzer for redundant single-element strings.Join calls.
Changes:
- Implements diagnostics and suggested fixes.
- Adds analyzer fixtures and golden tests.
- Updates registry and documentation.
Show a summary per file
| File | Description |
|---|---|
pkg/linters/stringsjoinone/stringsjoinone.go |
Implements the analyzer. |
pkg/linters/stringsjoinone/stringsjoinone_test.go |
Runs analyzer fix tests. |
pkg/linters/stringsjoinone/testdata/src/stringsjoinone/stringsjoinone.go |
Adds test cases. |
pkg/linters/stringsjoinone/testdata/src/stringsjoinone/stringsjoinone.go.golden |
Defines expected fixes. |
pkg/linters/registry.go |
Registers the analyzer. |
pkg/linters/spec_test.go |
Adds documentation-sync metadata. |
pkg/linters/doc.go |
Documents the analyzer. |
pkg/linters/README.md |
Updates analyzer reference tables. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
pkg/linters/stringsjoinone/stringsjoinone.go:86
- Replacing the call with the element text without preserving expression precedence can generate invalid or changed code. For example,
strings.Join([]string{a + b}, "")[0]is rewritten asa + b[0], rather than(a + b)[0]. Parenthesize non-atomic element expressions (or conservatively parenthesize every replacement) and cover a call used as an indexed/sliced operand.
NewText: []byte(elemText),
pkg/linters/stringsjoinone/stringsjoinone.go:86
- These edits only replace the call, so applying the fix leaves an unused
stringsimport when this was the file's sole use, producing code that no longer compiles. The repository already handles this case for package-replacing fixes (for example,pkg/linters/sprintfint/sprintfint.go:180-200). Add an import-removal edit when the matched call is the final use, with a single-use fixture.
TextEdits: []analysis.TextEdit{{
Pos: call.Pos(),
End: call.End(),
NewText: []byte(elemText),
- Files reviewed: 8/9 changed files
- Comments generated: 1
- Review effort level: Medium
| return | ||
| } | ||
|
|
||
| elemText, ok := matchSingleElementStringSlice(pass, joinCall.Args[0]) |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
🤖 PR Triage
Score breakdown: Impact 25 + Urgency 15 + Quality 15 New
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (2 tests)
Verdict
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — requesting changes on one correctness issue with the auto-fix.
📋 Key Themes & Highlights
Issue
- Auto-fix correctness:
SuggestedFixesreplaces the entirestrings.Join(...)call with only the single element, unconditionally discarding thesepargument. Whensepis a call expression (e.g.getSep()), this silently drops its side effects — a semantic change, not just a simplification.
Positive Highlights
- ✅ Consistent with the existing linter pattern (uses
astutil,nolint,filecheck) - ✅ Good test coverage for the common cases (flagged/not-flagged, literal/variable, nolint)
- ✅ Golden-file tests with
RunWithSuggestedFixes— the right tool for auto-fix linters - ✅ Clean
matchSingleElementStringSliceextraction makes the logic easy to follow
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 25.8 AIC · ⌖ 4.72 AIC · ⊞ 6.7K
Comment /matt to run again
| return | ||
| } | ||
|
|
||
| elemText, ok := matchSingleElementStringSlice(pass, joinCall.Args[0]) |
There was a problem hiding this comment.
[/tdd] The auto-fix unconditionally drops the sep argument; if sep is a function call (e.g. strings.Join([]string{s}, getSep())), removing it silently elides its side effects.
💡 Suggested fix
Only emit SuggestedFixes when the separator is a side-effect-free expression (literal or identifier):
func isSafeSep(expr ast.Expr) bool {
switch expr.(type) {
case *ast.BasicLit, *ast.Ident:
return true
}
return false
}When !isSafeSep(joinCall.Args[1]), report the diagnostic without any SuggestedFixes. Add a test case covering a call-expression separator.
@copilot please address this.
| // not flagged: a nolint directive suppresses the diagnostic. | ||
| func joinSuppressed(s string) string { | ||
| return strings.Join([]string{s}, ", ") //nolint:stringsjoinone | ||
| } |
There was a problem hiding this comment.
[/tdd] The test data has no case for strings.Join([]string{s}, getSep()) where the separator is a call expression. This is precisely the edge case where the auto-fix would drop the getSep() call — without a pinning test it can silently regress.
Add a "not fixed" case with a call-expression separator to the test file (and ensure the golden file keeps it unchanged).
@copilot please address this.
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (235 new lines in Draft ADR committed: This PR cannot merge until an ADR is linked in the PR body. 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 MatterADRs 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
|
There was a problem hiding this comment.
Review: stringsjoinone analyzer
The implementation is clean and follows existing linter conventions. One correctness issue should be addressed before merge.
Separator side-effects (existing inline comment, line 72): If the second argument to strings.Join is a function call with side effects (e.g., strings.Join([]string{name}, getSep())), the suggested fix drops the call silently. The analyzer should guard against this by only flagging/fixing when the separator argument is a string literal or a pure expression with no side effects.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 31 AIC · ⌖ 5.57 AIC · ⊞ 5K
|
@copilot run pr-finisher skill |
There was a problem hiding this comment.
REQUEST_CHANGES — one blocking correctness issue with the autofix; the existing side-effects comment also needs resolution before merge.
### Findings
Blocking (high): The SuggestedFix in analyzeJoinOne replaces the entire strings.Join call with the raw source text of the single element without parenthesising it. When that element is a compound expression (a+b, f().(T), etc.) inside a higher-precedence context, the fix produces semantically wrong but compilation-passing code. See inline comment.
Existing open comment (high): The separator argument (Args[1]) is never inspected. When it has side effects — strings.Join([]string{s}, computeSep()) — the suggested fix silently drops the computeSep() call. Either gate the fix on the separator being a constant/literal, or emit a diagnostic-only finding with no SuggestedFix for non-trivial separators.
Low / non-blocking: No test case for compound-element or side-effecting-separator patterns; the test suite only exercises the happy-path flag and the nolint suppression.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 45.5 AIC · ⌖ 4.7 AIC · ⊞ 5.7K
Comment /review to run again
| End: call.End(), | ||
| Message: fmt.Sprintf("strings.Join called with a single-element slice; use %s directly", elemText), | ||
| SuggestedFixes: []analysis.SuggestedFix{{ | ||
| Message: "Replace strings.Join call with " + elemText, |
There was a problem hiding this comment.
Unsafe suggested fix: replacing elemText without parentheses corrupts operator precedence. If the single element is a binary/compound expression (e.g., a + b) and the call appears in a higher-precedence context, the rewrite silently produces wrong code.
💡 Suggested fix
Example:
// Input: n * strings.Join([]string{a + b}, ",")
// Autofix produces: n * a + b (parsed as (n*a)+b — semantically wrong)
// Should produce: n * (a + b)Fix: check whether the element is a literal or plain identifier; if not, wrap elemText in parentheses or omit the SuggestedFix for complex expressions:
if _, isIdent := elem.(*ast.Ident); !isIdent {
if _, isLit := elem.(*ast.BasicLit); !isLit {
elemText = "(" + elemText + ")"
}
}Without this guard, applying the fix via gopls or go fix on code like x * strings.Join([]string{a+b}, ",") produces a compilation-passing but semantically broken result.
Only emit the diagnostic when the separator argument is a compile-time constant (tv.Value != nil). This prevents the suggested fix from silently discarding observable runtime behaviour such as channel receives or function calls in the separator position. Add test fixtures for a function-call separator and a channel-receive separator to document these "not flagged" cases. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in the latest commit. Added |
|
🎉 This pull request is included in a new release. Release: |
Adds a new
stringsjoinonecustom Go analyzer that flagsstrings.Join([]string{s}, sep)calls with a single-element slice literal — the separator is never applied and the call is equivalent tosdirectly.Before:
After:
Changes
pkg/linters/stringsjoinone/— new analyzer package; detectsstrings.Joincalls whose first argument is a[]string{…}composite literal with exactly one element and emits a suggested fix replacing the entire call with that element. Respects//nolint:stringsjoinoneand skips generated files.pkg/linters/registry.go— registersstringsjoinone.AnalyzerinAll()pkg/linters/doc.go— adds bullet, bumps count 55 → 56pkg/linters/README.md— adds entry to Overview, Subpackages table, and Dependenciespkg/linters/spec_test.go— adds todocumentedAnalyzers()to keep registry/doc sync tests green