diff --git a/docs/roadmap.md b/docs/roadmap.md index 66a8dd7..927ec0b 100644 --- a/docs/roadmap.md +++ b/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) diff --git a/src/csg/CsgEvaluator.cpp b/src/csg/CsgEvaluator.cpp index 8ec79dd..d9b1467 100644 --- a/src/csg/CsgEvaluator.cpp +++ b/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)); diff --git a/src/lang/AST.h b/src/lang/AST.h index 06ff203..91dac9e 100644 --- a/src/lang/AST.h +++ b/src/lang/AST.h @@ -184,6 +184,10 @@ struct ForNode { std::vector 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) { diff --git a/src/lang/Parser.cpp b/src/lang/Parser.cpp index 8a049cf..84d861d 100644 --- a/src/lang/Parser.cpp +++ b/src/lang/Parser.cpp @@ -39,6 +39,8 @@ static const std::unordered_map 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 diff --git a/src/lang/Parser.h b/src/lang/Parser.h index 6398692..2b311b6 100644 --- a/src/lang/Parser.h +++ b/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); diff --git a/src/lang/Token.h b/src/lang/Token.h index e8ff254..6def2d5 100644 --- a/src/lang/Token.h +++ b/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, // : diff --git a/tests/test_csg_evaluator.cpp b/tests/test_csg_evaluator.cpp index f70d92f..6944b79 100644 --- a/tests/test_csg_evaluator.cpp +++ b/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 diff --git a/tests/test_parser.cpp b/tests/test_parser.cpp index b37aba7..f0a1520 100644 --- a/tests/test_parser.cpp +++ b/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(*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").