Preserve type refinements in closures created past last assignment #56908
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
|
@typescript-bot test top100 |
|
Heya @ahejlsberg, I've started to run the diff-based user code test suite on this PR at 2739399. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 2739399. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the tsc-only perf test suite on this PR at 2739399. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the diff-based top-repos suite on this PR at 2739399. You can monitor the build here. Update: The results are in! |
|
@ahejlsberg Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@ahejlsberg Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
|
Hey @ahejlsberg, the results of running the DT tests are ready. |
|
@ahejlsberg Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
| @@ -187,15 +183,9 @@ controlFlowAliasing.ts(280,5): error TS2454: Variable 'a' is used before being a | |||
| const isFoo = obj.kind === 'foo'; | |||
| if (isFoo) { | |||
| obj.foo; // Not narrowed because obj is mutable | |||
| ~~~ | |||
| !!! error TS2339: Property 'foo' does not exist on type '{ kind: "foo"; foo: string; } | { kind: "bar"; bar: number; }'. | |||
| !!! error TS2339: Property 'foo' does not exist on type '{ kind: "bar"; bar: number; }'. | |||
| } | |||
| else { | |||
| obj.bar; // Not narrowed because obj is mutable | |||
There was a problem hiding this comment.
I guess these comments are outdated now
There was a problem hiding this comment.
Yeah, I'll fix those.
|
Shouldn't the assigned type be preserved at least in the second branch? declare function action(cb: () => void): void
function test(arg: string | number | undefined) {
if (arg === undefined) {
arg = 'foo'
action(() => {
arg
// ^?
})
} else {
arg = 42
action(() => {
arg
// ^?
})
}
} |
Ideally, the assigned type would be preserved in both branches since the closures are both created following the last assignment. That, however, would require full control flow analysis which is significantly more complex and expensive than the single pass we do now. The next best alternative, at least in my opinion, is to extend assignment effects in compound statements to the entire statement. That's what the PR currently does. It is a bit more conservative than just using the lexical position of the last assignment, but it is more consistent. |
Yeah, that would - indeed - be the dream :P I haven't spent much time so far understanding how CFA is implemented. Is it always an append-only graph that is attached to nodes? In other words, is the complexity here that we can't easily peek into flow nodes that follow a specific node aka it's not easy to assess if the control flow is joined with another branch later on? |
Right, the control flow graph is a "reverse linked" graph optimized for exploring execution effects that occurred prior to reaching a specific node. It is less well suited for exploring execution effects that occur after reaching a node. The most similar scenario we currently handle in the type checker is definite property initialization in constructors (the |
|
@typescript-bot perf test this faster |
|
Heya @ahejlsberg, I've started to run the tsc-only perf test suite on this PR at a490ef4. You can monitor the build here. Update: The results are in! |
|
@typescript-bot pack this |
|
Heya @ahejlsberg, I've started to run the tarball bundle task on this PR at a490ef4. You can monitor the build here. |
|
Hey @ahejlsberg, 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 |
|
@ahejlsberg Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
The change with the latest commits is that we don't preserve type refinements for mutable global variables in closures since such globals might be modified by code in other files or modules. The issue with the spooky narrowing of |
|
@typescript-bot perf test public |
|
Heya @jakebailey, I've started to run the public perf test suite on this PR at cd9b30a. You can monitor the build here. Update: The results are in! |
|
@jakebailey, the perf run you requested failed. You can check the log here. |
|
This PR appears to conflict with main and needs a merge and fix: |
|
@typescript-bot perf test public |
|
Heya @jakebailey, I've started to run the public perf test suite on this PR at 28da1c5. You can monitor the build here. Update: The results are in! |
| case SyntaxKind.SwitchStatement: | ||
| case SyntaxKind.TryStatement: | ||
| case SyntaxKind.ClassDeclaration: | ||
| pos = node.end; |
There was a problem hiding this comment.
I don't know if it's worth noting that LabeledStatement isn't covered here. I don't think that it really matters because its end position should be identical to any of the other cases.
There was a problem hiding this comment.
Yeah, no special handling needed for LabeledStatement.
|
Let's wait for the perf results before merging. |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Performance is unaffected. We're good to merge. |
|
Coming here from the twitter thread :)
Class declarations are not hoisted, so this improvement could also applied to them. The only hoisted values are function declarations. |
|
Already discussed here (with this response):
|
With microsoft/TypeScript#56908, TS should better preserve type refinements. To help test this out, I made a quick pass through our code to remove type assertions that are no longer needed Most of these are not impacted by microsoft/TypeScript#56908 but removing them helps TS test changes like this Not adding an eslint rule for now as it requires whole program intellisense, which is too slow for commit hooks
* Remove extra not null assertions With microsoft/TypeScript#56908, TS should better preserve type refinements. To help test this out, I made a quick pass through our code to remove type assertions that are no longer needed Most of these are not impacted by microsoft/TypeScript#56908 but removing them helps TS test changes like this Not adding an eslint rule for now as it requires whole program intellisense, which is too slow for commit hooks * Fix merge
ghost
mentioned this pull request
|
The change with the latest commits is where type refinements for mutable global variables in closures are no longer preserved. This adjustment is made because such global variables may be altered by code in other files or modules. The issue with narrowing x.a is a separate concern. While narrowing discriminated unions is technically unsafe due to the potential mutation of discriminants (which are mutable), such mutations are uncommon in practice. Despite this, the benefits of narrowing are significant and desirable. |
|
ChatGPT alert 🙄 |
We currently preserve type refinements in closures for
constvariables and parameters that are never targeted in assignments. With this PR we also preserve type refinements for parameters, localletvariables, andcatchclause variables in closures that are created past the last assignment to those parameters or variables. For example:Above, it is unknown when (or even if) the
actionfunction will invoke a callback function previously passed to it. Thus, when the first arrow function is created, it is unknown whether it will be invoked withxhaving typestringornumberbecause there are subsequent assignments tox. However, the second arrow function is created past the last assignment tox, soxcan safely be narrowed to typenumberin that arrow function.A similar example that adjusts an argument value before using it in a closure:
Type refinements are not preserved in inner function and class declarations (due to hoisting):
Implicit
anyvariables have a known type following the last assignment:Type refinements for
catchvariables are preserved past the last assignment (if any):Note that effects of assignments in compound statements extend to the entire statement:
Above, the type of
xis narrowed tonumberonly in the last arrow function, even though it would be safe to narrow in the other two arrow functions. That, however, would require full control flow analysis which is significantly more complex and expensive than the single pass we do now.This PR fixes multiple issues that have previously been attributed to #9998.
Fixes #13142.
Fixes #13560.
Fixes #13572.
Fixes #14748.
Fixes #16285.
Fixes #17240.
Fixes #17449.
Fixes #19606.
Fixes #19683.
Fixes #19698.
Fixes #19918.
Fixes #22120.
Fixes #22635.
Fixes #23776.
Fixes #29392.
Fixes #29916.
Fixes #31266.
Fixes #32625.
Fixes #33319.
Fixes #34669.
Fixes #35124.
Fixes #37339.
Fixes #38755.
Fixes #40202.
Fixes #43827.
Fixes #44218.
Fixes #46118.
Fixes #50580.
Fixes #52104.
Fixes #55528.
Fixes #56854.
Fixes #56973.