Improve inference by not considering thisless functions to be context-sensitive #62243
Conversation
typescript-bot
added
the
For Uncommitted Bug
There was a problem hiding this comment.
Pull Request Overview
This PR improves TypeScript's type inference by making functions that don't reference this not context-sensitive. Previously, all object literal methods were considered context-sensitive, causing poor inference when type parameters depended on inferring from these methods first. The change helps TypeScript better infer types in common patterns involving object literals with functions.
Key Changes:
- Modified
hasContextSensitiveParametersto check for actualthisusage rather than assuming all function-like declarations are context-sensitive - Updated binder to track
thiskeyword usage withNodeFlags.ContainsThisflag - Extended context-sensitive checking to include yield expressions in generators
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/compiler/binder.ts | Adds tracking of this keyword usage and sets ContainsThis flag on functions |
| src/compiler/checker.ts | Updates context-sensitive checking to include yield expressions and generators |
| src/compiler/utilities.ts | Modifies hasContextSensitiveParameters and forEachYieldExpression to support new logic |
| tests/cases/compiler/*.ts | New test cases demonstrating improved inference for thisless functions |
| tests/baselines/reference/. | Updated baselines showing improved type inference results |
src/compiler/binder.ts
Outdated
| (node as FunctionLikeDeclaration | ClassStaticBlockDeclaration).endFlowNode = currentFlow; | ||
| } | ||
| if (seenThisKeyword) { | ||
| node.flags |= NodeFlags.ContainsThis; |
There was a problem hiding this comment.
There's trailing whitespace at the end of this line. Remove the extra spaces.
| node.flags |= NodeFlags.ContainsThis; | |
| node.flags |= NodeFlags.ContainsThis; |
| case SyntaxKind.JSDocFunctionType: | ||
| case SyntaxKind.FunctionType: | ||
| case SyntaxKind.ConstructSignature: | ||
| case SyntaxKind.ConstructorType: |
There was a problem hiding this comment.
Type-level AST nodes had ContainerFlags.IsControlFlowContainer here. This was messing up some of the changes I made since it was interfering with the implemented seenThisKeyword tracking. I don't see why those would be considered control flow containers and there are no tests proving it was needed.
Other changes in this function are basically of the same kind - I just removed ContainerFlags.IsControlFlowContainer from the type-level nodes.
There was a problem hiding this comment.
We seem to have added at least one of those on purpose:
#8941
So I'm wondering why it's not needed anymore. @ahejlsberg do you remember why this was needed?
There was a problem hiding this comment.
As to SyntaxKind.PropertyDeclaration - given the test from the referenced PR still works just OK, I'd assume that its needs are covered by arrow functions being treated as flow containers (arrows were used as property declaration initializers in that test).
src/compiler/checker.ts
Outdated
|
|
||
| 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)); |
There was a problem hiding this comment.
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 forEachYieldExpression to cover for this
There was a problem hiding this comment.
Just in terms of readability, maybe this would be better as its own function, then it can have a descriptive name like hasContextSensitiveYieldExpression and that example can be its documentation.
| // 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 { |
There was a problem hiding this comment.
this basically changes this function to work in the same way as forEachReturnStatement defined above
| 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> |
There was a problem hiding this comment.
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
| > : ^^^^^^^^^^^^^^^ | ||
| >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> |
There was a problem hiding this comment.
similarly here, this is a result of inferSignatureInstantiationForOverloadFailure no longer skipping the generator function on the basis it's context-sensitive (inferSignatureInstantiationForOverloadFailure uses CheckMode.SkipContextSensitive)
| }, | ||
| } | ||
| impl.explicitVoid1 = function () { return 12; }; | ||
| >impl.explicitVoid1 = function () { return 12; } : (this: void) => number |
There was a problem hiding this comment.
those this parameters were not used by the assigned implementation - they were just auto-assigned to it based on the this parameter in the contextual signature
|
@typescript-bot test it |
|
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 |
|
Hey @jakebailey, the results of running the DT tests are ready. There were interesting changes: Branch only errors:Package: jqrangeslider |
|
@jakebailey Here are the results of running the user tests with tsc comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
|
…dates-based returns `any`/`unknown`
|
@typescript-bot test it |
|
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 |
|
Hey @jakebailey, the results of running the DT tests are ready. There were interesting changes: Branch only errors:Package: jqrangeslider |
…y-for-this-less-functions
…y-for-this-less-functions
Andarist
force-pushed
the
no-context-sensitivity-for-this-less-functions
branch
from
76c7aa0 to
73d04f5
Compare
…y-for-this-less-functions
|
@typescript-bot test it |
|
@jakebailey, the perf run you requested failed. You can check the log here. |
|
Hey @jakebailey, it looks like the DT test run failed. Please check the log for more details. |
|
@typescript-bot test it |
|
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 |
|
Hey @jakebailey, the results of running the DT tests are ready. There were interesting changes: Branch only errors:Package: jqrangeslider |
|
@jakebailey Here are the results of running the user tests with tsc comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
gabritto
left a comment
gabritto
left a comment
There was a problem hiding this comment.
Looks good so far, I just had some questions about some implementation details. Still need to go through some of the breaks/regression tests.
src/compiler/checker.ts
Outdated
|
|
||
| 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)); |
There was a problem hiding this comment.
Just in terms of readability, maybe this would be better as its own function, then it can have a descriptive name like hasContextSensitiveYieldExpression and that example can be its documentation.
| // 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); |
There was a problem hiding this comment.
This addresses an effect break caught here. You can take a deeper look at the added test tests/cases/compiler/returnTypeInferenceContextualParameterTypesInGenerator1.ts. Without this the test runs into:
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 instantiateContextualType is, unfortunately, that its "lossy" at times (at least that can happen when return mappers are involved) - it can lose meaningful information for some nodes deeper in the tree. any/unknown don't provide any useful contextual information so this helps by keeping the contextual type in its uninstantiated form.
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.
There was a problem hiding this comment.
Ok, so I refreshed myself a little bit on this. As I mentioned, instantiateContextualType is tricky. I could probably easily find at least 5 issues related to it. Conceptually, the best contextual type might be one of the following:
- uninstantiated contextual type (its constraints might end up being useful)
- instantiated contextual type using inferred types and/or default
- instantiated contextual type using return mapper
Only one of those survives instantiateContextualType though and at the time it gets called it's not exactly which one might be best (usually for nodes deeper in the AST). So given the current implementation, it's guaranteed there might be cases when this loses some useful information and it already happens today.
The above fix is targeted at the mentioned effect-related break but it should benefit more than that. Given the changes in this PR, a returnOnlyType was created: () => Generator<never, { Register: anyFunctionType; }, any>. That type is only created when hasContextSensitiveParameters(node) and this PR doesn't consider thisless generators to automatically have a context sensitive parameter. It's worth noting that returnOnlyType itself has ObjectFlags.NonInferrableType but its parts doesnt have that so inferences can be made from them. In fact, that's kinda the goal - as per the code comment:
// 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 never into Eff and that makes the first branch in instantiateContextualType to kick in. But the instantiated type isn't all that useful - it's just unknown - and a better candidate can be discovered based on the returnMapper: HandlersFrom<Rpc<"Register", number, string>>.
So this PR just ignores those simple intrinsic types here as they won't benefit any contextual typing anyway (tbf, we could also ignore never). This allows the logic to try the other source of instantiation - the returnMapper. And, in there, it discovers the more interesting/useful type (HandlersFrom<Rpc<"Register", number, string>>). In fact, this branch could also ignore any/unknown and let the type to trickle down to the uninstantiated T. I pushed this out in ec8f425
| case SyntaxKind.JSDocFunctionType: | ||
| case SyntaxKind.FunctionType: | ||
| case SyntaxKind.ConstructSignature: | ||
| case SyntaxKind.ConstructorType: |
There was a problem hiding this comment.
We seem to have added at least one of those on purpose:
#8941
So I'm wondering why it's not needed anymore. @ahejlsberg do you remember why this was needed?
|
@jakebailey Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
|
The perf run shows 5 new errors in vscode; I am not sure why they are not showing up in the top or user tests... |
|
@jakebailey thanks for noticing, I'll look into it |
|
microsoft/vscode#282223 should address VS Code break. A general fix for this issue could be a performant version of #57421 . I have prototyped locally a version of |
4 participants
implements @RyanCavanaugh's suggestion from #47599 :
fixes #62204
fixes #60986
fixes #58630
fixes #57572
fixes #56067
fixes #55489
fixes #55124
fixes #53924
fixes #50258