From 842a592c59b9ef8f5d73bbe216386388c7a72bf3 Mon Sep 17 00:00:00 2001 From: Revar Desmera Date: Thu, 6 Aug 2026 09:04:03 -0700 Subject: [PATCH] Fix StringLiteral source spans covering only the closing quote Every StringLiteral's position spanned a single `"` instead of the whole literal, and so did any node wrapping one -- a PositionalArgument, a vector element. A string is matched by SEVERAL lexer rules (opening quote, content runs, escape sequences, closing quote), and YY_USER_ACTION calls updateLocation() on every one of them, so tokenStart always describes the most recent match. Building the token's location from currentTokenLoc() in the closing-quote rule therefore described just that character. STRING is the only multi-rule token, which is why nothing else was affected. ParserDriver::stringStart now records the opening quote's position when that rule fires, and stringTokenLoc() spans from there to the current end. This mattered downstream: a consumer slicing source by these offsets could land inside string CONTENT. BelfrySCAD's reformatter rewrote a comma inside "h,l,height,length" in BOSL2's isosurface.scad, silently changing the string's value -- found because the reformat was checked for AST-shape preservation across the corpus rather than eyeballed. Tests cover the plain case, empty strings, embedded escaped quotes, escape sequences, commas inside strings, the spans of enclosing nodes (how the bug actually did damage), strings in vectors, and a multi-line string still reporting its OPENING line rather than its closing one. Co-Authored-By: Claude Opus 5 (1M context) --- src/grammar/driver.hpp | 21 ++++++++++++ src/grammar/lexer.l | 6 +++- tests/test_lexical.cpp | 73 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 99 insertions(+), 1 deletion(-) diff --git a/src/grammar/driver.hpp b/src/grammar/driver.hpp index 59a4d3a..9793dd0 100644 --- a/src/grammar/driver.hpp +++ b/src/grammar/driver.hpp @@ -49,6 +49,18 @@ class ParserDriver { LexPos tokenEnd; std::string stringBuffer; // accumulates a STRING token's content while in the %x STR lexer state + // Where the opening quote of the STRING currently being lexed began. + // + // Needed because YY_USER_ACTION resets tokenStart on EVERY rule match, + // and a string is matched by several rules (opening quote, content + // runs, escapes, closing quote). currentTokenLoc() at the closing + // quote therefore describes just that one character, which made every + // StringLiteral's source span a single `"` -- and dragged the span of + // anything wrapping it (a PositionalArgument, a vector element) along + // with it. Recorded when the opening quote is matched and used by + // stringTokenLoc() below. + LexPos stringStart; + Position toPosition(const OscadLocation& loc) const { return Position{origin_, loc.first_line, loc.first_column, loc.first_offset, loc.last_offset}; } @@ -58,6 +70,15 @@ class ParserDriver { tokenEnd.column, tokenStart.offset, tokenEnd.offset}; } + // As currentTokenLoc(), but spanning from the string's OPENING quote + // (stringStart) to the current token's end -- i.e. the whole literal + // including both quotes, which is what a caller slicing the source by + // this span expects to get back. + OscadLocation stringTokenLoc() const { + return OscadLocation{stringStart.line, stringStart.column, tokenEnd.line, + tokenEnd.column, stringStart.offset, tokenEnd.offset}; + } + void reportError(const OscadLocation& loc, const std::string& message); }; diff --git a/src/grammar/lexer.l b/src/grammar/lexer.l index 764c560..5ffab6e 100644 --- a/src/grammar/lexer.l +++ b/src/grammar/lexer.l @@ -93,13 +93,17 @@ NUMLIT 0[xX][0-9A-Fa-f]+|[0-9]+([.][0-9]*)?([eE][+-]?[0-9]+)?|[.][0-9]+([eE][+- \" { driver.stringBuffer.clear(); + // YY_USER_ACTION has just set tokenStart to this opening quote; keep + // it, because the rules that follow will overwrite it (see + // ParserDriver::stringStart). + driver.stringStart = driver.tokenStart; BEGIN(STR); } \\. { driver.stringBuffer.append(yytext, yyleng); } [^"\\]+ { driver.stringBuffer.append(yytext, yyleng); } \" { BEGIN(INITIAL); - return yy::parser::make_STRING(driver.stringBuffer, driver.currentTokenLoc()); + return yy::parser::make_STRING(driver.stringBuffer, driver.stringTokenLoc()); } [ \t\r\n]+ { /* skip whitespace before < */ } diff --git a/tests/test_lexical.cpp b/tests/test_lexical.cpp index bc7ad6e..e3919d9 100644 --- a/tests/test_lexical.cpp +++ b/tests/test_lexical.cpp @@ -363,3 +363,76 @@ TEST(LexicalIdentifiers, IdentifierStr) { std::vector> ast; EXPECT_EQ(exprSrc("foo", ast)->toString(), "foo"); } + +// -- Source spans --------------------------------------------------------- + +namespace { +// The source text a node's own position spans -- what a consumer slicing +// by start/end_offset actually gets back. +std::string spanned(const std::string& src, const oscad::ASTNode& n) { + const auto& p = n.position(); + return src.substr(static_cast(p.start_offset), + static_cast(p.end_offset - p.start_offset)); +} +} // namespace + +// A string is matched by SEVERAL lexer rules (opening quote, content runs, +// escapes, closing quote), and YY_USER_ACTION resets tokenStart on every +// one of them -- so building the token's location from currentTokenLoc() at +// the closing quote described just that one character. Every StringLiteral +// spanned a bare `"`, and so did anything wrapping it, which silently +// corrupted a downstream consumer that sliced source by those offsets. +TEST(SourceSpans, StringLiteralSpansTheWholeLiteral) { + struct Case { std::string src; std::string want; }; + const Case cases[] = { + {"a = \"txt\";", "\"txt\""}, + {"a = \"\";", "\"\""}, // empty + {"a = \"with \\\"quote\\\" in it\";", "\"with \\\"quote\\\" in it\""}, + {"a = \"esc \\n seq\";", "\"esc \\n seq\""}, + {"a = \"comma,inside\";", "\"comma,inside\""}, + }; + for (const Case& c : cases) { + auto ast = oscad::parseSrc(c.src); + ASSERT_EQ(ast.size(), 1u) << c.src; + auto* assign = dynamic_cast(ast[0].get()); + ASSERT_NE(assign, nullptr) << c.src; + EXPECT_EQ(spanned(c.src, *assign->expr), c.want) << c.src; + } +} + +// The enclosing node's span has to be right too -- that is how the bug +// actually did damage, by dragging an argument's end_offset into the +// middle of the string. +TEST(SourceSpans, NodesWrappingAStringSpanCorrectly) { + const std::string src = "f(\"x,y\", b);"; + auto ast = oscad::parseSrc(src); + auto* call = dynamic_cast(ast[0].get()); + ASSERT_NE(call, nullptr); + ASSERT_EQ(call->arguments.size(), 2u); + EXPECT_EQ(spanned(src, *call->arguments[0]), "\"x,y\""); + EXPECT_EQ(spanned(src, *call->arguments[1]), "b"); +} + +TEST(SourceSpans, StringsInAVectorSpanCorrectly) { + const std::string src = "x = [\"a,b\",\"c\"];"; + auto ast = oscad::parseSrc(src); + auto* assign = dynamic_cast(ast[0].get()); + ASSERT_NE(assign, nullptr); + auto* vec = dynamic_cast(assign->expr.get()); + ASSERT_NE(vec, nullptr); + ASSERT_EQ(vec->elements.size(), 2u); + EXPECT_EQ(spanned(src, *vec->elements[0]), "\"a,b\""); + EXPECT_EQ(spanned(src, *vec->elements[1]), "\"c\""); +} + +// Multi-line strings must still report the OPENING quote's line/column, not +// the closing one's. +TEST(SourceSpans, MultiLineStringReportsItsStartingLine) { + const std::string src = "a = 1;\nb = \"one\ntwo\";\n"; + auto ast = oscad::parseSrc(src); + ASSERT_EQ(ast.size(), 2u); + auto* assign = dynamic_cast(ast[1].get()); + ASSERT_NE(assign, nullptr); + EXPECT_EQ(assign->expr->position().line, 2); + EXPECT_EQ(spanned(src, *assign->expr), "\"one\ntwo\""); +}