fix for 45006 #45020
fix for 45006 #45020
Conversation
|
|
softwareCobbler
force-pushed
the
fix/45006
branch
from
85a39c6 to
5920993
Compare
src/compiler/binder.ts
Outdated
| @@ -1835,6 +1833,8 @@ namespace ts { | |||
| case SyntaxKind.ClassStaticBlockDeclaration: | |||
| return ContainerFlags.IsContainer | ContainerFlags.IsControlFlowContainer | ContainerFlags.HasLocals | ContainerFlags.IsFunctionLike; | |||
|
|
|||
| case SyntaxKind.GetAccessor: | |||
| case SyntaxKind.SetAccessor: | |||
There was a problem hiding this comment.
This move doesn't seem right -- get/set aren't function expressions. Do the tests still pass with this change reverted?
There was a problem hiding this comment.
reverting just this change causes the new test to fail. The goal here was (admittedly opaquely) to have bindContainer bind the getter/setter node to the fresh start flow, which returning these container flags succeeded at doing (the desired flow binding occurs if the containerFlags for a given container are either ContainerFlags.IsFunctionExpression or ContainerFlags.IsObjectLiteralOrClassExpressionMethod). Your comments about consolidating isObjectLiteralOrClassExpressionMethod makes it look like moving these switch cases to be with the MethodDeclaration case makes sense.
This made me realize I should have made explicit the intent to attach GetAccessorDeclaration and SetAccessorDeclarations to FlowStart.node; doing so has the possibly unfortunate side effect of changing the shape of the FlowStart interface in 2 publicly exposed interface files. Not sure what the policy is on that.
| @@ -3372,7 +3372,7 @@ namespace ts { | |||
| emitFlags |= NodeFlags.HasAsyncFunctions; | |||
| } | |||
|
|
|||
| if (currentFlow && isObjectLiteralOrClassExpressionMethod(node)) { | |||
There was a problem hiding this comment.
There are only 3 callers of isObjectLiteralOrClassExpressionMethod. Two of them need this addition, and the third is guarded by case SyntaxKind.MethodDeclaration. So I'd like to see the new check moved into that function, and the name should be ... uhh ... isObjectLiteralOrClassExpressionMethodOrAccessor ???
Maybe there's a better name if there's a term that means "method or accessor".
There was a problem hiding this comment.
I just went with choice 1, isObjectLiteralOrClassExpressionMethodOrAccessor; there was also a ContainerFlags by the same name that looked like it could be part of the same renaming effort.
| @@ -37,3 +37,13 @@ interface I1 { | |||
| } | |||
|
|
|||
| var i:I1 = function (n) {return n;} | |||
|
|
|||
There was a problem hiding this comment.
Please add a // @target: es2020 (or any target newer than es3) at the top of this file. I wish our tests didn't target ES3 by default.
There was a problem hiding this comment.
yes, no problem
in `getContainerFlags`, move cases for get/set accessors to fallthrough into the block that currently handles MethodDeclaration; so get/set accessors and method declarations all get the same container flags, such that during `bindContainer`, `startFlow.node` is assigned to getters/accessors (this changes a public api in tsserverlibrary.d.ts and typescript.d.ts by adding `GetAccessorDeclaration` and `SetAccessorDeclaration` to the type of `FlowStart.node`) consolidate predicates checking whether a node is either a get or set accessor, into `isObjectLiteralOrClassExpressionMethodOrAccessor` (formerly `isObjectLiteralOrClassExpressionMethod`) annotate updated test with `@target: es2020`
src/compiler/utilities.ts
Outdated
| export function isObjectLiteralOrClassExpressionMethod(node: Node): node is MethodDeclaration { | ||
| return node.kind === SyntaxKind.MethodDeclaration && | ||
| export function isObjectLiteralOrClassExpressionMethodOrAccessor(node: Node): node is MethodDeclaration { | ||
| return (node.kind === SyntaxKind.MethodDeclaration && |
There was a problem hiding this comment.
The parent of a get/set should also have to be an object literal or class expression; they're the same as method declarations in that regard.
There was a problem hiding this comment.
OK, I see that now.
require that Getter/Setters are parented by an ObjectLiteralExpression or ClassExpression
* fix for 45006 * treat setters like getters in preceding commit; move test accordingly * fix test baselines * changes per code review in `getContainerFlags`, move cases for get/set accessors to fallthrough into the block that currently handles MethodDeclaration; so get/set accessors and method declarations all get the same container flags, such that during `bindContainer`, `startFlow.node` is assigned to getters/accessors (this changes a public api in tsserverlibrary.d.ts and typescript.d.ts by adding `GetAccessorDeclaration` and `SetAccessorDeclaration` to the type of `FlowStart.node`) consolidate predicates checking whether a node is either a get or set accessor, into `isObjectLiteralOrClassExpressionMethodOrAccessor` (formerly `isObjectLiteralOrClassExpressionMethod`) annotate updated test with `@target: es2020` * fix `isObjectLiteralOrClassExpressionMethodOrAccessor` require that Getter/Setters are parented by an ObjectLiteralExpression or ClassExpression
Fixes #45006
thanks, typescript is great