Use missingType instead of undefinedType for optional methods under exactOptionalPropertyTypes
#52519
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
…er `exactOptionalPropertyTypes`
typescript-bot
added
the
For Uncommitted Bug
Member
|
@typescript-bot test this |
Collaborator
|
Heya @jakebailey, I've started to run the diff-based user code test suite (tsserver) on this PR at f40beb0. You can monitor the build here. Update: The results are in! |
Collaborator
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at f40beb0. You can monitor the build here. Update: The results are in! |
Collaborator
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at f40beb0. You can monitor the build here. |
Collaborator
|
Heya @jakebailey, I've started to run the extended test suite on this PR at f40beb0. You can monitor the build here. |
Collaborator
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at f40beb0. You can monitor the build here. Update: The results are in! |
Collaborator
|
Heya @jakebailey, I've started to run the perf test suite on this PR at f40beb0. You can monitor the build here. Update: The results are in! |
Collaborator
|
Heya @jakebailey, I've started to run the diff-based top-repos suite (tsserver) on this PR at f40beb0. You can monitor the build here. Update: The results are in! |
Collaborator
|
@jakebailey Here are the results of running the user test suite comparing Everything looks good! |
1 similar comment
Collaborator
|
@jakebailey Here are the results of running the user test suite comparing Everything looks good! |
Collaborator
|
Heya @jakebailey, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
Collaborator
|
@jakebailey Here they are:
CompilerComparison Report - main..52519
System
Hosts
Scenarios
TSServerComparison Report - main..52519
System
Hosts
Scenarios
StartupComparison Report - main..52519
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Collaborator
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
Collaborator
|
@jakebailey Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Detailspalantir/blueprint
|
jakebailey
reviewed
Feb 1, 2023
| } | ||
| else { | ||
| return strictNullChecks && symbol.flags & SymbolFlags.Optional ? getOptionalType(type) : type; | ||
| return strictNullChecks && symbol.flags & SymbolFlags.Optional ? getOptionalType(type, /*isProperty*/ true) : type; |
Member
There was a problem hiding this comment.
I'm honestly really suspicious of every other call to this function now; I went and made isProperty required and about half of the callers are actually working with things that are explicitly property types (and so likely need to also pass true).
No tests change when I make everyone pass in this parameter, though, which makes me think that we don't have enough coverage of this particular option.
Contributor
Author
There was a problem hiding this comment.
This option has a default though - did you try passing true to all calls of this function?
Member
There was a problem hiding this comment.
No, I mean I changed it from isProperty = false to isProperty: boolean and then examined all of the places where it error'd.
Not that this is directly required for this PR, just something I noticed.
Contributor
Author
There was a problem hiding this comment.
The missingType thing is only relevant here for with exactOptionalPropertyTypes - and that's not the most popular compiler option. So I definitely wouldn't be surprised if the test suite wouldn't be that comprehensive in this regard.
weswigham
approved these changes
Feb 13, 2023
Member
weswigham
left a comment
weswigham
left a comment
There was a problem hiding this comment.
As jake says, we might need to audit our other usages of getOptionalType to be more intentional about passing the second parameter; but this does look like the correct fix for this case.
jakebailey
approved these changes
Mar 1, 2023
Member
|
I'm going to merge this because it's correct, but, I do think we need to audit the rest and come up with some test cases. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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 #52494