uncheckedflushreturn: flag discarded errors in defer ...Flush()

[Copilot]

uncheckedflushreturn only inspected ExprStmt and AssignStmt, so defer w.Flush() (the most common dropped-flush-error pattern) was never visited or reported. This change extends analyzer coverage to deferred calls and adds focused testdata for deferred flagged and suppressed forms.

• Analyzer scope expansion

  • Added *ast.DeferStmt to the inspector node filter.
  • Added DeferStmt handling in run() and wired it to existing flush/error detection and reporting flow.

• Deferred flush check

  • Introduced checkDiscardedFlushDefer(...) to inspect stmt.Call and report when Flush() returns an error that is discarded.

• Testdata coverage

  • Added deferBad() case expecting the standard unchecked-flush diagnostic.
  • Added deferSuppressed() case validating //nolint:uncheckedflushreturn suppression for deferred form.

nodeFilter := []ast.Node{
    (*ast.ExprStmt)(nil),
    (*ast.AssignStmt)(nil),
    (*ast.DeferStmt)(nil),
}

• Fixes uncheckedflushreturn: node filter omits DeferStmt — defer w.Flush() (the most common discard pattern) is never flagged #49902

[github-actions]

Triage Result

Category: refactor | Risk: low | Score: 38/100 (impact 12, urgency 10, quality 16)

Recommended action: defer

Draft PR, CI pending, small lint-focused change (2 files, 31+/1-) flagging discarded defer Flush() errors. Low urgency, safe to batch with similar lint PRs later.

Generated by 🔧 PR Triage Agent · auto · 27.4 AIC · ⌖ 2.42 AIC · ⊞ 8K · ◷

[github-actions]

✅ Design Decision Gate 🏗️ completed the design decision gate check.

No ADR enforcement needed: PR #49908 does not have the implementation label and has only 31 new lines of code in business logic directories (threshold: 100).

[github-actions]

🧠 Matt Pocock Skills Reviewer failed during the skills-based review.

[github-actions]

⚠️ PR Code Quality Reviewer failed during code quality review.

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

[Copilot]

Pull request overview

Extends uncheckedflushreturn to detect discarded errors from deferred Flush() calls.

Changes:

• Adds ast.DeferStmt analysis using existing flush validation/reporting.
• Adds tests for diagnostics and nolint suppression.

Show a summary per file

File	Description
pkg/linters/uncheckedflushreturn/uncheckedflushreturn.go	Detects unchecked deferred flush calls.
pkg/linters/uncheckedflushreturn/testdata/src/uncheckedflushreturn/uncheckedflushreturn.go	Covers flagged and suppressed deferred calls.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 2/2 changed files
• Comments generated: 0
• Review effort level: Balanced

[github-actions]

The implementation is correct and well-structured. DeferStmt is properly added to the node filter, checkDiscardedFlushDefer follows the established pattern, nolint suppression resolves correctly via call.Pos(), and testdata covers both the flagged and suppressed forms.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 17 AIC · ⌖ 7.36 AIC · ⊞ 5.4K

[github-actions]

🧪 Test Quality Sentinel Report

✅ Test Quality Score: N/A — No Test Changes

No *_test.go or .test.cjs/.test.js files were added or modified in this PR.
The only changed files are:

• pkg/linters/uncheckedflushreturn/uncheckedflushreturn.go (production linter logic)
• pkg/linters/uncheckedflushreturn/testdata/src/uncheckedflushreturn/uncheckedflushreturn.go (analyzer test fixture/testdata)

The testdata file is an input corpus for the Go analysis framework (analysistest), not a behavioral test file. No scoring applies.

Verdict

✅ passed. No test files changed; Test Quality Sentinel skipped.

🧪 Test quality analysis by Test Quality Sentinel · sonnet46 · 24 AIC · ⌖ 8.3 AIC · ⊞ 8.5K · ◷
_{Comment /review to run again}

[github-actions]

✅ Test Quality Sentinel: N/A — no test files changed. The testdata fixture is an analysistest corpus, not a behavioral test. No violations.

[pelikhan]

/matt

[github-actions]

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

[github-actions]

Skills-Based Review 🧠

Applied /tdd and /codebase-design — changes are solid, approving.

📋 Key Themes & Highlights

Key Themes

• Consistent pattern reuse: DeferStmt handling mirrors the existing ExprStmt/AssignStmt pattern faithfully — filename-skip guard in the switch case, logic delegated to a dedicated checkDiscardedFlushDefer helper.
• Defensive nil check: stmt.Call == nil guard in checkDiscardedFlushDefer is technically unreachable for well-formed AST but follows safe-coding convention.
• Test coverage: both deferBad (flagged) and deferSuppressed (nolint suppression) cases are present, mirroring the coverage shape of the existing tests.

Positive Highlights

• ✅ Minimal, surgical diff — only the missing code path is added
• ✅ New function is self-contained and easy to reason about
• ✅ Testdata covers both the positive (diagnostic fired) and suppression (nolint respected) cases
• ✅ PR description is clear and accurately describes what was changed

Minor Observation (non-blocking)

The defer x.Flush() pattern is genuinely hard to fix at the call site. The idiomatic workaround is a named-return closure:

defer func() { err = bw.Flush() }()

Consider adding a suggested fix hint to the diagnostic message so the linter is immediately actionable. Not a blocker.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 19 AIC · ⌖ 14.4 AIC · ⊞ 7.1K
_{Comment /matt to run again}

[github-actions]

🎉 This pull request is included in a new release.

Release: v0.84.4