Fix issue #89: assign() and intersection_for() not recognized as builtins - #104
Open
particlesector wants to merge 1 commit into
Open
Fix issue #89: assign() and intersection_for() not recognized as builtins#104particlesector wants to merge 1 commit into
particlesector wants to merge 1 commit into
Conversation
…tins
Root cause: neither name was ever added to Parser's kBuiltinNodeNames
table, so a statement-position call to either fell through to
parseModuleCall() looking for a (nonexistent) user-defined module,
silently producing zero geometry for every root in a file that used
either construct — not a MeshEvaluator/PrimitiveGen tessellation bug
as originally suspected.
assign(x = ..., ...) { ... } is the deprecated statement form of
let() and shares its exact grammar/semantics, so it's routed straight
to the existing parseLetNode(). intersection_for(i = ...) { ... }
shares for()'s entire grammar, differing only in combining its
iterations with Intersection instead of Union; added a ForNode
isIntersection flag threaded through Parser::parseFor() and read by
CsgEvaluator::evalFor().
Added parser + evaluator regression tests for both constructs.
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
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.
Summary
Closes #89.
Root-caused: neither
assign()norintersection_for()was ever added toParser::kBuiltinNodeNames, the table the parser uses to recognize builtin module names by text (since builtin names aren't reserved lexer keywords, matching real OpenSCAD). A statement-position call to either name therefore fell through toparseModuleCall(), which looks up a user-defined module of that name — since neither test file defines one, every top-level statement in bothassign-tests.scadandintersection_for-tests.scadsilently resolved to nothing. This explains "zero valid combined geometry" for every root in both files, matching the issue's own observation that these two are unlike the harness-bug files it also investigated. It is not aMeshEvaluator/PrimitiveGentessellation bug.assign(x = ..., ...) { ... }— the deprecated statement form oflet()— is grammatically and semantically identical tolet()(block-scoped bindings visible only to its children), so it's routed straight to the existingParser::parseLetNode(). No new AST node or evaluator code needed.intersection_for(i = ...) { ... }sharesfor()'s entire grammar, differing only in how the loop's results are combined. Added aForNode::isIntersectionflag, threaded through a newParser::parseFor(bool isIntersection = false)parameter (default preserves every existingfor()call site), and read byCsgEvaluator::evalFor()to pickCsgBoolean::Op::Intersectioninstead ofOp::Unionwhen combining the flattened per-iteration children.Verification
This session's network egress is scoped to this repo only, so the full CMake/vcpkg/Manifold build isn't available here. Followed the same approach as a prior pass (v3.10, see
docs/roadmap.md): compiled the actual Manifold-freesrc/lang/src/csgsources directly withg++against realglmand a from-source Catch2 (extras/catch_amalgamated.*), then ran the real test suites:tests/test_parser.cpp,tests/test_csg_evaluator.cpp,tests/test_lexer.cpp,tests/test_interpreter.cpp,tests/test_source_loader.cppintersection_for's intersect-not-union behavior, its empty-range/single-iteration edge cases, andassign()'slet()-equivalent block scoping).Also hand-verified with corpus-shaped snippets (a rotate-and-loop
intersection_for, a nestedassign()shadowing an outer variable) fed through the realCsgEvaluatordirectly, confirming each now produces non-empty geometry where it previously produced none.Exact volumetric correctness against a live OpenSCAD oracle (the v3.9-style
sym_diff_volumecheck) is still unverified — no oracle was available in this session — but the root cause is confirmed and fixed, answering the specific question issue #89 raised.Files changed
src/lang/Token.h— newIntersectionFor/Assigntoken tagssrc/lang/Parser.cpp/Parser.h— recognize both builtins;parseFor()gainsisIntersectionsrc/lang/AST.h—ForNode::isIntersectionfieldsrc/csg/CsgEvaluator.cpp—evalFor()combines with Intersection when the flag is settests/test_parser.cpp,tests/test_csg_evaluator.cpp— new regression testsdocs/roadmap.md— v3.13 entry documenting the root cause and fixTest plan
intersection_forparses toForNode{isIntersection=true};assign()parses toLetNodeintersection_forcombines viaIntersection, handles empty-range/single-iteration edge cases;assign()bindings scope correctly and don't leakfor()/let()/intersection()Generated by Claude Code