Make function properties context-sensitive based on their return statements #50903
Conversation
src/compiler/checker.ts
Outdated
| @@ -17588,8 +17588,13 @@ namespace ts { | |||
| } | |||
|
|
|||
| function hasContextSensitiveReturnExpression(node: FunctionLikeDeclaration) { | |||
| // TODO(anhans): A block should be context-sensitive if it has a context-sensitive return value. | |||
There was a problem hiding this comment.
Basically the fix implements this TODO comment 😉
|
@typescript-bot test this |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 0a5da2f. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the abridged perf test suite on this PR at 0a5da2f. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 0a5da2f. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at 0a5da2f. You can monitor the build here. |
|
@jakebailey Here are the results of running the user test suite comparing Everything looks good! |
|
@jakebailey Here they are:Comparison Report - main..50903
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Related -- #47599 |
|
@typescript-bot pack this |
|
Heya @RyanCavanaugh, I've started to run the tarball bundle task on this PR at 0a5da2f. You can monitor the build here. |
|
Hey @RyanCavanaugh, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running |
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
|
@RyanCavanaugh all cases from the first post in 47599 work OK since 4.7: TS playground. Do you have any additional test cases for this PR based on that issue? Could that issue be already closed as solved based on this prepared playground? |
|
@weswigham @ahejlsberg I remember from prior conversations that this approach is maybe not quite right (due to CFA in function bodies?), but I wasn't able to craft a demonstration of a problem. Thoughts? |
|
Iirc it's because the |
|
|
Those builds expire, but it's supposed to go and build an npm package / playground too. I'll rerun it. @typescript-bot pack this |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at 0a5da2f. You can monitor the build here. |
|
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 |
function fn<ToInferred>(
callback: (unused: string) => {
key: ToInferred;
keyCallback: (key: ToInferred) => number;
},
) {}
fn(() => ({
key: 5,
keyCallback: (key) => {
return key; // Success, key is number
},
}));
fn((unused) => ({
key: 5,
keyCallback: (key) => {
return key; // Error, key is unknown.
},
}));The above code is not working. Should I create new issue? |
|
I would say that yes, this might deserve a new issue - it doesn't exactly look like the one that is being fixed here. |
…text-sensitive-return-statement
|
Hey - is there any update on this? Would be good to get this merged 🙏 |
|
@RyanCavanaugh @weswigham any further thoughts on this one? |
There was a problem hiding this comment.
Honestly, I've written this exact change 2 or 3 times since that TODO was first introduced, and given how limited return type inference is, I think this is good enough™️. Plus, any issue you could bring forward as block-specific and a good reason to forbid blocks, I can probably rewrite into a blockless arrow using the comma operator and an iife, which we should already track context sensitivity through (spoiler: we don't, same as we don't across block statements), so any limitation in considering the block is probably already equivalently demonstrable today.
Unless @ahejlsberg feels really strongly and still has good reason to block this, I'm inclined to take it. (pending test results)
|
@typescript-bot run dt |
|
Heya @weswigham, I've started to run the diff-based top-repos suite on this PR at a5ffd7e. You can monitor the build here. Update: The results are in! |
|
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at a5ffd7e. You can monitor the build here. Update: The results are in! |
|
@weswigham Here are the results of running the top-repos suite comparing Everything looks good! |
|
Hey @weswigham, the results of running the DT tests are ready. |
Andarist
deleted the
fix/property-with-context-sensitive-return-statement
branch
fixes #50687
cc @Rugvip