[linter-miner] Add type-assertion-ok-discarded linter - #61388
Conversation
Implements a new linter that detects type assertions using the two-value
form where the ok return is explicitly discarded via blank identifier.
This pattern can hide runtime panics and should be replaced with either:
1. Single-value form: x.(Type) for intentional panic cases
2. Checked ok: x, ok := y.(Type); if ok { ... }
This linter was mined from code analysis patterns discovered in the
gh-aw codebase. It complements the existing uncheckedtypeassertion
linter by catching a different anti-pattern.
Linter features:
- Detects both assignments and var/const declarations with discarded ok
- Supports nolint directives for suppression
- Skips generated files
- Includes comprehensive test fixtures
Test results: ✅ All tests pass
Build verification: ✅ Build successful
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
✅ Ponytail Reviewer completed successfully! Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
ADR requiredThis PR adds a new architectural/code-quality decision but did not include an ADR with the required Michael Nygard sections. Why this gate triggered
Draft ADR added
Evidence used
Next action
|
There was a problem hiding this comment.
🟡 Changes recommended
Update the required documentation/spec and CI registry synchronization before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds a custom Go analyzer that detects discarded type-assertion ok values.
Changes:
- Implements AST-based detection with
nolintand generated-file support. - Adds analyzer tests and fixtures.
- Registers the analyzer in the linter registry.
File summaries
| File | Summary |
|---|---|
pkg/linters/typeassertionokdiscarded/typeassertionokdiscarded.go |
Analyzer implementation |
pkg/linters/typeassertionokdiscarded/typeassertionokdiscarded_test.go |
Analyzer test harness |
pkg/linters/typeassertionokdiscarded/testdata/src/typeassertionokdiscarded/typeassertionokdiscarded.go |
Test fixtures |
pkg/linters/registry.go |
Analyzer registration; required documentation and CI synchronization is missing |
Review details
Suppressed comments (3)
pkg/linters/registry.go:156
- Adding this analyzer to
linters.All()also requires updating the synchronized public surfaces.TestRegistryMatchesDocumentationandTestDocSurfacesMatchRegistryAndSpecListcompare the registry withpkg/linters/doc.go,pkg/linters/README.md, anddocumentedAnalyzers(); none of those listtypeassertionokdiscarded, so the package tests will fail. Add the analyzer to all three surfaces and update the analyzer count.
typeassertionokdiscarded.Analyzer,
pkg/linters/registry.go:156
- The registry now contains
typeassertionokdiscarded, but it is absent from both.github/workflows/cgo.yml'sLINTER_FLAGSand thenotYetEnforcedmap inpkg/linters/doc_sync_test.go.TestCIEnforcedLintersMatchRegistrytherefore fails. Since the repository already contains many, _ := ...type assertions, add this analyzer tonotYetEnforcedwith a remediation reason (or remediate all existing findings and add it to both CI flag lists).
typeassertionokdiscarded.Analyzer,
pkg/linters/typeassertionokdiscarded/typeassertionokdiscarded.go:117
- This
buildParentMapimplementation is duplicated verbatim inpkg/linters/uncheckedtypeassertion/uncheckedtypeassertion.go:111-130. Keeping the two type-assertion analyzers' AST-parent logic separate lets future fixes apply to only one rule and produce inconsistent diagnostics. Move the helper to a shared internal AST utility and reuse it from both packages.
// buildParentMap constructs a map from each AST node to its direct parent node.
func buildParentMap(root ast.Node) map[ast.Node]ast.Node {
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
| timenowsub.Analyzer, | ||
| tolowerequalfold.Analyzer, | ||
| trimleftright.Analyzer, | ||
| typeassertionokdiscarded.Analyzer, |
| ) | ||
|
|
||
| // Analyzer is the type-assertion-ok-discarded analysis pass. | ||
| var Analyzer = analyzerutil.New("typeassertionokdiscarded", "reports type assertions using the two-value form where the ok return is explicitly discarded via blank identifier, which can hide runtime panics", run) |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — no blocking issues, two suggestions posted inline.
📋 Key Themes & Highlights
Key Themes
- Duplication risk:
buildParentMapand the two-value-form detection logic are copied nearly verbatim fromuncheckedtypeassertion. Worth extracting to a sharedastutilhelper since both linters solve the same "is this a two-value type assertion" sub-problem. - Test gap: The
ValueSpec/const branch inisTwoValueBlankOkAssertionisn't exercised by aconstfixture (may be dead code forconst, worth confirming).
Positive Highlights
- ✅ Correctly skips type-switch guards (nil
Type) and generated files - ✅ Handles parenthesized assertions via paren-unwrapping loop
- ✅ Good fixture coverage for assign/var-decl/reassign bad cases and safe good cases, plus a
nolintsuppression test - ✅ Complements the existing
uncheckedtypeassertionlinter cleanly (single-value vs. two-value-with-blank-ok)
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 46 AIC · ⌖ 14 AIC · ⊞ 10.4K
Comment /matt to run again
|
|
||
| // 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) |
There was a problem hiding this comment.
[/codebase-design] buildParentMap and the parent-walking two-value detection logic here are a near byte-for-byte copy of the same functions in pkg/linters/uncheckedtypeassertion/uncheckedtypeassertion.go. Since these two linters both need "is this type assertion in two-value form, and is the ok slot blank?", this is a good candidate to deepen a shared module instead of maintaining two copies.
💡 Suggested consolidation
Move buildParentMap (and a helper like TwoValueAssertionForm(typeAssert, parents) (isTwoValue, isBlankOk bool)) into pkg/linters/internal/astutil, and have both uncheckedtypeassertion and typeassertionokdiscarded call the shared helper. That removes ~60 duplicated lines and ensures future bug fixes (e.g. parent-map edge cases) only need to happen once.
@copilot please address this.
| } | ||
|
|
||
| // Bad: two-value var declaration with blank ok identifier. | ||
| func BadBlankOkVarDecl(v interface{}) { |
There was a problem hiding this comment.
[/tdd] The analyzer's isTwoValueBlankOkAssertion explicitly handles *ast.ValueSpec to cover both var and const two-value declarations, but the fixtures only exercise the var form (BadBlankOkVarDecl). There's no const case, so a regression in const-handling wouldn't be caught.
💡 Suggested addition
Note: type assertions aren't valid in const initializers in real Go, so this branch of ValueSpec handling may actually be dead/unreachable for const. Worth double-checking whether the ValueSpec case ever fires outside var, and either adding a comment clarifying that, or removing the now-misleading "var/const" wording in the code comment if const can't apply here.
@copilot please address this.
There was a problem hiding this comment.
New typeassertionokdiscarded linter is well-structured (parent-map based two-value assertion detection, nolint support, good test fixtures), but it currently breaks go test ./pkg/linters/....
Blocking: TestDocSurfacesMatchRegistryAndSpecList fails because the registry now has 72 analyzers while pkg/linters/README.md / doc.go / spec_test.go's documentedAnalyzers() still only document 71. See inline comment on registry.go for the specific files that need updating.
Once the docs/spec list are updated to include typeassertionokdiscarded, this looks good to merge — logic mirrors the existing uncheckedtypeassertion linter pattern closely and the test fixtures cover the relevant cases (assign, var decl, reassign, checked ok, type switch, parenthesized forms, nolint suppression).
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 69.9 AIC · ⌖ 13.7 AIC · ⊞ 8.4K
| timenowsub.Analyzer, | ||
| tolowerequalfold.Analyzer, | ||
| trimleftright.Analyzer, | ||
| typeassertionokdiscarded.Analyzer, |
There was a problem hiding this comment.
This new analyzer is registered but the documentation surfaces required by pkg/linters are not updated to match, which breaks the build:
pkg/linters/README.mdhas notypeassertionokdiscardedrow in the "Subpackages" table (or the top summary list).pkg/linters/doc.godoc comment doesn't listtypeassertionokdiscarded.pkg/linters/spec_test.go'sdocumentedAnalyzers()doesn't include an entry for it.
Running go test ./pkg/linters/... fails with TestDocSurfacesMatchRegistryAndSpecList because the registry (72 analyzers) no longer matches the documented list (71 analyzers). Please add the corresponding README row, doc.go bullet, and documentedAnalyzers() entry for typeassertionokdiscarded so the suite passes.
@copilot please address this.
|
@copilot this PR still needs follow-through on the review feedback before a maintainer can land it. Please address the documented sync items and CI registration gaps called out in review, then run the Current review highlights:
The ADR comment is also present on the PR, so please make sure the branch includes the required design record updates if they are still missing. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in 676597e: synchronized the linter docs/spec and unenforced-CI rationale, extracted the shared type-assertion AST helper, corrected the zero-value diagnostic, and confirmed the ADR is present. Local fmt, lint, targeted tests, and |
|
@copilot this PR still needs forward progress before a maintainer can investigate quickly. Please request a fresh Copilot review after the follow-up changes already pushed, refresh the branch if needed, and then run the Previously raised review points that should be re-evaluated:
Run URL: https://github.com/github/gh-aw/actions/runs/35174719104 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
The branch is current with |
|
🎉 This pull request is included in a new release. Release: |
Linter Miner Run #135
Summary
This PR implements a new custom Go linter called
type-assertion-ok-discardedthat was discovered through systematic mining of GitHub discussions, issues, and code patterns in the gh-aw repository.What This Linter Catches
The linter detects type assertions using the two-value form where the
okreturn is explicitly discarded via blank identifier (e.g.,x, _ := y.(Type)). This pattern is problematic because:x := y.(Type)panics if the type doesn't match, which is sometimes intentional. Using the two-value form suggests the developer knows about the panic risk.Examples
Bad (flagged by this linter):
Good:
Linter Discovery Process
This linter was identified through:
Implementation Details
pkg/linters/typeassertionokdiscarded/typeassertionokdiscarded(nolint/redacted):typeassertionokdiscardeddirectivesTesting
go test ./pkg/linters/typeassertionokdiscarded/...go build ./cmd/lintersComplementary Linter
This linter complements the existing
uncheckedtypeassertionlinter:Together they provide comprehensive coverage of type assertion anti-patterns.
Files Changed
pkg/linters/typeassertionokdiscarded/typeassertionokdiscarded.go- Main analyzerpkg/linters/typeassertionokdiscarded/typeassertionokdiscarded_test.go- Test filepkg/linters/typeassertionokdiscarded/testdata/src/typeassertionokdiscarded/typeassertionokdiscarded.go- Test fixturespkg/linters/registry.go- Register analyzer in the linter suiteWarning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.com/ghapi[!TIP]
github.com/ghapiis blocked because GitHub API access uses the built-in GitHub tools by default. Instead of addinggithub.laiyagushi.com/ghapitonetwork.allowed, usetools.github.mode: gh-proxyfor direct pre-authenticated GitHub CLI access without requiring network access togithub.laiyagushi.com/ghapi:See GitHub Tools for more information on
gh-proxymode.To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.
Run: https://github.com/github/gh-aw/actions/runs/35174719104
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.laiyagushi.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.