Analyze control flow effects of lambdas passed as arguments #58729
+3,434
−582
Conversation
This file contains 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
added
Author: Team
For Uncommitted Bug
|
@typescript-bot test it |
|
Hey @ahejlsberg, the results of running the DT tests are ready. Everything looks the same! |
|
@ahejlsberg Here are the results of running the user tests with tsc comparing Something interesting changed - please have a look. Details
|
|
@ahejlsberg Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
I feel like this is just going to flip the problem on its head in the end - instead of people complaining because their immediately-invoked callbacks didn't affect narrowing, an equal number of people will probably now complain instead that callbacks they know will be called out-of-band do affect narrowing. The goalposts don't even move really - the other team just gets possession is all 😄 |
|
@ahejlsberg Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
|
I think it's clear that there's no one-size-fits-all answer to whether effects of executing lambda arguments should be reflected in CFA types. Our current assumption is that lambdas are never executed synchronously. This PR experiments with the assumption they're possibly executed, which, given no additional information about the function to which the lambda arguments are passed, is definitely a more sound assumption. Also, the PR validates an implementation strategy and gives us data on the performance cost of analyzing lambda effects. I would nice to avoid modifiers (like |
|
@typescript-bot test it |
|
Hey @ahejlsberg, the results of running the DT tests are ready. Everything looks the same! |
|
@ahejlsberg Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@ahejlsberg Here are the results of running the user tests with tsc comparing Something interesting changed - please have a look. Details
|
|
@ahejlsberg Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
|
@ahejlsberg If I'm understanding correctly, this would apply to arguments that are declared as arrow functions or function expressions, but not function declarations. Is that right? If so, why exclude function declarations? (FWIW, I do like this experiment and the attempt at added strictness, if it ends up not being too breaking.) |
|
@ethanresnick It only applies to arrow functions and function expressions passed directly as arguments. It does not apply to functions referenced through identifiers or other indirect expression constructs. |
|
Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page. Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up. |
|
@typescript-bot test it |
andrewbranch
previously requested changes
Aug 5, 2024
There was a problem hiding this comment.
There’s a slight issue with not considering deferred (or immediate, if we go that direction) in signature relations. I know we briefly discussed this having no effect on assignability, and I think that’s probably for the best, but I think it’s worth fixing subtype / strict subtype relations for union reduction. The current approach leads to a type ID order dependence bug [PR playground]:
// Swap the order of these two calls
fDeferred(() => {});
fImmediate(() => {});
declare function fDeferred(deferred cb: () => void): void;
declare function fImmediate(cb: () => void): void;
let f = Math.random() > 0.5 ? fDeferred : fImmediate;
let x: string | number = "OK";
f(() => {
x = 10;
});
x;
// ^?The type of x is dependent on the type of f which is dependent on the order of the first two calls, since the signatures of fDeferred and fImmediate are indistinguishable in the strict subtype relation.
# Conflicts: # src/compiler/diagnosticMessages.json # src/compiler/utilities.ts
|
@typescript-bot test it |
|
Hey @ahejlsberg, the results of running the DT tests are ready. Everything looks the same! |
|
@ahejlsberg Here are the results of running the user tests with tsc comparing Something interesting changed - please have a look. Details
|
|
@ahejlsberg Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@ahejlsberg Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
|
@typescript-bot perf test this faster |
|
@ahejlsberg Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot perf test this faster predictable=true |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Asking for predictable seems to have very unpredictable effects. 😮 |
|
@jakebailey The issue with switching to actual |
The error bars are 3-4x smaller! 😄 But yes, don't use this feature, I'm still not sure if it's a good thing or not.
Ah, right. Theoretically we can exclude |
|
Even with the keyword, #59580 (comment) shows roughly the same perf regression in bind. Still not sure what it could be reading |
|
I'm just a passer-by (who writes a lot of TypeScript), who got excited by the prospect of a new keyword that would improve CFA or soundness, but I'm not sure I understand the motivation here, relative to the magnitude of the change (new syntax). I guess it started as an effort to improve soundness by considering that called functions might have side effects, and now it basically helps correct for overzealous CFA. Here is another case (not related to this feature) where CFA thinks it knows everything and narrows the type annotation of a local variable: function foo(a: number[], i: number) {
// I'm serious, Typescript, I'm saying this could be out-of-bounds!
const n: number | undefined = a[i];
const n2 = n; // : number
}In both cases, you have to occasionally override CFA, e.g. in the example at the top of this thread, one could write: declare function mystery(cb: () => void): void;
let x: string | number = "OK" as string | number;
mystery(() => {
x = 10;
});
if (x === 10) { // works
}In terms of the larger issue of side effects, I think it should at least be acknowledged that passing any closure to any function could lead to functions having side effects that affect local variables; it doesn't really depend on whether a callback is invoked "immediately" or "deferred." For example: const foo = new Foo(() => { /* ... set a local variable */ });
foo.execute();Here, the callback is not called immediately, but it is called during the course of the function. The point is, the question of whether a callback might be called immediately or not by a function is only loosely connected to the question of whether a function call has side effects that might affect the analysis of local variables. I think an annotation that says "this callback is called exactly once, synchronously" or "this callback is called at least once, synchronously" might even be more useful? It would serve a slightly different purpose. If this keyword opens the door to other keywords, I'm for it. There are a lot of keywords I would want. |
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.
This PR introduces a new
immediatemodifier that can be applied to callback parameters to indicate that control flow analysis should assume that function expressions or arrow functions (in the following called lambda expressions) passed as arguments in a function call may be invoked synchronously during that call. For example:Without the
immediatemodifier, control flow analysis will assume that the lambda expression is never invoked synchronously and thus will consider the type ofxto bestringfollowing the call. With theimmediatemodifier, control flow analysis assumes that the lambda expression may have been invoked synchronously and thus considers the type ofxto bestring | numberfollowing the call.It an error to apply the
immediatemodifier to anything but a parameter with a type that permits functions. Async arrow functions, async function expressions, and generator function expressions are always assumed to be deferred, regardless of whether their corresponding parameter includes animmediatemodifier.In JavaScript files, an immediate callback parameter can be declared using an
/** @immediate */JSDoc annotation:Since the
immediatemodifier is considered an error by earlier versions of TypeScript, it isn't possible to addimmediatemodifiers to frameworks and libraries without also requiring the latest version of the compiler. The compiler therefore also recognizes/** @immediate */JSDoc comments in TypeScript files such that they can be used in cases where backwards compatibility matters. However, use ofimmediatemodifiers is recommended whenever possible.NOTE: This PR initially changed control flow analysis to assume possibly-immediately-called semantics by default and implemented a
deferredmodifier to indicate never-immediately-called semantics. While technically more sound (absent knowledge of whether or when a callback is called, it is safest and most conservative to assume it may have been called), this turned out to require too much remediation in existing code. The PR now preserves the default never-immediately-called semantics and allows code to opt into possibly-immediately-called analysis using theimmediatemodifier.Fixes #11498.
Fixes #15380.
Fixes #57880.