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

docs/roadmap.md

@@ -416,11 +416,14 @@ fixed**:
   above), `primitive-inf-tests` (83%), `ifelse-tests` (175%),
   `module-recursion`, `resize-tests` (1.5%), `surface-simple` — real
   mismatches, not yet individually triaged.
-- [ ] `assign-tests` and `intersection_for-tests` (issue #89) still produce zero valid
+- [x] `assign-tests` and `intersection_for-tests` (issue #89) still produce zero valid
   geometry even after the harness fix — unlike the files that fix unblocked,
   these appear to genuinely fail in `MeshEvaluator`/`PrimitiveGen` itself
   (every root invalid, not just one), not just in the test tool. Needs the
   same per-root `chiselcad_cli --stats` triage the harness bug above got.
+  Root-caused and fixed in v3.13 — not a `MeshEvaluator`/`PrimitiveGen` bug
+  at all, but `assign()`/`intersection_for()` not being recognized as
+  builtins by the parser.
 - [ ] Several other files (issue #90) (`polyhedron-tests`, `minkowski3-difference-test`,
   `scale3D-tests`, `for-nested-tests`, `render-tests`, `mirror-tests`,
   `for-tests`, `edge-cases`, `rotate-parameters`,
@@ -759,6 +762,64 @@ cases:
   ring-angle formula — left as a known, low-priority gap rather than
   guessed at further.
 
+## v3.13 — issue #89 (`assign-tests`/`intersection_for-tests` zero geometry) root-caused and fixed
+
+Followed issue #89's own suggested triage: rather than assuming a
+`MeshEvaluator`/`PrimitiveGen` bug, checked whether ChiselCAD's parser
+recognized `assign()`/`intersection_for()` as builtins at all. It didn't —
+neither name appeared anywhere in `src/lang/Parser.cpp`'s `kBuiltinNodeNames`
+table (the mechanism real builtin module names are recognized through, since
+they aren't reserved lexer keywords — see the table's own comment). A
+statement-position call to either name fell through to `parseModuleCall()`,
+which looks up a *user-defined* module of that name; since neither file
+defines one, every top-level statement in both files resolved to nothing,
+explaining "zero valid combined geometry" for every root in both files (not
+just one bad root among otherwise-valid ones, matching the issue's own
+observation that these two are unlike the harness-bug files).
+
+- [x] **`assign(x = ..., ...) { ... }` — the deprecated statement form of
+  `let()` — wasn't recognized as a builtin at all.** Real OpenSCAD's
+  `assign()` predates `let()` and is semantically identical: block-scoped
+  bindings visible only to its children. `Parser::parseLetNode()` already
+  implements exactly this grammar/semantics and doesn't care which keyword
+  spelling it was invoked through (it just `advance()`s past whatever token
+  is at the current position), so `assign` was added to `kBuiltinNodeNames`
+  and routed straight to the existing `parseLetNode()` — no new AST node or
+  evaluator code needed.
+- [x] **`intersection_for(i = ...) { ... }` — the `for()` variant that
+  intersects its iterations instead of unioning them — wasn't recognized as
+  a builtin at all.** Added `intersection_for` to `kBuiltinNodeNames` and
+  gave `Parser::parseFor()` a new `isIntersection` parameter (default
+  `false`, so every existing `for()` call site is unaffected) that's
+  threaded onto a new `ForNode::isIntersection` field. `CsgEvaluator::evalFor`
+  now combines the loop's flattened, iterations-worth-of-children list with
+  `CsgBoolean::Op::Intersection` instead of `Op::Union` when the flag is set
+  — the only behavioral difference from a plain `for()` loop, matching real
+  OpenSCAD's documented semantics ("intersects the children rather than
+  doing a union").
+
+Verified for real: this environment's network egress is scoped to
+`particlesector/chiselcad` only (no `vcpkg`/Manifold download), so — same
+approach as v3.10 — compiled the actual `src/lang`/`src/csg` sources
+(GPU/Manifold-free, per `tests/tools/README.md`) against real `glm` and a
+from-source-built Catch2 (`extras/catch_amalgamated.*` from the upstream
+Catch2 repo), then ran the real `tests/test_parser.cpp`/
+`tests/test_csg_evaluator.cpp`/`tests/test_lexer.cpp`/
+`tests/test_interpreter.cpp`/`tests/test_source_loader.cpp` suites: 580 test
+cases / 3266 assertions, all passing, including 7 new regression tests for
+both fixes (parser-shape tests for each, plus evaluator tests confirming
+`intersection_for`'s combine-with-Intersection behavior, its
+empty-range/single-iteration edge cases, and `assign()`'s let()-equivalent
+scoping). Also hand-fed OpenSCAD-corpus-shaped snippets of both constructs
+(loop-and-rotate `intersection_for`, nested `assign()` shadowing an outer
+variable) through the real `CsgEvaluator` directly and confirmed 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 — this pass had no oracle
+available — but the root cause (both constructs being completely
+unrecognized, not a `MeshEvaluator`/`PrimitiveGen` tessellation bug) is
+confirmed, closing the specific question issue #89 asked.
+
 ## v4 — Tooling & Visual Quality
 
 - [ ] VS Code LSP extension (syntax highlighting, error squiggles, completions)

src/csg/CsgEvaluator.cpp

@@ -751,7 +751,9 @@ CsgNodePtr CsgEvaluator::evalFor(const ForNode& node, const glm::mat4& xform,
         return all[0];
 
     CsgBoolean u;
-    u.op = CsgBoolean::Op::Union;
+    // intersection_for(...) combines every iteration's children with
+    // Intersection instead of Union — otherwise identical to for().
+    u.op = node.isIntersection ? CsgBoolean::Op::Intersection : CsgBoolean::Op::Union;
     u.color = color;
     u.children = std::move(all);
     return makeBoolean(std::move(u));

src/lang/AST.h

@@ -184,6 +184,10 @@ struct ForNode {
     std::vector<AstNodePtr> children;
     SourceLoc               loc;
     uint8_t                 modifiers = ModNone;
+    // true for intersection_for(...) { ... } — every iteration's
+    // instantiated children are combined with Intersection instead of
+    // Union once the loop finishes (see CsgEvaluator::evalFor).
+    bool                    isIntersection = false;
 };
 
 inline AstNodePtr makeFor(ForNode n) {

src/lang/Parser.cpp

@@ -39,6 +39,8 @@ static const std::unordered_map<std::string_view, TokenKind> kBuiltinNodeNames =
     {"rotate_extrude", TokenKind::RotateExtrude},
     {"offset", TokenKind::Offset},
     {"projection", TokenKind::Projection},
+    {"intersection_for", TokenKind::IntersectionFor},
+    {"assign", TokenKind::Assign},
 };
 
 // A token can be used as a named-parameter name (e.g. `scale=`) if it's an
@@ -372,6 +374,15 @@ AstNodePtr Parser::parseNodeInner() {
                     return parseRender();
                 case TokenKind::Color:
                     return parseColor();
+                case TokenKind::IntersectionFor:
+                    return parseFor(/*isIntersection=*/true);
+                case TokenKind::Assign:
+                    // assign(x = ..., ...) { ... } is the deprecated
+                    // statement form of let() — identical grammar and
+                    // semantics (block-scoped bindings, children evaluated
+                    // with those bindings in effect), so it reuses
+                    // parseLetNode() outright.
+                    return parseLetNode(/*isAssign=*/true);
                 default:
                     // Every kBuiltinNodeNames value is handled above; reaching
                     // here means the map and this switch have diverged (a
@@ -651,12 +662,14 @@ AstNodePtr Parser::parseIf() {
 // upstream test corpus's for-tests.scad) — node.clauses stays empty in that
 // case.
 // ---------------------------------------------------------------------------
-AstNodePtr Parser::parseFor() {
-    const Token& kw = advance(); // consume 'for'
+AstNodePtr Parser::parseFor(bool isIntersection) {
+    const Token& kw = advance(); // consume 'for' or 'intersection_for'
     ForNode node;
     node.loc = kw.loc;
+    node.isIntersection = isIntersection;
 
-    expect(TokenKind::LParen, "expected '(' after 'for'");
+    expect(TokenKind::LParen,
+           isIntersection ? "expected '(' after 'intersection_for'" : "expected '(' after 'for'");
 
     while (!check(TokenKind::RParen) && !atEnd()) {
         const size_t prevPos = m_pos; // guard against zero-progress infinite loops
@@ -1229,13 +1242,16 @@ ExprPtr Parser::parseFunctionLit() {
 
 // ---------------------------------------------------------------------------
 // let statement — let(x = expr, ...) { children }
+// Also reused for assign(x = expr, ...) { children }, the deprecated
+// statement form of let() — identical grammar/semantics, see parseNodeInner.
 // ---------------------------------------------------------------------------
-AstNodePtr Parser::parseLetNode() {
-    const Token& kw = advance(); // consume 'let'
+AstNodePtr Parser::parseLetNode(bool isAssign) {
+    const Token& kw = advance(); // consume 'let' or 'assign'
     LetNode node;
     node.loc = kw.loc;
 
-    expect(TokenKind::LParen, "expected '(' after 'let'");
+    expect(TokenKind::LParen,
+           isAssign ? "expected '(' after 'assign'" : "expected '(' after 'let'");
     while (!check(TokenKind::RParen) && !atEnd()) {
         // Binding name: ident or $special (e.g. let($fn=64) ...) — see the
         // matching ModuleCallNode comment for why $special must be accepted

src/lang/Parser.h

@@ -43,7 +43,10 @@ class Parser {
     AstNodePtr parseRender();
     AstNodePtr parseColor();
     AstNodePtr parseIf();
-    AstNodePtr parseFor();
+    // isIntersection: true for intersection_for(...) { ... }, which shares
+    // for()'s entire grammar and only differs in how CsgEvaluator combines
+    // the iterations' results (intersection instead of union).
+    AstNodePtr parseFor(bool isIntersection = false);
     AstNodePtr parseModuleCall();
     AstNodePtr parseExtrusion(TokenKind k);
     AstNodePtr parseOffset();
@@ -64,7 +67,10 @@ class Parser {
     AstNodePtr  parseLocalFunctionDef();
 
     // ---- let statement ---------------------------------------------------
-    AstNodePtr parseLetNode();
+    // isAssign: true when reached via assign(...) { ... } — the deprecated
+    // statement form of let() — so error messages can name the keyword the
+    // caller actually wrote instead of always saying "let".
+    AstNodePtr parseLetNode(bool isAssign = false);
 
     // ---- expressions (Pratt parser) --------------------------------------
     ExprPtr parseExpr(int minPrec = 0);

src/lang/Token.h

@@ -95,6 +95,13 @@ enum class TokenKind : uint8_t {
     // 3-D → 2-D operations (see Ident above — not a Lexer-level keyword)
     Projection,
 
+    // intersection_for(i = ...) { ... } — a for() variant that intersects
+    // its iterations instead of unioning them; assign(x = ...) { ... } — the
+    // deprecated statement form of let(). Neither is a Lexer-level keyword,
+    // same as the other builtins above.
+    IntersectionFor,
+    Assign,
+
     // Range separator
     Colon, // :
 

tests/test_csg_evaluator.cpp

@@ -820,6 +820,56 @@ TEST_CASE("CsgEval:for() with no arguments under a transform still yields no geo
     REQUIRE(s.roots.empty());
 }
 
+TEST_CASE("CsgEval:intersection_for combines iterations with Intersection, not Union",
+          "[csg][bugfix]") {
+    // intersection_for(...) shares for()'s entire grammar/iteration
+    // machinery (see Parser::parseFor's isIntersection flag) but must
+    // combine the instantiated children with CsgBoolean::Op::Intersection
+    // instead of Op::Union — this is what distinguishes it from a plain
+    // for() loop, per OpenSCAD's own documented semantics ("a variant of
+    // the for statement that intersects the children rather than doing a
+    // union"). See issue #89: the parser previously didn't recognise
+    // intersection_for at all, so it fell through to an undefined
+    // module-call and produced no geometry whatsoever.
+    auto s = evaluate("intersection_for(i = [0:2]) translate([i*5, 0, 0]) cube(10);");
+    const auto& b = asBool(s.roots[0]);
+    REQUIRE(b.op == CsgBoolean::Op::Intersection);
+    REQUIRE(b.children.size() == 3);
+}
+
+TEST_CASE("CsgEval:intersection_for empty range yields no geometry", "[csg][bugfix]") {
+    auto s = evaluate("intersection_for(i = [5:3]) sphere(r=1);");
+    REQUIRE(s.roots.empty());
+}
+
+TEST_CASE("CsgEval:intersection_for with a single iteration yields that child directly",
+          "[csg][bugfix]") {
+    // Matches plain for()'s existing single-iteration behavior (see
+    // CsgEvaluator::evalFor: all.size() == 1 returns the child as-is rather
+    // than wrapping a single-child boolean node).
+    auto s = evaluate("intersection_for(i = [0:0]) sphere(r=1);");
+    REQUIRE(s.roots.size() == 1);
+    asLeaf(s.roots[0]); // would throw std::bad_variant_access if wrapped in a CsgBoolean
+}
+
+TEST_CASE("CsgEval:assign() is the deprecated statement form of let()", "[csg][bugfix]") {
+    // assign(x = ..., ...) { ... } is OpenSCAD's original (later deprecated)
+    // syntax for what let() does today — block-scoped bindings visible to
+    // its children only. See issue #89: the parser previously didn't
+    // recognise assign() as a builtin at all, so it fell through to an
+    // undefined module-call and produced no geometry whatsoever.
+    auto s = evaluate("assign(x = 5, y = x + 1) cube(y);");
+    REQUIRE(s.roots.size() == 1);
+    REQUIRE(asLeaf(s.roots[0]).params.at("x") == Approx(6.0));
+}
+
+TEST_CASE("CsgEval:assign() bindings do not leak past the block", "[csg][bugfix]") {
+    auto s = evaluate("x = 99; assign(x = 5) cube(x); cube(x);");
+    REQUIRE(s.roots.size() == 2);
+    REQUIRE(asLeaf(s.roots[0]).params.at("x") == Approx(5.0));
+    REQUIRE(asLeaf(s.roots[1]).params.at("x") == Approx(99.0));
+}
+
 TEST_CASE("CsgEval:multi-variable for() iterates the Cartesian product of all clauses",
           "[csg][bugfix]") {
     // for (x=[0:1], y=[0:1], z=[0:1]) — real OpenSCAD's multi-variable

tests/test_parser.cpp

@@ -518,6 +518,41 @@ TEST_CASE("Parser:multi-variable for() parses one clause per comma-separated bin
     REQUIRE(f.clauses[2].range.list.size() == 3);
 }
 
+TEST_CASE("Parser:intersection_for parses like for() with isIntersection set",
+          "[parser][bugfix]") {
+    // intersection_for(i = ...) { ... } previously wasn't recognised as a
+    // builtin at all — it fell through to parseModuleCall() looking for a
+    // user-defined module named "intersection_for", which doesn't exist, so
+    // the whole construct silently produced no geometry (issue #89).
+    auto r = parse("intersection_for(i = [0:2]) { translate([i,0,0]) cube(10); }");
+    REQUIRE(r.roots.size() == 1);
+    auto& f = asFor(r.roots[0]);
+    REQUIRE(f.isIntersection == true);
+    REQUIRE(f.clauses.size() == 1);
+    REQUIRE(f.clauses[0].var == "i");
+    REQUIRE(f.children.size() == 1);
+}
+
+TEST_CASE("Parser:plain for() is not isIntersection", "[parser]") {
+    auto r = parse("for (i = [0:2]) cube(10);");
+    auto& f = asFor(r.roots[0]);
+    REQUIRE(f.isIntersection == false);
+}
+
+TEST_CASE("Parser:assign() parses as the deprecated statement form of let()",
+          "[parser][bugfix]") {
+    // assign(x = ..., ...) { ... } previously wasn't recognised as a builtin
+    // either, for the same reason as intersection_for above — it fell
+    // through to an undefined module call named "assign" (issue #89).
+    auto r = parse("assign(x = 1, y = x + 1) { cube(y); }");
+    REQUIRE(r.roots.size() == 1);
+    const auto& letNode = std::get<LetNode>(*r.roots[0]);
+    REQUIRE(letNode.bindings.size() == 2);
+    REQUIRE(letNode.bindings[0].first == "x");
+    REQUIRE(letNode.bindings[1].first == "y");
+    REQUIRE(letNode.children.size() == 1);
+}
+
 TEST_CASE("Parser:rotate() with no arguments parses", "[parser][bugfix]") {
     // Real OpenSCAD accepts a bare rotate() (seen in the upstream test
     // corpus's rotate-parameters.scad: "rotate() //same as undef").