refactor(pkg/linters): extract helpers to eliminate all 33 large-function findings#47022
Conversation
…dings 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
Refactors pkg/linters analyzers to eliminate large-function findings while preserving behavior.
Changes:
- Extracts traversal callbacks and analysis helpers.
- Splits complex matching and import-edit construction.
- Adds focused helper documentation and required inspector imports.
Show a summary per file
| File | Description |
|---|---|
pkg/linters/appendbytestring/appendbytestring.go |
Extracts append-call analysis. |
pkg/linters/appendoneelement/appendoneelement.go |
Extracts spread-literal matching. |
pkg/linters/bytesbufferstring/bytesbufferstring.go |
Extracts buffer-call matching. |
pkg/linters/bytescomparestring/bytescomparestring.go |
Extracts comparison and import-edit logic. |
pkg/linters/errorfwrapv/errorfwrapv.go |
Extracts error-argument classification. |
pkg/linters/execcommandwithoutcontext/execcommandwithoutcontext.go |
Extracts cursor-based command analysis. |
pkg/linters/hardcodedfilepath/hardcodedfilepath.go |
Extracts path-literal analysis. |
pkg/linters/httpnoctx/httpnoctx.go |
Extracts HTTP-call analysis. |
pkg/linters/ioutildeprecated/ioutildeprecated.go |
Separates qualified and dot-import checks. |
pkg/linters/largefunc/largefunc.go |
Extracts function-length checking. |
pkg/linters/lenstringzero/lenstringzero.go |
Extracts length-comparison analysis. |
pkg/linters/mapclearloop/mapclearloop.go |
Extracts range and delete matching. |
pkg/linters/mapdeletecheck/mapdeletecheck.go |
Extracts conditional-delete matching. |
pkg/linters/nilctxpassed/nilctxpassed.go |
Extracts cursor-based context checking. |
pkg/linters/seenmapbool/seenmapbool.go |
Separates candidate collection and validation. |
pkg/linters/sprintfbool/sprintfbool.go |
Separates collection, fixability, and reporting. |
pkg/linters/sprintferrdot/sprintferrdot.go |
Extracts format-call analysis. |
pkg/linters/sprintfint/sprintfint.go |
Extracts integer-format analysis. |
pkg/linters/strconvparseignorederror/strconvparseignorederror.go |
Extracts assignment analysis. |
pkg/linters/stringscountcontains/stringscountcontains.go |
Extracts count-comparison analysis. |
pkg/linters/stringsindexcontains/stringsindexcontains.go |
Extracts index-comparison analysis. |
pkg/linters/timeafterleak/timeafterleak.go |
Extracts single-case select detection. |
pkg/linters/timenowsub/timenowsub.go |
Extracts time-call analysis. |
pkg/linters/trimleftright/trimleftright.go |
Extracts trim-call analysis. |
pkg/linters/writebytestring/writebytestring.go |
Extracts call, argument, and import-edit helpers. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 25/25 changed files
- Comments generated: 2
- Review effort level: Medium
| // A default clause (CommClause with nil Comm) can preempt the timer and is | ||
| // not counted as "another case" here; thus such selects are still flagged. |
There was a problem hiding this comment.
Fixed in commit f7ffb10. Updated the doc to say the default clause is counted as another clause, so a select with a timer case plus a default returns false (multi-case) and is reportable.
| // Handle dot imports: import . "io/ioutil" followed by bare ReadAll(r) or Discard. | ||
| // In this case the identifier is an *ast.Ident (not a SelectorExpr), and | ||
| // TypesInfo.Uses resolves it to an object whose package path is "io/ioutil". | ||
| // checkDotImportIoutilUsage reports bare ioutil function names used via dot-imports. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. This PR (#47022) contains only refactoring of linter source files in pkg/linters/ to extract helper functions and eliminate large-function findings. Test Quality Sentinel skipped. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
❌ Design Decision Gate 🏗️ failed during design decision gate check. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Clean mechanical refactoring
This is a well-executed, purely mechanical refactoring: 33 Preorder callback bodies and cursor for-loop bodies are extracted into named helpers across 25 files. I reviewed the changed lines for correctness, behavioral equivalence, and logic errors.
Behavioural equivalence verified for the trickiest cases:
errorfwrapv:classifyErrorArgs+hasVerbVgate correctly replicates the original inline%vshort-circuit. Thesuppressednolint check is applied at the same call position in both paths.seenmapbool:collectSeenMapCandidates/findNonSetMapssplit preserves the early-exit on empty candidates and the two-pass detection logic.sprintfbool: the three-function decomposition is equivalent;filesByPosis correctly threaded through.mapclearloop/mapdeletecheck:matchDeleteBodyandmatchDeleteCallStmtpreserve all previous guards (builtin identity check, arg count, same-object/same-expr tests).
Two existing review comments already call out the only real issues found:
timeafterleakline 152 — theisSingleCaseSelectdoc comment states that adefaultclause is not counted as another case, but the implementation returnsfalsewhen it encounters one, making the explanation misleading.ioutildeprecatedline 96 —checkDotImportIoutilUsagedescription calls all entries "function names" butioutil.Discardis a variable.
No additional high-signal issues found in the changed lines.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 53.6 AIC · ⌖ 4.47 AIC · ⊞ 4.9K
Records the architectural decision to extract large inline function literals and cursor-loop bodies into named package-level helpers in pkg/linters to resolve 33 largefunc lint violations. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Mechanical refactor is sound — APPROVE
Reviewed all 25 changed files. Every extracted helper faithfully replicates the original inline logic: no variable shadowing, no changed early-return semantics, no incorrect parameter conversions, no resource leaks.
Key points verified
Control-flow equivalence
- All Preorder closures and cursor-range bodies pass the same pass, generatedFiles, noLintIndex (and seenImportFiles / cursor where applicable) — no implicit captures dropped.
- errorfwrapv: formatArgOffset promotion from local const to package-level const is safe; const is not addressable, no data race possible.
- timeafterleak/isSingleCaseSelect: correctly returns false (do not flag) only when cc is the sole non-default clause, matching the original loop semantics.
- sprintfbool: two-phase split (collect → compute orphan → report) correctly threads seenImportFiles into the reporting phase only, where import edits are emitted.
- bytescomparestring buildBytesImportTextEdit split: three-strategy (grouped block / single non-grouped / standalone) logic is preserved intact.
No new blocking issues found. Two existing review comments (timeafterleak doc at line 152, ioutildeprecated naming at line 96) were already filed.
🔎 Code quality review by PR Code Quality Reviewer · 143.7 AIC · ⌖ 4.44 AIC · ⊞ 5.6K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — three observations on the extracted helpers, no blocking correctness issues.
📋 Key Themes & Highlights
Key Themes
- Doc/code mismatch on
isSingleCaseSelect: the comment says default clauses are excluded from the multi-case count, but the implementation treats them as another CommClause and flags them. This needs either a doc fix or a code fix (depending on intended semantics). - Dropped inline comment in
largefunc: the-1in the line-count formula had an explanatory comment that was not carried over to the extracted function. nolintcheck moved earlier inerrorfwrapv:suppressedis now computed unconditionally for everyfmt.Errorfcall rather than only when a violation is about to be reported — minor performance regression for hot paths.
Positive Highlights
- ✅ Mechanical extraction is consistent across all 25 files — naming conventions (
analyzeX,checkX,matchX,buildX) are applied uniformly. - ✅ All doc comments on new helpers are present and correctly scoped.
- ✅
classifyErrorArgsandneedsWrappingare genuinely reusable units; theerrorfwrapvrefactoring is the most architecturally meaningful extraction in the PR. - ✅
matchDeleteBodyandextractRangeKeyObjgivemapclearloopa clean two-phase structure (validate shape → validate semantics) that's easier to audit than the original monolithic callback. - ✅ Existing test suite covers behavior; no regressions expected.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 119.7 AIC · ⌖ 4.91 AIC · ⊞ 6.6K
Comment /matt to run again
| } | ||
| start := pass.Fset.Position(body.Lbrace) | ||
| end := pass.Fset.Position(body.Rbrace) | ||
| lines := end.Line - start.Line - 1 |
There was a problem hiding this comment.
[/codebase-design] The comment explaining the -1 in the line-count formula was dropped during extraction — this is a non-obvious off-by-one guard that new readers will need to re-derive.
💡 Restore the inline comment
lines := end.Line - start.Line - 1 // subtract 1: exclude the closing brace line, count only body linesThe original closure had // Subtract 1 to exclude the closing brace line itself, counting only body lines. at this site. Keeping it here avoids future readers having to consult git history to understand why the subtraction exists.
@copilot please address this.
There was a problem hiding this comment.
Fixed in commit f7ffb10. Restored the inline comment: // subtract 1: exclude the closing brace line, count only body lines.
| return | ||
| } | ||
| suppressed := nolint.HasDirectiveForLinter(position, noLintIndex, "errorfwrapv") | ||
| verbs := parseFormatVerbs(lit.Value) |
There was a problem hiding this comment.
[/codebase-design] After extraction, suppressed is computed before it is used — but when hasVerbV is false and no unwrapped error args are found the value is wasted. More importantly, calling nolint.HasDirectiveForLinter unconditionally on every fmt.Errorf call (even those that are already safe) adds a small but measurable overhead on large codebases.
💡 Defer the nolint check
In the original code suppressed was computed only after we already knew the call had a %v or unwrapped error arg. Moving it back to just before each ReportRangef (or passing it lazily) preserves the original performance profile:
// only look up the directive when we're about to report
if nolint.HasDirectiveForLinter(position, noLintIndex, "errorfwrapv") {
return
}
pass.ReportRangef(...)This is low priority for correctness but worth noting for hot-path linter functions.
@copilot please address this.
There was a problem hiding this comment.
Fixed in commit f7ffb10. Removed the upfront suppressed variable and moved the nolint.HasDirectiveForLinter call to just before each pass.ReportRangef. The two call sites are in mutually exclusive branches (the if hasVerbV block always returns before the for-loop), so at most one lookup executes per invocation.
| // Handle dot imports: import . "io/ioutil" followed by bare ReadAll(r) or Discard. | ||
| // In this case the identifier is an *ast.Ident (not a SelectorExpr), and | ||
| // TypesInfo.Uses resolves it to an object whose package path is "io/ioutil". | ||
| // checkDotImportIoutilUsage reports bare ioutil function names used via dot-imports. |
There was a problem hiding this comment.
[/codebase-design] The doc comment says "bare ioutil function names" but ioutil.Discard is an io.Writer variable, not a function — the description is inaccurate and will mislead future contributors who add new entries to replacements.
💡 Suggested fix
// checkDotImportIoutilUsage reports bare ioutil identifiers (functions and variables)
// used via dot-imports (import . "io/ioutil").This also makes the skip guard on line 101 easier to understand: it's skipping identifiers that are already the Sel field of a qualified selector, not just function selectors.
@copilot please address this.
There was a problem hiding this comment.
Fixed in commit f7ffb10. Updated the doc to: // checkDotImportIoutilUsage reports bare ioutil identifiers (functions and variables) // used via dot-imports (import . "io/ioutil").
| // timer must fire — no accumulation is possible. | ||
| // A default clause (CommClause with nil Comm) can preempt the timer and is | ||
| // not counted as "another case" here; thus such selects are still flagged. | ||
| func isSingleCaseSelect(clauseCur inspector.Cursor, cc *ast.CommClause) bool { |
There was a problem hiding this comment.
[/codebase-design] The doc comment says a default clause (nil Comm) "is not counted as another case here; thus such selects are still flagged," but the implementation does the opposite: it iterates sel.Body.List and returns false (not single-case) only for non-cc CommClauses — a default: clause is a *ast.CommClause with Comm == nil, so it would cause return false (mark as multi-case) and be flagged. The doc contradicts the code.
💡 Align doc with implementation
If the intent is that a select with a timer case + default should still be flagged, the current code is correct but the doc is wrong. Update the comment:
// isSingleCaseSelect reports whether cc is the only CommClause in its enclosing
// SelectStmt. A select with a default clause is treated as multi-case and will
// be flagged, because a default can preempt the timer and still accumulate timers.If the intent was to not count default clauses (and thus not flag timer+default selects), the loop condition needs to exclude other.Comm == nil.
@copilot please address this.
There was a problem hiding this comment.
Fixed in commit f7ffb10. Updated the doc to accurately describe the implementation: a default clause (CommClause with nil Comm) is a *ast.CommClause and IS counted as another clause, so a select with a timer case plus a default returns false (multi-case) and is flagged as reportable.
|
@copilot run pr-finisher skill |
|
@copilot please run the
Run: https://github.com/github/gh-aw/actions/runs/29826418446 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
…eferral - timeafterleak: fix isSingleCaseSelect doc — default clause IS counted as another clause, making timer+default selects reportable - ioutildeprecated: expand checkDotImportIoutilUsage doc to say 'identifiers (functions and variables)' and mention import path - largefunc: restore inline comment explaining -1 in line-count formula - errorfwrapv: defer nolint lookup to just before each ReportRangef Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
All four items were already addressed in commit
CI needs a human re-trigger (close/reopen or |
make golint-customreported 33 large-function violations (>60 body lines) acrosspkg/lintersanalyzer files. The root cause wasrunfunctions accumulating traversal, type-checking, and fix-generation logic inline — both as Preorder callbacks and cursor-range for-loop bodies.Strategy
Two mechanical patterns applied across 25 files:
Preorder callback → named function
Cursor for-loop body → named function (for
for cur := range insp.Root().Preorder(...)pattern)Files changed
mapclearloop,errorfwrapv,seenmapbool,mapdeletecheck,writebytestring,sprintfbool,bytescomparestringexeccommandwithoutcontext,hardcodedfilepath,httpnoctx,largefunc,lenstringzero,nilctxpassed,sprintferrdot,sprintfint,strconvparseignorederror,stringscountcontains,stringsindexcontains,appendoneelement,appendbytestring,bytesbufferstring,timenowsub,trimleftright,ioutildeprecatedNon-obvious details
errorfwrapv:formatArgOffset = 1promoted from localconstto package-level — eliminates one line and removes the need to pass it as a parameter toclassifyErrorArgstimeafterleak:isInsideLoopSelectComm(65 lines, not arunfunction) split by extractingisSingleCaseSelect(clauseCur, cc)for the single-case select guardexeccommandwithoutcontextandnilctxpassedneeded"golang.org/x/tools/go/ast/inspector"added to imports for theinspector.Cursorparameter type in extracted functionsbytescomparestringandwritebytestring: their import-edit helpers (addBytesImportEdit,addIOImportEdit) were also over limit — split into a guard function +buildXxxImportTextEditfor the three-strategy edit constructionTextEdits are unchanged