fix: parse scientific notation in numbers - #4490
Conversation
The dimension rule matched a number without an exponent part, so the `e` in `1e3px` was taken as the unit and the rest as a separate value. Valid CSS was silently compiled to a different value — `scale(1e-2)` became `scale(-1e)` and `padding: .5e-2px` became `-1.5e` — or to output that is not CSS at all, and `(1e3px + 1px)` failed to parse. The exponent requires at least one digit after `e`, so units that begin with `e` keep parsing as units: `1em` and `2ex` are unchanged, while `1e2em` is now 100em.
dosubot
Bot
added
the
size:M
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe dimension parser now accepts scientific-notation numbers such as ChangesScientific-Notation Dimension Parsing
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| const value = parserInput.$re(/^([+-]?\d*\.?\d+)(%|[a-z_]+)?/i); | ||
| const value = parserInput.$re(/^([+-]?\d*\.?\d+(?:e[+-]?\d+)?)(%|[a-z_]+)?/i); | ||
| if (value) { |
There was a problem hiding this comment.
When a dimension contains a sufficiently large exponent such as 1e309px, the expanded regex passes 1e309 to Dimension, where parseFloat produces Infinity; serialization then emits invalid CSS such as Infinitypx instead of safely handling the numeric token.
There was a problem hiding this comment.
The Infinitypx output is real, but it isn't reachable by the example given, and it isn't introduced here — it's a pre-existing overflow in Dimension.genCSS.
The stated repro doesn't reproduce. A declaration value isn't numerically re-serialized, so 1e309px passes through verbatim — on this branch and on master alike:
a { width: 1e309px; } /* both: width: 1e309px */Infinity appears only once something forces the Dimension back through genCSS: arithmetic, unary minus, a variable reference, or unit().
And it isn't about exponents. genCSS does String(this.fround(context, this.value)) over a value from parseFloat, so any literal past Number.MAX_VALUE (1.797e308) overflows. The defect reproduces on master with no exponent syntax anywhere — just a digit run:
input, on master |
output |
|---|---|
-1 followed by 308 zeros, then px |
-1e+308px |
-1 followed by 309 zeros, then px |
-Infinitypx |
One extra zero flips it, and nothing from this PR is on that path.
The 308-zero row is worth a second look, because it's this PR's own argument. master emits -1e+308px — notation master's parser cannot read back. Feeding that output in again:
a { width: -1e+308px + 0px; }
/* master: width: 307e; ← sign, mantissa and unit all gone */
/* branch: width: -1e+308px; */So the exponent gap was never only an input problem: less already produces exponent notation and then fails to round-trip it. That's the hole this change closes.
On fixing the overflow here — I'd rather not, and I don't think it belongs in this PR. The honest fix lives in Dimension.genCSS (reject or clamp a non-finite value), and it would change master's behaviour for the plain digit-run case above, which has nothing to do with parsing scientific notation. It's also a real decision rather than a mechanical one: throw "number too large" versus clamp to MAX_VALUE. Turning a one-line regex change into a numeric-output change seems like the wrong trade. Happy to file it separately and fix it there — just say which behaviour you'd want.
All seven test legs are green on 0f99a40.
There was a problem hiding this comment.
You're right on all counts. The overflow predates this PR, isn't introduced by the regex change, and the round-trip argument you've made is actually a stronger case for this fix — master already emits notation it can't re-parse.
The original concern doesn't hold up against the repro evidence. A separate issue/PR scoped to Dimension.genCSS is the right place to decide between throwing and clamping for non-finite values. That decision shouldn't block a correct parser fix.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
|
@coderabbitai - thoughts on this discussion here? #4490 (review) Is this a correct change? How does this align with other CSS pre-processors? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="less/less.js"
pr="4490"
review_id="4890810872"
echo "== Pull request metadata =="
gh api "repos/$repo/pulls/$pr" \
--jq '{number, title, state, base: .base.ref, head: .head.ref, body}'
echo
echo "== Referenced review =="
gh api "repos/$repo/pulls/$pr/reviews/$review_id" \
--jq '{id, user: .user.login, state, body, submitted_at, commit_id}'
echo
echo "== Referenced review comments =="
gh api --paginate "repos/$repo/pulls/$pr/comments?per_page=100" \
--jq '.[] | select(.pull_request_review_id == 4890810872) | {id, path, line, side, body, diff_hunk, user: .user.login}'
echo
echo "== Parser structure =="
ast-grep outline packages/less/lib/less/parser/parser.js --items all --type function --match 'dimension|entities'
echo
echo "== Affected parser context =="
rg -n -C 16 'dimension\s*[:=]|dimension\s*\(' packages/less/lib/less/parser/parser.js
echo
echo "== Exponent fixture files =="
fd -t f '^numbers-exponent\.(less|css)$' packages/test-data/tests-unit -x sh -c '
echo "---- $1 ----"
cat -n "$1"
' sh {}Length of output: 14899 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Dimension implementation candidates =="
fd -t f . packages/less/lib/less/tree | rg '/dimension\.(js|ts)$|/unit\.(js|ts)$'
echo
echo "== Dimension implementation outline and relevant code =="
fd -t f '^dimension\.(js|ts)$' packages/less/lib/less/tree -x sh -c '
ast-grep outline "$1" --items all
echo "---- $1: Infinity, parseFloat, toCSS, value ----"
rg -n -C 10 "parseFloat|Infinity|NaN|toCSS|value" "$1"
' sh {}
echo
echo "== Numeric serialization and finite-value checks =="
rg -n -C 6 'isFinite|Number\.isFinite|Infinity|NaN|parseFloat|toPrecision|toFixed' \
packages/less/lib packages/test-data/tests-unit \
-g '*.js' -g '*.less' -g '*.css'
echo
echo "== Existing scientific-notation and large-number tests =="
rg -n -i -C 5 'scientific|exponent|1e[+-]?[0-9]+|e[+-]?[0-9]+|infinity|nan|[0-9]{100,}' \
packages/test-data packages/less \
-g '*.less' -g '*.css' -g '*.js'Length of output: 201460 🤖 web_search executed:
Length of output: 7647 🤖 web_search executed:
Length of output: 6466 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Less Dimension source =="
cat -n packages/less/lib/less/tree/dimension.js
echo
echo "== Less number formatting source =="
rg -n -C 12 'numPrecision|toFixed|toPrecision|Number\.isFinite|isFinite|Infinity|NaN' \
packages/less/lib/less \
-g '*.js' \
-g '!**/tree/dimension.js'
echo
echo "== Base and PR parser rules =="
git show "origin/master:packages/less/lib/less/parser/parser.js" \
| rg -n -C 8 'dimension: function|const value = parserInput\.\$re'
printf '\n-- PR branch --\n'
sed -n '906,924p' packages/less/lib/less/parser/parser.js
echo
echo "== Existing large-decimal parsing path =="
rg -n -C 8 'new\(tree\.Dimension\)|new tree\.Dimension|parseFloat' \
packages/less/lib/less \
-g '*.js'
echo
echo "== Stylus lexer source candidates =="
stylus_tree="$(mktemp)"
gh api "repos/stylus/stylus/git/trees/dev?recursive=1" > "$stylus_tree"
jq -r '.tree[] | select(.type == "blob") | .path' "$stylus_tree" \
| rg '(^|/)(lexer|parser|token|nodes)/.*\.(js|ts)$|^lib/.*\.(js|ts)$' \
| head -80
echo
echo "== Stylus source references to numeric exponent lexing =="
jq -r '.tree[] | select(.type == "blob") | .path' "$stylus_tree" \
| rg '\.(js|ts)$' \
| while IFS= read -r path; do
content="$(gh api "repos/stylus/stylus/contents/$path?ref=dev" --jq .content 2>/dev/null | base64 -d 2>/dev/null || true)"
if printf '%s' "$content" | rg -q 'unit|[eE]\[|[eE].*\\d|exponent'; then
printf '\n---- %s ----\n' "$path"
printf '%s\n' "$content" | rg -n -i -C 4 'unit|exponent|\\d\+|\\d\*|number'
fi
doneLength of output: 29817 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Stylus lexer number-token rule =="
gh api "repos/stylus/stylus/contents/lib/lexer.js?ref=dev" --jq .content \
| base64 -d \
| tee /tmp/stylus-lexer.js \
| rg -n -i -C 12 'unit|number|numeric|float|[eE].*\\d'
echo
echo "== Stylus lexer test coverage for exponent literals =="
tree_json="$(mktemp)"
gh api "repos/stylus/stylus/git/trees/dev?recursive=1" > "$tree_json"
jq -r '.tree[] | select(.type == "blob") | .path' "$tree_json" \
| rg '(^|/)(test|tests)/.*\.(styl|css|js)$' \
| while IFS= read -r path; do
content="$(gh api "repos/stylus/stylus/contents/$path?ref=dev" --jq .content 2>/dev/null | base64 -d 2>/dev/null || true)"
if printf '%s' "$content" | rg -qP '(?<![[:alnum:]_])[0-9]+(?:\.[0-9]*)?[eE][+-]?[0-9]+'; then
printf '\n---- %s ----\n' "$path"
printf '%s\n' "$content" | rg -n -P -C 3 '(?<![[:alnum:]_])[0-9]+(?:\.[0-9]*)?[eE][+-]?[0-9]+'
fi
doneLength of output: 1333 🏁 Script executed: #!/bin/bash
set -euo pipefail
lexer_file="/tmp/stylus-lexer.js"
gh api "repos/stylus/stylus/contents/lib/lexer.js?ref=dev" --jq .content \
| base64 -d > "$lexer_file"
echo "== Numeric lexer rules and unit handling =="
rg -n -i -C 12 'unit|number|numeric|float|[eE].*\\d' "$lexer_file"
echo
echo "== Full candidate methods =="
ast-grep outline "$lexer_file" --items all --type method,function --match 'unit|number'Length of output: 4247
The bot comment is technically correct about this result: .a { width: (1e309px); }
// can emit: InfinitypxHowever, it is not a regression from this PR. I would withdraw the P1 finding. The PR newly makes exponent tokens parse correctly. It does not introduce the underlying non-finite-number serialization behavior. A
That change would affect more than this exponent parser rule. It should not block this focused compatibility fix. This aligns with other preprocessors as follows:
I would merge this PR after normal test validation. I would track non-finite You are interacting with an AI system. |
|
@matthew-dean — happy to answer both directly. Is it correct? The change adds one optional group, How it aligns. I ran the same values through Sass 1.103.1, Stylus 0.64.0 and Less on both branches, each one through a variable so nothing takes the verbatim-declaration fast path:
Two things worth pulling out of that:
The bottom three rows are the guard against the obvious risk: On the P1 finding, I'd still keep it out of this PR. CI is green on all seven test jobs at |
2 participants
What: Teach the
dimensionparser rule about the exponent part of a CSS number, so1e3px,.5e-2pxand2e+2pxcompile to the value they mean.Why: CSS numbers may carry an exponent — css-syntax-3 §4.3.12 defines
<number-token>as[+-]? (\d+ | \d*\.\d+) ([eE][+-]?\d+)?, and a<dimension-token>is that number followed by an ident. The rule atpackages/less/lib/less/parser/parser.jsmatched the number without the exponent:/^([+-]?\d*\.?\d+)(%|[a-z_]+)?/iso in
.5e-2pxit took.5as the number andeas the unit, leaving-2pxto be parsed as a separate term. The two then combine, and the result is neither an error nor the right value:padding: .5e-2px-1.5e0.005px0.005pxmargin: 2e+2px4e200px200pxpadding: 5e-1em4e0.5em0.5emopacity: 1e-10e0.10.1transform: scale(1e-2)scale(-1e)scale(0.01)scale(0.01)flex-basis: 1.5e2%1.5e 2%150%150%min-width: calc(1e3px + 1px)calc(1e 3px + 1px)calc(1000px + 1px)width: (1e3px + 1px)ParseError: Expected ')'1001pxThe last column is
el.style.widthafter assigning each value, read back from Chromium 133 and WebKit 18.2.A bare literal declaration such as
width: 1e3px;happens to survive today because it takes the verbatim fast path for simple values and is never parsed as a dimension. That protection disappears as soon as the value meets any Less feature, which is what makes this easy to miss:Exponents mostly reach Less from generated or minified CSS rather than from hand-written source, so the failure tends to show up as a rule the browser drops, or as a value that is quietly wrong, well away from the code that produced it.
The fix appends an optional
(?:e[+-]?\d+)?to the number group. The exponent needs at least one digit aftere, so a unit that merely begins withestill parses as a unit:1emand2exare unchanged.1e2emnow means 100em, which it did not before. No unit contains a digit, and[a-z_]+never matched one, so nothing that used to parse as a unit stops doing so.Tests: new fixture
packages/test-data/tests-unit/numbers-exponent, covering literal declarations, units that start withe, arithmetic in parens, variables, a mixin argument, a guard and an@mediaquery. Every expected value in the.csswas checked against the two browsers above. Revertingparser.jsand keeping the fixture makesgrunt test:nodeexit 6 withERROR: Expected ')'.pnpm testpasses on this branch:All Passed 211 run, including the headless-Chrome browser suite.pnpm --filter less typecheckis clean.pnpm lintreports one pre-existing parse error inbenchmark/benchmark-runner.jsthat is present onmasterand is unrelated to this change (#4453 appears to cover it).Checklist:
Summary by CodeRabbit
New Features
1e3px.emandex.Bug Fixes
Tests