chore(lint): enable forcetypeassert and check every assertion - #926
Merged
Conversation
forcetypeassert enforces the uber-go guide's "Handle Type Assertion Failures". It measured 218 findings: 156 in _test.go and 62 outside. The 156 test findings join the existing `path: _test\.go` exclusion block alongside errcheck/gosec, for the same reason — a failed assertion in a test panics loudly, which is the outcome the test wants. The 62 non-test sites are fixed rather than excluded, in four shapes: - testing/mocks (44): a package-local generic `arg[T](arguments, method, index)` replaces `arguments.Get(i).(T)`. These files are on the public import surface, so the `v, _ := ...` rewrite was rejected: it swaps a precise panic for a nil return that explodes later somewhere unrelated. The helper keeps the panic and adds the mocked method, the return index, and the actual type to the message. Note the pre-existing `arguments.Get(0) == nil` guards are nil-only, never type checks, so they did not make the assertions they precede safe. - Container invariants (11): deleted rather than guarded. MockCache's `sync.Map` becomes `map[string]*cacheEntry` under the mutex that already serialized every mutator (Stats/Has/AllKeys/Dump now take it too, closing a lock-discipline gap); tenantstore's per-tenant lock map becomes `map[string]*sync.Mutex` guarded by the existing mu; the column registry's two assertions move behind typed load/loadOrStore helpers. resourcepool's singleflight and container/list values cannot be typed away, so those three use comma-ok in shapes that generate no mutant. - Values the function just built (4): newRouteGroup returns *routeGroup (unexported, zero API impact), the JOSE error body is hoisted into a local instead of re-asserted out of the map it was just put in, and the generic request allocator discards ok with a comment naming why the assertion cannot fail — no branch and no allocation, so the ADR-026 alloc guards stay green on the typed-handler hot path. - Caller-supplied data (2): NewDatabaseWithData now derives columns from the converted rows, leaving one assertion that panics naming the query key and offending row index, matching the NOSONAR panic beside it. scheduler.executeJob took the exported JobContext and asserted it to *jobContextImpl, so a foreign implementation would panic inside the very function whose job is to contain panics. Both it and newJobContext now use the concrete unexported type; the exported surface is unchanged. Verified by planting the same violation twice: in a non-test file it is reported, in a _test.go file the new exclusion suppresses it. Non-test findings are now 0.
Contributor
WalkthroughThe PR enables ChangesAssertion safety and test infrastructure
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/handler.go`:
- Around line 353-355: The request allocation path around newRequestAllocator
must preserve defined pointer types such as RequestPtr. Convert elem to
ra.elemType before the assertion assigning request, or explicitly reject defined
pointer types in newRequestAllocator, ensuring validateNotNil receives the
allocated request instead of nil.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c78dbaff-d1e8-457f-bdae-65a86623b64f
📒 Files selected for processing (19)
.golangci.ymlcache/testing/mock_cache.gocache/testing/mock_cache_test.godatabase/internal/columns/registry.gointernal/resourcepool/resourcepool.gointernal/tenantstore/tenantstore.goscheduler/job.goscheduler/module.goserver/handler.goserver/jose.goserver/route_registrar.goserver/route_registrar_test.gotesting/fixtures/database.gotesting/mocks/arg.gotesting/mocks/database.gotesting/mocks/query_builder.gotesting/mocks/registry.gotesting/mocks/statement.gotesting/mocks/transaction.go
For a defined pointer type (type P *Request), reflect.New(elemType.Elem()) yields the UNNAMED *Request, which is not assertable to the named P. The comma-ok introduced by the forcetypeassert sweep discarded that failure, so `request` stayed nil, validateNotNil rejected it, and the caller got "request cannot be nil" for a perfectly good payload. Previously the single-value assertion panicked there instead — both wrong, but a silent rejection is worse than a loud panic. Converting to elemType first — T's own reflect.Type — makes the assertion exact for both the plain *Request and defined-P forms, so discarding ok is now genuinely safe. The comment claiming it "cannot fail" was false as written and is corrected. Convert on a pointer adds no allocation: the ADR-026 AllocsStable guards pass unchanged, and the hot path stays branch-free. requestAllocator had no direct test — it was only ever exercised through the binder, which is why this went unnoticed. Added one that pins the defined-pointer case and asserts the typed request aliases the binder target; verified it fails on the pre-fix code at the NotNil(request) assertion and nowhere else. Reported by CodeRabbit on #926. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The forcetypeassert conversion left three residues that the linter is happy with but that cost more than they buy. MockCache.GetOrSet was a literal transcription of sync.Map.LoadOrStore: store-then-check-expiration, with the "expired" arm building and storing a second identical cacheEntry and the return computing wasSet as !loaded. With a plain map the missing and expired cases are the same case, so both collapse into one guarded store. Same three outcomes, one entry construction instead of two, and wasSet is a literal instead of a derived value that can only ever be false where it is read. Pool.GetOrCreate and Pool.evictIfNeeded each gained an "Unreachable:" branch guarding an assertion whose producer is the only writer in the file. Those are branches no test can enter, which is exactly what the diff-scoped mutation gate has no way to kill. GetOrCreate now uses the `ok &&` short-circuit that releaseAbandoned already uses forty lines below — an impossible false falls into the existing bounded retry loop rather than into a fabricated error string — and evictIfNeeded discards ok the way server/handler.go's allocator does, since createEntry is the LRU list's only writer. mocks.arg called arguments.Get(index) three times to build one panic message; it now reads the value once. No behavior change: every branch removed is either provably equivalent to the one it merged into or provably unenterable.
SonarCloud S3776 (cognitive complexity, CRITICAL) on GetOrCreate. The function already carried a retry loop around a singleflight closure and a select; the comma-ok this branch added at the bottom tipped it past the threshold, with the nesting penalties compounding down the body. Dropping the earlier Unreachable: guard was not enough on its own — Sonar still reported the finding after that commit — so the slow path moves wholesale into acquireShared: the DoChan closure, the select on ctx.Done, and the result error handling. GetOrCreate now reads as guard, fast path, slow path, claim. Every comment moved with the code it explains rather than being dropped. Suppressing with NOSONAR was not an option: the rule is correct here, the function really was doing too much, and this repo's convention is to refactor rather than annotate. acquireShared returns a nil entry for the assertion that its sole producer makes impossible, so the caller's `e != nil &&` short-circuits into the existing bounded retry loop. That keeps the shape the mutation gate needs — no fabricated error branch for a case no test can enter — matching how releaseAbandoned and evictIfNeeded already handle the same assertion. Behavior is unchanged: same singleflight collapsing, same per-caller context cancellation via DoChan, same single incErrors accounting in the leader, same abandoned-lease settlement. internal/resourcepool passes under -race. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/resourcepool/resourcepool.go`:
- Around line 191-216: Update acquireShared so createEntry receives
context.WithoutCancel(ctx), preventing the leader’s cancellation or deadline
from terminating the shared resource creation while preserving the caller’s
context for acquisition. Revise the deadline-inheritance test to assert shared
creation outlives an expired leader and succeeds for valid waiters, and add
coverage where the leader deadline expires while a waiter remains active.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f8dbe75b-acd7-4548-9090-5714176872e0
📒 Files selected for processing (6)
.golangci.ymlinternal/resourcepool/resourcepool.goscheduler/module.goserver/handler.goserver/jose.gotesting/mocks/query_builder.go
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Enables
forcetypeassert(uber-go "Handle Type Assertion Failures") and fixes all 62non-test sites; the 156
_test.gofindings are excluded alongsideerrcheck/gosec, onthe same reasoning that a failed assertion in a test panics loudly, which is what the test
wants. Where a container allowed it the assertion was deleted rather than guarded —
sync.Mapbecame a typed map under the mutex that already serialized every mutator, andnewRouteGroup/executeJobnarrowed to their concrete unexported types.Impact
No exported signature changes.
cache/testing'sMockCacheis the largest behavioralsurface:
Stats/Has/AllKeys/Dumpnow take the mutex they previously bypassed, andAllKeysreturns an empty non-nil slice rather thannil— a downstreamassert.Nilwould need
assert.Empty.Verification
Guarding instead of deleting would have left unreachable
!okbranches, whose mutants thediff-scoped gate cannot kill; the surviving sites use shapes gremlins generates no mutant
for.
make mutate: all mutants on changed lines killed.Summary by CodeRabbit
Bug Fixes
Refactor