fix: load negative integer literals as numbers (#307) - #311
Open
livingstaccato wants to merge 1 commit into
Open
fix: load negative integer literals as numbers (#307)#311livingstaccato wants to merge 1 commit into
livingstaccato wants to merge 1 commit into
Conversation
`x = -3` serialized to the expression string `${-3}` rather than the int
`-3`, while `x = -3.5` still produced a float. Downstream code reading
numbers out of a parsed configuration got a string whenever the value
happened to be integral, and the 7.x behaviour (fixed in amplify-education#182) was lost.
MINUS is both the unary sign and the binary subtraction operator, so the
sign cannot simply be folded into INT_LITERAL: `10 -3` has to keep
parsing as a subtraction. FLOAT_LITERAL escapes this only because its
pattern cannot be confused with an operator followed by a digit.
Recombine the two at serialization instead, where the parse has already
settled the question: when a unary `-` is applied to something that
serialized to a number, and the operation is the whole value, emit the
negated number. Everything else keeps the `${...}` form -- `-var.count`
has no literal value, `!flag` is not arithmetic, `1 + -3` is a larger
expression whose operand must stay concatenable text, and a
`force_operation_parentheses` result cannot carry its parentheses as a
bare number.
Add an `integers` round-trip suite mirroring the existing `floats` one,
plus unit tests covering both halves of the trade-off. Without the fix,
12 of the new tests fail.
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.
Fixes #307.
x = -3serialized to the expression string${-3}rather than the int-3, whilex = -3.5still produced a float. Code reading numbers out of a parsed configuration got a string whenever the value happened to be integral.Verified in clean per-version environments (all on lark 1.3.1): correct in 7.2.1 and 7.3.1, wrong in 8.0.0rc1, 8.1.0 and 8.1.2 — so it arrived with the v8 rewrite, and it undoes the grammar fix from #182.
Why not fix the lexer
INT_LITERALalready permits a leading minus, so the tempting fix is to let it win. That breaks subtraction:MINUSis also the binary operator, and10 -3has to keep parsing as10 - 3— which it does in both 7.2.1 and 8.1.2 today.FLOAT_LITERALescapes the ambiguity only because its pattern cannot be confused with an operator followed by a digit.So the fix goes where the parse has already settled the question. In
UnaryOpRule.serialize, when a unary-applies to something that serialized to a number and the operation is the whole value, emit the negated number.Everything else keeps the
${...}form:-var.counthas no literal value,!flagis not arithmetic,1 + -3is a larger expression whose operand must stay concatenable text,-1e10serializes to a string underpreserve_scientific_notation, and aforce_operation_parenthesesresult cannot carry its parentheses as a bare number.Tests
integersround-trip suite mirroring the existingfloatsone, exercising negative literals bare, in tuples and in objects, alongside subtraction and negated references.UnaryOpRuleand API-level tests covering both halves of the trade-off.Without the source change, 12 of the new tests fail.
nose2 --config tox.ini: 1410 tests, OK.ruff checkandruff format --checkclean. No change to default behaviour beyond the reported bug.This pull request, and the investigation behind it, were produced by an AI assistant (Claude) working on behalf of the author. Every reproduction, test run and benchmark cited was executed rather than inferred, but please review with that provenance in mind.