StrictCommaScope: reject the trailing commas OpenSCAD 2021.01 rejected

include/openscad_cpp_parser/api.hpp

@@ -60,6 +60,44 @@ std::unique_ptr<Scope> buildScopes(const std::vector<ASTNode*>& ast);
 std::unique_ptr<Scope> buildScopesInto(const std::vector<std::unique_ptr<ASTNode>>& ast, ScopeTable& table);
 std::unique_ptr<Scope> buildScopesInto(const std::vector<ASTNode*>& ast, ScopeTable& table);
 
+// Makes a trailing comma in a CALL ARGUMENT LIST a syntax error for as long
+// as it is in scope, matching OpenSCAD 2021.01:
+//
+//     cube(1,);                  // error while this is in scope
+//     echo(let(x = 1, y = 2,) x) // error -- let() parses as a call
+//     a = [2, 4,];               // still fine: 2021.01 accepts it too
+//     module m(a, b,) {}         // still fine, same reason
+//
+// One production (`arguments`) feeds every call form -- module
+// instantiation, function calls, echo, assert, render and let -- so this is
+// one rule, not a family of them. Parameter lists and list literals are
+// deliberately untouched: 2021.01 accepts a trailing comma in both, and
+// rejecting them would fail files it loads happily.
+//
+// A scope object rather than a parameter on all nine parse entry points:
+// this is a parse-wide mode, the same shape ParseNumberingScope already
+// uses, and threading a bool through getASTFromString/getASTFromFile/
+// getASTFromLibraryFile/getProgramFromFile and their helpers would touch
+// every one of them to say the same thing. Nests and restores on scope
+// exit, exceptions included; thread-local, so one thread parsing strictly
+// cannot change what another thread sees.
+//
+// getProgramFromFile()'s cache keys on this too -- a strict parse must not
+// be handed a tree an earlier lenient parse of the same file left behind.
+class StrictCommaScope {
+public:
+    StrictCommaScope();
+    ~StrictCommaScope();
+    StrictCommaScope(const StrictCommaScope&) = delete;
+    StrictCommaScope& operator=(const StrictCommaScope&) = delete;
+
+private:
+    bool previous_;
+};
+
+// Whether a StrictCommaScope is currently in effect on this thread.
+bool strictCommasEnabled();
+
 // Parses `code`. Throws ParseError (with the full caret diagnostic) on a
 // syntax error.
 //

src/api.cpp

@@ -89,13 +89,25 @@ std::string formatSyntaxError(const ParserDriver& driver, const std::string& cod
 
 } // namespace
 
+namespace {
+// See StrictCommaScope (api.hpp). Thread-local so a strict parse on one
+// thread cannot change what another thread is parsing.
+thread_local bool g_strictCommas = false;
+} // namespace
+
+StrictCommaScope::StrictCommaScope() : previous_(g_strictCommas) { g_strictCommas = true; }
+StrictCommaScope::~StrictCommaScope() { g_strictCommas = previous_; }
+
+bool strictCommasEnabled() { return g_strictCommas; }
+
 std::vector<std::unique_ptr<ASTNode>> parseAst(const std::string& code, const std::string& origin, SourceMap* sourceMap) {
     // Stamps every node this parse builds with one treeId and a dense
     // slot, so a ScopeTable can address it without the node carrying a
     // Scope pointer of its own -- see ASTNode::slot().
     ParseNumberingScope numbering;
 
     ParserDriver driver(origin);
+    driver.strictCommas = g_strictCommas;
     lexerBeginString(code);
     yy::parser parser(driver);
     int rc = parser.parse();
@@ -258,19 +270,27 @@ namespace {
 using FileAst = std::vector<std::unique_ptr<ASTNode>>;
 using FileAstPtr = std::shared_ptr<const FileAst>;
 
-// One entry per (file, comments) -- keyed by PATH, with the content stamp
+// One entry per (file, comments, strict-commas) -- keyed by PATH, with the
+// content stamp
 // stored beside the tree rather than in the key. A stale stamp REPLACES the
 // entry instead of adding a second one: an editor re-renders on every save,
 // and a stamp-in-the-key cache would keep a full copy of every version the
 // file ever had.
 struct CacheKey {
     std::string path;
     bool comments;
-    bool operator==(const CacheKey& o) const { return comments == o.comments && path == o.path; }
+    // Part of the key, not incidental: the same file parses differently
+    // under StrictCommaScope, so a strict parse must not be served a tree an
+    // earlier lenient parse of it left here (and vice versa).
+    bool strictCommas;
+    bool operator==(const CacheKey& o) const {
+        return comments == o.comments && strictCommas == o.strictCommas && path == o.path;
+    }
 };
 struct CacheKeyHash {
     size_t operator()(const CacheKey& k) const {
-        return std::hash<std::string>{}(k.path) ^ (k.comments ? 0x5bf03635U : 0U);
+        return std::hash<std::string>{}(k.path) ^ (k.comments ? 0x5bf03635U : 0U)
+               ^ (k.strictCommas ? 0x9e3779b9U : 0U);
     }
 };
 struct CacheEntry {
@@ -290,7 +310,7 @@ FileAstPtr parseFileShared(const std::string& absPath, bool includeComments) {
     const auto written = fs::last_write_time(absPath, ec);
     const std::int64_t stampMtime = ec ? 0 : static_cast<std::int64_t>(written.time_since_epoch().count());
 
-    const CacheKey key{absPath, includeComments};
+    const CacheKey key{absPath, includeComments, g_strictCommas};
     {
         std::lock_guard<std::mutex> lock(g_astCacheMutex);
         auto it = g_astCache.find(key);

src/grammar/driver.hpp

@@ -39,6 +39,9 @@ class ParserDriver {
     std::string origin_;
     NodeList result;
     bool hadError = false;
+    // Copied from the ambient StrictCommaScope at parse start (api.cpp), so
+    // one parse's mode cannot change under it mid-run.
+    bool strictCommas = false;
     int errorLine = 0;
     int errorColumn = 0;
     int errorOffset = 0;

src/grammar/parser.y

@@ -247,7 +247,18 @@ parameter:
 arguments:
     %empty                    { $$ = NodeList{}; }
   | argument_seq               { $$ = std::move($1); }
-  | argument_seq ","            { $$ = std::move($1); }
+    // The trailing comma OpenSCAD 2021.01 rejected. Still parsed, then
+    // reported -- @2 is the comma itself, so the caret lands on it rather
+    // than on the whole argument list, which a %expect-style grammar split
+    // could not manage. This one production feeds every call form (module
+    // instantiation, function calls, echo, assert, render, and let), which
+    // is why the whole feature is one rule. See StrictCommaScope (api.hpp).
+  | argument_seq ","            {
+        if (driver.strictCommas) {
+            driver.reportError(@2, "trailing comma in argument list");
+        }
+        $$ = std::move($1);
+    }
   ;
 
 argument_seq:
@@ -263,7 +274,18 @@ argument:
 assignments_expr:
     %empty                          { $$ = NodeList{}; }
   | assignment_expr_seq              { $$ = std::move($1); }
-  | assignment_expr_seq ","           { $$ = std::move($1); }
+    // 2021.01 rejected this one too. `let` does NOT go through `arguments`
+    // in this grammar -- it has its own assignment list, shared with `for`
+    // and `intersection_for` -- so the feature is two productions, not the
+    // one it looks like. 2021.01 rejects a trailing comma in all four:
+    // let(x=1,), for(i=[0:2],), intersection_for(i=[0:1],) and the list
+    // comprehension forms of for. See StrictCommaScope (api.hpp).
+  | assignment_expr_seq ","           {
+        if (driver.strictCommas) {
+            driver.reportError(@2, "trailing comma in assignment list");
+        }
+        $$ = std::move($1);
+    }
   ;
 
 assignment_expr_seq:

tests/CMakeLists.txt

@@ -26,6 +26,7 @@ add_executable(oscad_tests
   test_ast_generation.cpp
   test_node_str.cpp
   test_render_expression.cpp
+  test_strict_commas.cpp
 )
 target_link_libraries(oscad_tests PRIVATE openscad_cpp_parser GTest::gtest_main)
 

tests/test_strict_commas.cpp

@@ -0,0 +1,117 @@
+// StrictCommaScope: a trailing comma in a call argument list is an error,
+// matching OpenSCAD 2021.01 (BelfrySCAD issue #362).
+//
+// The scope is deliberately narrow. Measured against 2021.01 itself, it
+// rejected a trailing comma in CALL ARGUMENTS and accepted one everywhere
+// else -- list literals, list comprehensions, and module/function parameter
+// declarations all take one, in 2021.01 as in every version since. Rejecting
+// those would fail files 2021.01 loads happily, so they are left alone and
+// this file says so case by case.
+#include "test_helpers.hpp"
+
+#include <gtest/gtest.h>
+
+using namespace oscad;
+
+namespace {
+
+// Two productions carry it: `arguments` (every call form) and
+// `assignments_expr` (let/for/intersection_for). `let` is NOT a call in this
+// grammar -- it has its own assignment list -- which the first draft of this
+// feature got wrong and this test caught.
+const char* const kRejected[] = {
+    "cube(1,);",                            // module instantiation
+    "translate([0,0,0],) cube(1);",         // ...with a child
+    "function g(x) = x; y = g(1,);",        // user function call
+    "y = max(1, 2,);",                      // builtin function call
+    "y = str(\"a\", \"b\",);",
+    "echo(1, 2,);",
+    "assert(true, \"m\",);",
+    // assignments_expr, not arguments
+    "y = let(x = 1, y = 2,) x + y;",
+    "let(x = 1,) cube(x);",
+    "for (i = [0:2],) cube(i);",
+    "intersection_for (i = [0:1],) cube(1);",
+    "a = [for (i = [0:1],) i];",
+};
+
+// Accepted by 2021.01, so accepted here even under the scope.
+const char* const kAccepted[] = {
+    "a = [2, 4,];",                         // list literal
+    "a = [for (i = [0:2]) i,];",            // list comprehension
+    "a = [each [1, 2],];",
+    "module m(a, b,) {} m(1, 2);",          // module parameter declaration
+    "function f(a, b,) = a + b;",           // function parameter declaration
+    "cube(1);",                             // no trailing comma at all
+    "y = max(1, 2);",
+};
+
+} // namespace
+
+TEST(StrictCommas, CallArgumentTrailingCommaIsAnErrorInScope) {
+    StrictCommaScope strict;
+    for (const char* src : kRejected) {
+        EXPECT_THROW(parseSrc(src), ParseError) << src;
+    }
+}
+
+TEST(StrictCommas, EverythingElseKeepsItsTrailingComma) {
+    StrictCommaScope strict;
+    for (const char* src : kAccepted) {
+        EXPECT_NO_THROW(parseSrc(src)) << src;
+    }
+}
+
+TEST(StrictCommas, OffByDefault) {
+    // The whole set parses without the scope -- this is opt-in, and every
+    // existing caller must be unaffected.
+    for (const char* src : kRejected) {
+        EXPECT_NO_THROW(parseSrc(src)) << src;
+    }
+}
+
+TEST(StrictCommas, ScopeNestsAndRestores) {
+    EXPECT_FALSE(strictCommasEnabled());
+    {
+        StrictCommaScope outer;
+        EXPECT_TRUE(strictCommasEnabled());
+        {
+            StrictCommaScope inner;
+            EXPECT_TRUE(strictCommasEnabled());
+        }
+        EXPECT_TRUE(strictCommasEnabled()) << "an inner scope must not switch it off";
+    }
+    EXPECT_FALSE(strictCommasEnabled());
+}
+
+TEST(StrictCommas, RestoresWhenAParseThrows) {
+    try {
+        StrictCommaScope strict;
+        parseSrc("cube(1,);");
+        FAIL() << "expected ParseError";
+    } catch (const ParseError&) {
+    }
+    EXPECT_FALSE(strictCommasEnabled()) << "the scope must unwind with the exception";
+}
+
+TEST(StrictCommas, TheErrorPointsAtTheComma) {
+    StrictCommaScope strict;
+    try {
+        parseSrc("cube(1,);");
+        FAIL() << "expected ParseError";
+    } catch (const ParseError& e) {
+        const std::string what = e.what();
+        EXPECT_NE(what.find("trailing comma in argument list"), std::string::npos) << what;
+    }
+}
+
+TEST(StrictCommas, AnAssignmentListSaysSo) {
+    StrictCommaScope strict;
+    try {
+        parseSrc("y = let(x = 1,) x;");
+        FAIL() << "expected ParseError";
+    } catch (const ParseError& e) {
+        const std::string what = e.what();
+        EXPECT_NE(what.find("trailing comma in assignment list"), std::string::npos) << what;
+    }
+}