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\""); +}