Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review summary
Solid, well-scoped change. It removes the reflection-based special-casing of x/tools opaqueType (cvtGoSSAOpaqueType/isGoSSAOpaqueType and the reflect import) and instead routes iterator/defer-stack handles through the ordinary named-type path, since x/tools now models them as named unsafe.Pointer types. The conversionNeedQuery state machine is simplified from a visiting/seen struct to a plain map[*types.Named]bool visited set, and completed nodes stay visited so shared subgraphs are not re-traversed.
Verified across all four review dimensions:
- Correctness: The caching invariants hold. A node needing conversion always propagates
trueupward, so the negative bulk-cache block never runs for a query touching it — no node needing conversion can be wrongly cached asconversionNotNeeded. Back-edgefalsereturns never become a node's final answer. - Performance: The exponential-to-linear claim checks out —
query[t]is set before recursion and never cleared, so a re-reached completed Named short-circuits.TestSharedNamedTypeConversionGraph(48-deep, two edges/node) collapses ~2^48 paths to 48 visits. - Security: The
unsafe.Pointermap keys are the standard pointer-identity idiom (live values kept reachable, round-tripped to their original concrete type) — no memory-safety concern. Dependency bumps (x/tools v0.50.0, x/mod, x/sys, x/sync) are forward-only with consistentgo.sumhashes. - Docs/tests: Updated comments accurately match the new traversal; the
buildGoSSAPackageForOpaque->buildGoSSAPackageForInternalTypesrename andTestGoSSAInternalPointerTypesreframing are complete and consistent.
Note: I could not compile/run tests locally (the xgo-dev/llvm cgo dependency needs LLVM headers not present in this environment) — relying on CI for build verification.
One non-blocking observation below.
Additional findings
ssa/type_cvt.go:236: [P3] Clean subgraphs under a conversion-needing query are not memoized: Negative results are bulk-cached only when the top-level query returnsfalse(this block). Positive results are cached per-node at line 291. As a consequence, when a query returnstrue, any conversion-free named subgraph explored before thetrue-returning sibling is leftconversionUnknown— it is neither in the persistentcvtneedcache nor bulk-stored here — so a later independent query re-traverses it from scratch.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
zhouguangyuan0718
force-pushed
the
codex/type-conversion-query-20260918
branch
2 times, most recently
from
September 20, 2026 16:25
563d7da to
49aa4e5
Compare
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
2 participants
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.
Conversion-need queries revisit a named type whenever a previous traversal of that type has completed. A shared type graph can therefore require exponentially many traversals; a 48-node diamond graph times out after five seconds. While compiling TypeScript Go tsc for js/wasm, process sampling showed repeated recursion in this query after several minutes of compilation.
Keep named types visited for the entire reachability query. Repeated edges need no further search, while positive results still propagate along proven paths and negative results enter the cross-query cache only after a complete negative query. This preserves the recursive conversion decisions and named-type identities.
Validation:
GOWORK=off go test ./ssa -count=1 -timeout=120spasses, including recursive generic types and traversal-order tests.The x/tools update from #2622 is now in main. With this fix integrated, LLGo successfully compiled and linked the full TypeScript Go
tsc/cmd/tsc(c975de5011fb7dfb32a491cf3fcf02d4f811f50e) forGOOS=js GOARCH=wasm.--versionprints7.1.0-devunder Node with the LLGo host adapter. The subsequent runtime nil-pointer symptom was traced to fixed WASM worker stack overflow. #2626 adds checked bounds and a general stack-budget option; with a 2 MiB worker stack, strict ES2022 compilation, emitted JS, declarations and diagnostics now match standard-Go tsc.