From dbb1104ba51e83b4f1cc85452207040821a25ef5 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 16 Sep 2026 17:47:42 +0000 Subject: [PATCH 1/4] Add type-assertion-ok-discarded linter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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> --- pkg/linters/registry.go | 2 + .../typeassertionokdiscarded.go | 74 ++++++++++ .../typeassertionokdiscarded.go | 136 ++++++++++++++++++ .../typeassertionokdiscarded_test.go | 17 +++ 4 files changed, 229 insertions(+) create mode 100644 pkg/linters/typeassertionokdiscarded/testdata/src/typeassertionokdiscarded/typeassertionokdiscarded.go create mode 100644 pkg/linters/typeassertionokdiscarded/typeassertionokdiscarded.go create mode 100644 pkg/linters/typeassertionokdiscarded/typeassertionokdiscarded_test.go diff --git a/pkg/linters/registry.go b/pkg/linters/registry.go index 72c0e8a4658..8617acd88aa 100644 --- a/pkg/linters/registry.go +++ b/pkg/linters/registry.go @@ -69,6 +69,7 @@ import ( "github.com/github/gh-aw/pkg/linters/timesleepnocontext" "github.com/github/gh-aw/pkg/linters/tolowerequalfold" "github.com/github/gh-aw/pkg/linters/trimleftright" + "github.com/github/gh-aw/pkg/linters/typeassertionokdiscarded" "github.com/github/gh-aw/pkg/linters/uncheckedflushreturn" "github.com/github/gh-aw/pkg/linters/uncheckedtypeassertion" "github.com/github/gh-aw/pkg/linters/walkfuncerrshadow" @@ -152,6 +153,7 @@ var allAnalyzers = []*analysis.Analyzer{ timenowsub.Analyzer, tolowerequalfold.Analyzer, trimleftright.Analyzer, + typeassertionokdiscarded.Analyzer, uncheckedtypeassertion.Analyzer, uncheckedflushreturn.Analyzer, walkfuncerrshadow.Analyzer, diff --git a/pkg/linters/typeassertionokdiscarded/testdata/src/typeassertionokdiscarded/typeassertionokdiscarded.go b/pkg/linters/typeassertionokdiscarded/testdata/src/typeassertionokdiscarded/typeassertionokdiscarded.go new file mode 100644 index 00000000000..544ec7b6392 --- /dev/null +++ b/pkg/linters/typeassertionokdiscarded/testdata/src/typeassertionokdiscarded/typeassertionokdiscarded.go @@ -0,0 +1,74 @@ +package typeassertionokdiscarded + +import "fmt" + +// Bad: two-value type assertion with blank ok identifier. +func BadBlankOkAssign(v interface{}) { + s, _ := v.(string) // want `type assertion ok value is explicitly discarded with blank identifier` + fmt.Println(s) +} + +// Bad: two-value var declaration with blank ok identifier. +func BadBlankOkVarDecl(v interface{}) { + var s, _ = v.(string) // want `type assertion ok value is explicitly discarded with blank identifier` + fmt.Println(s) +} + +// Bad: two-value reassignment with blank ok identifier. +func BadBlankOkReassign(v interface{}) string { + var s string + s, _ = v.(string) // want `type assertion ok value is explicitly discarded with blank identifier` + return s +} + +// Good: single-value type assertion that may panic. +func GoodSingleValue(v interface{}) string { + return v.(string) +} + +// Good: single-value assignment. +func GoodSingleValueAssign(v interface{}) { + s := v.(string) + fmt.Println(s) +} + +// Good: two-value assertion with checked ok. +func GoodTwoValueChecked(v interface{}) { + s, ok := v.(string) + if ok { + fmt.Println(s) + } +} + +// Good: two-value assertion with named ok variable. +func GoodTwoValueNamed(v interface{}) { + s, err := v.(string) + fmt.Println(s, err) +} + +// Good: type switch is safe. +func GoodTypeSwitch(v interface{}) { + switch t := v.(type) { + case string: + fmt.Println(t) + } +} + +// Good: parenthesized two-value assignment with checked ok. +func GoodParenTwoValueChecked(v interface{}) { + s, ok := (v.(string)) + if ok { + fmt.Println(s) + } +} + +// Good: parenthesized single-value assertion. +func GoodParenSingleValue(v interface{}) string { + return (v.(string)) +} + +func suppressed(v interface{}) { + //nolint:typeassertionokdiscarded + s, _ := v.(string) + fmt.Println(s) +} diff --git a/pkg/linters/typeassertionokdiscarded/typeassertionokdiscarded.go b/pkg/linters/typeassertionokdiscarded/typeassertionokdiscarded.go new file mode 100644 index 00000000000..f5df3d4b21a --- /dev/null +++ b/pkg/linters/typeassertionokdiscarded/typeassertionokdiscarded.go @@ -0,0 +1,136 @@ +// Package typeassertionokdiscarded implements a Go analysis linter that flags +// type assertions in the two-value form where the ok return is explicitly +// discarded via blank identifier, which can hide runtime panics. +package typeassertionokdiscarded + +import ( + "go/ast" + + "golang.org/x/tools/go/analysis" + + "github.com/github/gh-aw/pkg/linters/internal/analyzerutil" + "github.com/github/gh-aw/pkg/linters/internal/astutil" + "github.com/github/gh-aw/pkg/linters/internal/filecheck" + "github.com/github/gh-aw/pkg/linters/internal/nolint" +) + +// 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) + +func run(pass *analysis.Pass) (any, error) { + noLintIndex, generatedFiles, err := analyzerutil.Indexes(pass) + if err != nil { + return nil, err + } + + // Build a parent map for each file so we can detect two-value assignments. + 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), + } + + return analyzerutil.Preorder(pass, nodeFilter, func(n ast.Node) { + inspectTypeAssertExpr(pass, noLintIndex, generatedFiles, fileParents, n) + }) +} + +func inspectTypeAssertExpr(pass *analysis.Pass, noLintIndex nolint.DirectiveIndex, generatedFiles filecheck.GeneratedIndex, fileParents map[*ast.File]map[ast.Node]ast.Node, n ast.Node) { + typeAssert, ok := n.(*ast.TypeAssertExpr) + if !ok { + return + } + + // Type-switch guards have nil Type; skip them. + if typeAssert.Type == nil { + return + } + + pos := pass.Fset.PositionFor(typeAssert.Pos(), false) + if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { + return + } + + // Find the parent map for the file containing this node. + f := astutil.FileForPos(pass.Files, typeAssert.Pos()) + var parents map[ast.Node]ast.Node + if f != nil { + parents = fileParents[f] + } + + // Check if this is a two-value assignment where the ok is blank. + if parents != nil { + if isTwoValueBlankOkAssertion(typeAssert, parents) { + if nolint.HasDirectiveForLinter(pos, noLintIndex, "typeassertionokdiscarded") { + return + } + + t := pass.TypesInfo.TypeOf(typeAssert.Type) + if t == nil { + return + } + + pass.ReportRangef( + typeAssert, + "type assertion ok value is explicitly discarded with blank identifier; use single-value form x.(%s) or check the ok value instead", + t, + ) + } + } +} + +func isTwoValueBlankOkAssertion(typeAssert *ast.TypeAssertExpr, parents map[ast.Node]ast.Node) bool { + parent := parents[typeAssert] + for parent != nil { + paren, ok := parent.(*ast.ParenExpr) + if !ok { + break + } + parent = parents[paren] + } + + switch p := parent.(type) { + case *ast.AssignStmt: + // Check for two-value assignment where second value is blank identifier. + if len(p.Lhs) == 2 && len(p.Rhs) == 1 { + // The second LHS must be a blank identifier. + if ident, ok := p.Lhs[1].(*ast.Ident); ok && ident.Name == "_" { + return true + } + } + case *ast.ValueSpec: + // Check for two-value var/const declaration where second value is blank identifier. + if len(p.Names) == 2 && len(p.Values) == 1 { + // The second name must be a blank identifier. + if p.Names[1].Name == "_" { + return true + } + } + } + return false +} + +// 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 +} diff --git a/pkg/linters/typeassertionokdiscarded/typeassertionokdiscarded_test.go b/pkg/linters/typeassertionokdiscarded/typeassertionokdiscarded_test.go new file mode 100644 index 00000000000..15bb6a6cc33 --- /dev/null +++ b/pkg/linters/typeassertionokdiscarded/typeassertionokdiscarded_test.go @@ -0,0 +1,17 @@ +//go:build !integration + +package typeassertionokdiscarded_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + + "github.com/github/gh-aw/pkg/linters/typeassertionokdiscarded" +) + +func TestAnalyzer(t *testing.T) { + t.Parallel() + testdata := analysistest.TestData() + analysistest.Run(t, testdata, typeassertionokdiscarded.Analyzer, "typeassertionokdiscarded") +} From dceac41411cb678a5ffb4d2a6b3cfb1092326764 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 16 Sep 2026 23:21:43 +0000 Subject: [PATCH 2/4] Add ADR for type-assertion-ok-discarded linter --- ...-add-type-assertion-ok-discarded-linter.md | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 docs/adr/61388-add-type-assertion-ok-discarded-linter.md diff --git a/docs/adr/61388-add-type-assertion-ok-discarded-linter.md b/docs/adr/61388-add-type-assertion-ok-discarded-linter.md new file mode 100644 index 00000000000..c815d789f61 --- /dev/null +++ b/docs/adr/61388-add-type-assertion-ok-discarded-linter.md @@ -0,0 +1,46 @@ +# ADR-61388: Add type-assertion-ok-discarded linter + +**Date**: 2026-09-16 +**Status**: Draft +**Deciders**: gh-aw maintainers + +--- + +### Context + +This pull request adds a new Go analyzer under `pkg/linters/typeassertionokdiscarded/` and registers it in the shared linter registry. The implementation targets a specific pattern: two-value type assertions where the second `ok` result is explicitly discarded with `_`, as shown in the new analyzer test fixtures and described in the PR body. The repository already contains linter infrastructure, generated-file skipping, nolint support, and a complementary `uncheckedtypeassertion` analyzer, so the architectural question is whether this codebase should treat discarded-`ok` type assertions as a first-class lint violation. The non-negotiable constraint visible in the PR is to implement the check as a standard AST-based analyzer that integrates with the existing linter suite and test harness. + +### Decision + +We will add a dedicated `typeassertionokdiscarded` analyzer to the gh-aw linter suite to report two-value type assertions whose `ok` result is discarded with the blank identifier. The analyzer will inspect `ast.TypeAssertExpr` nodes, determine whether they participate in a two-value assignment or declaration with `_` in the second position, and emit a diagnostic directing authors to either use the single-value assertion form or actually check the `ok` result. We chose this because the PR evidence shows the repository wants explicit enforcement of this type-assertion anti-pattern through the same reusable analyzer framework used by other custom Go linters. + +### Alternatives Considered + +#### Alternative 1: Rely on the existing uncheckedtypeassertion linter only + +The repository already has an `uncheckedtypeassertion` analyzer, so one alternative was to keep enforcing only single-value assertion misuse and leave discarded-`ok` cases unaddressed. This was considered because it avoids another custom linter and keeps type-assertion guidance consolidated in one existing rule. It was not chosen because the PR body and new fixtures make clear that discarded-`ok` assertions are treated as a distinct anti-pattern with different remediation: either intentionally use the single-value form or check `ok` instead of discarding it. + +#### Alternative 2: Depend on code review or a generic external linter rule + +Another option was to document this as a style expectation and catch it during review, or to wait for a generic upstream lint rule to cover it. This was considered because it would avoid maintaining a repository-specific analyzer with AST parent tracking and tests. It was not chosen because the diff explicitly invests in an in-repo analyzer integrated with the current registry, generated-file handling, nolint directives, and analysistest fixtures, indicating the decision is to automate enforcement rather than rely on manual review or unavailable generic tooling. + +### Consequences + +#### Positive +- The repository gains automated enforcement for a specific misleading type-assertion pattern that was previously easy to miss in review. +- The new rule integrates with the existing custom linter registry, test harness, generated-file filtering, and `nolint` support. +- The test fixtures document accepted and rejected type-assertion forms, making the intended coding standard more explicit. + +#### Negative +- The project must maintain another custom analyzer, including AST parent mapping logic and ongoing compatibility with analyzer infrastructure. +- Some contributors may need to rewrite existing code patterns or add justified suppressions when this rule is enabled against broader code. +- The linter suite becomes slightly more complex because type-assertion guidance is now split across complementary analyzers. + +#### Neutral +- The change affects static analysis behavior and tests, but does not alter runtime behavior of production code directly. +- The analyzer distinguishes only assignments and declarations with a blank second result, so other type-assertion patterns remain governed by existing rules. +- Registration in `pkg/linters/registry.go` makes the new analyzer part of the standard linter bundle used by the repository. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* From 011f02467ee9666ca9e08d41c2036c42671fb676 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 16 Sep 2026 23:52:58 +0000 Subject: [PATCH 3/4] Plan linter review follow-up Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- .github/aw/actions-lock.json | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/.github/aw/actions-lock.json b/.github/aw/actions-lock.json index ca06a403b97..793e266f243 100644 --- a/.github/aw/actions-lock.json +++ b/.github/aw/actions-lock.json @@ -118,10 +118,10 @@ "version": "v2.0.5", "sha": "22d081ff2d3a40755e97629de92e3bcbfa7cf2ed" }, - "docker/build-push-action@v7.4.0": { + "docker/build-push-action@v7.3.0": { "repo": "docker/build-push-action", - "version": "v7.4.0", - "sha": "c3c9e263c25d99ce0380d002d59b67737d91b0dc" + "version": "v7.3.0", + "sha": "53b7df96c91f9c12dcc8a07bcb9ccacbed38856a" }, "docker/login-action@v4.6.0": { "repo": "docker/login-action", @@ -133,10 +133,10 @@ "version": "v6.2.0", "sha": "dc802804100637a589fabce1cb79ff13a1411302" }, - "docker/setup-buildx-action@v4.4.0": { + "docker/setup-buildx-action@v4.3.0": { "repo": "docker/setup-buildx-action", - "version": "v4.4.0", - "sha": "594f3bf4285d9ea8dc53c9a0c9c4092420091003" + "version": "v4.3.0", + "sha": "37fe631027851001ddb9b187196cc803df7f5f0e" }, "erlef/setup-beam@v1.24.1": { "repo": "erlef/setup-beam", From 676597ea44c1ec06c6c77195eb564be79d7e5969 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 17 Sep 2026 00:04:02 +0000 Subject: [PATCH 4/4] Complete type assertion linter follow-up Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- .github/aw/actions-lock.json | 12 ++-- pkg/linters/README.md | 3 + pkg/linters/doc.go | 3 +- pkg/linters/doc_sync_test.go | 1 + pkg/linters/internal/astutil/astutil.go | 48 +++++++++++++++ pkg/linters/spec_test.go | 4 +- .../typeassertionokdiscarded.go | 58 ++----------------- .../uncheckedtypeassertion.go | 43 +------------- 8 files changed, 71 insertions(+), 101 deletions(-) diff --git a/.github/aw/actions-lock.json b/.github/aw/actions-lock.json index 793e266f243..ca06a403b97 100644 --- a/.github/aw/actions-lock.json +++ b/.github/aw/actions-lock.json @@ -118,10 +118,10 @@ "version": "v2.0.5", "sha": "22d081ff2d3a40755e97629de92e3bcbfa7cf2ed" }, - "docker/build-push-action@v7.3.0": { + "docker/build-push-action@v7.4.0": { "repo": "docker/build-push-action", - "version": "v7.3.0", - "sha": "53b7df96c91f9c12dcc8a07bcb9ccacbed38856a" + "version": "v7.4.0", + "sha": "c3c9e263c25d99ce0380d002d59b67737d91b0dc" }, "docker/login-action@v4.6.0": { "repo": "docker/login-action", @@ -133,10 +133,10 @@ "version": "v6.2.0", "sha": "dc802804100637a589fabce1cb79ff13a1411302" }, - "docker/setup-buildx-action@v4.3.0": { + "docker/setup-buildx-action@v4.4.0": { "repo": "docker/setup-buildx-action", - "version": "v4.3.0", - "sha": "37fe631027851001ddb9b187196cc803df7f5f0e" + "version": "v4.4.0", + "sha": "594f3bf4285d9ea8dc53c9a0c9c4092420091003" }, "erlef/setup-beam@v1.24.1": { "repo": "erlef/setup-beam", diff --git a/pkg/linters/README.md b/pkg/linters/README.md index fe7b59cfd23..6e08514df02 100644 --- a/pkg/linters/README.md +++ b/pkg/linters/README.md @@ -72,6 +72,7 @@ This package currently provides custom Go analyzers in the following subpackages - `timenowsub` — reports `time.Now().Sub(t)` calls that should be simplified to `time.Since(t)`. - `tolowerequalfold` — reports case-insensitive string comparisons using `strings.ToLower`/`ToUpper` that should use `strings.EqualFold`. - `trimleftright` — reports `strings.TrimLeft`/`TrimRight` calls with a multi-character literal cutset where `TrimPrefix`/`TrimSuffix` was likely intended. +- `typeassertionokdiscarded` — reports two-value type assertions whose `ok` result is discarded. - `uncheckedtypeassertion` — reports single-value type assertions where unchecked panics are possible. - `uncheckedflushreturn` — reports `Flush()` method calls where the error return is discarded, which silently drops buffered data on failure. - `wgdonenotdeferred` — reports non-deferred `sync.WaitGroup.Done()` calls that can deadlock on panics or early returns. @@ -171,6 +172,7 @@ environment variable and gates findings on the recorded execution hit count for | `timenowsub` | Custom `go/analysis` analyzer that flags `time.Now().Sub(t)` calls that should use `time.Since(t)` | | `tolowerequalfold` | Custom `go/analysis` analyzer that flags case-insensitive comparisons via `strings.ToLower`/`ToUpper` that should use `strings.EqualFold` | | `trimleftright` | Custom `go/analysis` analyzer that flags `strings.TrimLeft`/`TrimRight` calls with a multi-character literal cutset where `TrimPrefix`/`TrimSuffix` was likely intended | +| `typeassertionokdiscarded` | Custom `go/analysis` analyzer that flags two-value type assertions whose `ok` result is discarded | | `uncheckedtypeassertion` | Custom `go/analysis` analyzer that flags unchecked single-value type assertions | | `uncheckedflushreturn` | Custom `go/analysis` analyzer that flags `Flush()` method calls where the error return is discarded | | `walkfuncerrshadow` | Custom `go/analysis` analyzer that flags `filepath.Walk`/`filepath.WalkDir` callbacks whose `err` parameter shadows an outer `err` variable assigned from the walk call | @@ -308,6 +310,7 @@ _ = trimleftright.Analyzer - `github.com/github/gh-aw/pkg/linters/timesleepnocontext` — time-sleep-no-context analyzer subpackage - `github.com/github/gh-aw/pkg/linters/tolowerequalfold` — to-lower-equal-fold analyzer subpackage - `github.com/github/gh-aw/pkg/linters/trimleftright` — trim-left-right analyzer subpackage +- `github.com/github/gh-aw/pkg/linters/typeassertionokdiscarded` — type-assertion-ok-discarded analyzer subpackage - `github.com/github/gh-aw/pkg/linters/uncheckedtypeassertion` — unchecked-type-assertion analyzer subpackage - `github.com/github/gh-aw/pkg/linters/uncheckedflushreturn` — unchecked-flush-return analyzer subpackage - `github.com/github/gh-aw/pkg/linters/walkfuncerrshadow` — walk-func-err-shadow analyzer subpackage diff --git a/pkg/linters/doc.go b/pkg/linters/doc.go index 47faac843fb..de5d43c4624 100644 --- a/pkg/linters/doc.go +++ b/pkg/linters/doc.go @@ -1,6 +1,6 @@ // Package linters is a namespace for gh-aw's custom Go analysis linters. // -// All 71 active analyzers: +// All 72 active analyzers: // // - appendbytestring — flags append(b, []byte(s)...) calls where s is a string that can be simplified to append(b, s...) // - appendoneelement — flags append(s, []T{x}...) calls where a single-element slice literal is spread and can be simplified to append(s, x) @@ -68,6 +68,7 @@ // - timenowsub — reports time.Now().Sub(t) calls that should be simplified to time.Since(t) // - tolowerequalfold — flags case-insensitive comparisons via ToLower/ToUpper that should use EqualFold // - trimleftright — flags strings.TrimLeft/TrimRight calls with a multi-character literal cutset where TrimPrefix/TrimSuffix was likely intended +// - typeassertionokdiscarded — flags two-value type assertions whose ok result is discarded // - uncheckedtypeassertion — flags unchecked single-value type assertions // - uncheckedflushreturn — flags Flush() method calls where the error return is discarded // - walkfuncerrshadow — flags filepath.Walk/WalkDir callbacks whose err parameter shadows an outer err variable assigned from the walk call diff --git a/pkg/linters/doc_sync_test.go b/pkg/linters/doc_sync_test.go index 656ae23ac48..2078d1e5f08 100644 --- a/pkg/linters/doc_sync_test.go +++ b/pkg/linters/doc_sync_test.go @@ -38,6 +38,7 @@ var notYetEnforced = map[string]string{ "sprintferrdot": "has not yet completed an enforcement-readiness audit", "ssljson": "has not yet completed an enforcement-readiness audit", "stringsconcatloop": "has not yet completed an enforcement-readiness audit", + "typeassertionokdiscarded": "existing production violations need remediation before enforcement; nolint suppression already works", } // TestDocGo_CountMatchesBullets validates that the "All N active analyzers:" diff --git a/pkg/linters/internal/astutil/astutil.go b/pkg/linters/internal/astutil/astutil.go index bd7b1fe8f50..342ba370739 100644 --- a/pkg/linters/internal/astutil/astutil.go +++ b/pkg/linters/internal/astutil/astutil.go @@ -463,6 +463,54 @@ func FileForPos(files []*ast.File, pos token.Pos) *ast.File { return 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 +} + +// TwoValueTypeAssertionOKIdent returns the ok identifier for a two-value type +// assertion assignment or variable declaration. +func TwoValueTypeAssertionOKIdent(typeAssert *ast.TypeAssertExpr, parents map[ast.Node]ast.Node) (*ast.Ident, bool) { + parent := parents[typeAssert] + for { + paren, ok := parent.(*ast.ParenExpr) + if !ok { + break + } + parent = parents[paren] + } + + switch p := parent.(type) { + case *ast.AssignStmt: + if len(p.Lhs) == 2 && len(p.Rhs) == 1 { + okIdent, ok := p.Lhs[1].(*ast.Ident) + return okIdent, ok + } + case *ast.ValueSpec: + if len(p.Names) == 2 && len(p.Values) == 1 { + return p.Names[1], true + } + } + return nil, false +} + // CountPkgUsesInFile returns the number of times the package at pkgPath is // referenced as a selector base within file (e.g. each "fmt.X" call counts // as one use of the "fmt" package). diff --git a/pkg/linters/spec_test.go b/pkg/linters/spec_test.go index 7b729cd7b00..8fdeeb0ec19 100644 --- a/pkg/linters/spec_test.go +++ b/pkg/linters/spec_test.go @@ -77,6 +77,7 @@ import ( "github.com/github/gh-aw/pkg/linters/timesleepnocontext" "github.com/github/gh-aw/pkg/linters/tolowerequalfold" "github.com/github/gh-aw/pkg/linters/trimleftright" + "github.com/github/gh-aw/pkg/linters/typeassertionokdiscarded" "github.com/github/gh-aw/pkg/linters/uncheckedflushreturn" "github.com/github/gh-aw/pkg/linters/uncheckedtypeassertion" "github.com/github/gh-aw/pkg/linters/walkfuncerrshadow" @@ -109,7 +110,7 @@ type docAnalyzer struct { // logfatallibrary, manualmutexunlock, manualpathconcat, mapclearloop, mapdeletecheck, nilctxpassed, osexitinlibrary, osgetenvlibrary, ossetenvlibrary, packagelevelmutableslicemap, panic-in-library-code, rawloginlib, // regexpcompileinfunction, regexpdynamicpattern, seenmapbool, slicemakezerolength, sortslice, sprintferrdot, sprintferrorsnew, sprintfbool, sprintfint, ssljson, // strconvparseignorederror, stringbytesroundtrip, stringreplaceminusone, stringsconcatloop, stringscountcontains, stringsindexcontains, stringsindexhasprefix, stringsjoinone, timeafterleak, timesleepnocontext, timenowsub, -// tolowerequalfold, trimleftright, uncheckedflushreturn, uncheckedtypeassertion, walkfuncerrshadow, wgdonenotdeferred, writebytestring +// tolowerequalfold, trimleftright, typeassertionokdiscarded, uncheckedflushreturn, uncheckedtypeassertion, walkfuncerrshadow, wgdonenotdeferred, writebytestring func documentedAnalyzers() []docAnalyzer { return []docAnalyzer{ {"appendbytestring", appendbytestring.Analyzer}, @@ -178,6 +179,7 @@ func documentedAnalyzers() []docAnalyzer { {"timenowsub", timenowsub.Analyzer}, {"tolowerequalfold", tolowerequalfold.Analyzer}, {"trimleftright", trimleftright.Analyzer}, + {"typeassertionokdiscarded", typeassertionokdiscarded.Analyzer}, {"uncheckedtypeassertion", uncheckedtypeassertion.Analyzer}, {"uncheckedflushreturn", uncheckedflushreturn.Analyzer}, {"walkfuncerrshadow", walkfuncerrshadow.Analyzer}, diff --git a/pkg/linters/typeassertionokdiscarded/typeassertionokdiscarded.go b/pkg/linters/typeassertionokdiscarded/typeassertionokdiscarded.go index f5df3d4b21a..c47b7501822 100644 --- a/pkg/linters/typeassertionokdiscarded/typeassertionokdiscarded.go +++ b/pkg/linters/typeassertionokdiscarded/typeassertionokdiscarded.go @@ -1,6 +1,6 @@ // Package typeassertionokdiscarded implements a Go analysis linter that flags // type assertions in the two-value form where the ok return is explicitly -// discarded via blank identifier, which can hide runtime panics. +// discarded via blank identifier, which can silently accept a zero value. package typeassertionokdiscarded import ( @@ -15,7 +15,7 @@ import ( ) // 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) +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 silently accept a zero value", run) func run(pass *analysis.Pass) (any, error) { noLintIndex, generatedFiles, err := analyzerutil.Indexes(pass) @@ -26,7 +26,7 @@ func run(pass *analysis.Pass) (any, error) { // Build a parent map for each file so we can detect two-value assignments. fileParents := make(map[*ast.File]map[ast.Node]ast.Node) for _, f := range pass.Files { - fileParents[f] = buildParentMap(f) + fileParents[f] = astutil.BuildParentMap(f) } nodeFilter := []ast.Node{ @@ -83,54 +83,6 @@ func inspectTypeAssertExpr(pass *analysis.Pass, noLintIndex nolint.DirectiveInde } func isTwoValueBlankOkAssertion(typeAssert *ast.TypeAssertExpr, parents map[ast.Node]ast.Node) bool { - parent := parents[typeAssert] - for parent != nil { - paren, ok := parent.(*ast.ParenExpr) - if !ok { - break - } - parent = parents[paren] - } - - switch p := parent.(type) { - case *ast.AssignStmt: - // Check for two-value assignment where second value is blank identifier. - if len(p.Lhs) == 2 && len(p.Rhs) == 1 { - // The second LHS must be a blank identifier. - if ident, ok := p.Lhs[1].(*ast.Ident); ok && ident.Name == "_" { - return true - } - } - case *ast.ValueSpec: - // Check for two-value var/const declaration where second value is blank identifier. - if len(p.Names) == 2 && len(p.Values) == 1 { - // The second name must be a blank identifier. - if p.Names[1].Name == "_" { - return true - } - } - } - return false -} - -// 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 + okIdent, isTwoValue := astutil.TwoValueTypeAssertionOKIdent(typeAssert, parents) + return isTwoValue && okIdent.Name == "_" } diff --git a/pkg/linters/uncheckedtypeassertion/uncheckedtypeassertion.go b/pkg/linters/uncheckedtypeassertion/uncheckedtypeassertion.go index 932d4c20186..a6f2e1f86e8 100644 --- a/pkg/linters/uncheckedtypeassertion/uncheckedtypeassertion.go +++ b/pkg/linters/uncheckedtypeassertion/uncheckedtypeassertion.go @@ -30,7 +30,7 @@ func run(pass *analysis.Pass) (any, error) { // 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) + fileParents[f] = astutil.BuildParentMap(f) } nodeFilter := []ast.Node{ @@ -89,43 +89,6 @@ func inspectTypeAssertExpr(pass *analysis.Pass, noLintIndex nolint.DirectiveInde } func isSafeTwoValueAssertion(typeAssert *ast.TypeAssertExpr, parents map[ast.Node]ast.Node) bool { - parent := parents[typeAssert] - for parent != nil { - paren, ok := parent.(*ast.ParenExpr) - if !ok { - break - } - parent = parents[paren] - } - - switch p := parent.(type) { - case *ast.AssignStmt: - return len(p.Lhs) == 2 && len(p.Rhs) == 1 - case *ast.ValueSpec: - return len(p.Names) == 2 && len(p.Values) == 1 - default: - return false - } -} - -// 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 + _, isTwoValue := astutil.TwoValueTypeAssertionOKIdent(typeAssert, parents) + return isTwoValue }