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

Definition of modulo is broken #1960

Closed
waldemarhorwat opened this issue Apr 21, 2020 · 10 comments
Closed

Definition of modulo is broken #1960

waldemarhorwat opened this issue Apr 21, 2020 · 10 comments
Labels

Comments

@waldemarhorwat
Copy link

waldemarhorwat commented Apr 21, 2020

The change to explicitly note mathematical values broke the definition of modulo in the spec:

The notation “x modulo t y” (y must be finite and nonzero) computes a value k of the same sign as y (or zero) such that abs t(k) <t abs t(y) and x -t k = q ×_t_ y for some integer t q.

where t is either ℝ for mathematical numbers or 𝔽 for IEEE doubles.

This definition of modulo works only when t is ℝ. The formula it uses is not meant to be used with numbers that are not mathematical numbers and breaks badly when t is 𝔽. There are multiple issues with finite precision and rounding, overflow, and underflow.

For example, this definition produces the nonsensical result that the exact IEEE double 200000000000000032𝔽 modulo𝔽 100𝔽 is, among other things, 19𝔽 because 200000000000000032𝔽 -𝔽 19𝔽 = 2000000000000000𝔽 ×𝔽 100𝔽. The correct answer, of course, is 32𝔽.

Similarly, by this definition, 4𝔽 modulo 2𝔽 evaluates to, among other things, 1.74e-75𝔽 because 4𝔽 -𝔽 1.74e-75𝔽 = 2𝔽 ×𝔽 2𝔽.

To fix it, use the formula only for the mathematical definition of modulo. If you need to define modulo on 𝔽, do it by converting to and from mathematical numbers.

@michaelficarra michaelficarra added editor call to be discussed in the next editor call es2020 spec bug labels Apr 21, 2020
@michaelficarra
Copy link
Member

@waldemarhorwat Would you be willing to send a PR with your suggested fix?

@ljharb
Copy link
Member

ljharb commented Apr 21, 2020

cc @littledan @caiolima

@waldemarhorwat
Copy link
Author

@michaelficarra will do. It's a bit tricky because I don't want to define it for things like ±∞ and NaN so I need to go through every point of use to see if those can arise. There are also some issues with the definition of abs in this section.

@syg
Copy link
Contributor

syg commented Apr 22, 2020

Looking forward to seeing the PR for this.

@michaelficarra michaelficarra removed es2020 editor call to be discussed in the next editor call labels Apr 22, 2020
@waldemarhorwat
Copy link
Author

Having seen so many other bugs introduced by #1135, I've now come to the conclusion that, rather than trying to hack modulo, exponentiation, addition, subtraction, counters, etc., the best way to fix them is to revert #1135 and instead explicitly mark the places where Number or Decimal arithmetic should be used. See #1964.

@littledan
Copy link
Member

It seems like the fix for this issue is quite simple, to make modulo only in terms of ℝ and disallow parameterizing it. Are there any downsides to this fix?

@waldemarhorwat
Copy link
Author

@littledan: Yes, that's what I'd recommend doing for modulo (and exponentiation, etc.).

@natedogith1
Copy link

Modulo also seems inconsistent when dealing with negatives. Mathematical Operations specifies that the result has the sign of the divisor, while Number::remainder specifies that the result has the sign of the dividend.

@jmdyck
Copy link
Collaborator

jmdyck commented Sep 9, 2020

So modulo behaves one way and Number::remainder behaves another. That's not really an inconsistency, not in the sense of the spec contradicting itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants