Repository navigation
fix(linters): handle parenthesized callees in walkdirignoredeerror, uncheckedflushreturn, sprintferrorsnew, httprespbodyclose - #75599
Conversation
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ PR Specification Reviewer completed successfully! PR Specification ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues were identified for the parenthesis-handling updates across the four Go linters.
No open issues 🎉
What changed in this PR
This PR extends the earlier parenthesis-handling fixes to four gh-aw Go linters, improving diagnostic consistency for equivalent Go expressions.
Changes:
- Preserve directory-walk diagnostics by using resolved function names, including for dot-imported calls.
- Unwrap parenthesized expressions in flush, error-formatting, and HTTP response-body checks.
- Add regression fixtures for missed diagnostics and correctly deferred response-body closes.
| File | Description |
|---|---|
pkg/linters/walk-dir-ignored-error/walkdirignoredeerror.go |
Report using resolved function names without rechecking selector syntax. |
pkg/linters/walk-dir-ignored-error/testdata/src/walkdirignoredeerror/walkdirignoredeerror.go |
Cover parenthesized Walk and WalkDir callees. |
pkg/linters/walk-dir-ignored-error/testdata/src/walkdirignoredeerror/dotimport.go |
Cover discarded and handled errors from dot-imported calls. |
pkg/linters/uncheckedflushreturn/uncheckedflushreturn.go |
Recognize parenthesized Flush callees. |
pkg/linters/uncheckedflushreturn/testdata/src/uncheckedflushreturn/uncheckedflushreturn.go |
Cover bare, blank-assigned, and deferred parenthesized Flush calls. |
pkg/linters/sprintferrorsnew/testdata/src/sprintferrorsnew/sprintferrorsnew.go |
Cover parentheses around outer callees, arguments, and inner callees. |
pkg/linters/sprintferrorsnew/sprintferrorsnew.go |
Unwrap expressions when matching errors.New(fmt.Sprintf(...)). |
pkg/linters/httprespbodyclose/testdata/src/httprespbodyclose/parens.go |
Cover manual parenthesized closes and a deferred-close regression. |
pkg/linters/httprespbodyclose/httprespbodyclose.go |
Unwrap close callees, receivers, and statement expressions. |
Review effort: Balanced (auto)
✅ Design Decision Gate — ADR VerifiedADR reviewed: ADR-75294: Unwrap Parenthesized Expressions in Linter Matchers (link unavailable: invalid HTTPS URL) — implementation aligns with the stated decision. Great work! 🏗️ 📋 Verification Summary
Next action: keep the ADR referenced in the PR description for discoverability.
|
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Found one blocking correctness gap: the sprintferrorsnew changes unwrap the outer callee and inner call, but package qualifiers remain unwrapped, so parenthesized forms like (errors).New(...) and (fmt).Sprintf(...) are still missed. Please address this and add coverage. Static review only; tests were not run in this environment.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · ~4 AIC · ⌖ ~1 AIC · ⊞ 1.6K
Comment /review to run again
| sel, ok := call.Fun.(*ast.SelectorExpr) | ||
| // Match errors.New(...), including a parenthesized callee such as | ||
| // (errors.New)(...). | ||
| sel, ok := astutil.UnwrapParenExpr(call.Fun).(*ast.SelectorExpr) |
There was a problem hiding this comment.
This unwraps only the call expression itself, but IsPkgSelector still requires sel.X to be a bare *ast.Ident. Therefore forms such as (errors).New(fmt.Sprintf("...")) and errors.New((fmt).Sprintf("...")) remain false negatives, despite the new parenthesized-callee coverage. Please unwrap sel.X (or update the helper) and add fixtures for package-qualifier parentheses.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnosing-bugs. Requesting changes for one missed-diagnostic gap in httprespbodyclose. The other three linters look correct.
📋 Key Themes & Highlights
Key Themes
- Incomplete paren handling in
httprespbodyclose: the PR fixes the callee, receiver, andExprStmt/Rhspaths inmarkBodyClose, buttrackHTTPAssignmentstill uses barelhs.(*ast.Ident)andrhs.(*ast.CallExpr). A fully parenthesized assignment RHS,resp, err := (client.Do(req)), is never tracked, so the leak diagnostic is silently dropped. Reproduced with a temporary fixture (since removed); it passes once those two assertions useastutil.UnwrapParenExpr. See the inline comment. - Regression coverage gap:
parens.gocovers the callee and receiver forms but not this RHS form. Adding aBadParenthesizedAssignRHScase would lock in the fix.
Verification
- Ran
go test -count=1on all four changed packages: all pass. The PR notesgo testwas not run in its sandbox, so this closes that gap. gofmt -lon the four packages: clean.
Positive Highlights
- ✅
walkDirCallNamederives the name from type info. This removes a silent false negative for dot-importedWalk(...)and deletes the early return. Clean fix. - ✅ The
(resp.Body.Close)()defer regression case (GoodParenDeferClose) correctly pins the false positive that was fixed. - ✅ Consistent use of the shared
astutil.UnwrapParenExprhelper across all four linters.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · routed: haiku55 xhigh · ~4 AIC · ⌖ ~1 AIC · ⊞ 5.5K
Comment /matt to run again
| // Also check RHS for manual Body.Close() in assignments like: err := resp.Body.Close() | ||
| for _, rhs := range assign.Rhs { | ||
| if call, ok := rhs.(*ast.CallExpr); ok { | ||
| if call, ok := astutil.UnwrapParenExpr(rhs).(*ast.CallExpr); ok { |
There was a problem hiding this comment.
[/tdd] [/diagnosing-bugs] The assignment tracking path still misses parenthesized forms. trackHTTPAssignment (lines 184 and 197) still uses bare lhs.(*ast.Ident) and rhs.(*ast.CallExpr), so resp, err := (client.Do(req)) is never tracked and the missing-defer diagnostic is silently dropped. I reproduced this with a temporary fixture: it fails on this branch and passes with the unwrap change below applied to lines 184 and 197.
💡 Suggested fix and regression test
In trackHTTPAssignment:
ident, ok := astutil.UnwrapParenExpr(lhs).(*ast.Ident)
...
if call, ok := astutil.UnwrapParenExpr(rhs).(*ast.CallExpr); ok {Add this case to parens.go so the gap stays covered:
// BadParenthesizedAssignRHS wraps the whole assignment RHS in parentheses.
func BadParenthesizedAssignRHS(client *http.Client, req *http.Request) ([]byte, error) {
resp, err := (client.Do(req)) // want `HTTP response Body\.Close\(\) should be deferred immediately after receiving the response to prevent resource leaks`
if err != nil {
return nil, err
}
data, readErr := io.ReadAll(resp.Body)
resp.Body.Close()
return data, readErr
}@copilot please address this.
Summary
Fixes four linters that missed parenthesized callees. One of them also dropped diagnostics it had already detected. This continues the Sergo paren-unwrap series (#74277, #74382, #74737, #75085, #75294); all of those PRs are merged.
Findings
walkdirignoredeerrorwalkdirignoredeerror.go:112astutil.PackageCallalready resolves(filepath.Walk)(...)and dot-importedWalk(...), butreportIfNotSkippedthen re-assertedcall.Fun.(*ast.SelectorExpr)only to get the name, and returned early when that faileduncheckedflushreturnisFlushCallReturningErrorcall.Fun.(*ast.SelectorExpr)without unwrapping(w.Flush)(),_ = (w.Flush)(),defer (w.Flush)()not reportedsprintferrorsnewrunlines 40/52/56(errors.New)(fmt.Sprintf(..)),errors.New((fmt.Sprintf(..))),errors.New((fmt.Sprintf)(..))not reported. gopls/staticcheck S1028 already flags all three forms, which confirms they are the same patternhttprespbodyclosebodyCloseReceiver,inspectNodeBodyreceiver, response ident, and theExprStmt/assignment RHS not unwrappeddefer (resp.Body.Close)()was not seen as a deferred close, so a function that also closed manually on an early-return branch was reported. Manual(resp.Body.Close)()/(resp.Body).Close()were missedChanges
walkdirignoredeerror:isWalkDirCallbecomeswalkDirCallName, which returns the function name resolved from type information. The reporter takes that name, so theSelectorExprre-check and its early return are gone.uncheckedflushreturn,sprintferrorsnew,httprespbodyclose: wrap the relevant expressions inastutil.UnwrapParenExpr.walkdirignoredeerror: parenthesized callees, plus a newdotimport.gocovering. "path/filepath"uncheckedflushreturn:badParenthesizedCalleecovering the bare, blank-assign and defer formssprintferrorsnew:badParenthesizedwith 3 formshttprespbodyclose: newparens.gowith 2 bad cases and 1 regression case for the false positive (GoodParenDeferClose)Validation
get_diagnostics_for_file(gopls) on all 4 changed analyzer sources: 0 diagnostics.unusedfuncinfo and S1028 warnings. That means they type-check, including the dot-import fixture.go testandgofmtwere not run: both commands are denied in this sandbox. CI should rungo test ./pkg/linters/walk-dir-ignored-error/ ./pkg/linters/uncheckedflushreturn/ ./pkg/linters/sprintferrorsnew/ ./pkg/linters/httprespbodyclose/. I aligned fixture comments by hand to match gofmt.🤖 Generated with [Claude Code]((claude.com/redacted)