-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
base: main
Are you sure you want to change the base?
Changes from 5 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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -1014,6 +1014,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) && | ||||||
|
|
@@ -1036,19 +1037,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; | ||||||
|
||||||
| node.flags |= NodeFlags.ContainsThis; | |
| node.flags |= NodeFlags.ContainsThis; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21135,9 +21135,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); | ||
| } | ||
| } | ||
|
|
@@ -21146,7 +21147,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) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2864,19 +2864,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: | ||
|
|
@@ -2889,14 +2894,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.
Show resolved
Hide 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); | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -10825,7 +10829,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 |
|---|---|---|
|
|
@@ -68,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" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,8 @@ | ||
| redefineArray.ts(1,1): error TS2741: Property 'isArray' is missing in type '<T>(n: number, s: string) => number' but required in type 'ArrayConstructor'. | ||
| redefineArray.ts(1,1): error TS2741: Property 'isArray' is missing in type '(n: number, s: string) => number' but required in type 'ArrayConstructor'. | ||
|
|
||
|
|
||
| ==== redefineArray.ts (1 errors) ==== | ||
| Array = function (n:number, s:string) {return n;}; | ||
| ~~~~~ | ||
| !!! error TS2741: Property 'isArray' is missing in type '<T>(n: number, s: string) => number' but required in type 'ArrayConstructor'. | ||
| !!! error TS2741: Property 'isArray' is missing in type '(n: number, s: string) => number' but required in type 'ArrayConstructor'. | ||
| !!! related TS2728 lib.es5.d.ts:--:--: 'isArray' is declared here. |
Uh oh!
There was an error while loading. Please reload this page.