Skip to content

Destructuring declaration prefers type annotation type #25282

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

Merged
merged 5 commits into from
Jun 28, 2018

Conversation

sandersn
Copy link
Member

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.

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.
@sandersn sandersn requested review from RyanCavanaugh and a user June 27, 2018 21:18
getUnionType([type, checkExpressionCached(declaration.initializer)], UnionReduction.Subtype) :
type;
}

function parentDeclarationHasTypeAnnotation(binding: BindingElement) {
let node: Node = binding;
while (node.parent && isBindingPattern(node.parent)) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@sandersn
Copy link
Member Author

OK, all the tests passed with the refactorings we worked on. I'll continue with modifications to isConst and getCombinedModifierFlags in a new PR.

@@ -903,7 +903,7 @@ namespace ts {

export function isConst(node: Node): boolean {
Copy link
Member Author

@sandersn sandersn Jun 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for other readers: all the changes below here are just refactorings based on an improved definition of walkupBindingElementsAndPatterns.

@sandersn
Copy link
Member Author

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.
@sandersn sandersn merged commit 5c2eeb2 into master Jun 28, 2018
@sandersn sandersn deleted the destructuring-parameter-prefers-type-annotation branch June 28, 2018 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant