feat(linters): add errortypeassertion analyzer for error-to-concrete assertions#42323
feat(linters): add errortypeassertion analyzer for error-to-concrete assertions#42323Copilot wants to merge 8 commits into
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Adds a new go/analysis-based custom linter (errortypeassertion) to the gh-aw linter suite to catch error→concrete type assertions that can miss wrapped errors, steering callsites toward errors.As.
Changes:
- Introduces
pkg/linters/errortypeassertionanalyzer and ananalysistestsuite with fixtures (including//nolint:errortypeassertioncoverage). - Registers the analyzer in the linter driver (
cmd/linters/main.go) and wires it into the linter spec verification (pkg/linters/spec_test.go). - Documents the new analyzer in
pkg/linters/README.md.
Show a summary per file
| File | Description |
|---|---|
pkg/linters/errortypeassertion/errortypeassertion.go |
Implements the analyzer that reports error type assertions to non-interface (concrete) targets. |
pkg/linters/errortypeassertion/errortypeassertion_test.go |
Adds analysistest runner for the new analyzer. |
pkg/linters/errortypeassertion/testdata/src/errortypeassertion/errortypeassertion.go |
Provides test fixtures covering allowed/flagged cases and nolint suppression. |
cmd/linters/main.go |
Registers errortypeassertion.Analyzer in the multichecker driver. |
pkg/linters/spec_test.go |
Adds the analyzer to the README-derived spec list to enforce public surface consistency. |
pkg/linters/README.md |
Documents the new analyzer in the subpackage lists/tables. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Low
| // Spec (README "Public API > Subpackages"): | ||
| // | ||
| // contextcancelnotdeferred, ctxbackground, deferinloop, errorfwrapv, excessivefuncparams, errormessage, | ||
| // errstringmatch, execcommandwithoutcontext, fileclosenotdeferred, fmterrorfnoverbs, fprintlnsprintf, | ||
| // errortypeassertion, errstringmatch, execcommandwithoutcontext, fileclosenotdeferred, fmterrorfnoverbs, fprintlnsprintf, |
There was a problem hiding this comment.
Fixed in aea9c62 — updated the count from 35 to 36.
🧪 Test Quality Sentinel — Score: 100/100 ✅ ExcellentThe single new test function ( 📊 Metrics & ClassificationLine change summary
Test function classification
Scoring breakdown
Guideline compliance
🔍 Detailed Test Analysis: TestAnalyzerFile: Pattern: Testdata fixture coverage (
Design invariants tested:
✅ Verdict: PASS — 100/100. All new tests are design tests (behavioral contracts), edge cases are thoroughly covered including nolint suppression and type-alias handling, no coding-guideline violations detected. Implementation ratio: 0% (threshold: 30%).
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 100/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). All new tests verify behavioral contracts with thorough edge case coverage including nolint suppression and type-alias handling.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (144 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Review: errortypeassertion analyzer
The implementation is correct and well-structured. The AST traversal logic, nolint suppression, alias handling, and test coverage for the stated scope all look solid.
Two non-blocking suggestions:
-
Document the intentional scope of
types.Identical(line 53 oferrortypeassertion.go). Only the exact built-inerrortype is matched; sub-interfaces likeinterface{ error; Timeout() bool }are silently skipped. The behavior is reasonable, but a one-line comment would prevent future maintainers from treating it as a gap to "fix". -
Add a test fixture for concrete types in type-switch case clauses (
testdata/.../errortypeassertion.go).GoodTypeSwitchuses only an interface case. Acase *os.PathError:example would explicitly document that case clauses are notTypeAssertExprnodes and will never produce false positives.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 57 AIC · ⌖ 6.95 AIC · ⊞ 4.9K
|
|
||
| assertedFrom := pass.TypesInfo.TypeOf(typeAssert.X) | ||
| if assertedFrom == nil || !types.Identical(assertedFrom, builtinErrorType) { | ||
| return |
There was a problem hiding this comment.
Scope of detection is narrower than it may appear. types.Identical matches only the exact built-in error type, so type assertions on sub-interfaces (e.g. interface { error; Timeout() bool } or named interfaces that embed error) are silently skipped:
func process(err interface{ error; Timeout() bool }) {
_ = err.(*os.PathError) // NOT flagged — assertedFrom is not identical to error
}This is a reasonable conservative choice, but it should be documented with a comment so future maintainers know the scope is intentional — not a gap:
// types.Identical is deliberate: only the exact built-in error type is in scope.
// Sub-interfaces (e.g. interface{ error; Timeout() bool }) are excluded to avoid
// false positives on legitimate narrowed-interface parameters.
if assertedFrom == nil || !types.Identical(assertedFrom, builtinErrorType) {@copilot please address this.
There was a problem hiding this comment.
Fixed in aea9c62. Added a prose comment above each types.Identical call explaining the exact-match scope and the rationale for excluding named interface types that embed error.
| func GoodTypeSwitch(err error) { | ||
| switch e := err.(type) { | ||
| case interface{ Temporary() bool }: | ||
| fmt.Println(e.Temporary()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing test for concrete type in type switch. GoodTypeSwitch only covers the interface-case branch. Adding a concrete-type case (e.g. case *os.PathError:) would explicitly document that type-switch case clauses are not false-positives — since those are ast.CaseClause nodes, not TypeAssertExpr, they're structurally excluded but the test doesn't demonstrate this.
func GoodTypeSwitchConcrete(err error) {
switch e := err.(type) {
case *os.PathError:
fmt.Println(e.Path) // should not be flagged
}
}@copilot please address this.
There was a problem hiding this comment.
Fixed in aea9c62. Added BadTypeSwitch to the fixture with a case *os.PathError: arm carrying a // want annotation to verify the new TypeSwitchStmt inspection fires correctly.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /zoom-out, /grill-with-docs, and /improve-codebase-architecture — approving with suggestions.
📋 Key Themes & Highlights
Key Themes
- Test coverage gap: All "bad" fixtures use only pointer types (
*os.PathError); a value-receiver struct case is missing.IsTestFilesuppression is also untested. - Scope boundary undocumented:
types.Identicalintentionally restricts to the exacterrortype — worth a comment to signal this is deliberate, not an oversight. - Auto-fix opportunity: No
analysis.SuggestedFixmeans IDEs can't offer auto-remediation for a mechanical transformation. - Doc string incomplete: The
Analyzer.Docfield doesn't mention the interface-assertion exemption, which will confuse users who read it ingo vetoutput.
Positive Highlights
- ✅ Clean, idiomatic implementation following the established analyzer pattern (
inspectpass,nolint,filecheck) - ✅ Alias case (
type errorAlias = error) is correctly covered bytypes.Identicaland tested - ✅ Type-switch guard skipped via
typeAssert.Type == nil— no special-casing needed - ✅ Spec test integration and README documentation updated atomically with the implementation
- ✅ Both single-value and two-value assertion forms are tested and correctly flagged
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 74.2 AIC · ⌖ 9.13 AIC · ⊞ 6.6K
Comment /matt to run again
|
|
||
| func BadAlias(err errorAlias) { | ||
| _ = err.(*os.PathError) // want `type assertion on error to \*os\.PathError bypasses wrapped errors; use errors\.As instead` | ||
| } |
There was a problem hiding this comment.
[/tdd] All three "bad" fixtures use *os.PathError (pointer to concrete type). A value-receiver concrete struct is not covered — the assertedTo.Underlying().(*types.Interface) branch for a plain struct type is never exercised.
💡 Suggested addition
type ValueTypeError struct{ code int }
func (e ValueTypeError) Error() string { return "error" }
func BadValueType(err error) {
_ = err.(ValueTypeError) // want `type assertion on error to errortypeassertion\.ValueTypeError bypasses...`
}This gives confidence the non-pointer code path is exercised and not accidentally gated behind a pointer-only check.
@copilot please address this.
| } | ||
|
|
||
| assertedFrom := pass.TypesInfo.TypeOf(typeAssert.X) | ||
| if assertedFrom == nil || !types.Identical(assertedFrom, builtinErrorType) { |
There was a problem hiding this comment.
[/zoom-out] types.Identical matches only the exact built-in error type. An interface that embeds error — e.g., interface { error; Code() int } — produces a different type object, so err.(*ConcreteType) on such a variable is silently not flagged.
💡 Context
This might be a deliberate choice to limit false positives (callers using a richer interface are presumed to know what they're doing). If so, a code comment here would clarify the intentional scope boundary:
// Intentionally matches only the exact built-in error type, not
// error-embedding interfaces, to avoid false positives on richer
// error interfaces whose callers have more context.
if assertedFrom == nil || !types.Identical(assertedFrom, builtinErrorType) {
return
}Alternatively, broaden to types.Implements(assertedFrom, builtinErrorType.Underlying().(*types.Interface)) after checking it's an interface type to catch the embedded case.
@copilot please address this.
| return | ||
| } | ||
|
|
||
| pass.ReportRangef( |
There was a problem hiding this comment.
[/improve-codebase-architecture] The analyzer reports but offers no analysis.SuggestedFix. Adding one would unlock IDE auto-fix support in gopls and go fix toolchains.
💡 Sketch of a suggested fix
For the single-value form _ = err.(*T) the fix is non-trivial (requires a fresh var declaration), but for the two-value form it's mechanical:
// Suggested fix for: if pathErr, ok := err.(*T); ok { ... }
// →
// var pathErr *T
// if errors.As(err, &pathErr) { ... }Even a partial fix (two-value form only) or a TextEdits-based replacement of the TypeAssertExpr node with a placeholder comment pointing to errors.As would be valuable. See how errstringmatch or other analyzers in this package handle SuggestedFix for a pattern to follow.
@copilot please address this.
| // Analyzer is the error-type-assertion analysis pass. | ||
| var Analyzer = &analysis.Analyzer{ | ||
| Name: "errortypeassertion", | ||
| Doc: "reports type assertions from error to concrete types; use errors.As for wrapped errors", |
There was a problem hiding this comment.
[/grill-with-docs] The Doc field only describes what's flagged. Per go/analysis convention, it should also document what's intentionally exempt, since IDEs and go vet surface this string directly to users.
💡 Suggested wording
Doc: "reports type assertions from the built-in error interface to concrete types; " +
"use errors.As for wrapped-error traversal. " +
"Interface-target assertions (err.(interface{ Timeout() bool })) and type-switch guards are not flagged.",This prevents confusion when users see a diagnostic and wonder why their interface assertion wasn't flagged.
@copilot please address this.
|
|
||
| func TestAnalyzer(t *testing.T) { | ||
| testdata := analysistest.TestData() | ||
| analysistest.Run(t, testdata, errortypeassertion.Analyzer, "errortypeassertion") |
There was a problem hiding this comment.
[/tdd] IsTestFile suppression is a deliberate feature (line 47 of the analyzer) but there's no fixture to verify it. If IsTestFile were accidentally removed or broken, no test would catch the regression.
💡 Suggested addition
Add a testdata/src/errortypeassertion/errortypeassertion_test.go fixture with a "bad" assertion that carries no // want annotation:
package errortypeassertion_test
import "os"
// Assertions in _test.go files should not be flagged.
func bad_in_test_file(err error) {
_ = err.(*os.PathError) // no want comment — suppressed by IsTestFile
}analysistest will fail if an unexpected diagnostic appears, making this a regression guard.
@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — one blocking correctness gap, two documentation/test gaps
The analyzer logic for type assertions from error to concrete types is sound and the test coverage for the cases it does handle is good. However, there is one blocking false-negative and two related documentation gaps that need to be resolved before merge.
Blocking issues
1. Type-switch concrete cases are silently undetected (line 40, errortypeassertion.go)
insp.Preorder([]ast.Node{(*ast.TypeAssertExpr)(nil)}, ...) only visits type-assertion expressions. In a type switch, the individual case-arm types are ast.Expr entries inside CaseClause.List — not TypeAssertExpr nodes — so:
switch e := err.(type) {
case *os.PathError: // NOT flagged — bypasses wrapped errors exactly like err.(*os.PathError)
log.Println(e.Path)
}...is never flagged. This is a material false negative given the stated goal (prevent
🔎 Code quality review by PR Code Quality Reviewer · 115.1 AIC · ⌖ 7.39 AIC · ⊞ 5.2K
Comment /review to run again
| } | ||
| builtinErrorType := builtinErrorObj.Type() | ||
|
|
||
| insp.Preorder([]ast.Node{(*ast.TypeAssertExpr)(nil)}, func(n ast.Node) { |
There was a problem hiding this comment.
Analyzer silently misses concrete-type arms in type-switch statements — false negative for a common wrapped-error pattern.
The walker only visits TypeAssertExpr nodes. In a type switch, the case arms are ast.Expr entries in CaseClause.List, not TypeAssertExpr nodes — so this pattern bypasses the linter entirely:
switch e := err.(type) {
case *os.PathError: // NOT flagged, but errors.As is still the correct fix
log.Println(e.Path)
}The typeAssert.Type == nil guard correctly skips the switch guard (err.(type)) itself, but the case-arm type expressions are never visited at all. The PR description mentions "type-switch guards" are excluded, but is silent on case arms — which have the identical wrapped-error traversal problem.
Fix: extend the node filter to also walk TypeSwitchStmt and check each CaseClause.List expression for concrete-error targets. If excluding case arms is intentional, document it and add a negative test to pin the scope boundary.
There was a problem hiding this comment.
Fixed in aea9c62. Added checkTypeSwitchStmt() which walks each CaseClause in switch err.(type) and flags any concrete-type arm (mirroring the same types.Identical + Underlying().(*types.Interface) guard used for TypeAssertExpr).
| _, _ = err.(interface{ Timeout() bool }) | ||
| } | ||
|
|
||
| func GoodTypeSwitch(err error) { |
There was a problem hiding this comment.
GoodTypeSwitch only tests interface-type cases — it does not document or test the concrete-type case gap.
The function name implies type switches are handled, but it only covers the interface-assertion case (interface{ Temporary() bool }), which is already excluded by the isInterface check. The more important boundary to document is what happens with a concrete-type case:
// This is also NOT flagged (case arms are CaseClause entries, not TypeAssertExpr):
func NotFlaggedTypeSwitch(err error) {
switch e := err.(type) {
case *os.PathError:
fmt.Println(e.Path)
}
}Without a test fixture covering this, future maintainers cannot tell whether the omission is intentional or a bug. Add a NotFlaggedTypeSwitch style function with no // want comment to explicitly document the scope boundary.
There was a problem hiding this comment.
Fixed in aea9c62. Added BadTypeSwitch to the fixture with case *os.PathError: carrying a // want annotation, alongside case interface{ Temporary() bool }: which is expected to remain unflagged.
| } | ||
|
|
||
| assertedFrom := pass.TypesInfo.TypeOf(typeAssert.X) | ||
| if assertedFrom == nil || !types.Identical(assertedFrom, builtinErrorType) { |
There was a problem hiding this comment.
types.Identical silently excludes named interface types that embed error — undocumented scope boundary.
types.Identical(assertedFrom, builtinErrorType) returns true only for the exact predeclared error type (and its type aliases). A named interface that embeds error produces a distinct *types.Named value, so this pattern is never flagged:
type AnnotatedError interface {
error
Code() int
}
func bad(e AnnotatedError) {
_ = e.(*os.PathError) // NOT flagged — same wrapped-error problem, different named type
}This may be intentional (the PR description says built-in
There was a problem hiding this comment.
Fixed in aea9c62. Added a comment above both types.Identical call-sites explaining that it intentionally matches only the exact built-in error type; named interface types that embed error are excluded because they carry additional methods that may justify a direct assertion.
|
|
||
| builtinErrorObj := types.Universe.Lookup("error") | ||
| if builtinErrorObj == nil { | ||
| return nil, nil |
There was a problem hiding this comment.
Silent nil, nil return on an impossible-but-checked condition hides misconfiguration silently.
types.Universe.Lookup("error") can never return nil in a well-formed Go toolchain — error is a predeclared type. By returning (nil, nil) here instead of an error, a hypothetical misconfiguration would produce a completely silent, clean-but-empty analysis result with no indication anything went wrong. Prefer failing loudly:
if builtinErrorObj == nil {
return nil, fmt.Errorf("errortypeassertion: could not find built-in error type in universe scope")
}A hard failure on invariant violation is far easier to diagnose than a silent no-op.
There was a problem hiding this comment.
Fixed in aea9c62. Replaced the return nil, nil with panic("errortypeassertion: types.Universe does not contain built-in error type") and added a comment explaining this branch is unreachable under a functioning Go toolchain.
|
@copilot please run the Unresolved review threads (newest first): CHANGES_REQUESTED — github-actions[bot] (PR Code Quality Reviewer):
COMMENTED — Copilot: COMMENTED — github-actions[bot] (Impeccable Skills Reviewer): Branch refresh: This branch is 4 commits behind ADR required: Complete and link the draft ADR in the PR body:
|
…ossible nil, document types.Identical scope, fix spec count Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
All unresolved threads addressed in aea9c62:
Local validation: |
|
@copilot lint go |
This adds a new custom Go analyzer,
errortypeassertion, to catcherrortype assertions to concrete types (including both single-value and two-value forms) and steer callsites toerrors.Asfor wrapped error traversal. The goal is to prevent correctness bugs where direct assertions miss wrapped errors.New analyzer:
pkg/linters/errortypeassertionTypeAssertExprwhen:errortype, anderrors.As.//nolint:errortypeassertion.Analyzer coverage
analysistestsuite and fixtures covering:err.(*os.PathError),pathErr, ok := err.(*os.PathError)errors.As(err, &pathErr),err.(interface{ Timeout() bool })type errorAlias = errornolintsuppression behaviorIntegration and docs
cmd/linters/main.go.pkg/linters/spec_test.go).pkg/linters/README.md(overview, subpackages, dependencies).