Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use floating-point numbers universally #2892

Closed
4 tasks done
connorskees opened this issue Aug 15, 2020 · 4 comments · Fixed by #3408
Closed
4 tasks done

Use floating-point numbers universally #2892

connorskees opened this issue Aug 15, 2020 · 4 comments · Fixed by #3408
Assignees
Labels
bug Something isn't working

Comments

@connorskees
Copy link
Contributor

connorskees commented Aug 15, 2020


Currently the to and from values for @for are unbounded and not formally defined.

This input is considered valid Sass,

@for $i from -999999999999999999999 to 999999999999999999999999 { ... }

In order to simplify implementations, I would like to propose the bounding of these values.

I believe the easiest solution would be to use a 32 bit signed integer in order to represent these bounds. This will provide a reasonable range of values without requiring much work on the part of an implementation.

In order to better avoid overflows, I would like to propose that the max and min values be actually defined as i32::MAX - 1 and i32::MIN + 1 respectively.

More concretely, the maximum value would be 2,147,483,646 and the minimum value would be -2,147,483,647.

It seems that dart-sass today silently overflows for the to and from values at some point. Bounding these in this way would remove this issue altogether.

@jathak jathak changed the title Explicitly define the bounds for @for Explicitly define how numbers work in Sass Aug 17, 2020
@jathak
Copy link
Member

jathak commented Aug 17, 2020

This ties in with a broader issue that we should better define how numbers work in Sass. I'm retargeting this to be for the broader issue, since whatever we decide there would probably determine the bounds of @for loops.

From the discussion in sass/dart-sass#807, we're currently leaning towards defining all Sass numbers as floating points (in line with JavaScript and how libsass is currently implemented), so we probably won't define @for in terms of integers of a certain size.

connorskees added a commit to connorskees/grass that referenced this issue Aug 20, 2020
1e999 and 1e-999 were able to cause hangs as we use arbitrary precision
numbers rather than floating point. this may change in the future (see
sass/sass#2892)
@Awjin Awjin added the enhancement New feature or request label Sep 23, 2020
@stof
Copy link
Contributor

stof commented Dec 4, 2020

the way numbers are implemented in dart-sass regarding the handling of precision and equality (and other operations) looks like it wants to be using fixed point arithmetic rather than floating point arithmetic though.

@nex3
Copy link
Contributor

nex3 commented Jun 15, 2022

From #3335:

Sass has never clearly specified its internal number format. Ruby Sass inherited Ruby's support for transparently switching to bignums when numbers got extremely large, and Dart Sass did the same thing initially until Dart dropped support for transparent bignums in 2.0.0. LibSass, on the other hand, has only ever used floating point numbers.

Although we could mandate the implementation of transparent bignums, this would add a substantial amount of implementation complexity and performance overhead since we'd have to do bounds-checking for every operation. Instead, I think just standardizing on double-width floating points makes sense, particularly because the compromises inherent in using floating point numbers everywhere are already well-understood by the web community thanks to JS.

Marking this as a bug because the behavior of large numbers is currently inconsistent in a problematic way.

@nex3 nex3 added bug Something isn't working and removed enhancement New feature or request labels Jun 15, 2022
@nex3 nex3 changed the title Explicitly define how numbers work in Sass Use floating-point numbers universally Jun 15, 2022
@nex3 nex3 self-assigned this Aug 31, 2022
@nex3
Copy link
Contributor

nex3 commented Aug 31, 2022

The proposal is now out for review. I'll give it two weeks before we mark it as accepted and move on to implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
5 participants