unconstrained type parameters are consistently checked for {} | null | undefined in strictNullChecks
#59059
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
Member
Author
|
@typescript-bot pack this |
Collaborator
iisaduan
commented
Jun 27, 2024
| > : ^^^^^^^ | ||
| >value : (T & {}) | null | ||
| > : ^^^^^^^^^^^^^^^ | ||
| >value : (T & ({} | null)) | null |
Member
Author
There was a problem hiding this comment.
this seems to be the only weirder/"I may need to fix that" break that happens in the test suite. This test pre-dates #49119, which adds "An unconstrained type parameter is no longer assignable to {}" and was the cause of the original regression that this PR fixes, so the code in this test doesn't do what the original author probably intended.
Collaborator
|
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 |
Member
Author
|
@typescript-bot test it |
Collaborator
Collaborator
|
Hey @iisaduan, the results of running the DT tests are ready. Everything looks the same! |
Collaborator
|
@iisaduan Here are the results of running the user tests with tsc comparing Something interesting changed - please have a look. Details
|
Collaborator
|
@iisaduan Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Collaborator
|
@iisaduan Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
Collaborator
…constrainedTypeParameter
Member
Author
|
With this PR, this lint check is necessary, as it was what was causing the self-check to fail earlier https://github.com/microsoft/TypeScript/actions/runs/9751008069/job/26911678446 |
Member
|
So one of the things that no longer works is the following: function f<T, K extends keyof T>(obj: T, key: K) {
return obj[key];
// ~~~
// error! 'obj' is possibly 'null' or 'undefined'.
}One of the reasons we've typically said that this is okay is because f(undefined, "hello");
// ~~~~~~~
// error! Argument of type 'string' is not assignable to parameter of type 'never'.I know @ahejlsberg was considering changing this for other reasons around the time that #56652 was out. Maybe he has some thoughts here or can give some pointers. I think we can fix #50603 well enough without disrupting the behaviors in place, but I'd have to take a deeper look to help out there. |
Member
Author
Thanks for the insight! I think so too, I will fix that. Those test cases looked alright at first, but after you brought the above up, I was reminded that the following is similarish and fine function f<T>(obj: T) {
for (const key in obj) {
obj[key]; // no errors
}
} |
Member
Author
|
@typescript-bot test it |
Collaborator
Collaborator
|
Hey @iisaduan, the results of running the DT tests are ready. Everything looks the same! |
Collaborator
|
@iisaduan Here are the results of running the user tests with tsc comparing Something interesting changed - please have a look. Details
|
Collaborator
|
@iisaduan Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Collaborator
|
@iisaduan Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
Member
Author
|
@DanielRosenwasser interestingly, this is also a current issue (5.5.3, playground) when function f<T extends {} | null | undefined , K extends keyof T>(obj: T, key: K) {
return obj[key];
~~~
!!! error TS18049: 'obj' is possibly 'null' or 'undefined'.
~~~~~~~~
!!! error TS2536: Type 'K' cannot be used to index type '{}'.
} |
{} | null | undefined in strictNullChecks{} | null | undefined in strictNullChecks
Member
Author
|
@typescript-bot test top400 |
Collaborator
Collaborator
|
@iisaduan Here are the results of running the user tests with tsc comparing Something interesting changed - please have a look. Details
|
Member
|
Looks like #50603 is being addressed in #59352 (and I'll comment on that separately there). Meanwhile, in the example function f<T, K extends keyof T>(obj: T, key: K) {
return obj[key]; // Ok
}
function g<T extends {} | null | undefined , K extends keyof T>(obj: T, key: K) {
return obj[key]; // Error, 'obj' is possibly 'null' or 'undefined'.
}the second error really shouldn't be there. As @DanielRosenwasser points out above, for any |
This was referenced Jul 24, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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 #50603
This behavior is due to a case that was unchecked in #49119. (Relevant summary of 49119:
{}";unknown, which is equivalent toextends {} | null | undefined;The behavior was previously inconsistent, as shown in the cases below (5.5.3 playground):
Implementation:
This PR
TypeFactsreturns for unconstrained parameters. Instead of finding that unconstrained type parameters haveUnknownFacts, we allow unconstrained type parameters to have all facts. SinceUnknownFacts = AllFacts & ~UndefinedOrNull, this change means that unconstrained type parameters now have the possibility to be treated asundefined | nulleven when not explicitly unioned.checkNonNullExpressionConsiderations left
-- in
function f<T, U extends T>,Uis not an unconstrained parameter-- (CFA needed?) to check previously ok cases (see
tests/baselines/reference/isomorphicMappedTypeInference.errors.txt)--
const b = "foo" in obj[key];is no longer okay when we have parameter(obj: T)(seekeyofAndIndexedAccessErrors.txt)