Analyze control flow effects of lambdas passed as arguments #58729
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 |
|
Are there any reasons about using function setTimeout(callback: deferred (args: void) => void, ms?: number): NodeJS.Timeout;
function setTimeout(callback: deferred Function, ms?: number): NodeJS.Timeout; |
class Foo {
constructor(deferred public a: () => void) { }
}
class Bar {
constructor(public deferred a: () => void) { }
}It seems that both orders are acceptable, is this expected? |
One reason is that the "deferred" nature of a parameter is an attribute of the parameter itself, not the parameter's type. Another reason is that a
That would make |
No, we should verify that |
# Conflicts: # src/compiler/diagnosticMessages.json # tests/baselines/reference/asyncYieldStarContextualType.types # tests/baselines/reference/awaitedType.types # tests/baselines/reference/contextuallyTypeAsyncFunctionReturnTypeFromUnion.types # tests/baselines/reference/declarationEmitExportAliasVisibiilityMarking.types # tests/baselines/reference/declarationEmitUsingAlternativeContainingModules1.types # tests/baselines/reference/declarationEmitUsingAlternativeContainingModules2.types # tests/baselines/reference/destructureOfVariableSameAsShorthand.types # tests/baselines/reference/esModuleInteropImportCall.types # tests/baselines/reference/genericFunctionInference1.types # tests/baselines/reference/importCallExpression1ES2020.types # tests/baselines/reference/importCallExpression2ES2020.types # tests/baselines/reference/importCallExpression4ES2020.types # tests/baselines/reference/importCallExpressionES5AMD.types # tests/baselines/reference/importCallExpressionES5CJS.types # tests/baselines/reference/importCallExpressionES5System.types # tests/baselines/reference/importCallExpressionES5UMD.types # tests/baselines/reference/importCallExpressionES6AMD.types # tests/baselines/reference/importCallExpressionES6CJS.types # tests/baselines/reference/importCallExpressionES6System.types # tests/baselines/reference/importCallExpressionES6UMD.types # tests/baselines/reference/importCallExpressionErrorInES2015.types # tests/baselines/reference/importCallExpressionInAMD1.types # tests/baselines/reference/importCallExpressionInAMD2.types # tests/baselines/reference/importCallExpressionInAMD4.types # tests/baselines/reference/importCallExpressionInCJS1.types # tests/baselines/reference/importCallExpressionInCJS3.types # tests/baselines/reference/importCallExpressionInCJS5.types # tests/baselines/reference/importCallExpressionInSystem1.types # tests/baselines/reference/importCallExpressionInSystem2.types # tests/baselines/reference/importCallExpressionInSystem4.types # tests/baselines/reference/importCallExpressionInUMD1.types # tests/baselines/reference/importCallExpressionInUMD2.types # tests/baselines/reference/importCallExpressionInUMD4.types # tests/baselines/reference/importCallExpressionNoModuleKindSpecified.types # tests/baselines/reference/importCallExpressionReturnPromiseOfAny.types # tests/baselines/reference/importCallExpressionShouldNotGetParen.types # tests/baselines/reference/importCallExpressionSpecifierNotStringTypeError.types # tests/baselines/reference/inferenceLimit.types # tests/baselines/reference/instantiateContextualTypes.types # tests/baselines/reference/mappedTypesGenericTuples2.types # tests/baselines/reference/modularizeLibrary_Dom.asynciterable.types # tests/baselines/reference/modularizeLibrary_NoErrorDuplicateLibOptions1.types # tests/baselines/reference/modularizeLibrary_NoErrorDuplicateLibOptions2.types # tests/baselines/reference/modularizeLibrary_TargetES5UsingES6Lib.types # tests/baselines/reference/modularizeLibrary_Worker.asynciterable.types # tests/baselines/reference/moduleResolutionWithoutExtension5.types # tests/baselines/reference/moduleResolutionWithoutExtension8.types # tests/baselines/reference/privateNameMethodAsync.types # tests/baselines/reference/promisePermutations.types # tests/baselines/reference/promisePermutations2.types # tests/baselines/reference/promisePermutations3.types # tests/baselines/reference/promiseTest.types # tests/baselines/reference/promiseType.types # tests/baselines/reference/promiseTypeStrictNull.types # tests/baselines/reference/promiseVoidErrorCallback.types # tests/baselines/reference/promises.types # tests/baselines/reference/specializationError.types # tests/baselines/reference/syntheticDefaultExportsWithDynamicImports.types # tests/baselines/reference/truthinessPromiseCoercion.types # tests/baselines/reference/unionAndIntersectionInference1.types # tests/baselines/reference/unionOfClassCalls.types
|
Function parameters can also be declared using tuples, would the function a(...args: [deferred () => void]): void {} // The tuple element can have no name, so this looks very differently
function b(...args: [deferred callback: () => void]): void {}
function c(...args: Parameters<typeof a>): void {} // Still deferred?EDIT: mentioned by @codehz , function parameter can also be an object with function properties: function d(props: { a: () => void, b: () => void }) {
props.a();
props.b();
}Would the Would TypeScript check the function body to ensure a non-deferred callback parameter is actually (or at least might) synchronously called? function a(callback: () => void) { } // Error: `callback` is marked as synchronous but not synchronously invoked
function b(callback: () => void) { // Error: `callback` is marked as synchronous but not synchronously invoked
setTimeout(callback, 1000);
}
function c(callback: () => void) { // OK
if (Math.random() > 0.5) {
callback();
}
}I think the It might also be beneficial to allow the call site to specify whether an argument will be invoked synchronously or not, especially when library authors has not adopted the change into their typings, so there will be many callbacks mistakenly considered synchronous. // library.d.ts
declare function doThingsAsync(callback: () => void): void; // Not updated
// src/index.ts
let x: string | undefined;
doThingsAsync((() => { x = "foo" }) as Deferred<() => void>);
x; // Still `string | undefined` |
No. The
We discussed doing this, but it's not clear how useful it would be, and there are several complex issues. We could potentially check that no calls are made directly through the deferred parameter in the body of the function, but that is quickly defeated by us not analyzing aliasing and calls made to other functions. And attempting to track it in types such as
The recommendation here is to use a simple wrapper function that just returns its argument: function deferred<T extends (...args: any) => any>(deferred cb: T) { return cb }
declare function doThingsAsync(callback: () => void): void; // Not updated
let xx: string | undefined = undefined;
doThingsAsync(deferred(() => { xx = "foo" }));
xx; // Still undefined |
ahejlsberg
requested review from
DanielRosenwasser,
jakebailey and
RyanCavanaugh
|
|
|
Hey @DanielRosenwasser, 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 test it |
|
Hey @DanielRosenwasser, the results of running the DT tests are ready. Everything looks the same! |
|
@DanielRosenwasser Here are the results of running the user tests with tsc comparing Something interesting changed - please have a look. Details
|
|
@DanielRosenwasser Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@DanielRosenwasser Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
|
I'm having a hard time constructing a simple use case for where this might come up, but I have a slight concern around how the export declare function doStuff(deferred f: () => void): void;
declare function recreate<Args extends unknown[], Return>(f: (...args: Args) => Return): (...args: Args) => Return;
declare function recreateDeferred<Args extends unknown[], Return>(f: (deferred ...args: Args) => Return): (...args: Args) => Return;
{
let x: string | number;
x = 123;
doStuff(() => {
x = "hi";
});
x;
// ^? let x: number
}
{
let y: string | number;
y = 123;
recreate(doStuff)(() => {
y = "hi";
});
y;
// ^? let y: string | number
}
{
let z: string | number;
z = 123;
recreateDeferred(doStuff)(() => {
z = "hi";
});
z;
// ^? let z: string | number
}At the very least, this would be a good test case to add. |
|
Not sure what your intent is, but did you mean to have declare function recreateDeferred<Args extends unknown[], Return>(f: (...args: Args) => Return): (deferred ...args: Args) => Return;Certainly that works as expected. |
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 changes control flow analysis to 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:
Previously, we wouldn't account for the possibility that the lambda expression might have been invoked during the call to
mysteryand thus we'd assume that the control flow type ofxwas stillstringfollowing the call. Effectively, our assumption was that lambda expression arguments are never invoked synchronously, which is less sound than assuming they may have executed.In some scenarios it is known that a callback parameter is never invoked synchronously (typical of many UI related callbacks), and therefore that possible effects of lambda expression arguments aren't immediate. To support those scenarios, this PR introduces a new
deferredmodifier that can be used to mark deferred callback parameters:When a lambda expression is passed as the argument for a
deferredparameter, control flow analysis assumes that the lambda isn't called synchronously. (Effectively,deferredreverts control flow analysis to it's previous behavior.)It an error to apply the
deferredmodifier to anything but a parameter with a type that permits functions.In JavaScript files, a deferred callback parameter can be declared using a
/** @deferred */JSDoc annotation:Async arrow functions, async function expressions, and generator function expressions are always assumed to be deferred, regardless of whether their corresponding parameter includes a
deferredmodifier.Fixes #11498.
Fixes #15380.
Fixes #57880.