Uh oh!
There was an error while loading. Please reload this page.
Allow implicit return with explicit undefined return type - #53092
Conversation
TypeScript Bot (typescript-bot)
commented
Mar 4, 2023
The TypeScript team hasn't accepted the linked issue #36288. If you can get it accepted, this PR will have a better chance of being reviewed. |
| function f9(): void { | ||
| function f9(): any { | ||
| // Fine since we are typed any and return undefined |
There was a problem hiding this comment.
(To match the comments, as otherwise these two are exactly same with f5 and f6.)
Jake Bailey (jakebailey)
commented
Mar 4, 2023
TypeScript Bot (@typescript-bot) test this |
Heya Jake Bailey (@jakebailey), I've started to run the diff-based top-repos suite on this PR at 3bee8eb. You can monitor the build here. Update: The results are in! |
Heya Jake Bailey (@jakebailey), I've started to run the tarball bundle task on this PR at 3bee8eb. You can monitor the build here. |
Heya Jake Bailey (@jakebailey), I've started to run the parallelized Definitely Typed test suite on this PR at 3bee8eb. You can monitor the build here. Update: The results are in! |
Heya Jake Bailey (@jakebailey), I've started to run the extended test suite on this PR at 3bee8eb. You can monitor the build here. |
Heya Jake Bailey (@jakebailey), I've started to run the perf test suite on this PR at 3bee8eb. You can monitor the build here. Update: The results are in! |
Heya Jake Bailey (@jakebailey), I've started to run the diff-based user code test suite on this PR at 3bee8eb. You can monitor the build here. Update: The results are in! |
Hey Jake Bailey (@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 |
TypeScript Bot (typescript-bot)
commented
Mar 4, 2023
Jake Bailey (@jakebailey) Here are the results of running the user test suite comparing Everything looks good! |
TypeScript Bot (typescript-bot)
commented
Mar 4, 2023
Jake Bailey (@jakebailey) Here they are:CompilerComparison Report - main..53092
System
Hosts
Scenarios
TSServerComparison Report - main..53092
System
Hosts
Scenarios
StartupComparison Report - main..53092
System
Hosts
Scenarios
Developer Information: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
TypeScript Bot (typescript-bot)
commented
Mar 4, 2023
Hey Jake Bailey (@jakebailey), the results of running the DT tests are ready. |
TypeScript Bot (typescript-bot)
commented
Mar 4, 2023
Jake Bailey (@jakebailey) Here are the results of running the top-repos suite comparing Everything looks good! |
Uh oh!
There was an error while loading. Please reload this page.
There's this check below your code elseif(type&&strictNullChecks&&!isTypeAssignableTo(undefinedType,type)){error(errorNode,Diagnostics.Function_lacks_ending_return_statement_and_return_type_does_not_include_undefined);}Do we need the assignability check anymore? - else if (type && strictNullChecks && !isTypeAssignableTo(undefinedType, type)) {+ else if (type && strictNullChecks) {
error(errorNode, Diagnostics.Function_lacks_ending_return_statement_and_return_type_does_not_include_undefined);
} |
…xpressions.errors.txt Co-authored-by: Daniel Rosenwasser <DanielRosenwasser@users.noreply.github.com>
| // Okay; return type allows implicit return of undefined | ||
| } | ||
| function f23(): undefined | number { |
There was a problem hiding this comment.
I think your last commit only included this baseline, as f23 doesn't appear to be anywhere else.
There was a problem hiding this comment.
I just tapped the commit button from GitHub, will add it with the another change for the feedback.
I'm not sure, isn't that for some different situation, as there's still a test for that: TypeScript/tests/cases/compiler/getterControlFlowStrictNull.ts Lines 3 to 11 in 79ef86f |
You're saying that TypeScript no longer issues an error in that test if you remove that assignability check? I can't see why that would happen. |
|
Uh oh!
There was an error while loading. Please reload this page.
| "code": 2354 | ||
| }, | ||
| "A function whose declared type is neither 'void' nor 'any' must return a value.": { | ||
| "A function whose declared type is neither 'void' nor 'any' must return a value. 'undefined' is also allowed with --strictNullChecks.": { |
There was a problem hiding this comment.
I think I'd rather us duplicate this message to have two (Daniel Rosenwasser (@DanielRosenwasser) may be able to better write these):
A function whose declared type is neither 'void' nor 'any' must return a value.
A function whose declared type is not 'void', 'any', or 'undefined' must return a value.
Then, conditionally emit one based on the variable strictNullChecks.
Then below in codefixes/returnValueCorrect.ts, handle both.
Jake Bailey (jakebailey)
left a comment
There was a problem hiding this comment.
LGTM but deferring to Daniel Rosenwasser (@DanielRosenwasser) for diagnostic wording.
| } | ||
| } | ||
| else if (type && strictNullChecks && !isTypeAssignableTo(undefinedType, type)) { | ||
| else if (type && strictNullChecks) { |
There was a problem hiding this comment.
Jake Bailey (@jakebailey)Daniel Rosenwasser (@DanielRosenwasser) The removal of this assignability check has caused a regression. This example now errors when it shouldn't:
functionfx2(x: boolean): unknown{if(x){return"hello";}};There was a problem hiding this comment.
I noticed the regression here: #53515 (comment)
There was a problem hiding this comment.
Funny. We were just talking about adding this check back again in #53490
There was a problem hiding this comment.
I'll look into why it's not working as we expected.
There was a problem hiding this comment.
Oh, no, that PR was exactly to fix the issue you mentioned. So it should be fixed in main now.
There was a problem hiding this comment.
Checked, and it is fixed, yep.
Fixes#36288