diff --git a/include/openscad_cpp_parser/api.hpp b/include/openscad_cpp_parser/api.hpp index 070a736..835baa5 100644 --- a/include/openscad_cpp_parser/api.hpp +++ b/include/openscad_cpp_parser/api.hpp @@ -60,6 +60,44 @@ std::unique_ptr buildScopes(const std::vector& ast); std::unique_ptr buildScopesInto(const std::vector>& ast, ScopeTable& table); std::unique_ptr buildScopesInto(const std::vector& 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. // diff --git a/src/api.cpp b/src/api.cpp index 645760d..33617e5 100644 --- a/src/api.cpp +++ b/src/api.cpp @@ -89,6 +89,17 @@ 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> 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 @@ -96,6 +107,7 @@ std::vector> parseAst(const std::string& code, const st ParseNumberingScope numbering; ParserDriver driver(origin); + driver.strictCommas = g_strictCommas; lexerBeginString(code); yy::parser parser(driver); int rc = parser.parse(); @@ -258,7 +270,8 @@ namespace { using FileAst = std::vector>; using FileAstPtr = std::shared_ptr; -// 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 @@ -266,11 +279,18 @@ using FileAstPtr = std::shared_ptr; 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{}(k.path) ^ (k.comments ? 0x5bf03635U : 0U); + return std::hash{}(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(written.time_since_epoch().count()); - const CacheKey key{absPath, includeComments}; + const CacheKey key{absPath, includeComments, g_strictCommas}; { std::lock_guard lock(g_astCacheMutex); auto it = g_astCache.find(key); diff --git a/src/grammar/driver.hpp b/src/grammar/driver.hpp index 19a299e..4352fc8 100644 --- a/src/grammar/driver.hpp +++ b/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; diff --git a/src/grammar/parser.y b/src/grammar/parser.y index 3f5a319..4196841 100644 --- a/src/grammar/parser.y +++ b/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: diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 6008c71..d8f9fe6 100644 --- a/tests/CMakeLists.txt +++ b/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) diff --git a/tests/test_strict_commas.cpp b/tests/test_strict_commas.cpp new file mode 100644 index 0000000..a31706d --- /dev/null +++ b/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 + +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; + } +}