-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Improve inference by not considering thisless functions to be context-sensitive #62243
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 13 commits
4e92f82
c7f90c9
c29601c
cf4544d
5fed81c
c069fbb
07e36ce
53047af
ee1c607
f4ced6e
4543ff1
73d04f5
e22b095
48f2995
c0ea121
ec8f425
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1000,6 +1000,7 @@ function createBinder(): (file: SourceFile, options: CompilerOptions) => void { | |
| const saveExceptionTarget = currentExceptionTarget; | ||
| const saveActiveLabelList = activeLabelList; | ||
| const saveHasExplicitReturn = hasExplicitReturn; | ||
| const saveSeenThisKeyword = seenThisKeyword; | ||
| const isImmediatelyInvoked = ( | ||
| containerFlags & ContainerFlags.IsFunctionExpression && | ||
| !hasSyntacticModifier(node, ModifierFlags.Async) && | ||
|
|
@@ -1022,19 +1023,22 @@ function createBinder(): (file: SourceFile, options: CompilerOptions) => void { | |
| currentContinueTarget = undefined; | ||
| activeLabelList = undefined; | ||
| hasExplicitReturn = false; | ||
| seenThisKeyword = false; | ||
| bindChildren(node); | ||
| // Reset all reachability check related flags on node (for incremental scenarios) | ||
| node.flags &= ~NodeFlags.ReachabilityAndEmitFlags; | ||
| // Reset flags (for incremental scenarios) | ||
| node.flags &= ~(NodeFlags.ReachabilityAndEmitFlags | NodeFlags.ContainsThis); | ||
| if (!(currentFlow.flags & FlowFlags.Unreachable) && containerFlags & ContainerFlags.IsFunctionLike && nodeIsPresent((node as FunctionLikeDeclaration | ClassStaticBlockDeclaration).body)) { | ||
| node.flags |= NodeFlags.HasImplicitReturn; | ||
| if (hasExplicitReturn) node.flags |= NodeFlags.HasExplicitReturn; | ||
| (node as FunctionLikeDeclaration | ClassStaticBlockDeclaration).endFlowNode = currentFlow; | ||
| } | ||
| if (seenThisKeyword) { | ||
| node.flags |= NodeFlags.ContainsThis; | ||
| } | ||
| if (node.kind === SyntaxKind.SourceFile) { | ||
| node.flags |= emitFlags; | ||
| (node as SourceFile).endFlowNode = currentFlow; | ||
| } | ||
|
|
||
| if (currentReturnTarget) { | ||
| addAntecedent(currentReturnTarget, currentFlow); | ||
| currentFlow = finishFlowLabel(currentReturnTarget); | ||
|
|
@@ -1051,12 +1055,15 @@ function createBinder(): (file: SourceFile, options: CompilerOptions) => void { | |
| currentExceptionTarget = saveExceptionTarget; | ||
| activeLabelList = saveActiveLabelList; | ||
| hasExplicitReturn = saveHasExplicitReturn; | ||
| seenThisKeyword = node.kind === SyntaxKind.ArrowFunction ? saveSeenThisKeyword || seenThisKeyword : saveSeenThisKeyword; | ||
| } | ||
| else if (containerFlags & ContainerFlags.IsInterface) { | ||
| const saveSeenThisKeyword = seenThisKeyword; | ||
| seenThisKeyword = false; | ||
| bindChildren(node); | ||
| Debug.assertNotNode(node, isIdentifier); // ContainsThis cannot overlap with HasExtendedUnicodeEscape on Identifier | ||
| node.flags = seenThisKeyword ? node.flags | NodeFlags.ContainsThis : node.flags & ~NodeFlags.ContainsThis; | ||
| seenThisKeyword = saveSeenThisKeyword; | ||
| } | ||
| else { | ||
| bindChildren(node); | ||
|
|
@@ -2852,6 +2859,9 @@ function createBinder(): (file: SourceFile, options: CompilerOptions) => void { | |
| } | ||
| // falls through | ||
| case SyntaxKind.ThisKeyword: | ||
| if (node.kind === SyntaxKind.ThisKeyword) { | ||
| seenThisKeyword = true; | ||
| } | ||
| // TODO: Why use `isExpression` here? both Identifier and ThisKeyword are expressions. | ||
| if (currentFlow && (isExpression(node) || parent.kind === SyntaxKind.ShorthandPropertyAssignment)) { | ||
| (node as Identifier | ThisExpression).flowNode = currentFlow; | ||
|
|
@@ -3833,28 +3843,27 @@ export function getContainerFlags(node: Node): ContainerFlags { | |
| // falls through | ||
| case SyntaxKind.Constructor: | ||
| case SyntaxKind.FunctionDeclaration: | ||
| case SyntaxKind.ClassStaticBlockDeclaration: | ||
| return ContainerFlags.IsContainer | ContainerFlags.IsControlFlowContainer | ContainerFlags.HasLocals | ContainerFlags.IsFunctionLike; | ||
| case SyntaxKind.MethodSignature: | ||
| case SyntaxKind.CallSignature: | ||
| case SyntaxKind.JSDocSignature: | ||
| case SyntaxKind.JSDocFunctionType: | ||
| case SyntaxKind.FunctionType: | ||
| case SyntaxKind.ConstructSignature: | ||
| case SyntaxKind.ConstructorType: | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Type-level AST nodes had Other changes in this function are basically of the same kind - I just removed
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We seem to have added at least one of those on purpose:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As to |
||
| case SyntaxKind.ClassStaticBlockDeclaration: | ||
| return ContainerFlags.IsContainer | ContainerFlags.IsControlFlowContainer | ContainerFlags.HasLocals | ContainerFlags.IsFunctionLike; | ||
| return ContainerFlags.IsContainer | ContainerFlags.HasLocals | ContainerFlags.IsFunctionLike; | ||
|
|
||
| case SyntaxKind.JSDocImportTag: | ||
| // treat as a container to prevent using an enclosing effective host, ensuring import bindings are scoped correctly | ||
| return ContainerFlags.IsContainer | ContainerFlags.IsControlFlowContainer | ContainerFlags.HasLocals; | ||
| return ContainerFlags.IsContainer | ContainerFlags.HasLocals; | ||
|
|
||
| case SyntaxKind.FunctionExpression: | ||
| case SyntaxKind.ArrowFunction: | ||
| return ContainerFlags.IsContainer | ContainerFlags.IsControlFlowContainer | ContainerFlags.HasLocals | ContainerFlags.IsFunctionLike | ContainerFlags.IsFunctionExpression; | ||
|
|
||
| case SyntaxKind.ModuleBlock: | ||
| return ContainerFlags.IsControlFlowContainer; | ||
| case SyntaxKind.PropertyDeclaration: | ||
| return (node as PropertyDeclaration).initializer ? ContainerFlags.IsControlFlowContainer : 0; | ||
|
|
||
| case SyntaxKind.CatchClause: | ||
| case SyntaxKind.ForStatement: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21143,9 +21143,10 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |
| const { initializer } = node as JsxAttribute; | ||
| return !!initializer && isContextSensitive(initializer); | ||
| } | ||
| case SyntaxKind.JsxExpression: { | ||
| case SyntaxKind.JsxExpression: | ||
| case SyntaxKind.YieldExpression: { | ||
| // It is possible to that node.expression is undefined (e.g <div x={} />) | ||
| const { expression } = node as JsxExpression; | ||
| const { expression } = node as JsxExpression | YieldExpression; | ||
| return !!expression && isContextSensitive(expression); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Previously generators were always context-sensitive as they can't even be arrow functions. At times, they are truly context-sensitive in cases like: declare function test(
gen: () => Generator<(arg: number) => string, void, void>,
): void;
test(function* () {
yield (arg) => String(arg);
});So I had to add this extra
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just in terms of readability, maybe this would be better as its own function, then it can have a descriptive name like |
||
| } | ||
| } | ||
|
|
@@ -21154,7 +21155,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |
| } | ||
|
|
||
| function isContextSensitiveFunctionLikeDeclaration(node: FunctionLikeDeclaration): boolean { | ||
| return hasContextSensitiveParameters(node) || hasContextSensitiveReturnExpression(node); | ||
| return hasContextSensitiveParameters(node) || hasContextSensitiveReturnExpression(node) || !!(getFunctionFlags(node) & FunctionFlags.Generator && node.body && forEachYieldExpression(node.body as Block, isContextSensitive)); | ||
| } | ||
|
|
||
| function hasContextSensitiveReturnExpression(node: FunctionLikeDeclaration) { | ||
|
|
@@ -32629,7 +32630,10 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |
| if (inferenceContext && contextFlags! & ContextFlags.Signature && some(inferenceContext.inferences, hasInferenceCandidatesOrDefault)) { | ||
| // For contextual signatures we incorporate all inferences made so far, e.g. from return | ||
| // types as well as arguments to the left in a function call. | ||
| return instantiateInstantiableTypes(contextualType, inferenceContext.nonFixingMapper); | ||
| const type = instantiateInstantiableTypes(contextualType, inferenceContext.nonFixingMapper); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need this?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This addresses an export const layerServerHandlers = Rpcs.toLayer(
gen(function* () {
return {
Register: (id) => String(id),
~~
!!! error TS7006: Parameter 'id' implicitly has an 'any' type.
};
})
);In a way, this is a targeted fix that might address a fair chunk of real world scenarios. The problem with If you don't feel this approach is right, I'd have to dig into this again to refresh my memory on it to provide a better explanation.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, so I refreshed myself a little bit on this. As I mentioned,
Only one of those survives The above fix is targeted at the mentioned // Skip parameters, return signature with return type that retains noncontextual parts so inferences can still be drawn in an early stageSo an early inference is made from So this PR just ignores those simple intrinsic types here as they won't benefit any contextual typing anyway (tbf, we could also ignore |
||
| if (!(type.flags & TypeFlags.AnyOrUnknown)) { | ||
| return type; | ||
| } | ||
| } | ||
| if (inferenceContext?.returnMapper) { | ||
| // For other purposes (e.g. determining whether to produce literal types) we only | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2880,19 +2880,24 @@ export function forEachReturnStatement<T>(body: Block | Statement, visitor: (stm | |
| } | ||
| } | ||
|
|
||
| // Warning: This has the same semantics as the forEach family of functions, | ||
| // in that traversal terminates in the event that 'visitor' supplies a truthy value. | ||
| /** @internal */ | ||
| export function forEachYieldExpression(body: Block, visitor: (expr: YieldExpression) => void): void { | ||
| export function forEachYieldExpression<T>(body: Block, visitor: (expr: YieldExpression) => T): T | undefined { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this basically changes this function to work in the same way as |
||
| return traverse(body); | ||
|
|
||
| function traverse(node: Node): void { | ||
| function traverse(node: Node): T | undefined { | ||
| switch (node.kind) { | ||
| case SyntaxKind.YieldExpression: | ||
| visitor(node as YieldExpression); | ||
| const value = visitor(node as YieldExpression); | ||
| if (value) { | ||
| return value; | ||
| } | ||
| const operand = (node as YieldExpression).expression; | ||
| if (operand) { | ||
| traverse(operand); | ||
| if (!operand) { | ||
| return; | ||
| } | ||
| return; | ||
| return traverse(operand); | ||
| case SyntaxKind.EnumDeclaration: | ||
| case SyntaxKind.InterfaceDeclaration: | ||
| case SyntaxKind.ModuleDeclaration: | ||
|
|
@@ -2905,14 +2910,13 @@ export function forEachYieldExpression(body: Block, visitor: (expr: YieldExpress | |
| if (node.name && node.name.kind === SyntaxKind.ComputedPropertyName) { | ||
| // Note that we will not include methods/accessors of a class because they would require | ||
| // first descending into the class. This is by design. | ||
| traverse(node.name.expression); | ||
| return; | ||
| return traverse(node.name.expression); | ||
| } | ||
|
gabritto marked this conversation as resolved.
|
||
| } | ||
| else if (!isPartOfTypeNode(node)) { | ||
| // This is the general case, which should include mostly expressions and statements. | ||
| // Also includes NodeArrays. | ||
| forEachChild(node, traverse); | ||
| return forEachChild(node, traverse); | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -10829,7 +10833,7 @@ export function hasContextSensitiveParameters(node: FunctionLikeDeclaration): bo | |
| // an implicit 'this' parameter which is subject to contextual typing. | ||
| const parameter = firstOrUndefined(node.parameters); | ||
| if (!(parameter && parameterIsThisKeyword(parameter))) { | ||
| return true; | ||
| return !!(node.flags & NodeFlags.ContainsThis); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,5 @@ | ||
| //// [tests/cases/compiler/circularlySimplifyingConditionalTypesNoCrash.ts] //// | ||
|
|
||
| === Performance Stats === | ||
| Instantiation count: 1,000 | ||
|
|
||
| === circularlySimplifyingConditionalTypesNoCrash.ts === | ||
| type Omit<T, K extends keyof T> = Pick<T, Exclude<keyof T, K>>; | ||
| >Omit : Omit<T, K> | ||
|
|
@@ -71,8 +68,8 @@ declare var connect: Connect; | |
| const myStoreConnect: Connect = function( | ||
| >myStoreConnect : Connect | ||
| > : ^^^^^^^ | ||
| >function( mapStateToProps?: any, mapDispatchToProps?: any, mergeProps?: any, options: unknown = {},) { return connect( mapStateToProps, mapDispatchToProps, mergeProps, options, );} : <TStateProps, TOwnProps>(mapStateToProps?: any, mapDispatchToProps?: any, mergeProps?: any, options?: unknown) => InferableComponentEnhancerWithProps<TStateProps, Omit<P, Extract<keyof TStateProps, keyof P>> & TOwnProps> | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this just changes the inferred type to match the type inferred from the equivalent arrow function with type parameters, it's purely a result of making a thisless function context-insensitive |
||
| > : ^ ^^ ^^ ^^^ ^^ ^^^ ^^ ^^^ ^^ ^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| >function( mapStateToProps?: any, mapDispatchToProps?: any, mergeProps?: any, options: unknown = {},) { return connect( mapStateToProps, mapDispatchToProps, mergeProps, options, );} : (mapStateToProps?: any, mapDispatchToProps?: any, mergeProps?: any, options?: unknown) => InferableComponentEnhancerWithProps<TStateProps, Omit<P, Extract<keyof TStateProps, keyof P>> & TOwnProps> | ||
| > : ^ ^^^ ^^ ^^^ ^^ ^^^ ^^ ^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
|
|
||
| mapStateToProps?: any, | ||
| >mapStateToProps : any | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -117,8 +117,8 @@ export const Nothing2: Strategy<State> = strategy("Nothing", function*(state: St | |
| export const Nothing3: Strategy<State> = strategy("Nothing", function* (state: State) { | ||
| >Nothing3 : Strategy<State> | ||
| > : ^^^^^^^^^^^^^^^ | ||
| >strategy("Nothing", function* (state: State) { yield ; return state; // `return`/`TReturn` isn't supported by `strategy`, so this should error.}) : (a: State) => IterableIterator<State, void> | ||
| > : ^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| >strategy("Nothing", function* (state: State) { yield ; return state; // `return`/`TReturn` isn't supported by `strategy`, so this should error.}) : (a: any) => IterableIterator<any, void> | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. similarly here, this is a result of |
||
| > : ^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| >strategy : <T extends StrategicState>(stratName: string, gen: (a: T) => IterableIterator<T | undefined, void>) => (a: T) => IterableIterator<T | undefined, void> | ||
| > : ^ ^^^^^^^^^ ^^ ^^ ^^ ^^ ^^^^^ | ||
| >"Nothing" : "Nothing" | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.