StrictCommaScope: reject the trailing commas OpenSCAD 2021.01 rejected - #9
Merged
Merged
Conversation
For BelfrySCAD#362, which wants a --strict-commas flag so a script can be
checked against 2021.01. This is the parser half.
The scope is narrower than "no trailing commas", because 2021.01 was
narrower. Measured against 2021.01 itself rather than assumed:
cube(1,); ERROR a = [2, 4,]; accept
translate([0,0,0],) ... ERROR a = [for (i=[0:2]) i,]; accept
max(1, 2,) ERROR a = [each [1,2],]; accept
str("a","b",) ERROR module m(a, b,) {} accept
let(x=1, y=2,) ERROR function f(a, b,) = .. accept
for (i=[0:2],) ERROR
intersection_for(i=[0:1],) ERROR
So list literals, list comprehensions and parameter DECLARATIONS keep their
trailing comma -- rejecting those would fail files 2021.01 loads happily.
TWO productions carry it, not the one it looks like: `arguments`, which every
call form routes through, and `assignments_expr`, which let/for/
intersection_for have of their own. `let` is not a call in this grammar. The
first draft guarded only `arguments` and let(x=1,) sailed through; the test
listing every rejected form caught it, which is the reason the test lists
them one by one rather than asserting a rule.
A scope object rather than a parameter on the parse entry points: this is a
parse-wide mode, the shape ParseNumberingScope already uses here, and a bool
would have to be threaded through getASTFromString/getASTFromFile/
getASTFromLibraryFile/getProgramFromFile and four helpers to say one thing.
Thread-local, nests, and restores on scope exit including when the parse
throws -- both tested, since a mode that leaks would silently change how
every later parse behaves.
getProgramFromFile's shared cache keys on it too. Without that, a strict
parse of a file already parsed leniently would be served the lenient tree
and quietly pass -- only for a file included twice, which is the hardest
version of that bug to ever notice.
655 tests pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AzS79GLeG7LpYB6KjgDbi1
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Parser half of BelfrySCAD#362, which wants a
--strict-commasflag so a script can be checked against OpenSCAD 2021.01.The scope is narrower than "no trailing commas"
Because 2021.01 was narrower. Measured against 2021.01 itself rather than assumed:
cube(1,);a = [2, 4,];translate([0,0,0],) cube(1);a = [for (i=[0:2]) i,];max(1, 2,)a = [each [1,2],];str("a","b",)module m(a, b,) {}let(x=1, y=2,)function f(a, b,) = a+b;for (i=[0:2],)intersection_for(i=[0:1],)List literals, list comprehensions and parameter declarations keep their trailing comma. Rejecting those would fail files 2021.01 loads happily.
Two productions, not one
arguments, which every call form routes through — andassignments_expr, whichlet/for/intersection_forhave of their own.letis not a call in this grammar.The first draft guarded only
arguments, andlet(x=1,)sailed straight through. The test that lists every rejected form caught it, which is why it lists them one by one rather than asserting a rule.Why a scope object
This is a parse-wide mode, the shape
ParseNumberingScopealready uses here. A bool would have to be threaded throughgetASTFromString/getASTFromFile/getASTFromLibraryFile/getProgramFromFileand four helpers to say one thing.Thread-local, nests, and restores on scope exit including when the parse throws — both tested, since a mode that leaked would silently change how every later parse behaves.
The cache
getProgramFromFile's shared cache keys on it too. Without that, a strict parse of a file already parsed leniently would be served the lenient tree and quietly pass — and only for a file included twice, which is the hardest version of that bug to ever notice.655 tests pass.
🤖 Generated with Claude Code
https://claude.ai/code/session_01AzS79GLeG7LpYB6KjgDbi1