Fix StringLiteral source spans covering only the closing quote

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);
 };
 

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);
 }
 <STR>\\. { driver.stringBuffer.append(yytext, yyleng); }
 <STR>[^"\\]+ { driver.stringBuffer.append(yytext, yyleng); }
 <STR>\" {
     BEGIN(INITIAL);
-    return yy::parser::make_STRING(driver.stringBuffer, driver.currentTokenLoc());
+    return yy::parser::make_STRING(driver.stringBuffer, driver.stringTokenLoc());
 }
 
 <INCFILE>[ \t\r\n]+       { /* skip whitespace before < */ }

tests/test_lexical.cpp

@@ -363,3 +363,76 @@ TEST(LexicalIdentifiers, IdentifierStr) {
     std::vector<std::unique_ptr<ASTNode>> 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<size_t>(p.start_offset),
+                       static_cast<size_t>(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<oscad::Assignment*>(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<oscad::ModularCall*>(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<oscad::Assignment*>(ast[0].get());
+    ASSERT_NE(assign, nullptr);
+    auto* vec = dynamic_cast<oscad::ListComprehension*>(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<oscad::Assignment*>(ast[1].get());
+    ASSERT_NE(assign, nullptr);
+    EXPECT_EQ(assign->expr->position().line, 2);
+    EXPECT_EQ(spanned(src, *assign->expr), "\"one\ntwo\"");
+}