fix: #3621 give range lower precedence than conditional expressions (breaking)#3680
Open
mvanhorn wants to merge 1 commit into
Open
fix: #3621 give range lower precedence than conditional expressions (breaking)#3680mvanhorn wants to merge 1 commit into
mvanhorn wants to merge 1 commit into
Conversation
…essions (breaking) The documented precedence table lists conditional < or < range, but the parser did not follow it: 'true ? 3 : -1 : 2 : 5' bound the conditional tighter than range while 'false or 1:3' bound range tighter than or, a non-transitive relation the docs never described. Per the owner's analysis in josdejong#3621, range cannot sit above conditional (that breaks 'a ? b : c', since 'b : c' would parse as a range), so range moves to a precedence between assignment and conditional. Reorder the recursive-descent chain to parseAssignment -> parseRange -> parseConditional -> ..., remove the now-redundant conditionalLevel bookkeeping (all 8 uses), and move the range band in operators.js so toString()/toTex() parenthesization matches. Accepted behavior changes (documented in HISTORY and syntax.md): - 'true ? 3 : -1 : 2 : 5' parses as '(true ? 3 : -1) : 2 : 5' - 'a:b==c:d' parses as 'a:(b==c):d' - 'true ? a = 1 : 7' now requires parentheses: 'true ? (a=1) : 7' - 'a = 1:5' still works (range stays above assignment) Targets v16 (breaking). Closes josdejong#3621
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
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.
Summary
Range
:now has a precedence between assignment and conditional, so the parser matches a strict precedence hierarchy. This targetsv16because it changes how a few expressions parse.Why this matters
The documented precedence table lists conditional
?:<or< range::, but the parser didn't follow it:true ? 3 : -1 : 2 : 5bound the conditional tighter than range, whilefalse or 1:3bound range tighter thanor. That relation is non-transitive, so the parser wasn't strictly precedence-based, and the docs never said so (#3621).In the issue thread you worked through the options and settled the direction: range can't be higher than conditional (that breaks
a ? b : c, sinceb : cwould parse as a range), so range moves below conditional and above assignment. This PR implements that, with the consequences you called out.Changes
src/expression/parse.js: reordered the recursive-descent chain toparseAssignment -> parseRange -> parseConditional -> parseLogicalOr -> ....parseConversionnow callsparseAddSubtract(range left that slot). Removed theconditionalLevelbookkeeping entirely (all 8 uses) — a:at range level is now unambiguously a range separator because conditional branches parse at a higher level that never consumes:. Net −27 lines in the parser.src/expression/operators.js: moved the range band directly above conditional (and below assignment) sotoString()/toTex()parenthesization stays consistent with parse precedence and round-tripping remains lossless.docs/expressions/syntax.mdandHISTORY.md: updated the precedence table and documented the breaking behavior changes.Accepted behavior changes:
true ? 3 : -1 : 2 : 5->(true ? 3 : -1) : 2 : 5a:b==c:d->a:(b==c):dtrue ? a = 1 : 7is now a syntax error; usetrue ? (a=1) : 7a = 1:5still works (range stays above assignment)Testing
Added #3621 regression tests and updated the tests that encoded the old ordering:
true ? 3 : -1 : 2 : 5parses as aRangeNodewhose start istrue ? 3 : (-1), evaluating to[3, 5]false or true ? 1 : 2still returns1(conditional belowor)false or 1:3parses as a range with startfalse or 1->[1, 2, 3]a:b==c:d->a:(b == c):d;true ? a = 1 : 7throws;true ? (a=1) : 7works;a = 1:5,A[1:3],f(1:3),1:2:10unchangedtoString()round-trip andtoTex()parenthesization for the reordered operatorsFull expression suite (
npx mocha test/unit-tests/expression/**/*.test.js): 2213 passing, no regressions.Closes #3621