Skip to content

Commit 816ad06

Browse files
committed
Fix seven parser/lexer/evaluator bugs (#36, #40, #41, #54, #55, #56, #59)
- #36: for-loop over a bracketed point-list literal (e.g. [[1,2,3],[4,5,6]]) no longer flattens into scalars; only the unbracketed expression form (for (v = someVar)) still expands a vector value. - #40/#41: lexer now accepts trailing-dot floats (3.) per OpenSCAD grammar, and only consumes an 'e'/'E' exponent (and its sign) when at least one digit follows, so a bare sign/exponent no longer swallows an adjacent operator or gets silently truncated. - #54: unrecognized top-level tokens now emit a diagnostic before being skipped, instead of being silently dropped. - #55: named-parameter detection now rejects Number/String tokens as param names (e.g. cube(3=5) is a parse error) while still accepting keyword-named params like scale=. - #56: unterminated block comments are now logged as a Warning, matching the adjacent comment's stated non-fatal intent, instead of as an Error. - #59: MeshCache(0) no longer grows unbounded — insertion is skipped entirely when maxEntries == 0 instead of relying on a no-op eviction. Verified via a scratch CMake target building the lang/csg test sources (Catch2/glm/spdlog only, no Vulkan/Manifold) under -Wall -Wextra -Wpedantic -Werror: 352 test cases / 1823 assertions pass, including new regression tests for each fix.
1 parent ca53104 commit 816ad06

9 files changed

Lines changed: 153 additions & 12 deletions

File tree

src/csg/CsgEvaluator.cpp

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -441,8 +441,15 @@ CsgNodePtr CsgEvaluator::evalFor(const ForNode& node, const glm::mat4& xform, co
441441
else
442442
for (double v = start; v >= end - 1e-10 && (int)values.size() < kMaxIter; v += step)
443443
values.push_back(Value::fromNumber(v));
444+
} else if (node.range.isBracketedList) {
445+
// Bracketed list literal `[a, b, c]` — each element is its own loop
446+
// value, even if it evaluates to a vector (e.g. a point list
447+
// `[[1,2,3], [4,5,6]]` must yield two vector iterations, not six
448+
// scalars).
449+
for (const auto& e : node.range.list)
450+
values.push_back(m_interp->evaluate(*e));
444451
} else {
445-
// List form — evaluate each element; if one evaluates to a Vector,
452+
// Expression form `for (v = expr)` — expr must evaluate to a vector;
446453
// expand it so that `for (pt = pts)` iterates over pts' elements.
447454
for (const auto& e : node.range.list) {
448455
Value v = m_interp->evaluate(*e);

src/csg/MeshCache.cpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,9 @@ manifold::Manifold MeshCache::getOrCompute(const std::string& key,
1515

1616
// Cache miss — compute, then insert
1717
manifold::Manifold mesh = compute();
18+
if (m_maxEntries == 0)
19+
return mesh; // caching disabled — nothing to insert or evict
20+
1821
if (m_map.size() >= m_maxEntries)
1922
evictLru();
2023

src/lang/AST.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,10 @@ struct ForRange {
112112
ExprPtr step; // range form — nullptr means step of 1
113113
ExprPtr end; // range form — required
114114
std::vector<ExprPtr> list; // list form
115+
// true when `list` came from a bracketed literal `[a, b, c]` (each entry
116+
// is its own loop value, even if it evaluates to a vector); false when it
117+
// came from `for (v = expr)` (single expr, expanded if it's a vector).
118+
bool isBracketedList = false;
115119
};
116120

117121
struct ForNode {

src/lang/Lexer.cpp

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -251,14 +251,19 @@ Token Lexer::scanNumber(uint32_t startOffset) {
251251
// Integer part
252252
while (std::isdigit(static_cast<unsigned char>(peek()))) advance();
253253

254-
// Fractional part
255-
if (peek() == '.' && std::isdigit(static_cast<unsigned char>(peek(1)))) {
254+
// Fractional part — OpenSCAD allows a trailing dot with no digits after
255+
// it (e.g. `3.`), so only require a leading digit before the dot.
256+
if (peek() == '.') {
256257
advance(); // consume '.'
257258
while (std::isdigit(static_cast<unsigned char>(peek()))) advance();
258259
}
259260

260-
// Exponent
261-
if (peek() == 'e' || peek() == 'E') {
261+
// Exponent — only consume 'e'/'E' (and an optional sign) if at least one
262+
// digit follows; otherwise leave them unconsumed so a bare 'e'/'-'/'+'
263+
// isn't silently swallowed into a truncated number.
264+
if ((peek() == 'e' || peek() == 'E') &&
265+
(std::isdigit(static_cast<unsigned char>(peek(1))) ||
266+
((peek(1) == '+' || peek(1) == '-') && std::isdigit(static_cast<unsigned char>(peek(2)))))) {
262267
advance();
263268
if (peek() == '+' || peek() == '-') advance();
264269
while (std::isdigit(static_cast<unsigned char>(peek()))) advance();
@@ -396,11 +401,15 @@ void Lexer::skipBlockComment() {
396401
// Unterminated block comment — not a fatal error, just note it
397402
SourceLoc loc;
398403
loc.line = m_line; loc.col = m_col; loc.offset = static_cast<uint32_t>(m_pos);
399-
addError("unterminated block comment", loc);
404+
addWarning("unterminated block comment", loc);
400405
}
401406

402407
void Lexer::addError(const std::string& msg, SourceLoc loc) {
403408
m_diags.push_back({DiagLevel::Error, msg, loc, m_filePath});
404409
}
405410

411+
void Lexer::addWarning(const std::string& msg, SourceLoc loc) {
412+
m_diags.push_back({DiagLevel::Warning, msg, loc, m_filePath});
413+
}
414+
406415
} // namespace chisel::lang

src/lang/Lexer.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ class Lexer {
4141
void skipBlockComment();
4242

4343
void addError(const std::string& msg, SourceLoc loc);
44+
void addWarning(const std::string& msg, SourceLoc loc);
4445

4546
// ---- state ------------------------------------------------------------
4647
std::string_view m_source;

src/lang/Parser.cpp

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,16 @@
55

66
namespace chisel::lang {
77

8+
// A token can be used as a named-parameter name (e.g. `scale=`) if it's an
9+
// identifier or a keyword scanned as one (both carry non-empty `.text`).
10+
// Number/String literals also carry non-empty `.text` but must NOT be
11+
// accepted as param names — `cube(3=5)` should be a syntax error, not a
12+
// named param called "3".
13+
static bool isParamNameToken(const Token& t) {
14+
return !t.text.empty() && t.kind != TokenKind::Number && t.kind != TokenKind::String &&
15+
t.kind != TokenKind::SpecialVar;
16+
}
17+
818
// ---------------------------------------------------------------------------
919
// Constructor
1020
// ---------------------------------------------------------------------------
@@ -97,8 +107,9 @@ void Parser::parseStatement(ParseResult& result) {
97107
auto node = parseNode();
98108
if (node) {
99109
result.roots.push_back(std::move(node));
100-
} else {
101-
if (!atEnd()) advance(); // skip unrecognised token
110+
} else if (!atEnd()) {
111+
addError("unexpected token '" + peek().text + "' at statement position", peek().loc);
112+
advance(); // skip unrecognised token
102113
}
103114
match(TokenKind::Semicolon);
104115
}
@@ -350,7 +361,7 @@ AstNodePtr Parser::parseColor() {
350361
while (!check(TokenKind::RParen) && !atEnd()) {
351362
const size_t prevPos = m_pos; // guard against zero-progress infinite loops
352363

353-
if (peek(1).kind == TokenKind::Equals && !peek().text.empty()) {
364+
if (peek(1).kind == TokenKind::Equals && isParamNameToken(peek())) {
354365
std::string name = peek().text;
355366
advance(); // name
356367
advance(); // =
@@ -431,6 +442,7 @@ AstNodePtr Parser::parseFor() {
431442
} else {
432443
// List form: [first, ...]
433444
node.range.isRange = false;
445+
node.range.isBracketedList = true;
434446
node.range.list.push_back(std::move(first));
435447
while (match(TokenKind::Comma)) {
436448
if (check(TokenKind::RBracket)) break;
@@ -501,8 +513,7 @@ void Parser::parseParamList(std::unordered_map<std::string, ExprPtr>& params,
501513
}
502514

503515
// Named param: any token (Ident or keyword like 'scale') followed by '='
504-
if (peek(1).kind == TokenKind::Equals && !peek().text.empty() &&
505-
!check(TokenKind::SpecialVar)) {
516+
if (peek(1).kind == TokenKind::Equals && isParamNameToken(peek())) {
506517
std::string name = peek().text;
507518
advance(); // name token
508519
advance(); // =
@@ -916,7 +927,7 @@ void Parser::parseExtrusionParams(std::unordered_map<std::string, ExprPtr>& para
916927
continue;
917928
}
918929
// Accept any token (Ident or keyword like 'scale') when followed by '='
919-
if (peek(1).kind == TokenKind::Equals && !peek().text.empty()) {
930+
if (peek(1).kind == TokenKind::Equals && isParamNameToken(peek())) {
920931
std::string name = advance().text;
921932
advance(); // consume '='
922933
params[name] = parseExpr();

tests/test_csg_evaluator.cpp

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -664,6 +664,32 @@ TEST_CASE("CsgEval:for empty range yields no geometry", "[csg]") {
664664
REQUIRE(s.roots.empty());
665665
}
666666

667+
TEST_CASE("CsgEval:for over bracketed point-list literal iterates per-point, not flattened", "[csg]") {
668+
// Regression test: a bracketed list literal whose own elements are
669+
// vectors — the common point-list idiom — must yield one iteration per
670+
// point (2), not one per flattened scalar (6).
671+
auto s = evaluate("for (p = [[1,2,3], [4,5,6]]) translate(p) sphere(r=1);");
672+
REQUIRE(s.roots.size() == 1);
673+
const auto& b = asBool(s.roots[0]);
674+
REQUIRE(b.children.size() == 2);
675+
REQUIRE(asLeaf(b.children[0]).transform[3][0] == Approx(1.0f));
676+
REQUIRE(asLeaf(b.children[0]).transform[3][1] == Approx(2.0f));
677+
REQUIRE(asLeaf(b.children[0]).transform[3][2] == Approx(3.0f));
678+
REQUIRE(asLeaf(b.children[1]).transform[3][0] == Approx(4.0f));
679+
REQUIRE(asLeaf(b.children[1]).transform[3][1] == Approx(5.0f));
680+
REQUIRE(asLeaf(b.children[1]).transform[3][2] == Approx(6.0f));
681+
}
682+
683+
TEST_CASE("CsgEval:for over variable holding a point list still expands per-point", "[csg]") {
684+
// Same point-list idiom via a variable (expression form) — must keep
685+
// working the same way it did before the bracketed-list fix.
686+
auto s = evaluate("pts = [[1,2,3], [4,5,6]]; for (p = pts) translate(p) sphere(r=1);");
687+
REQUIRE(s.roots.size() == 1);
688+
const auto& b = asBool(s.roots[0]);
689+
REQUIRE(b.children.size() == 2);
690+
REQUIRE(asLeaf(b.children[1]).transform[3][0] == Approx(4.0f));
691+
}
692+
667693
// ---------------------------------------------------------------------------
668694
// User-defined modules
669695
// ---------------------------------------------------------------------------

tests/test_lexer.cpp

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,48 @@ TEST_CASE("Lexer:leading dot float", "[lexer]") {
125125
REQUIRE(t[0].numberValue() == Approx(0.5));
126126
}
127127

128+
TEST_CASE("Lexer:trailing dot float", "[lexer]") {
129+
auto t = lex("3.");
130+
REQUIRE(t.size() == 1);
131+
REQUIRE(t[0].kind == TokenKind::Number);
132+
REQUIRE(t[0].numberValue() == Approx(3.0));
133+
}
134+
135+
TEST_CASE("Lexer:trailing dot float followed by comma", "[lexer]") {
136+
auto t = kinds("[3., 1, 1]");
137+
REQUIRE(t == std::vector{TokenKind::LBracket, TokenKind::Number, TokenKind::Comma,
138+
TokenKind::Number, TokenKind::Comma, TokenKind::Number,
139+
TokenKind::RBracket});
140+
}
141+
142+
TEST_CASE("Lexer:exponent with digits", "[lexer]") {
143+
auto t = lex("5e3");
144+
REQUIRE(t.size() == 1);
145+
REQUIRE(t[0].kind == TokenKind::Number);
146+
REQUIRE(t[0].numberValue() == Approx(5000.0));
147+
}
148+
149+
TEST_CASE("Lexer:exponent with signed digits", "[lexer]") {
150+
auto t = lex("5e+3");
151+
REQUIRE(t.size() == 1);
152+
REQUIRE(t[0].numberValue() == Approx(5000.0));
153+
}
154+
155+
TEST_CASE("Lexer:bare 'e' after number is not consumed as exponent", "[lexer]") {
156+
// No digits follow 'e', so it must not be swallowed into the number —
157+
// it should tokenize as a separate identifier.
158+
auto t = kinds("5e;");
159+
REQUIRE(t == std::vector{TokenKind::Number, TokenKind::Ident, TokenKind::Semicolon});
160+
}
161+
162+
TEST_CASE("Lexer:exponent sign without digits does not eat a following operator", "[lexer]") {
163+
// Previously `2e-h` would swallow the '-' into a truncated "2e-" number,
164+
// silently dropping the intended binary minus. It must now tokenize as
165+
// Number(2) Ident(e) Minus Ident(h).
166+
auto t = kinds("2e-h");
167+
REQUIRE(t == std::vector{TokenKind::Number, TokenKind::Ident, TokenKind::Minus, TokenKind::Ident});
168+
}
169+
128170
TEST_CASE("Lexer:negative number", "[lexer]") {
129171
// '-' is now always a Minus token; negation is handled by the parser.
130172
auto t = lex("-1");
@@ -200,6 +242,14 @@ TEST_CASE("Lexer:block comment skipped", "[lexer]") {
200242
REQUIRE(kinds("cu/* mid */be") == (std::vector{TokenKind::Ident, TokenKind::Ident}));
201243
}
202244

245+
TEST_CASE("Lexer:unterminated block comment is a warning, not an error", "[lexer]") {
246+
Lexer lexer("cube /* never closed");
247+
lexer.tokenize();
248+
REQUIRE_FALSE(lexer.hasErrors());
249+
REQUIRE(lexer.diagnostics().size() == 1);
250+
REQUIRE(lexer.diagnostics()[0].level == DiagLevel::Warning);
251+
}
252+
203253
// ---------------------------------------------------------------------------
204254
// Identifiers (unrecognised words)
205255
// ---------------------------------------------------------------------------

tests/test_parser.cpp

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -716,3 +716,33 @@ TEST_CASE("Parser:include inside a block is a parse error, not silently dropped"
716716
const auto& u = std::get<BooleanNode>(*r.roots[0]);
717717
REQUIRE(u.children.size() == 1);
718718
}
719+
720+
// ---------------------------------------------------------------------------
721+
// Diagnostics for otherwise-silent parse gaps
722+
// ---------------------------------------------------------------------------
723+
TEST_CASE("Parser:unrecognized top-level statement reports an error", "[parser]") {
724+
// A bare identifier not followed by '(' or '=' is not a valid statement
725+
// and must not be silently skipped.
726+
Lexer lexer("y;");
727+
auto tokens = lexer.tokenize();
728+
Parser parser(std::move(tokens));
729+
auto r = parser.parse();
730+
REQUIRE(parser.hasErrors());
731+
REQUIRE(r.roots.empty());
732+
}
733+
734+
TEST_CASE("Parser:numeric token as a named-param name is a parse error", "[parser]") {
735+
// `3=5` must not be silently accepted as a named param keyed "3".
736+
Lexer lexer("cube(3=5);");
737+
auto tokens = lexer.tokenize();
738+
Parser parser(std::move(tokens));
739+
parser.parse();
740+
REQUIRE(parser.hasErrors());
741+
}
742+
743+
TEST_CASE("Parser:keyword-named param still parses as a named param", "[parser]") {
744+
// Regression guard: the numeric-token fix must not break keyword-named
745+
// params like 'scale=' (a keyword token, not Number/String).
746+
auto r = parse("linear_extrude(height=5, scale=2) square(1);");
747+
REQUIRE(r.roots.size() == 1);
748+
}

0 commit comments

Comments
 (0)