[linter-miner] feat(linters): add uncheckedtypeassertion linter (run #18)#34738
Conversation
Reports single-value type assertions x.(T) that may panic at runtime when the dynamic type does not match. Flags cases where the safe two-value form v, ok := x.(T) should be used instead. Evidence: issue #34580 — unchecked type assertion in GraphQL response parsing caused a panic on unexpected nil/wrong-type values, while sibling fields used the safe ok idiom. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
automation
cookie
|
@copilot lint go |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Adds a new custom Go go/analysis analyzer (uncheckedtypeassertion) to flag single-value type assertions (x.(T)) in non-test code, encouraging the safer two-result form to avoid runtime panics.
Changes:
- Introduces
uncheckedtypeassertionanalyzer implementation and wiring. - Adds
analysistestcoverage with fixtures for good/bad assertion patterns. - Registers the analyzer in
cmd/lintersso it runs with the existing multichecker binary.
Show a summary per file
| File | Description |
|---|---|
| pkg/linters/uncheckedtypeassertion/uncheckedtypeassertion.go | Implements the analyzer logic to detect unchecked single-value type assertions. |
| pkg/linters/uncheckedtypeassertion/uncheckedtypeassertion_test.go | Adds an analysistest harness for the new analyzer. |
| pkg/linters/uncheckedtypeassertion/testdata/src/uncheckedtypeassertion/uncheckedtypeassertion.go | Provides good/bad fixtures with // want expectations. |
| cmd/linters/main.go | Registers the new analyzer in the linter multichecker. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
pkg/linters/uncheckedtypeassertion/uncheckedtypeassertion.go:66
- The “safe two-value form” skip logic only handles
*ast.AssignStmt(e.g.v, ok := x.(T)), butvar v, ok = x.(T)is also the safe two-result form and will be incorrectly reported. Consider also recognizing a parent*ast.ValueSpec(and verifying thisTypeAssertExpris the sole RHS value) as a safe two-value usage.
// Skip the safe two-value form: v, ok := x.(T) or v, ok = x.(T)
if parents != nil {
if assign, ok := parents[typeAssert].(*ast.AssignStmt); ok {
if len(assign.Lhs) == 2 && len(assign.Rhs) == 1 {
return
}
}
- Files reviewed: 4/4 changed files
- Comments generated: 3
| "go/ast" | ||
|
|
||
| "golang.org/x/tools/go/analysis" | ||
| "golang.org/x/tools/go/analysis/passes/inspect" | ||
| "golang.org/x/tools/go/ast/inspector" | ||
|
|
||
| "github.com/github/gh-aw/pkg/linters/internal/filecheck" | ||
| ) | ||
|
|
||
| // Analyzer is the unchecked-type-assertion analysis pass. | ||
| var Analyzer = &analysis.Analyzer{ | ||
| Name: "uncheckedtypeassertion", | ||
| Doc: "reports single-value type assertions that may panic if the dynamic type does not match", | ||
| URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/uncheckedtypeassertion", | ||
| Requires: []*analysis.Analyzer{inspect.Analyzer}, | ||
| Run: run, | ||
| } | ||
|
|
||
| func run(pass *analysis.Pass) (any, error) { | ||
| insp := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) | ||
|
|
||
| // Build a parent map for each file so we can detect the two-value form. | ||
| fileParents := make(map[*ast.File]map[ast.Node]ast.Node) | ||
| for _, f := range pass.Files { | ||
| fileParents[f] = buildParentMap(f) | ||
| } | ||
|
|
||
| nodeFilter := []ast.Node{ | ||
| (*ast.TypeAssertExpr)(nil), | ||
| } | ||
|
|
||
| insp.Preorder(nodeFilter, func(n ast.Node) { | ||
| typeAssert := n.(*ast.TypeAssertExpr) | ||
|
|
||
| // Type-switch guards have nil Type; skip them. | ||
| if typeAssert.Type == nil { | ||
| return | ||
| } | ||
|
|
||
| pos := pass.Fset.PositionFor(typeAssert.Pos(), false) | ||
| if filecheck.IsTestFile(pos.Filename) { | ||
| return | ||
| } | ||
|
|
||
| // Find the parent map for the file containing this node. | ||
| var parents map[ast.Node]ast.Node | ||
| for _, f := range pass.Files { | ||
| if f.Pos() <= typeAssert.Pos() && typeAssert.Pos() <= f.End() { | ||
| parents = fileParents[f] | ||
| break | ||
| } | ||
| } | ||
|
|
||
| // Skip the safe two-value form: v, ok := x.(T) or v, ok = x.(T) | ||
| if parents != nil { | ||
| if assign, ok := parents[typeAssert].(*ast.AssignStmt); ok { | ||
| if len(assign.Lhs) == 2 && len(assign.Rhs) == 1 { | ||
| return | ||
| } | ||
| } | ||
| } | ||
|
|
||
| t := pass.TypesInfo.TypeOf(typeAssert.Type) | ||
| if t == nil { | ||
| return | ||
| } | ||
| pass.ReportRangef( | ||
| typeAssert, | ||
| "type assertion x.(%s) is unchecked and may panic; use the two-value form v, ok := x.(%s) instead", | ||
| t, t, | ||
| ) | ||
| }) | ||
|
|
||
| return nil, nil | ||
| } | ||
|
|
||
| // buildParentMap constructs a map from each AST node to its direct parent node. | ||
| func buildParentMap(root ast.Node) map[ast.Node]ast.Node { | ||
| parents := make(map[ast.Node]ast.Node) | ||
| var stack []ast.Node | ||
|
|
||
| ast.Inspect(root, func(n ast.Node) bool { | ||
| if n == nil { | ||
| if len(stack) > 0 { | ||
| stack = stack[:len(stack)-1] | ||
| } | ||
| return false | ||
| } | ||
| if len(stack) > 0 { | ||
| parents[n] = stack[len(stack)-1] | ||
| } | ||
| stack = append(stack, n) | ||
| return true | ||
| }) | ||
|
|
||
| return parents |
| // Good: two-value type assertion is safe. | ||
| func GoodTwoValue(v interface{}) { | ||
| s, ok := v.(string) | ||
| if ok { | ||
| fmt.Println(s) | ||
| } | ||
| } | ||
|
|
||
| // Good: type switch is safe — not flagged. | ||
| func GoodTypeSwitch(v interface{}) { | ||
| switch t := v.(type) { | ||
| case string: | ||
| fmt.Println(t) | ||
| } | ||
| } |
| // Package uncheckedtypeassertion implements a Go analysis linter that flags | ||
| // single-value type assertions x.(T) that may panic at runtime if the dynamic | ||
| // type does not match, and where the two-value safe form x.(T) is not used. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /grill-with-docs — commenting with suggestions, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Use
inspector.WithStack: The hand-rolledbuildParentMap+ per-assertion O(N files) lookup can be replaced with the built-ininsp.WithStack, which is both simpler and more efficient. - Fixture gaps: Argument-position assertions (
foo(v.(T))) and struct-field-initializer forms are untested. The message "use the two-value form" is also misleading for contexts where that form is syntactically unavailable. - Style: Test fixtures use
interface{}instead ofany, inconsistent with the project's Go 1.18+ style rule.
Positive Highlights
- ✅ Correct type-switch exclusion (
typeAssert.Type == nil) — shows solid understanding of the Go AST - ✅ Test-file exclusion via
filecheck.IsTestFileis consistent with sibling linters - ✅ Motivated by a real runtime panic (issue #34580) — good signal-to-noise for the linter category
- ✅ Clean
analysis.Analyzerstruct withURLfield populated - ✅
analysistest-based test harness with// wantannotations
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.3M
| return | ||
| } | ||
|
|
||
| // Find the parent map for the file containing this node. |
There was a problem hiding this comment.
[/tdd] The per-file parent-map lookup is O(N files) for each type assertion node. Consider using inspector.WithStack instead — it provides the ancestor stack directly during traversal without requiring a pre-built map.
💡 Suggested refactor
Replace the fileParents pre-build and the parent-lookup loop with insp.WithStack:
insp.WithStack(nodeFilter, func(n ast.Node, push bool, stack []ast.Node) bool {
if !push {
return true
}
typeAssert, ok := n.(*ast.TypeAssertExpr)
if !ok || typeAssert.Type == nil {
return true
}
// parent is stack[len(stack)-2]
if len(stack) >= 2 {
if assign, ok := stack[len(stack)-2].(*ast.AssignStmt); ok {
if len(assign.Lhs) == 2 {
return true // safe two-value form
}
}
}
// ... report
return true
})This eliminates the buildParentMap function entirely, removes the O(files) scan per node, and avoids the potential correctness gap where the file-boundary check (f.Pos() <= ... <= f.End()) could silently fail to find a parent map.
| func GoodTwoValueBlankOk(v interface{}) string { | ||
| s, _ := v.(string) | ||
| return s | ||
| } |
There was a problem hiding this comment.
[/tdd] Missing fixture: type assertion in a function-call argument position (e.g. foo(v.(string))) is a common real-world pattern and the current fixtures don't confirm whether it is correctly flagged.
💡 Suggested test cases to add
// Bad: assertion passed directly as a function argument
func BadArgPosition(v interface{}) {
fmt.Println(v.(string)) // want `type assertion x\.\(string\) is unchecked and may panic`
}
// Bad: assertion in a return expression with no comma-ok
func BadReturnExpr(v interface{}) string {
return v.(string) // want `type assertion x\.\(string\) is unchecked and may panic`
}
// Good: explicit type switch with default branch
func GoodTypeSwitchDefault(v interface{}) {
switch v.(type) {
case string:
default:
}
}Argument-position assertions are especially risky because the two-value form isn't syntactically available there, making them a source of unavoidable panics that deserve an explicit // nolint or a refactor.
| } | ||
| pass.ReportRangef( | ||
| typeAssert, | ||
| "type assertion x.(%s) is unchecked and may panic; use the two-value form v, ok := x.(%s) instead", |
There was a problem hiding this comment.
[/tdd] Correctness gap: argument-position assertions like foo(v.(T)) have no parent *ast.AssignStmt, so the two-value check (len(assign.Lhs) == 2) never fires. The analyzer will correctly report them — but there is no fixture confirming this. More importantly, v, ok := someFunc(x.(T)) (assertion inside a function call that itself is the RHS of a two-value assignment) would not be caught by the current parent check, which only looks one level up.
💡 Detail
The current guard:
if assign, ok := parents[typeAssert].(*ast.AssignStmt); ok {
if len(assign.Lhs) == 2 && len(assign.Rhs) == 1 {
return
}
}only skips the assertion when the direct parent is an assignment. If the assertion is nested (e.g. results, ok := db.Query(v.(string))), the assertion's direct parent is a *ast.CallExpr, not an *ast.AssignStmt, so it is correctly flagged — but the error message says "use the two-value form" which is not possible in that position. Consider detecting argument-position assertions and emitting a different, more actionable message (e.g., "extract the assertion before the call").
|
|
||
| import "fmt" | ||
|
|
||
| // Good: two-value type assertion is safe. |
There was a problem hiding this comment.
[/grill-with-docs] Testdata uses interface{} instead of any. The project style guide (AGENTS.md) mandates any over interface{} for Go 1.18+ code. The linter itself should arguably flag interface{} usages via a separate linter, but at minimum the fixture file should model the preferred style.
💡 One-line fix
Replace interface{} with any throughout the test fixtures:
func GoodTwoValue(v any) {
func BadSingleValue(v any) string {
// etc.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (1 test analyzed)
Test Classification Details
Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
|
Generated by the Design Decision Gate. Documents the decision to add a new in-house static analyzer that flags single-value type assertions x.(T) which may panic at runtime, recommending the safe two-value form v, ok := x.(T) instead. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (155 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 References: §26413798298
|
There was a problem hiding this comment.
REQUEST_CHANGES — three issues must be fixed before merge; one will break CI immediately.
### Blocking issues (3)
-
Missing
gofmtindentation (CI-breaking) — the entireuncheckedtypeassertion.gobody is at column 0;make fmtwill fail the format gate before any other check runs. -
False positive on
var s, ok = v.(T)— the safe two-valuevardeclaration form has a*ast.ValueSpecparent, not*ast.AssignStmt, so it will be incorrectly reported as unsafe. The parent-type guard needs a second branch for*ast.ValueSpec. -
insp.WithStackvs manualfileParents+ inner loop — the O(assertions × files) scan inside the hot Preorder callback should be replaced withinsp.WithStack, which is the idiomatic approach used elsewhere in this package, eliminatesbuildParentMap, and provides the parent for free.
Test coverage gaps (missing CallExpr-parent and blank-assign cases) are noted inline.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 2M
|
|
||
| // Analyzer is the unchecked-type-assertion analysis pass. | ||
| var Analyzer = &analysis.Analyzer{ | ||
| Name: "uncheckedtypeassertion", |
There was a problem hiding this comment.
Entire file body lacks gofmt indentation — CI will fail the format check immediately.
Every line inside function bodies, struct literals, and the import block is missing the leading tab(s) that gofmt requires. Compare to pkg/linters/ssljson/ssljson.go where struct fields and function bodies are tab-indented.
💡 Fix
Run gofmt -w pkg/linters/uncheckedtypeassertion/uncheckedtypeassertion.go (or make fmt). Example of what the struct should look like:
var Analyzer = &analysis.Analyzer{
Name: "uncheckedtypeassertion",
Doc: "reports single-value type assertions ...",
Requires: []*analysis.Analyzer{inspect.Analyzer},
Run: run,
}The CI make fmt gate will reject this file as-is.
|
|
||
| // Skip the safe two-value form: v, ok := x.(T) or v, ok = x.(T) | ||
| if parents != nil { | ||
| if assign, ok := parents[typeAssert].(*ast.AssignStmt); ok { |
There was a problem hiding this comment.
False positive: var s, ok = v.(T) is safe but will be incorrectly flagged — the two-value check only handles *ast.AssignStmt, missing *ast.ValueSpec.
💡 Details and fix
The safe two-value declaration form:
var s, ok = v.(string) // safe — ValueSpec parent, NOT AssignStmthas a *ast.ValueSpec as the parent node, not *ast.AssignStmt. The current guard:
if assign, ok := parents[typeAssert].(*ast.AssignStmt); ok {
if len(assign.Lhs) == 2 ...will not match, so this valid safe pattern gets falsely reported as unsafe.
Fix — also check for *ast.ValueSpec with two names:
if spec, ok := parents[typeAssert].(*ast.ValueSpec); ok {
if len(spec.Names) == 2 {
return
}
}Also add a testdata case:
// Good: var declaration two-value form is safe.
func GoodVarDecl(v interface{}) {
var s, ok = v.(string)
if ok {
fmt.Println(s)
}
}| insp := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) | ||
|
|
||
| // Build a parent map for each file so we can detect the two-value form. | ||
| fileParents := make(map[*ast.File]map[ast.Node]ast.Node) |
There was a problem hiding this comment.
O(assertions × files) inner loop — use insp.WithStack instead, which is the idiomatic approach used by other linters in this package.
💡 Details and fix
The code pre-builds fileParents (one full AST walk per file), then for every TypeAssertExpr node iterates pass.Files again to find which file it belongs to. In a package with F files and A type assertions that's O(F×A) iterations plus the O(nodes-per-file) build cost — all wasted when no assertions are found.
The idiomatic fix is insp.WithStack, which provides the ancestor stack on-demand with zero pre-allocation:
insp.WithStack(nodeFilter, func(n ast.Node, push bool, stack []ast.Node) bool {
if !push {
return true
}
typeAssert := n.(*ast.TypeAssertExpr)
if typeAssert.Type == nil {
return true
}
// stack[0] is always the *ast.File — no file-range loop needed
// Parent is stack[len(stack)-2]
parent := stack[len(stack)-2]
if assign, ok := parent.(*ast.AssignStmt); ok && len(assign.Lhs) == 2 {
return true // safe two-value form
}
...
})This eliminates fileParents, buildParentMap, and the per-node file scan entirely.
| func GoodTwoValueBlankOk(v interface{}) string { | ||
| s, _ := v.(string) | ||
| return s | ||
| } |
There was a problem hiding this comment.
Missing test coverage for assertion passed as a function argument — the most common real-world pattern is not exercised.
💡 Suggested additions
Add at least these two cases:
// Bad: assertion as function call argument.
func BadCallArg(v interface{}) {
fmt.Println(v.(string)) // want `type assertion x\.\(string\) is unchecked and may panic`
}
// Bad: single-value blank assign still panics.
func BadBlankAssign(v interface{}) {
_ = v.(string) // want `type assertion x\.\(string\) is unchecked and may panic`
}The CallExpr-parent path through the parent-lookup logic is untested; a regression there would silently produce false negatives for a pattern that appears frequently in real code.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Summary
Adds a new
uncheckedtypeassertionstatic analysis linter togh-awthat detects single-value type assertions (x.(T)) that may panic at runtime and recommends the safe two-value form (v, ok := x.(T)). Motivated by issue #34580, where a production panic was caused byproject["id"].(string)when the API returnednil.What Changed
New Linter:
uncheckedtypeassertionpkg/linters/uncheckedtypeassertion/uncheckedtypeassertion.go(added, high impact)golang.org/x/tools/go/analysisanalyzer that walks each file's AST looking for*ast.TypeAssertExprnodes.ast.Inspectfor O(1) parent lookup during traversal.TypeAssertExpr.Type == nil)*ast.AssignStmtwithlen(Lhs) == 2 && len(Rhs) == 1filecheck.IsTestFile)inspect.Analyzerfor efficient AST traversal.pkg/linters/uncheckedtypeassertion/uncheckedtypeassertion_test.go(added, medium impact)analysistestto validate correct diagnostic reporting against the testdata fixtures.pkg/linters/uncheckedtypeassertion/testdata/src/uncheckedtypeassertion/uncheckedtypeassertion.go(added, low impact)v, ok := x.(T))Registration
cmd/linters/main.go(modified, medium impact)uncheckedtypeassertionanalyzer with the linters binary so it runs as part of the standard linter suite.Documentation
docs/adr/34738-add-uncheckedtypeassertion-linter.md(added, low impact)Motivation
Issue #34580 surfaced a runtime panic from
project["id"].(string)when the GitHub API returnednilfor that key. Single-value type assertions are idiomatic Go but silently dangerous when the asserted type is not guaranteed. This linter catches the pattern at analysis time before it reaches production.Impact Assessment
No breaking changes. Existing code that already uses the two-value form or type switches is unaffected.
Commit History
a6756c32ffeat(linters): add uncheckedtypeassertion linter8902d80c3docs(adr): add draft ADR-34738 for uncheckedtypeassertion linter22dece503fix(linters): satisfy Go lint in uncheckedtypeassertion analyzer2cc06da31fix(linters): satisfy Go lint in uncheckedtypeassertion analyzer