cmd/compile: support type assertion diagnostics - #2201
Conversation
There was a problem hiding this comment.
Review: type assertion diagnostics
The change is well-scoped and correct. The inlined / not inlined classification in cl/compile.go matches gc semantics — non-interface and empty-interface targets are inlined, non-empty interfaces are not inlined — and the emitted strings match gc's -d=typeassert reference (go/test/interface/assertinline.go), so removing the xfail.yaml entry is justified. No security concerns; when -d=typeassert is off the diagnostic block is fully guarded, so normal builds pay nothing beyond one boolean branch per assertion.
Two minor findings are inline. A few nits not worth blocking on:
SetTypeAssertDebughas no doc comment, but this matches the sibling setters (EnableDebug,EnableTrace), so it's consistent as-is.typeAssertNoColscould betypeAssertNoColumnsto match thenoColumnsparam andNoErrorColumnconfig field.- The
DebugTypeAssertfield comment ("report type-assertion lowering decisions") is slightly imprecise: the diagnostic re-derives the verdict from the asserted type rather than observing the actual lowering path inssa/interface.go. "report whether each type assertion is inlined" would read truer.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Review summary
This PR adds a -d=typeassert type-assertion lowering diagnostic to match gc's test/interface/assertinline.go errorcheck, and removes that case's xfail entry. The core logic is sound:
- The inlined / not-inlined classification via
intf.NumMethods() != 0on the asserted type's underlying interface matches gc's rule (assertions to a non-empty interface are "not inlined"; concrete types and empty interfaces are "inlined"). typeAssertDebugValuecorrectly handlestypeassert,typeassert=0,typeassert=N, and rejectstypeassert=garbage/nottypeassert=1, and is shared by bothrunCmd(behavioral) andcompatibleDebugSetting(compatibility gate) so parsing is not duplicated.- The stderr
Fprintfandfset.Positioncall are correctly gated behindif debugTypeAssert, so there is no cost on the normal compile path.
No bugs, regressions, security, or performance issues found. The findings below are minor comment-accuracy nits (left inline).
Note (not blocking): internal/build/build.go now calls cl.SetDebug(...) and its deferred reset unconditionally for every package, whereas previously this only happened inside the showDetail branch. cl's debug state is package-level global (debugInstr, debugTypeAssert, noErrorColumn), and buildPkg currently runs serially (buildAllPkgs iterates buildOne in plain for loops), so this is safe today. But createSSAPkg already carries a // TODO: build concurrently note — if package builds are ever parallelized, this per-package global set/reset would race and could clear the initial package's flags mid-compile. A short comment near the defer documenting the sequential-build assumption would make that coupling visible.
There was a problem hiding this comment.
Review summary
This is a clean, well-scoped change that wires a -d=typeassert diagnostic end-to-end (flag parsing → cl emission) to mirror gc's assertinline.go errorcheck behavior, and correctly removes the now-obsolete xfail.yaml entry. Security and performance are not a concern here (the diagnostic block is fully gated behind debugTypeAssert, and parsing runs once at flag-parse time). The defer in buildPkg correctly resets both llssa and cl debug state, so no flag leaks into subsequently built packages.
The findings below are non-blocking; the most substantive is the classification gap between the diagnostic label and the actual lowering (inline comment on ssa/interface.go).
Findings are left as inline comments on the relevant diff lines.
4bbb760 to
fd8476b
Compare
No description provided.