Skip to content

Preserve expression precedence in stringsjoinone autofix replacements #47482

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 2 commits into from
Jul 23, 2026
+43 −13
Merged

Preserve expression precedence in stringsjoinone autofix replacements #47482

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a6ee588
Initial plan
Copilot Jul 23, 2026
ee32ec8
Fix stringsjoinone suggested-fix precedence handling
Copilot Jul 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 23 additions & 13 deletions pkg/linters/stringsjoinone/stringsjoinone.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,11 @@ func analyzeJoinOne(pass *analysis.Pass, n ast.Node, generatedFiles filecheck.Ge
return
}

elemText, ok := matchSingleElementStringSlice(pass, joinCall.Args[0])
elem, elemText, ok := matchSingleElementStringSlice(pass, joinCall.Args[0])
if !ok {
return
}
replacementText := formatReplacementText(elem, elemText)
// Only flag when the separator is a compile-time constant so that the
// suggested fix does not silently drop observable side effects. For
// example, strings.Join([]string{s}, <-ch) receives from a channel before
Expand All @@ -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(),

@github-actions github-actions Bot Jul 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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), ...}},
}},

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),

@github-actions github-actions Bot Jul 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

Copilot AI Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

SuggestedFixes: []analysis.SuggestedFix{{
Message: "Replace strings.Join call with " + elemText,
Message: "Replace strings.Join call with " + replacementText,
TextEdits: []analysis.TextEdit{{
Pos: call.Pos(),
End: call.End(),
NewText: []byte(elemText),
NewText: []byte(replacementText),
}},
}},
})
Expand All @@ -107,30 +108,39 @@ func isSafeToDiscardSeparator(pass *analysis.Pass, sep ast.Expr) bool {

// matchSingleElementStringSlice reports whether expr is a []string{...} composite
// literal with exactly one element and returns the text of that element.
func matchSingleElementStringSlice(pass *analysis.Pass, expr ast.Expr) (elemText string, ok bool) {
func matchSingleElementStringSlice(pass *analysis.Pass, expr ast.Expr) (elem ast.Expr, elemText string, ok bool) {
lit, ok := expr.(*ast.CompositeLit)
if !ok {
return "", false
return nil, "", false
}
// The type must be []string (array type with no length, element type "string").
arrayType, ok := lit.Type.(*ast.ArrayType)
if !ok || arrayType.Len != nil {
return "", false
return nil, "", false
}
ident, ok := arrayType.Elt.(*ast.Ident)
if !ok || ident.Name != "string" {
return "", false
return nil, "", false
}
if len(lit.Elts) != 1 {
return "", false
return nil, "", false
}
elem := lit.Elts[0]
elem = lit.Elts[0]
if _, isKV := elem.(*ast.KeyValueExpr); isKV {
return "", false
return nil, "", false
}
text := astutil.NodeText(pass.Fset, elem)
if text == "" {
return "", false
return nil, "", false
}
return elem, text, true
}

func formatReplacementText(elem ast.Expr, elemText string) string {
switch elem.(type) {
case *ast.Ident, *ast.BasicLit, *ast.ParenExpr, *ast.SelectorExpr, *ast.IndexExpr, *ast.CallExpr:

@github-actions github-actions Bot Jul 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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 elemText

Both 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.

Copilot AI Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

return elemText
default:
return "(" + elemText + ")"
}
return text, true
}

@github-actions github-actions Bot Jul 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 of s[1:3]
  • []string{x.(string)} → suggested fix becomes (x.(string)) instead of x.(string)

Suggested fix:

case *ast.Ident, *ast.BasicLit, *ast.ParenExpr, *ast.SelectorExpr,
     *ast.IndexExpr, *ast.CallExpr, *ast.SliceExpr, *ast.TypeAssertExpr:
    return elemText

This is not a correctness regression (the wrapped form compiles identically), but it produces misleading suggested fixes for these common patterns.

10 changes: 10 additions & 0 deletions pkg/linters/stringsjoinone/testdata/src/stringsjoinone/stringsjoinone.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,16 @@ func joinOneAssigned(s string) string {
return result
}

// flagged: compound element used in an index context; fix must preserve precedence.
func joinOneIndexed(a, b string) byte {
return strings.Join([]string{a + b}, "")[0] // want `strings\.Join called with a single-element slice`
}

// flagged: compound element used in a slice context; fix must preserve precedence.
func joinOneSliced(a, b string) string {
return strings.Join([]string{a + b}, "")[1:] // want `strings\.Join called with a single-element slice`
}

// not flagged: two-element slice literal.
func joinTwo(a, b string) string {
return strings.Join([]string{a, b}, ", ")
Expand Down
10 changes: 10 additions & 0 deletions pkg/linters/stringsjoinone/testdata/src/stringsjoinone/stringsjoinone.go.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,16 @@ func joinOneAssigned(s string) string {
return result
}

// flagged: compound element used in an index context; fix must preserve precedence.
func joinOneIndexed(a, b string) byte {
return (a + b)[0] // want `strings\.Join called with a single-element slice`
}

// flagged: compound element used in a slice context; fix must preserve precedence.
func joinOneSliced(a, b string) string {
return (a + b)[1:] // want `strings\.Join called with a single-element slice`
}

// not flagged: two-element slice literal.
func joinTwo(a, b string) string {
return strings.Join([]string{a, b}, ", ")
Expand Down
Loading