Fix the two MathClamp calls the prefer-math-clamp fixer mis-ordered - #21725
Open
Jaybhade wants to merge 1 commit into
Open
Fix the two MathClamp calls the prefer-math-clamp fixer mis-ordered#21725Jaybhade wants to merge 1 commit into
Jaybhade wants to merge 1 commit into
Conversation
calixteman
reviewed
Aug 7, 2026
Contributor
|
/botio browsertest |
Collaborator
From: Bot.io (Windows)ReceivedCommand cmd_browsertest from @calixteman received. Current queue size: 0 Live output at: http://54.193.163.58:8877/d0a0c4efe2b3485/output.txt |
Collaborator
From: Bot.io (Linux m4)ReceivedCommand cmd_browsertest from @calixteman received. Current queue size: 0 Live output at: http://54.241.84.105:8877/18d8cf641e6599e/output.txt |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #21725 +/- ##
==========================================
+ Coverage 90.04% 90.07% +0.03%
==========================================
Files 264 264
Lines 66935 67094 +159
==========================================
+ Hits 60270 60438 +168
+ Misses 6665 6656 -9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Snuffleupagus
requested changes
Aug 7, 2026
Collaborator
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/18d8cf641e6599e/output.txt Total script time: 16.56 mins
|
Collaborator
From: Bot.io (Windows)SuccessFull output at http://54.193.163.58:8877/d0a0c4efe2b3485/output.txt Total script time: 23.40 mins
|
The rule rewrites `Math.max(C, Math.min(A, B))` to `MathClamp(A, C, B)`, always taking the inner call's first operand as the value. `Math.min` is commutative, so the value is just as often the second one, and then the fixer swaps it with the upper bound: the result is `Math.min(C, v)`, which no longer applies the lower bound at all. Both call sites the fixer rewrote hit that case, so the max-outer patterns are now reported without a fix. Reporting them still needs its own message. `Math.max(C, Math.min(A, B))` is only `MathClamp(A, C, B)` when `C <= B`, and the rule can't know whether that holds, so telling the reader to "use MathClamp" is wrong advice. The message states the condition instead, and `useClamp` is left to the min-outer patterns, which are exact. In `PSStackBasedInterpreter.build` the outputs were therefore never clamped up to the Range minimum. The interpreter runs whenever `PSStackToTree` can't turn the program into a tree — a stack-shrinking `if`, or a `copy`/`index`/`roll` whose operand isn't a constant — and it then let a Type 4 function return e.g. -1.5 for a component declared as `/Range [0 1]`. In `SplitView.#clampFirstSize` the first pane's size was never clamped up to `#minSize`: dragging the resizer past it wrote a negative `flexGrow`. The existing range-clamping test only checked the upper bound, which is the one the wrong operand order preserves.
Jaybhade
force-pushed
the
fix-math-clamp-operand-order
branch
from
August 7, 2026 18:51
ec1d2d5 to
a163deb
Compare
Author
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
6 participants
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.
The
prefer-math-clampfixer can swap a value with a bound, and it did so at both of the call sites it rewrote in #21030.The rule
MathClamp(v, min, max)isMath.min(Math.max(v, min), max), so the two min-outer patterns are exact — whichever inner operand is the value,Math.min(Math.max(A, B), C)isMathClamp(A, B, C).The max-outer ones aren't. The fixer turns
Math.max(C, Math.min(A, B))intoMathClamp(A, C, B), always assuming the inner call's first operand is the value.Math.minis commutative, so it's just as often the second one, and thenAis really the upper bound: the emitted call isMath.min(Math.max(max, min), v)—Math.min(max, v)— which drops the lower bound entirely. This PR reports those two patterns without offering a fix.PSStackBasedInterpreter.buildOutputs were clamped down to the Range maximum but never up to the Range minimum.
PSStackBasedInterpreteris the fallback used wheneverPSStackToTreecan't build a tree — a stack-shrinkingif, or acopy/index/rollwhose operand isn't a constant — and the Wasm compiler bails on exactly the same programs, so nothing else catches it:/Range{ dup 0 lt { pop } if 2 sub }[0 1]{ dup cvi index 1 sub }[0.25 0.75]The existing "clamps output to declared range" test only exercises the upper bound, which is the one the wrong order preserves — hence the lower-bound case added here. It fails on master with
Expected -0.5 to be close to 0, through the samecompileAndRunhelper that already cross-checks the three implementations.SplitView.#clampFirstSizeSame shape, so the first pane is never clamped up to
#minSize. With#minSize120 andtotal800, dragging the resizer to a requested size of -60 setsflexGrowto -60 instead of 120. The other call in that method changed order too, butMath.max(0, requestedFirst)survives the swap, so that one is equivalent — it's reordered here only to read as a clamp.npx gulp lintis clean andtest/unit/postscript_spec.jspasses (208 specs).