- Notifications
You must be signed in to change notification settings - Fork 13.2k
consistently check expressions with >, >=, <, <= for unconstrained types in strictNullChecks#59352
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
base:main
Are you sure you want to change the base?
consistently check expressions with >, >=, <, <= for unconstrained types in strictNullChecks #59352
Uh oh!
There was an error while loading. Please reload this page.
Conversation
iisaduan commented Jul 19, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
iisaduan commented Jul 19, 2024
@typescript-bot test it |
typescript-bot commented Jul 19, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
typescript-bot commented Jul 19, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Hey @iisaduan, 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 commented Jul 19, 2024
Hey @iisaduan, the results of running the DT tests are ready. Everything looks the same! |
typescript-bot commented Jul 19, 2024
@iisaduan Here are the results of running the user tests with tsc comparing Everything looks good! |
typescript-bot commented Jul 19, 2024
@iisaduan Here they are:tscComparison Report - baseline..pr
System info unknown Hosts
Scenarios
Developer Information: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
typescript-bot commented Jul 19, 2024
@iisaduan Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
tests/baselines/reference/unconstrainedTypeComparison.errors.txt Outdated Show resolvedHide resolved
Uh oh!
There was an error while loading. Please reload this page.
src/compiler/checker.ts Outdated
| // use to determine if a parameter may be undefined or null (or is unknown/unconstrained) | ||
| function getUnknownIfMaybeUnknown(type: Type){ | ||
| return (strictNullChecks && type.flags & TypeFlags.Instantiable) ? getBaseConstraintOfType(type) || unknownType : type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming from #59059 that making this change to getTypeFacts breaks a lot of existing code, is that right?
Also, does making this change to checkNonNullType instead also break a lot of code?
iisaduanJul 19, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I tried that and putting this change directly into checkNonNullType gets a very similar result to #59059 in terms of breaking object index checking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of the cases that break relate to this type of index access #59059 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if this it's noticeably different in semantics, but istead of doing this, you might want to try to modify getBaseTypeOfLiteralTypeForComparison and rename it getBaseTypeForComparison. Then pass the result of that into checkNonNullType.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also consider renaming this to getBaseConstraintOrUnknown, use ?? instead of ||, and just get rid of the strictNullChecks check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also - any clue if this works on unions of unconstrained type parameters? Can you add a test?
functionf<T,U>(x: T|U,y: T|U){returnx<y;}DanielRosenwasser commented Jul 20, 2024
Checking this @typescript-bot perf test this |
typescript-bot commented Jul 20, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
src/compiler/checker.ts Outdated
| // use to determine if a parameter may be undefined or null (or is unknown/unconstrained) | ||
| function getUnknownIfMaybeUnknown(type: Type){ | ||
| return (strictNullChecks && type.flags & TypeFlags.Instantiable) ? getBaseConstraintOfType(type) || unknownType : type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if this it's noticeably different in semantics, but istead of doing this, you might want to try to modify getBaseTypeOfLiteralTypeForComparison and rename it getBaseTypeForComparison. Then pass the result of that into checkNonNullType.
src/compiler/checker.ts Outdated
| // use to determine if a parameter may be undefined or null (or is unknown/unconstrained) | ||
| function getUnknownIfMaybeUnknown(type: Type){ | ||
| return (strictNullChecks && type.flags & TypeFlags.Instantiable) ? getBaseConstraintOfType(type) || unknownType : type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also consider renaming this to getBaseConstraintOrUnknown, use ?? instead of ||, and just get rid of the strictNullChecks check.
src/compiler/checker.ts Outdated
| // use to determine if a parameter may be undefined or null (or is unknown/unconstrained) | ||
| function getUnknownIfMaybeUnknown(type: Type){ | ||
| return (strictNullChecks && type.flags & TypeFlags.Instantiable) ? getBaseConstraintOfType(type) || unknownType : type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also - any clue if this works on unions of unconstrained type parameters? Can you add a test?
functionf<T,U>(x: T|U,y: T|U){returnx<y;}typescript-bot commented Jul 20, 2024
@DanielRosenwasser Here they are:tscComparison Report - baseline..pr
System info unknown Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown Hosts
Scenarios
Developer Information: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
iisaduan commented Jul 22, 2024
@DanielRosenwasser Thanks for the suggestions! Good catch--it does work for unions but not intersections, so I added those cases as well. |
gabritto commented Jul 23, 2024
@typescript-bot test it |
typescript-bot commented Jul 23, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
typescript-bot commented Jul 23, 2024
@gabritto Here are the results of running the user tests with tsc comparing Everything looks good! |
typescript-bot commented Jul 23, 2024
Hey @gabritto, the results of running the DT tests are ready. Everything looks the same! |
typescript-bot commented Jul 23, 2024
@gabritto Here they are:tscComparison Report - baseline..pr
System info unknown Hosts
Scenarios
Developer Information: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
typescript-bot commented Jul 23, 2024
@gabritto Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
ahejlsberg commented Jul 26, 2024
I have just put up #59437 which I believe is the right fix for #50603 plus the inconsistencies related to unconstrained type parameters vs. type parameters constrained to |
fixes#50603, which bisected to #49119
Previously: unconstrained type parameters were inconsistently checked in comparisons, as shown below (5.5.3 playground):
Now with this PR:
This PR does not affect the checking of unconstrained types in other locations.