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.* 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/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/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/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..c47b7501822 --- /dev/null +++ b/pkg/linters/typeassertionokdiscarded/typeassertionokdiscarded.go @@ -0,0 +1,88 @@ +// 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 silently accept a zero value. +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 silently accept a zero value", 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] = astutil.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 { + okIdent, isTwoValue := astutil.TwoValueTypeAssertionOKIdent(typeAssert, parents) + return isTwoValue && okIdent.Name == "_" +} 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") +} 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 }