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
Rule proposal: Disallow number literals that lose precision #11279
Comments
Open question: If someone writes (If we choose to allow the exact literal, it could be kind of annoying to figure out what the exact mathematical value actually is without How about Maybe the general rule here is to report an error if a significant digit can be replaced with a zero to create the same |
If @not-an-aardvark isn't already working on this, I can take a stab at it! |
@jmoore914 Please go right ahead! We look forward to seeing what you come up with. Please reach out here or in our Gitter chat if you need help. Thanks! |
Thanks for working on this. FWIW I ended up creating a small parser for this rule to try to handle weird cases for counting significant figures (e.g. disambiguating octal integers). Feel free to use it (or not) if you think it would be useful. |
Thanks @not-an-aardvark, I'll take a look! This issue got more interesting than I thought it would be. You actually lose one digit of precision if you remove any non-significant zeros. And the same behavior happens if the number contains a decimal point. So, for instance // 16 significant digits
const num1 = 8888888888888884
const num2 = 8888888888888883
num===num2
// output: false
// 16 significant digits and 1 zero
const num3 = 88888888888888840
const num4 = 88888888888888830
num3===num4
// output: true
// 16 significant digits and a decimal point
const num5 = 8.888888888888884
const num6 = 8.888888888888883
num1===num2
// output: true
// 15 significant digits and 11 zeros
const num7 = 88888888888888400000000000
const num8 = 88888888888888300000000000
num7===num8
// output: false
// 16 significant digits and 1 decimal point which is removed in expansion
const num9 = 888888888888888.3e1
const num10 = 888888888888888.2e1
num9===num10
// output: false
// 16 significant digits and 1 zero which is removed in expansion
const num11 = 88888888888888830e-1
const num12 = 88888888888888820e-1
num11===num12
// output: false So what that seems to indicate is that if there is a non-significant zero or decimal point in the numeric expansion, then you only have precision up to 15 digits. |
And the behavior I find even odder is that if any of the above conditions applies, you have a lower max safe integer. If you have precision up to 16 sig figs, then the max safe integer is 9007199254740991 but if you only have precision up to 15, the max safe integer is somewhere between 700000000000000 and 800000000000000. |
The internal representation of a I think significant digits are only relevant here in the sense that they can help determine user intent based on how a user chooses to represent their number. If someone writes |
Basically, I think the behavior of the rule should be equivalent to the following pseudocode:
This seems like it would have the desired effect from #11279 (comment) of allowing things like I'm not sure what the best way to actually implement this is, given that it involves operating on mathematical values that might Other implementation notes:
|
OK, here's what I came up with: https://github.com/jmoore914/eslint/tree/no-loss-of-precision It's working perfectly for decimal values and values with scientific notation. Not sure how to approach the other cases. I'm mainly unsure how to determine the level of precision requested but I'll give it some more thought. |
Thanks. From taking a look at your implementation it seems like it works by getting rid of any exponential part, adding a small power of ten based on how many significant figures are used, and seeing if the resulting value changes. This is an interesting approach, but I'm not sure it's going to work well. I don't think we want to report an error for literals like Using What are your thoughts on the implementation strategy from #11279 (comment)? To me it seems like that would be less likely to run into weird edge cases, and it might end up being a lot simpler anyway when using
At that point the only difficulty for base-ten numbers would be to check whether two different (possibly-scientific-notation) number strings represent the same mathematical value, which seems like a solvable problem by normalizing them both to scientific notation manually. |
Thanks for taking a look. If I remove the adding/subtracting the small power of ten, if I understand correctly, it would be implementing the strategy in #11279. So if we look at the function: function checkRawMatchesValue(node) {
const requestedPrecision = getRequestedLevelOfPrecision(node);
// getRequestedLevelOfPrecision("90071992540993")=16
return stripToSignificantDigits(node.raw) === stripToSignificantDigits(node.value.toPrecision(requestedPrecision));
// stripToSignificantDigits("90071992540993") = "90071992540993"
// stripToSignificantDigits(90071992540993.toPrecision(16))="90071992540992"
// "90071992540993" !== "90071992540992" so this would report an error
} If I remove the +/- check then the only deviation from that strategy is the extra step of stripping the numbers down to sig figs which is how I'm normalizing the numbers and avoiding the need to manually convert to scientific notation. |
Got it, that makes sense. In that case I think the overall approach seems fine if we remove the +/- check. The fact that Some other potentially useful test cases/edge cases to consider:
For non-base-10 literals: function toPrecisionWithBase(number, significantDigits, base) {
const lastSignificantPlaceValue = base ** (Math.floor(Math.log(number) / Math.log(base)) - significantDigits + 1);
const roundedValue = Math.round(number / lastSignificantPlaceValue) * lastSignificantPlaceValue;
// Note: Unlike Number#toPrecision, this does not add any trailing zeros after decimal points.
return roundedValue.toString(base);
} Aside from that I think the procedure for non-base-10 numbers should work the same as the procedure for base 10 numbers, except a bit simpler because everything is an integer and there's no scientific notation. |
Thanks, those are some interesting edge cases! I ended up implementing a function to convert to proper scientific notation since I also felt weird about numbers with different orders of magnitude being converted to the same number. Aside from that, wouldn't it be even simpler than that? Since there is no way to specify the level of sigificance in non-base 10, I would assume that the user wouls want the exact integer. For instance, |
Better example:
|
That's a good point. After thinking about it some more, though, does that issue ever change the behavior in practice? We only care about whether zeros are significant because there are cases where the floating-point approximation of the base b literal "(something) followed by n zeros" is different from the mathematical value of (something) * bn. In these cases, rounding the floating-point value to a certain number of significant figures might result in a value with fewer than n trailing zeros, in which case we might need to report the literal if the trailing zeros were considered significant. But I think that case is only possible for base 10 literals, because all the other available bases (2, 8, and 16) are powers of two. So adding a trailing zero just has the effect of multiplying by a power of two, which won't create any additional floating-point errors (it just increments the exponent without changing the mantissa). As a result, a binary/octal/hex literal with n trailing zeros will have a floating-point value that is also a multiple of bn. So won't doesn't matter whether the trailing zeros are considered significant, because they'll be correct anyway. tl;dr: Yes, I think we can just assume users want the exact integer for non-base-10 literals, although the behavior of the rule won't change regardless of whether we make that assumption. |
Seems like #12889 would also be useful and is related to this rule (in that they both enforce safer usages of |
IMO, this rule already contains most of the useful functionality from #12889, so that rule wouldn't be necessary when this one is added. |
According to the Contribution Guidelines a rule should be simple and easy to understand. Checking which "Number" loses precision and which doesn't makes things complicated. Furthermore it does not really solve the problem with wrong usage of large (e.g. 64bit) integers in JavaScript. |
* Created rule and test files * Working rules and tests * Working for decimals and scientific notation * Check all digits match instead of just most precise * Created rule and test files * Working rules and tests * Working for decimals and scientific notation * Check all digits match instead of just most precise * Expanded rule to non-base ten numbers * Added docs and updated config files * Update docs/rules/no-loss-of-precision.md Co-Authored-By: Teddy Katz <teddy.katz@gmail.com> * Update lib/rules/no-loss-of-precision.js Co-Authored-By: Teddy Katz <teddy.katz@gmail.com> * Removed rule from recommended * Renamed functions; fixed function description * Update lib/rules/no-loss-of-precision.js Co-Authored-By: Teddy Katz <teddy.katz@gmail.com> * Removed always-true conditional * Removed rule from recommended * Fixing octal cases * Working with octals * Changed isNotBaseTen to isBaseTen * Simplify isBaseTen test * Added regression tests * Additional regression test Co-authored-by: Teddy Katz <teddy.katz@gmail.com>
Please describe what the rule should do:
This rule would report number literals that immediately lose precision at runtime when converted to a JS
Number
(due to 64-bit floating-point rounding).What category of rule is this? (place an "X" next to just one item)
[x] Warns about a potential error (problem)
Provide 2-3 code examples that this rule will warn about:
largeConstant1
andlargeConstant2
will actually end up with aNumber
of1123456789012345676800
, because the mathematical value 1234567890123456789 is not precisely representable as a JSNumber
. The use of the digits89
at the end of the literal are an attempt by the programmer to provide extra precision, but this extra precision doesn't appear at runtime, so the code probably contains a bug.Similarly,
longDecimal1
will end up with aNumber
value of 1.2345678901234567, effectively dropping the digits89
. The use of the trailing89
is potentially confusing and probably a bug.The rule uses significant digits in order to determine the "precision" of a literal. As a result, there are a few cases where the rule would consider a literal to be valid despite the fact that the literal is not precisely representable as a
Number
. For example, the following examples would be considered valid:Although the literal
0.11
gets converted to a mathematical value of 0.11000000000000000056..., the literal0.11
is a correct approximation of that mathematical value to two significant digits. As a result, the use of the literal0.11
is probably not a bug.Similarly, although
largeConstant3
andlargeConstant4
get converted to a mathematical value of 99999999999999991611392,1e23
and100000000000000000000000
are both correct approximations of that mathematical value to one significant digit. As a result, the usage of these literals is probably not a bug.The choice to not report an error for these cases is the main difference between this proposal and #8447.
Note that the rule would report an error for a literal like
100000000000000000000000.0
(with a decimal point and a zero at the end), because that literal contains 25 significant digits rather than one significant digit, and the literal is not a correct approximation of that mathematical value to 25 significant digits.The above rules also apply to other representations of number literals (binary, hex, and octal), although those literal representations only support integers. However, the rule would not report an error for
BigInt
literals, becauseBigInt
literals don't have precision loss.I considered always reporting an error for imprecise binary/hex/octal literals (without looking at significant digits) because large binary/hex/octal literals are more often used to represent exact values (e.g. memory addresses) rather than approximate measurements. However, I decided to keep the behavior consistent to minimize false positives (e.g. to allow it to be added to
eslint:recommended
in the future).Why should this rule be included in ESLint (instead of a plugin)?
This rule would identify bugs resulting from a potentially-confusing aspect of the language, with very few false positives.
For an example of a bug like this, see tc39/proposal-bigint#170. (The code in that example uses the experimental
BigInt
proposal, but fundamentally the footgun exists independent of theBigInt
proposal.)Are you willing to submit a pull request to implement this rule?
Yes
The text was updated successfully, but these errors were encountered: