Improve comparison operators type checking to disallow unions containing numbers as an operand #52048
Conversation
This file contains 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
…ing numbers as an operand
typescript-bot
added
the
For Uncommitted Bug
Andarist
commented
Dec 29, 2022
Comment on lines
+35646
to
+35647
| return leftAssignableToNumber && rightAssignableToNumber || | ||
| !leftAssignableToNumber && !rightAssignableToNumber && areTypesComparable(left, right); |
There was a problem hiding this comment.
essentially, when one of the operands is assignable to a number (or big int) then we require the other one to be assignable to it too. areTypesComparable kicks in only when both sides are not assignable to numberOrBigIntType
Andarist
commented
Dec 29, 2022
| @@ -6,9 +6,41 @@ tests/cases/conformance/expressions/binaryOperators/comparisonOperator/compariso | |||
| tests/cases/conformance/expressions/binaryOperators/comparisonOperator/comparisonOperatorWithNoRelationshipTypeParameter.ts(17,14): error TS2367: This comparison appears to be unintentional because the types 'T' and 'U' have no overlap. | |||
| tests/cases/conformance/expressions/binaryOperators/comparisonOperator/comparisonOperatorWithNoRelationshipTypeParameter.ts(18,14): error TS2367: This comparison appears to be unintentional because the types 'T' and 'U' have no overlap. | |||
| tests/cases/conformance/expressions/binaryOperators/comparisonOperator/comparisonOperatorWithNoRelationshipTypeParameter.ts(19,14): error TS2367: This comparison appears to be unintentional because the types 'T' and 'U' have no overlap. | |||
| tests/cases/conformance/expressions/binaryOperators/comparisonOperator/comparisonOperatorWithNoRelationshipTypeParameter.ts(23,16): error TS2365: Operator '<' cannot be applied to types 'T' and 'number'. | |||
There was a problem hiding this comment.
To bring back the old behavior we should probably also allow unconstrained generics - but I wasn't sure what's the best check for them. Should I just check someType with a predicate testing if the type is a TypeParameter without a constraint?
Andarist
commented
Dec 29, 2022
| @@ -0,0 +1,113 @@ | |||
| tests/cases/conformance/controlFlow/controlFlowWhileStatement.ts(81,12): error TS2365: Operator '>' cannot be applied to types 'string | number' and 'number'. | |||
There was a problem hiding this comment.
Perhaps this indicated that the improvement requires some more work - or maybe that it's not a good improvement after all, as maybe "swallowing" non-numbers in unions is desired? Kinda easy to miss a mistake because of this behavior but some existing code can depend on it
Andarist
commented
Dec 29, 2022
Comment on lines
+84
to
+87
| x = ""; | ||
| while (x > 1) { | ||
| ~~~~~ | ||
| !!! error TS2365: Operator '>' cannot be applied to types 'string | number' and 'number'. |
There was a problem hiding this comment.
I feel like perhaps the error here is actually desirable - since the initial CFA type is string and for such this while loop is ignored at runtime (and could be ignored by the CFA analysis). We never enter the body of this loop
|
@typescript-bot test this |
|
Heya @RyanCavanaugh, I've started to run the perf test suite on this PR at 7996bcb. You can monitor the build here. Update: The results are in! |
|
Heya @RyanCavanaugh, I've started to run the diff-based top-repos suite (tsserver) on this PR at 7996bcb. You can monitor the build here. Update: The results are in! |
|
Heya @RyanCavanaugh, I've started to run the parallelized Definitely Typed test suite on this PR at 7996bcb. You can monitor the build here. |
|
Heya @RyanCavanaugh, I've started to run the extended test suite on this PR at 7996bcb. You can monitor the build here. |
|
Heya @RyanCavanaugh, I've started to run the diff-based top-repos suite on this PR at 7996bcb. You can monitor the build here. Update: The results are in! |
|
Heya @RyanCavanaugh, I've started to run the diff-based user code test suite on this PR at 7996bcb. You can monitor the build here. Update: The results are in! |
|
Heya @RyanCavanaugh, I've started to run the diff-based user code test suite (tsserver) on this PR at 7996bcb. You can monitor the build here. Update: The results are in! |
|
@RyanCavanaugh Here are the results of running the user test suite comparing Everything looks good! |
|
@RyanCavanaugh Here are the results of running the user test suite comparing Something interesting changed - please have a look. Details
|
|
Heya @RyanCavanaugh, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
|
Breaks from an internal codebase (an ancient snapshot FWIW): |
|
Most of the breaks here look very desirable to me. The Number vs number one should probably be corrected though. |
|
@RyanCavanaugh Here they are:
CompilerComparison Report - main..52048
System
Hosts
Scenarios
TSServerComparison Report - main..52048
System
Hosts
Scenarios
StartupComparison Report - main..52048
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@RyanCavanaugh Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Detailsdirectus/directus
|
|
@RyanCavanaugh Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
|
The TypeScript team hasn't accepted the linked issue #52036. If you can get it accepted, this PR will have a better chance of being reviewed. |
|
@RyanCavanaugh the union cases that you mentioned are conceptually the same as the issue that I'm trying to fix here, so I guess that if we decide to move this PR forward (in some shape) then that would be a breaking change for those situations. I don't see any additional heuristics that would keep those cases working while erroring on the case linked to this PR. The Cases discovered by the top are also related to unions and one is related to All in all, it kinda boils down to making a decision about the desired behavior here and only your team can do that. I'm more than happy to work on an adjusted algorithm for this but I'd have to know what that is (if it would be any different from what I have in this PR). It's not that I'm not willing to experiment and propose algorithm adjustments - it's just that the whole thing is very subtle and you have the best idea of what kind of disputable you might want to allow. So at the very least, I could use a little bit of a push from your team's side to explore the improved algorithm. Let me know what you think, if you want to act on this anyhow or if you prefer to close the PR. From my PoV, most of the new errors are desired - those cases won't throw, so they are "type-safe" but they rely on some implicit coercions~ that are often super easy to miss. This change would require adding some additional |
|
I see in the latest design notes that:
What are the next steps here then? Which cases should I fix and which cases do we want to keep as "broken"? cc @RyanCavanaugh @DanielRosenwasser |
|
There might be some more corner cases to explore here, but I think this is effectively what we want to "try" in the 5.0 beta to gauge reaction and figure out what to tweak (or if it's just going to have to roll back entirely). I'm leaning toward merge as-is. @DanielRosenwasser good to go? |
jakebailey
approved these changes
Jan 20, 2023
|
While working on this, I initially made a different change but then discovered that TS currently allows this: class A {
foo() {}
}
class B extends A {
bar() {}
}
declare const a: A
declare const b: B
a < b // ok?So I went with a different route, trying to keep this behavior as I thought that it was desired. But given the discussion and an attempt to change the base behavior - should this case (and similar ones) be considered an error? How do we define what TS treats as a valid comparison? From my PoV, allowing the comparison to be OK based on both types being comparable is not that useful and is a footgun. I'm specifically calling out this part of the check: !leftAssignableToNumber && !rightAssignableToNumber && areTypesComparable(left, right); |
|
@Andarist |
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.
fixes #52036
at this moment this is just a spike to check if this is even a desired change