Replace missingType with undefinedType in the IndexedAccessTypeNode's type
#52016
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
typescript-bot
added
the
For Milestone Bug
sandersn
requested changes
Dec 30, 2022
| @@ -16856,6 +16856,8 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| const propType = getTypeOfSymbol(prop); | |||
| return accessExpression && getAssignmentTargetKind(accessExpression) !== AssignmentKind.Definite ? | |||
| getFlowTypeOfReference(accessExpression, propType) : | |||
| accessNode && isIndexedAccessTypeNode(accessNode) ? | |||
| mapType(propType, type => type === missingType ? undefinedType : type) : | |||
There was a problem hiding this comment.
this is a good fix for the problem encountered here, and probably for most cases when referencing a single property, but I think it might have undesired effects for cases where the indexType is a union of property names, especially something like keyof T.
Am I wrong? Does it still make sense to turn missing into undefined there too? @weswigham I'd like your advice on this.
There was a problem hiding this comment.
But even in such cases - we just end up distributing~ the indexType over objectType. Essentially it's the same as T['a'] | T['b'] | .... So the open question is - is there any case for which indexed access type should "transfer" the optionality~ into the target type? The only operation that should do it, that I know of, involved homomorphic mapped types - and in this case, indexed access types are not involved directly.
sandersn
reviewed
Feb 2, 2023
src/compiler/checker.ts
Outdated
Comment on lines
16857
to
16858
| return accessExpression && getAssignmentTargetKind(accessExpression) !== AssignmentKind.Definite ? | ||
| getFlowTypeOfReference(accessExpression, propType) : |
There was a problem hiding this comment.
now that this has multiple entries, can you format it cond-style? Like so:
return cond1 ? cons2
: cond2 ? cons2
: default;There was a problem hiding this comment.
done #prettierwhereareyou 😉
weswigham
approved these changes
Feb 3, 2023
There was a problem hiding this comment.
This looks pretty reasonable to me, but @RyanCavanaugh should probably sign off, since he's probably got the most complete mental model for how missingType is supposed to work.
|
It looks like you've sent a pull request to update our 'lib' files. These files aren't meant to be edited by hand, as they consist of last-known good states of the compiler and are generated from 'src/lib' or possibly our lib generator. Unless this is necessary, consider closing the pull request and sending a separate PR to update 'src/lib' or https://github.com/microsoft/TypeScript-DOM-lib-generator |
|
Sorry, the bot is off the rails. We've asked the team owning it to investigate. |
|
@typescript-bot test this |
|
Heya @RyanCavanaugh, I've started to run the perf test suite on this PR at ea832bf. You can monitor the build here. Update: The results are in! |
|
Heya @RyanCavanaugh, I've started to run the diff-based top-repos suite on this PR at ea832bf. 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 ea832bf. 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 ea832bf. You can monitor the build here. |
|
Heya @RyanCavanaugh, I've started to run the diff-based top-repos suite (tsserver) on this PR at ea832bf. You can monitor the build here. Update: The results are in! |
|
Heya @RyanCavanaugh, I've started to run the extended test suite on this PR at ea832bf. You can monitor the build here. |
|
Heya @RyanCavanaugh, I've started to run the diff-based user code test suite (tsserver) on this PR at ea832bf. You can monitor the build here. Update: The results are in! |
RyanCavanaugh
approved these changes
Feb 3, 2023
|
@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 Everything looks good! |
|
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. |
|
@RyanCavanaugh Here they are:
CompilerComparison Report - main..52016
System
Hosts
Scenarios
TSServerComparison Report - main..52016
System
Hosts
Scenarios
StartupComparison Report - main..52016
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@RyanCavanaugh Here are the results of running the top-repos suite comparing Everything looks good! |
|
@RyanCavanaugh Here are the results of running the top-repos suite comparing Everything looks good! |
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 #51953
cc @sandersn