Add review pattern lint analyzers - #23
Conversation
|
Warning Review limit reached
Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds a lookuptablelint benchmark suite comparing map and switch-based lookups, plus a new stringbuilderlint Go analyzer package detecting inefficient string concatenation and fmt.Sprintf usage, complete with suggested fixes, suppression handling, module config, tests, benchmarks, and testdata fixtures. ChangesLookup Table Benchmark
stringbuilderlint Analyzer
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Pass as analysis.Pass
participant Run as run(pass)
participant CheckConcat
participant CheckSprintf
participant FixGen as SuggestedFix generator
participant Report as pass.Report
Pass->>Run: inspect AST nodes
Run->>CheckConcat: visit BinaryExpr (+)
CheckConcat->>CheckConcat: filter unsupported/suppressed cases
CheckConcat->>FixGen: build strings.Builder rewrite
FixGen->>Report: Diagnostic with SuggestedFix
Run->>CheckSprintf: visit fmt.Sprintf CallExpr
CheckSprintf->>CheckSprintf: parse format string, validate verbs
CheckSprintf->>FixGen: build strconv/Builder rewrite
FixGen->>Report: Diagnostic with SuggestedFix
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
stringbuilderlint/benchmark_test.go (1)
74-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNon-sequential variable names reduce readability.
part0,part2,part4(skippingpart1/part3) matches the analyzer's piece indexing where literal segments don't receive temp variables, but in standalone benchmark code this looks like a typo. Consider adding a brief comment explaining the gap, or renaming to sequentialpart0/part1/part2since the benchmark doesn't need to match internal indexing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stringbuilderlint/benchmark_test.go` around lines 74 - 76, The benchmark setup in benchmark_test.go uses non-sequential temp names in the string assembly path, which reads like a typo. Update the variables used around the benchmark input construction to either use sequential names or add a brief comment near the relevant code to explain that the gaps mirror the analyzer’s piece indexing; reference the benchmark setup around the benchA/strconv.FormatInt/strconv.FormatBool assignments so it’s easy to locate.stringbuilderlint/analyzer.go (4)
79-85: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueIgnore check should precede
stringConcatPartsdecomposition.The suppression check at line 83 happens after
stringConcatPartsat line 79, meaning the decomposition work is wasted for ignored expressions. Moving the ignore check before the decomposition avoids unnecessary work.♻️ Proposed refactor
if r.pass.TypesInfo.Types[expr].Value != nil && r.pass.TypesInfo.Types[expr].Value.Kind() == constant.String { return } + if r.ignores.ignored(expr.Pos()) { + return + } parts := stringConcatParts(r.pass, expr) if len(parts) <= 2 { return } - if r.ignores.ignored(expr.Pos()) { - return - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stringbuilderlint/analyzer.go` around lines 79 - 85, The ignore guard in the analyzer currently runs after stringConcatParts, so ignored expressions still get decomposed unnecessarily. In the logic around stringbuilderlint/analyzer.go’s handling of expr, move the r.ignores.ignored(expr.Pos()) check before calling stringConcatParts, then keep the existing len(parts) validation and early return behavior unchanged. This keeps the suppression path cheap while preserving the current analysis flow.
460-465: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFallback to unformatted source on
format.Sourcefailure may produce invalid Go.When
format.Sourcefails at line 461, the raw buffer content is returned as-is (line 462). This unformatted code may not be syntactically valid Go if the template wrapping was the issue. Consider logging the formatting error or returningfalseto signal that no safe fix could be generated.♻️ Proposed refactor
formatted, err := format.Source([]byte("package p\n\nvar _ = " + buf.String())) if err != nil { - return buf.String(), true + // If formatting fails, the generated code is likely invalid; + // signal that no safe fix can be produced. + return "", false }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stringbuilderlint/analyzer.go` around lines 460 - 465, The fallback in the formatting path can return raw, potentially invalid Go when format.Source fails. Update the logic around the format.Source call in the analyzer function so a formatting failure does not emit unsafe output: either return false to indicate no fix can be generated, or record the formatting error and avoid using buf.String() as a replacement. Keep the handling scoped to the code that trims the prefix and returns the formatted string.
766-782: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
directiveAppliesrequires a reason text forlint:ignoredirectives.The
len(fields) < 3check at line 769 means//lint:ignore stringbuilderlint(without a reason) is silently ignored. While requiring a justification is good practice, this behavior is undocumented and may surprise users who follow the common convention of bare ignore directives. Consider either documenting this requirement or relaxing the minimum to 2 fields.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stringbuilderlint/analyzer.go` around lines 766 - 782, The directiveApplies helper currently rejects bare lint:ignore and lint:file-ignore directives because it requires at least three fields from commentText(raw), so a directive like lint:ignore stringbuilderlint is ignored. Update directiveApplies in analyzer.go to either accept directives with only the kind and analyzer list, or keep the reason requirement but document it clearly; use the existing directiveApplies logic and strings.Fields/commentText parsing to make the behavior consistent with the intended policy.
176-222: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
sprintfPiecesdoesn't reject format strings with zero arguments.
fmt.Sprintf("hello")(a constant format with no args) passes all checks:parseFormatreturns one literal segment,argIndex(1) equalslen(call.Args)(1), and the function returns a single-literalpiecesslice. This generates astrings.Builderfix that wraps a constant string in an IIFE — an unnecessary transformation that degrades readability with no performance benefit. Consider requiring at least one non-literal segment.♻️ Proposed fix
if argIndex != len(call.Args) { return nil, false } + hasExpr := false + for _, piece := range pieces { + if piece.expr != nil { + hasExpr = true + break + } + } + if !hasExpr { + return nil, false + } return pieces, true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stringbuilderlint/analyzer.go` around lines 176 - 222, The sprintfPieces helper currently treats constant-only format strings like a valid rewrite target, which causes pointless builder transformations for calls such as fmt.Sprintf with no dynamic arguments. Update reporter.sprintfPieces to reject cases where parseFormat returns only literal segments, and only return true when there is at least one non-literal conversion piece to rewrite. Keep the check localized in sprintfPieces so the existing callers in analyzer.go continue to use the same path without changing other behavior.stringbuilderlint/analyzer_test.go (1)
9-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding unit tests for
parseFormatandprimitiveConversion.The integration test via
analysistestcovers end-to-end behavior but doesn't isolate edge cases in format parsing (e.g., trailing%, empty format,%%only, precision boundaries). Table-driven unit tests for these functions would catch regressions more precisely and document expected behavior for unsupported formats.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stringbuilderlint/analyzer_test.go` around lines 9 - 11, Add focused unit tests for parseFormat and primitiveConversion in addition to TestAnalyzer, since analysistest only covers end-to-end behavior. Create table-driven tests that exercise edge cases like trailing %, empty format strings, %% only, and precision boundary cases, and assert the expected handling of unsupported formats. Use the parseFormat and primitiveConversion symbols directly so the tests document their standalone behavior and catch regressions more precisely.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@stringbuilderlint/analyzer_test.go`:
- Around line 9-11: Add focused unit tests for parseFormat and
primitiveConversion in addition to TestAnalyzer, since analysistest only covers
end-to-end behavior. Create table-driven tests that exercise edge cases like
trailing %, empty format strings, %% only, and precision boundary cases, and
assert the expected handling of unsupported formats. Use the parseFormat and
primitiveConversion symbols directly so the tests document their standalone
behavior and catch regressions more precisely.
In `@stringbuilderlint/analyzer.go`:
- Around line 79-85: The ignore guard in the analyzer currently runs after
stringConcatParts, so ignored expressions still get decomposed unnecessarily. In
the logic around stringbuilderlint/analyzer.go’s handling of expr, move the
r.ignores.ignored(expr.Pos()) check before calling stringConcatParts, then keep
the existing len(parts) validation and early return behavior unchanged. This
keeps the suppression path cheap while preserving the current analysis flow.
- Around line 460-465: The fallback in the formatting path can return raw,
potentially invalid Go when format.Source fails. Update the logic around the
format.Source call in the analyzer function so a formatting failure does not
emit unsafe output: either return false to indicate no fix can be generated, or
record the formatting error and avoid using buf.String() as a replacement. Keep
the handling scoped to the code that trims the prefix and returns the formatted
string.
- Around line 766-782: The directiveApplies helper currently rejects bare
lint:ignore and lint:file-ignore directives because it requires at least three
fields from commentText(raw), so a directive like lint:ignore stringbuilderlint
is ignored. Update directiveApplies in analyzer.go to either accept directives
with only the kind and analyzer list, or keep the reason requirement but
document it clearly; use the existing directiveApplies logic and
strings.Fields/commentText parsing to make the behavior consistent with the
intended policy.
- Around line 176-222: The sprintfPieces helper currently treats constant-only
format strings like a valid rewrite target, which causes pointless builder
transformations for calls such as fmt.Sprintf with no dynamic arguments. Update
reporter.sprintfPieces to reject cases where parseFormat returns only literal
segments, and only return true when there is at least one non-literal conversion
piece to rewrite. Keep the check localized in sprintfPieces so the existing
callers in analyzer.go continue to use the same path without changing other
behavior.
In `@stringbuilderlint/benchmark_test.go`:
- Around line 74-76: The benchmark setup in benchmark_test.go uses
non-sequential temp names in the string assembly path, which reads like a typo.
Update the variables used around the benchmark input construction to either use
sequential names or add a brief comment near the relevant code to explain that
the gaps mirror the analyzer’s piece indexing; reference the benchmark setup
around the benchA/strconv.FormatInt/strconv.FormatBool assignments so it’s easy
to locate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b7cc965-de05-4aca-a797-925297ccd5ec
⛔ Files ignored due to path filters (2)
go.workis excluded by!**/*.workstringbuilderlint/go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
lookuptablelint/benchmark_test.gostringbuilderlint/analyzer.gostringbuilderlint/analyzer_test.gostringbuilderlint/benchmark_test.gostringbuilderlint/doc.gostringbuilderlint/go.modstringbuilderlint/testdata/src/a/a.gostringbuilderlint/testdata/src/a/a.go.goldenstringbuilderlint/testdata/src/fileignore/fileignore.gostringbuilderlint/testdata/src/sprintf/sprintf.gostringbuilderlint/testdata/src/sprintf/sprintf.go.golden
286be32 to
74a03a5
Compare
1 participant
Summary
stringbuilderlintso it covers supported primitivefmt.Sprintfcalls with suggested fixesstrings.Builderexpressionsuncheckederrlintfrom this PR after research founderrcheckwithcheck-blank: trueis the better commodity-linter pathStringbuilder Fix Shape
The generated fix uses an inline anonymous function so it can replace expressions in-place:
Dynamic operands are evaluated exactly once into generated locals before
Grow/writes. Unsupportedfmt.Sprintfformats and non-primitive arguments are skipped.Cutoffs
32dynamic operandsfmt.Sprintf: report when the builder output has at least two pieces, e.g. literal text plus a primitive conversionBenchmark Sample
Apple M4 Pro,
go test -run '^$' -bench ... -benchmem -count=3:15.6-16.0 ns/op, builder14.7-14.8 ns/op, both1 alloc17.4-18.0 ns/op, builder16.8-17.0 ns/op, both1 alloc20.3-20.4 ns/op, builder20.2-20.4 ns/op, both1 alloc26.4-26.7 ns/opvs builder29.7-30.0 ns/op46.9-47.4 ns/opvs builder60.6-61.1 ns/op91.0 ns/opvs builder87.2-87.6 ns/opfmt.Sprintf("id=%d", ...):33-34 ns/op,2 allocs; builder+strconv18.7-19.2 ns/op,2 allocsfmt.Sprintf("%s/%d/%t", ...):63.8-65.1 ns/op,3 allocs; builder+strconv25.3-25.4 ns/op,2 allocs5-9 ns/op1.6-1.9 ns/opThe benchmark files are committed so CI/devs can rerun on their machines.
Tests
PATH=/opt/homebrew/bin:$PATH go test ./...instringbuilderlintPATH=/opt/homebrew/bin:$PATH go test ./...inlookuptablelintPATH=/opt/homebrew/bin:$PATH go test -run '^$' -bench 'Benchmark(Concat|Builder|Sprintf)' -benchmem -count=3instringbuilderlintPATH=/opt/homebrew/bin:$PATH go test -run '^$' -bench 'Benchmark(Map|Switch)Lookup' -benchmem -count=3inlookuptablelint