Preserve expression precedence in stringsjoinone autofix replacements#47482
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
stringsjoinone autofix replacements
There was a problem hiding this comment.
Pull request overview
Fixes stringsjoinone autofixes to preserve expression precedence.
Changes:
- Parenthesizes non-atomic replacement expressions.
- Returns the element AST alongside its source text.
- Adds index and slice regression tests.
Show a summary per file
| File | Description |
|---|---|
pkg/linters/stringsjoinone/stringsjoinone.go |
Builds precedence-safe replacements. |
pkg/linters/stringsjoinone/testdata/src/stringsjoinone/stringsjoinone.go |
Adds regression inputs. |
pkg/linters/stringsjoinone/testdata/src/stringsjoinone/stringsjoinone.go.golden |
Verifies parenthesized fixes. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Medium
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (43 additions across 3 files). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The precedence-preservation logic in formatReplacementText is correct: high-precedence expression types (identifiers, literals, parens, selectors, index expressions, call expressions) are passed through as-is, while lower-precedence expressions (binary, unary, etc.) get wrapped in parentheses.
Test coverage for a + b in index and slice contexts confirms the fix.
Minor non-blocking note: *ast.SliceExpr is not in the safe list, so strings.Join([]string{s[1:2]}, "") would produce (s[1:2]) — redundant parens but semantically correct and harmless.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 15.5 AIC · ⌖ 5.42 AIC · ⊞ 5K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (2 tests)
Note: Tests are expressed as Verdict
|
There was a problem hiding this comment.
The core fix is correct — wrapping compound elements in parens before replacing the full strings.Join(...) call properly preserves operator precedence in postfix contexts.
Issues found
Incomplete allowlist in formatReplacementText
*ast.SliceExpr (s[1:3]) and *ast.TypeAssertExpr (x.(string)) fall into the default wrapping case even though neither requires parens. The fix compiles and runs correctly but produces a cosmetically wrong suggestion with spurious outer parens.
Diagnostic message shows parentheses for compound elements
The warning text uses replacementText rather than the raw elemText, so users see use (a + b) directly instead of use a + b directly. The paren-wrapping is an autofix detail and should not leak into the human-readable message.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 41.7 AIC · ⌖ 4.77 AIC · ⊞ 5.7K
Comment /review to run again
| return "(" + elemText + ")" | ||
| } | ||
| return text, true | ||
| } |
There was a problem hiding this comment.
Missing *ast.SliceExpr and *ast.TypeAssertExpr from the allowlist causes unnecessary wrapping in valid cases.
💡 Details and suggested fix
The allowlist in formatReplacementText omits *ast.SliceExpr and *ast.TypeAssertExpr. Both are already atomic for postfix indexing and don't need parens:
[]string{s[1:3]}→ suggested fix becomes(s[1:3])instead ofs[1:3][]string{x.(string)}→ suggested fix becomes(x.(string))instead ofx.(string)
Suggested fix:
case *ast.Ident, *ast.BasicLit, *ast.ParenExpr, *ast.SelectorExpr,
*ast.IndexExpr, *ast.CallExpr, *ast.SliceExpr, *ast.TypeAssertExpr:
return elemTextThis is not a correctness regression (the wrapped form compiles identically), but it produces misleading suggested fixes for these common patterns.
| @@ -84,13 +85,13 @@ func analyzeJoinOne(pass *analysis.Pass, n ast.Node, generatedFiles filecheck.Ge | |||
| pass.Report(analysis.Diagnostic{ | |||
| Pos: call.Pos(), | |||
| End: call.End(), | |||
There was a problem hiding this comment.
Diagnostic message leaks implementation parens into user-visible output for compound expressions.
💡 Details
When the element is a + b, replacementText becomes (a + b). The diagnostic message then reads:
strings.Join called with a single-element slice; use (a + b) directly
The parentheses are an autofix-internal detail and look confusing in the warning message. The message should use the raw elemText while the SuggestedFix uses replacementText:
Message: fmt.Sprintf("strings.Join called with a single-element slice; use %s directly", elemText),
SuggestedFixes: []analysis.SuggestedFix{{
Message: "Replace strings.Join call with " + replacementText,
TextEdits: []analysis.TextEdit{{NewText: []byte(replacementText), ...}},
}},
There was a problem hiding this comment.
Skills-Based Review
Applied /diagnosing-bugs — two focused observations on the fix.
Positive Highlights:
- Root cause correctly identified at the AST level
- Regression tests cover both indexing and slicing postfix contexts
- formatReplacementText is clean and covers common primary-expression cases
Issues Found:
- *ast.SliceExpr missing from atomic allowlist (line 141) — s[1:] as a slice element would be unnecessarily parenthesized to (s[1:]). See inline comment.
- Diagnostic message uses parenthesized text (line 88) — 'use (a + b) directly' reads awkwardly; elemText is the better fit for the human-facing message. See inline comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 40.7 AIC · ⌖ 4.8 AIC · ⊞ 6.7K
Comment /matt to run again
|
|
||
| func formatReplacementText(elem ast.Expr, elemText string) string { | ||
| switch elem.(type) { | ||
| case *ast.Ident, *ast.BasicLit, *ast.ParenExpr, *ast.SelectorExpr, *ast.IndexExpr, *ast.CallExpr: |
There was a problem hiding this comment.
[/diagnosing-bugs] *ast.SliceExpr is missing from the atomic-expression allowlist, so strings.Join([]string{s[1:]}, sep) would be autofixed to (s[1:]) — syntactically valid, but unnecessarily parenthesized.
💡 Suggested fix
Add *ast.SliceExpr (and *ast.TypeAssertExpr for completeness) to the safe list:
case *ast.Ident, *ast.BasicLit, *ast.ParenExpr, *ast.SelectorExpr,
*ast.IndexExpr, *ast.SliceExpr, *ast.CallExpr, *ast.TypeAssertExpr:
return elemTextBoth slice expressions (s[1:]) and type assertions (v.(string)) are already at primary-expression precedence in Go — they do not need wrapping when used as the operand of a further [i] or [i:j] suffix.
Companion test case to add:
func joinOneSliceElem(s string) string {
return strings.Join([]string{s[1:]}, "") // want `strings\.Join called with a single-element slice`
}Golden:
func joinOneSliceElem(s string) string {
return s[1:] // want `strings\.Join called with a single-element slice`
}@copilot please address this.
There was a problem hiding this comment.
Fixed in 37c8f45. Added *ast.SliceExpr and *ast.TypeAssertExpr to the allowlist in formatReplacementText, and added a regression test for the s[1:] element case (golden: s[1:] with no spurious parens).
| Pos: call.Pos(), | ||
| End: call.End(), | ||
| Message: fmt.Sprintf("strings.Join called with a single-element slice; use %s directly", elemText), | ||
| Message: fmt.Sprintf("strings.Join called with a single-element slice; use %s directly", replacementText), |
There was a problem hiding this comment.
[/diagnosing-bugs] The diagnostic Message now shows the parenthesized form (e.g. (a + b)) even when the call appears in a plain value position, making the message slightly misleading for users who see it outside a postfix context.
💡 Suggested improvement
Consider using the raw elemText in the diagnostic message and reserving replacementText only for the NewText of the suggested fix:
Message: fmt.Sprintf("strings.Join called with a single-element slice; use %s directly", elemText),
// ...
Message: "Replace strings.Join call with " + replacementText,
// ...
NewText: []byte(replacementText),This keeps the human-readable message clean ("use a + b directly") while the machine-applied fix still emits the correctly-parenthesized text.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 37c8f45. The diagnostic Message now uses the raw elemText (e.g. use a + b directly), while replacementText is kept only for the SuggestedFix message and NewText.
|
@copilot run pr-finisher skill |
|
🎉 This pull request is included in a new release. Release: |
stringsjoinonereplacedstrings.Join([]string{x}, sep)with rawxtext, which is unsafe whenxis compound and the call is used in tighter postfix contexts (index/slice). That could change behavior or emit non-compiling code after-fix.Autofix correctness: precedence-safe replacement
(...)before replacing the fullstrings.Join(...)call.[...]/[i:j].Coverage for known failure modes
strings.Join([]string{a + b}, "")[0]strings.Join([]string{a + b}, "")[1:](a + b)[0](a + b)[1:]Example of the behavior addressed: