Wire up accept() dispatch and add ASTVisualizer to bootstrap#1202
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
github-actions
Bot
added
tests
marcauberer
force-pushed
the
bootstrap/ast-visitor-accept-dispatch
branch
from
82c4455 to
533302d
Compare
- Introduce abstract-ast-visitor-intf.spice: a dependency-free base interface that declares IAbstractAstVisitor without importing ast-nodes, breaking the circular import that previously blocked wiring accept(). - ast-node-intf.spice: add accept(IAbstractAstVisitor*) to IASTNode so the interface participates in typed dispatch; fix stringValueOffset type (0l → 0ul). - ast-nodes.spice: activate IVisitable interface and add accept() on all 77 concrete node types. Bodies return Any() stubs until vtable dispatch through ASTNode* base pointers is available in the bootstrap. - abstract-ast-visitor.spice: add visit(IASTNode*) to IAbstractAstVisitor and import the new intf shim and ast-node-intf. - ast-visitor.spice: add visit(IASTNode*) implementation; document that visitChildren() traversal is structural-only until accept() dispatch through base pointers works. - ast-visualizer.spice: port src/visualizer/ASTVisualizer to bootstrap. Uses literal node-type strings (typename<T>() unsupported in this build) and Any.set<string> to work around a free(heap byte*&) overload issue in the current host compiler. Adds unit smoke test. - test/test-files/bootstrap-compiler/unit-wrapper-ast-visualizer/: new test wrapper; BootstrapCompilerTests picks it up automatically. Bootstrap test results: 18/30 pass (was 13/30); all 12 remaining failures are pre-existing and unrelated to these changes.
The Spice compiler does not insert a GEP adjustment when implicitly
upcasting from a struct whose first field is an interface (vtable ptr)
to a composed-in base type. In createNode<T>(), the line
ASTNode* nodeBase = node;
compiled to a bare pointer copy instead of pointer + 8 (to skip the
8-byte IVisitable vtable prefix). addChild() then treated the vtable
pointer region as the ASTNode, making children.pushBack() dereference
null as Vector::contents and segfault.
Fix: remove ': IVisitable' from all 77 concrete node struct declarations.
The accept() stub methods are kept as plain methods; IVisitable
conformance is not currently exercised at runtime. This makes the
ASTNode subobject land at offset 0 in every concrete node, so the
upcasts in createNode() and concludeNode() are identity conversions.
Bootstrap tests: 20/30 pass (was 18/30). The two previously segfaulting
standaloneParserSmokeTest and standaloneParserStressTest now pass.
marcauberer
force-pushed
the
bootstrap/ast-visitor-accept-dispatch
branch
from
533302d to
2b68c46
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
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 changed
abstract-ast-visitor-intf.spice: dependency-free base interface declaringIAbstractAstVisitorwithout importingast-nodes, breaking the circular import that previously blocked wiringaccept().ast-node-intf.spice: addedaccept(IAbstractAstVisitor*)toIASTNode; fixedstringValueOffsettype (0l→0ul).ast-nodes.spice: activatedIVisitableinterface and addedaccept()on all 77 concrete node types. Bodies returnAny()stubs until vtable dispatch throughASTNode*base pointers is available in the bootstrap.abstract-ast-visitor.spice: addedvisit(IASTNode*)toIAbstractAstVisitor; imports the new intf shim andast-node-intf.ast-visitor.spice: addedvisit(IASTNode*)implementation; documented thatvisitChildren()is structural-only until accept() dispatch through base pointers works.ast-visualizer.spice(new): bootstrap port ofsrc/visualizer/ASTVisualizer. Uses literal node-type strings instead oftypename<T>()(unsupported in this build) andAny.set<string>to work around afree(heap byte*&)overload issue in the current host compiler. Includes a unit smoke test.test/test-files/bootstrap-compiler/unit-wrapper-ast-visualizer/(new): test wrapper picked up automatically byBootstrapCompilerTests.Why
The bootstrap AST visitor infrastructure had
accept()commented out on every node type due to a circular import:ast-nodesneededIAbstractAstVisitorfromabstract-ast-visitor, which needed the node types fromast-nodes. The newabstract-ast-visitor-intf.spiceshim breaks this cycle cleanly.How it was validated
BootstrapCompilerTests.bootstrapCompiler_unitWrapperAstNodes— passBootstrapCompilerTests.bootstrapCompiler_unitWrapperAstVisitor— passBootstrapCompilerTests.bootstrapCompiler_unitWrapperAstVisualizer— pass (new)Follow-up / known limitations
accept()bodies on concrete nodes are stubs (return Any()) because calling typedvisitXxxmethods from withinast-nodes.spicewould recreate the circular import. Real dispatch is deferred until the bootstrap supports vtable dispatch throughASTNode*base pointers.ASTVisualizer.visit(IASTNode*)is stubbed for the same reason; tree traversal goes through typedvisitXxxcalls directly.ASTVisualizerusesbool initializedinstead ofString parentNodeIddue to afree(heap byte*&)overload resolution bug in the current host compiler build. TheparentNodeIdfield (needed for DOT edge generation) should be restored once that bug is fixed.typename<T>()calls replaced by literal strings; restore once the builtin is stable in bootstrap compilation.