Suggest simplifications for overzealous shifts #59519
Merged
Conversation
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
typescript-bot
added
Author: Team
For Uncommitted Bug
|
Can this me made a regular error temporarily to run user/topXYZ? (I also sort of feel like we should make it a regular error in general...) |
|
Oh boy a failing self-check build :) Always error user/top600 results rolling in here. |
|
Are there other common ways people can end up with two enum members of the same value by accident? |
jakebailey
reviewed
Aug 2, 2024
src/compiler/checker.ts
Outdated
| case SyntaxKind.GreaterThanGreaterThanGreaterThanToken: | ||
| case SyntaxKind.GreaterThanGreaterThanGreaterThanEqualsToken: | ||
| const rhsEval = evaluate(right); | ||
| if (typeof rhsEval.value === "number" && Math.abs(rhsEval.value) >= 32) { |
There was a problem hiding this comment.
Should this convert the value to an integer or something?
There was a problem hiding this comment.
The Math.abs also seems unnecessary - any value below 0 seems wrong and 1 << -1 == 1 << 31, 1 << -2 == 1 << 30 etc. so Math.abs "reverses" it.
Interestingly, 1 << -0.9 == 1 << 0 so I guess you want Math.trunc (or | 0) rather than Math.floor.
There was a problem hiding this comment.
any value below 0 seems wrong
It's a useful shorthand. Similar to negative slice indices in python. Not doing an abs on the evaluated result here, either - just using it to measure the magnitude of the number to issue the error or not.
Should this convert the value to an integer or something?
Eh. Don't really need to. Fractional parts just get dropped anyway. Issuing an error on any fractional literal in a shift position would probably be fine (0.5 is the same as 0 for this operator after all), but I don't think it's particularly related to this range error.
weswigham
force-pushed
the
error-on-erroneous-shifts
branch
from
1ef0e40 to
70afb84
Compare
Other arithmetic errors in calculated enum values, perhaps, but it may also be intentional. Especially with flags enums and bitwise arithmetic - you may end up deriving two sets of flags that just so happen to resolve to the same thing via different means. |
|
@typescript-bot pack this |
|
Hey @gabritto, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@typescript-bot test it |
gabritto
approved these changes
Aug 14, 2024
|
Hey @gabritto, the results of running the DT tests are ready. Everything looks the same! |
|
@gabritto Here are the results of running the user tests with tsc comparing Everything looks good! |
|
@gabritto Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot perf test this faster |
|
@gabritto Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@weswigham there seems to be a slight increase in check times for our codebase but also for vscode, webpack and xstate. Any ideas of why that could be? Do we now issue more suggestions on those repos? Is it just the extra work of evaluating operators? |
|
@gabritto Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
|
Yeah, just a little bit of extra work evaluating the shifts, I imagine. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR adds a suggestion on bitwise shifts where the right side operand is evaluable (which is implicitly non-
bigint) to be outside of the range-31to31to simplify the shift down into that range. Doing so should make it a bit more obvious when you've accidentally exceeded the maximum shift available for a number. 😄This suggestion is escalated to an error in the context of
enummembers, since that's where it's most likely an overshift may result in an unintentionally duplicatedenummember that is otherwise silent.So this:
results in
while this:
results in
This logic is applied for all the shift operators -
<<,>>,>>>,<<=,>>=, and>>>=.