Skip to content

runtime: preserve type metadata in assertion panics - #2252

Open
cpunion wants to merge 2 commits into
xgo-dev:mainfrom
cpunion:codex/goroot-typeassert-errors-20260801
Open

runtime: preserve type metadata in assertion panics#2252
cpunion wants to merge 2 commits into
xgo-dev:mainfrom
cpunion:codex/goroot-typeassert-errors-20260801

Conversation

@cpunion

@cpunion cpunion commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator

Type assertion failures currently lower the asserted type to a string before calling the runtime. That loses package identity, the source interface type, and instantiated generic type information.

Pass ABI type descriptors for the source interface and asserted type instead, and construct the existing Go-compatible TypeAssertionError directly. Empty source interfaces remain nil so ordinary interface-conversion messages keep their Go spelling.

This removes the xfails for:

  • fixedbugs/issue18911.go
  • interface/embed3.go
  • typeparam/mdempsky/16.go
  • fixedbugs/issue26094.go

The scoped same-name type regression is covered in TestInterfaceAssertRejectsSameNameTypesFromDifferentScopes; it checks both local-to-local and local-to-package assertions.

Validation:

  • go test ./ssa ./test/goroot -count=1 -timeout=15m
  • affected 14 cl runtime/IR fixtures
  • focused llgo test assertion regressions
  • the first three GOROOT cases with Go 1.24.11, 1.25.0, and 1.26.5 on darwin/arm64
  • fixedbugs/issue26094.go with empty xfail on all three Go versions on darwin/arm64

@github-actions

github-actions Bot commented Aug 1, 2026

Copy link
Copy Markdown

LLGo baseline benchmarks

6477f8e80fbb | workflow run | long-term charts

Program measurements

Platform Workload File size vs main Build vs main Run vs main
Linux cprintf 18512 B new 372.222 ms new 1.340 ms new
Linux fmtprintf 2215624 B new 3.537 s new 2.537 ms new
Linux println 71472 B new 370.604 ms new 1.597 ms new
macOS cprintf 84672 B new 509.855 ms new 4.079 ms new
macOS fmtprintf 2344880 B new 5.793 s new 32.580 ms new
macOS println 125712 B new 377.698 ms new 4.136 ms new
Core language and compiler benchmarks
Platform Benchmark ns/op vs main
Linux BenchmarkLookupPCRandom 13.310 ns/op new
Linux BenchmarkMergeCompilerFlags 154.700 ns/op new
Linux BenchmarkMergeLinkerFlags 94.960 ns/op new
Linux BenchmarkChannelBuffered 34.550 ns/op new
Linux BenchmarkChannelHandoff 25694 ns/op new
Linux BenchmarkDefer 46.440 ns/op new
Linux BenchmarkDirectCall 1.556 ns/op new
Linux BenchmarkGlobalRead 1.559 ns/op new
Linux BenchmarkGlobalWrite 2.486 ns/op new
Linux BenchmarkGoroutine 33071 ns/op new
Linux BenchmarkInterfaceCall 7.787 ns/op new
Linux BenchmarkRuntimeGetG 2.809 ns/op new
macOS BenchmarkLookupPCRandom 17.010 ns/op new
macOS BenchmarkMergeCompilerFlags 183.200 ns/op new
macOS BenchmarkMergeLinkerFlags 118.900 ns/op new
macOS BenchmarkChannelBuffered 32.030 ns/op new
macOS BenchmarkChannelHandoff 7869 ns/op new
macOS BenchmarkDefer 50.960 ns/op new
macOS BenchmarkDirectCall 1.495 ns/op new
macOS BenchmarkGlobalRead 1.358 ns/op new
macOS BenchmarkGlobalWrite 2.151 ns/op new
macOS BenchmarkGoroutine 43872 ns/op new
macOS BenchmarkInterfaceCall 7.314 ns/op new
macOS BenchmarkRuntimeGetG 3.016 ns/op new

No main baseline exists yet; all metrics are marked new.

Warning

  • The rendered benchmark data could not be pushed.

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review summary

Reviewed the type-assertion panic changes against the PR's actual diff (head 959307217 vs its parent). Four review passes (code quality, performance, security, docs) were run; findings that turned out to target unrelated code already on main (the RewriteMainPrefix removal) were discarded as out of scope for this PR.

No blocking issues found. This is a clean, well-scoped change.

What the change does and why it holds up:

  • runtime/internal/runtime/errors.go: PanicTypeAssert now takes (source, concrete, asserted *_type, missingMethod) and constructs a TypeAssertionError, delegating all message formatting to the existing Error() method instead of building strings inline. The dead helpers sameTypeAssertName/typeNameSuffix are removed with no remaining references (hasPrefix is still used elsewhere). Error() already distinguishes "different packages" vs "different scopes" and handles the nil-concrete and missing-method cases, so behavior is preserved and made more gc-accurate.
  • ssa/interface.go: the failure branch now passes the source interface's abi type — real pointer for non-empty interfaces (NumMethods() > 0), nil for empty interfaces. The source computation is emitted only into the panic block (blks[1]), so the successful-assertion hot path is unaffected; the single TypeAssertionError heap allocation is confined to the cold, terminal panic path.
  • The regenerated FileCheck goldens are consistent with the new call shape: any assertions emit ptr null for source, non-empty-interface assertions emit a real type pointer (e.g. cl/_testrt/any/in.go, cl/_testgo/tpinst/main.go).
  • test/go/interface_type_assert_panic_test.go: TestGenericNilInterfaceAssertMentionsSourceType correctly exercises the new source-type-in-message behavior for a nil non-empty interface.

Non-blocking observations (no action required):

  • TypeAssertionError.Error() dereferences e.asserted.String() without a nil guard, unlike the _interface/concrete fields. This line is pre-existing (not changed by this PR) and every construction site (ssa/interface.go, z_face.go) passes a non-nil asserted, so it is safe today — noting only as a latent invariant.
  • The removed test/goroot/xfail.yaml entries (typeparam/mdempsky/16.go, fixedbugs/issue18911.go, interface/embed3.go) match what this change fixes. The embed3.go case depends on _type.String()/pkgpath() output, which this PR does not modify; worth confirming those three goroot cases are green in CI before merge.

@codecov

codecov Bot commented Aug 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant