Destructuring declaration prefers type annotation type#25282
Destructuring declaration prefers type annotation type#25282
Conversation
Previously, getTypeForBindingElement would always union the declarations type and the type of the default initializer. Now, if the declaration has a type annotation, it does not union with the initializer type. The type annotation's type is the one used.
|
|
||
| function parentDeclarationHasTypeAnnotation(binding: BindingElement) { | ||
| let node: Node = binding; | ||
| while (node.parent && isBindingPattern(node.parent)) { |
There was a problem hiding this comment.
node.parent test should be unnecessary.
Following is a bit more strongly typed (no need to test couldHaveType):
let node = binding.parent;
while (isBindingElement(node.parent)) {
node = node.parent.parent;
}
return !!node.parent.type;But actually, shouldn't we no longer access .type and use getEffectiveTypeAnnotationNode instead?
There was a problem hiding this comment.
Good catch on getEffectiveTypeAnnotationNode!
Unfortunately, I tried your proposed change and node ends up 1-too-high after the loop -- that is, it's the node above the ParameterDeclaration in the tests below, so node.parent.type ends up looking for the type two nodes above the ParameterDeclaration. Also I still had to use Node as the type for binding.parent, since node.parent.parent is a big union type that isn't assignable to BindingPattern. The elaboration points out that CallSignatureDeclaration, at least, is wrong.
I did work around the 2-too-high problem by tracking the previous parent on each iteration:
let node: Node = binding.parent;
let decl = binding.parent.parent;
while (isBindingPattern(node)) {
decl = node.parent;
node = node.parent.parent;
}
return !!getEffectiveTypeAnnotationNode(decl);But this looks worse to me than the original.
There was a problem hiding this comment.
That can't be right -- if node.parent.parent is the node above the ParameterDeclaration, then node.parent must be the ParameterDeclaration. But we just tested isBindingElement(node.parent).
Following passed tests when I tried it:
function parentDeclarationHasTypeAnnotation(binding: BindingElement) {
let node = binding.parent;
while (isBindingElement(node.parent)) {
node = node.parent.parent;
}
return !!getEffectiveTypeAnnotationNode(node.parent);
}Node node has a static type of BindingPattern. The node above the ParameterDeclaration would be some kind of SignatureDeclaration.
|
OK, all the tests passed with the refactorings we worked on. I'll continue with modifications to |
| @@ -903,7 +903,7 @@ namespace ts { | |||
|
|
|||
| export function isConst(node: Node): boolean { | |||
There was a problem hiding this comment.
for other readers: all the changes below here are just refactorings based on an improved definition of walkupBindingElementsAndPatterns.
|
isConst/isLet also apply to VariableDeclarationList, which is handled nicely by getCombinedFlags already, so I don't think it's worth refactoring that. @Andy-MS I think I'll push the getCombinedFlags refactoring into this PR since it's not much more churn than is already there. |
Retain the individual functions since they are used a lot.
Previously, getTypeForBindingElement would always union the declaration's type and the type of the default initializer. Now, if the declaration has a type annotation, it does not union with the initializer type. The type annotation's type is the one used.
Fixes #16970, which was broken by #10577. This is a refinement of #10577 in that it checks for a type annotation on the declaration before unioning types.