Skip to content

Fix issue #89: assign() and intersection_for() not recognized as builtins - #104

Open
particlesector wants to merge 1 commit into
mainfrom
claude/issue-89-51c2wq
Open

Fix issue #89: assign() and intersection_for() not recognized as builtins#104
particlesector wants to merge 1 commit into
mainfrom
claude/issue-89-51c2wq

Conversation

@particlesector

Copy link
Copy Markdown
Owner

Summary

Closes #89.

Root-caused: neither assign() nor intersection_for() was ever added to Parser::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 to parseModuleCall(), which looks up a user-defined module of that name — since neither test file defines one, every top-level statement in both assign-tests.scad and intersection_for-tests.scad silently 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 a MeshEvaluator/PrimitiveGen tessellation bug.

  • assign(x = ..., ...) { ... } — the deprecated statement form of let() — is grammatically and semantically identical to let() (block-scoped bindings visible only to its children), so it's routed straight to the existing Parser::parseLetNode(). No new AST node or evaluator code needed.
  • intersection_for(i = ...) { ... } shares for()'s entire grammar, differing only in how the loop's results are combined. Added a ForNode::isIntersection flag, threaded through a new Parser::parseFor(bool isIntersection = false) parameter (default preserves every existing for() call site), and read by CsgEvaluator::evalFor() to pick CsgBoolean::Op::Intersection instead of Op::Union when 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-free src/lang/src/csg sources directly with g++ against real glm and 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.cpp
  • 580 test cases / 3266 assertions, all passing — including 7 new regression tests added by this change (parser-shape tests for both constructs, plus evaluator tests covering intersection_for's intersect-not-union behavior, its empty-range/single-iteration edge cases, and assign()'s let()-equivalent block scoping).

Also hand-verified with corpus-shaped snippets (a rotate-and-loop intersection_for, a nested assign() shadowing an outer variable) fed through the real CsgEvaluator directly, 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_volume check) 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 — new IntersectionFor/Assign token tags
  • src/lang/Parser.cpp / Parser.h — recognize both builtins; parseFor() gains isIntersection
  • src/lang/AST.hForNode::isIntersection field
  • src/csg/CsgEvaluator.cppevalFor() combines with Intersection when the flag is set
  • tests/test_parser.cpp, tests/test_csg_evaluator.cpp — new regression tests
  • docs/roadmap.md — v3.13 entry documenting the root cause and fix

Test plan

  • New parser tests: intersection_for parses to ForNode{isIntersection=true}; assign() parses to LetNode
  • New evaluator tests: intersection_for combines via Intersection, handles empty-range/single-iteration edge cases; assign() bindings scope correctly and don't leak
  • Full existing test suite (580 cases / 3266 assertions) still passes — no regressions to plain for()/let()/intersection()

Generated by Claude Code

…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.
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.

[High] assign-tests/intersection_for-tests produce zero valid geometry (real MeshEvaluator/PrimitiveGen bug, not test-tool)

2 participants