Allow implicit undefined returns when the contextual union type contains it
#57912
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
|
@typescript-bot test top400 @typescript-bot perf test this faster |
|
Hey @jakebailey, 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 |
|
Hey @jakebailey, the results of running the DT tests are ready. There were interesting changes: Branch only errors:Package: nodes7 |
|
@jakebailey Here are the results of running the user tests comparing Everything looks good! |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
I think there's a possible parsing ambiguity in the referenced comment
|
|
@jakebailey Here are the results of running the top 400 repos comparing Everything looks good! |
|
The single reported failure here comes from those lines here. We can quickly verify on this TS playground that the contextual type of this function is |
…turn-contextual-union # Conflicts: # tests/baselines/reference/functionsMissingReturnStatementsAndExpressionsStrictNullChecks.types # tests/baselines/reference/inferenceDoesNotAddUndefinedOrNull.types
andrewbranch
approved these changes
Jun 18, 2024
Comment on lines
32
to
34
| const f21: () => undefined | number = () => { | ||
| ~~~ | ||
| !!! error TS2322: Type '() => void' is not assignable to type '() => number | undefined'. | ||
| !!! error TS2322: Type 'void' is not assignable to type 'number | undefined'. | ||
| // Error, regular void function because contextual type for implicit return isn't just undefined | ||
| // Ok, contextual type for implicit return contains undefined | ||
| } |
There was a problem hiding this comment.
I’m not the right person to review this, because I personally lean strongly toward the belief that distinctions between return, return undefined, and no return belong in a linter and not in TS, which I think is not the consensus view on the team. I can’t tell whether the design meeting notes on this were meant to be descriptive or prescriptive:
But no implicit returns when you have
4 | undefined
If it was intended to be prescriptive, this baseline change violates the intention that was captured in the notes. However, moving in this direction is my preference, so I’m going to approve in hopes that it moves the review along 🤷♂️
There was a problem hiding this comment.
I believe that was descriptive, stating what the current behavior is and that it's inconsistent.
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 #57840
@RyanCavanaugh triaged this as a bug here. I'm pretty sure that @ahejlsberg didn't want this to work this way though (as per the description of #53607 and the comment in tests that I had to touch here).