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

Added codefix for numeric literals >= 2 ** 53 #33300

Merged

Conversation

JoshuaKGoldberg
Copy link
Contributor

Number.MAX_SAFE_INTEGER is 2 ** 53 - 1, so anything greater than that is a 'dangerous' integer to store as a traditional number. This adds a codefix to suggest converting them to a bigint literal.

Fixes #29863.

`Number.MAX_SAFE_INTEGER` is `2 ** 53 - 1`, so anything greater than that is a 'dangerous' integer to store as a traditional number. This adds a codefix to suggest converting them to a `bigint` literal.
@fatcerberus
Copy link

Note that mixing regular numbers and bigints in the same operation will throw runtime errors. I wonder if there’s a way to warn the user not to apply this fix blindly.

Also what does this do for scientific notation literals like 1e40?

@AnyhowStep
Copy link
Contributor

AnyhowStep commented Sep 7, 2019

I would rather this thing not warn if I explicitly say I want type literal 1e300.

1e300 can be represented perfectly fine using a double.
Sure, math on it is lossy, but the number itself is not.


One can't always just easily substitute a "large number" for a "bigint". The two types are inherently incompatible.


The same goes for 1.2e+301. It can be represented perfectly fine using a double.
Also, 12e300 is just 1.2e+301 but still no loss in representation.

@AnyhowStep
Copy link
Contributor

AnyhowStep commented Sep 7, 2019

I wouldn't mind if the fix was for lossy number literals.

1.7976931348623e+308 is not a lossy number literal. So, no code fix needed.

However, 1.7976931348624e+308 is lossy (becomes Infinity). So, a code fix is needed here.
Either suggest the user picks a non-lossy number literal or suggest they use bigint.


It makes me a little sad that this will probably remove Infinity from the type system, though.
I really want Infinity, -Infinity, and NaN types.

@JoshuaKGoldberg
Copy link
Contributor Author

Also what does this do for scientific notation literals like 1e40?
1e300

Nothing. It only adds the warning if the number's length is >= 16 characters:

if (node.text.length <= 15 || node.text.indexOf(".") !== -1) {
    return;
}

Per the fourslash test cases in tests/cases/fourslash/codeFixUseBigIntLiteral.ts, there are no fixes applied to 2e52 through 2e54.

...but actually, on second thought, there are scientific notations with >=16 characters that would still be caught:

1e000000000000000010;

Added an explicit check for node.numericLiteralFlags & TokenFlags.Scientific to catch that case more explicitly. Thanks for the prompting! 😄

However, 1.7976931348624e+308 is lossy (becomes Infinity). So, a code fix is needed here.

That's a good point, but I think I'd rather discuss & tackle scientific numbers separately from this. Parsing them could be tricky.

@fatcerberus
Copy link

The main thing that concerns me with this is that someone will write a large numeric literal, e.g. a big prime number in a hashing algorithm, and get the codefix. You click the lightbulb, it says you should convert this to bigint, so you do, and all is right the world... except it isn’t because it will now throw a TypeError at runtime if you try to use it in a calculation with the “regular” numbers.

I guess what I’m getting at here is, how often will it be safe in practice to simply change a large number to bigint without also needing to refactor all the surrounding code as well?

@JoshuaKGoldberg
Copy link
Contributor Author

and all is right the world

Well, no, not likely. TypeScript already provides type safety around number/bigint comparisons:

let regular = 3; // type 'number'
let deluxe = 3n; // type 'bigint'

// Operator '+' cannot be applied to types 'number' and 'bigint'.
regular + deluxe;

how often will it be safe in practice to simply change a large number to bigint without also needing to refactor all the surrounding code as well?

From another perspective, it's not generally safe in practice to use these numbers to begin with. Example: #33298.

My understanding of the codefixes is that they're of a lesser 'strictness' than the regular compiler itself - that they're useful utilities to make editing the code easier. That's why this is being surfaced as a suggestion + codefix instead of error, to help with that refactoring.

@fatcerberus
Copy link

True, but if at least 50% of the time I’m going to have to refactor the surrounding code because adding a single character introduced 5 new type errors then the automated fix isn’t actually saving me any time.

I would be more at ease if this was made to simply be a suggestion (“consider using bigint here”) rather than a full-fledged codefix, though I don’t know if that’s possible? It’s definitely worth calling out that the huge number literal is unsafe, I’m just not really comfortable with the automated fix is all.

@AnyhowStep
Copy link
Contributor

I think I need better reading comprehension. I thought this was still trying to make lossy number literals an error.

It's just recommending a code fix that one is free to ignore.

I share @fatcerberus ' thoughts on the matter.

Sorry for the brain fart =(

src/compiler/checker.ts Show resolved Hide resolved
@orta orta merged commit e8fc62e into microsoft:master Sep 11, 2019
@JoshuaKGoldberg JoshuaKGoldberg deleted the too-large-integer-bigint-codefix branch September 11, 2019 20:59
@DanielRosenwasser
Copy link
Member

Wait, this still has the same problem I described from last time, which is that the integer is still "safe" if it can be stored in the mantissa, which could make this annoying.

@JoshuaKGoldberg
Copy link
Contributor Author

@DanielRosenwasser I'm a little confused as to the intended tone of these suggestions then (which I think is also what fatcerberus and AnyhowStep are getting at). There are a lot of suggestions already that target seemingly unnecessary fixes - where is the line between adding a 💡 or not?

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Sep 11, 2019

We don't have a great guiding philosophy about it right now, but my comment was less about whether or not we should have the info diagnostic, and more about when it's appropriate to issue this error (and I think it's arguable which is why I thought this was better off as a lint rule). There's nothing wrong with the number 1000000000000000000000000000000 because it can be accurately represented.

Maybe it's fine for now though since these messages aren't that invasive?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Update Docs on Next Release Indicates that this PR affects docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue an error when numeric literals can incur precision loss
5 participants