Fix get symbol at location to behave correctly for parameter assignments and jsx attributes by weswigham · Pull Request #20706 · microsoft/TypeScript

weswigham · 2017-12-14T22:36:35Z

Fixes #19041

The corrected type baselines have exposed another bug - note all the literal types in TSX files (opened #20705 to track it).

…nts and jsx attributes

sandersn

Some minor questions left, although it turns out you either answered or fixed a lot of them.

sandersn · 2017-12-15T18:11:01Z

        // Return the type of a binding element parent. We check SymbolLinks first to see if a type has been
        // assigned by contextual typing.
-        function getTypeForBindingElementParent(node: VariableLikeDeclaration) {
+        function getTypeForBindingElementParent(node: BindingElement["parent"]["parent"]) {


This is too cute + too obscure for me. Since it appears to be equivalent to VariableDeclaration | ParameterDeclaration | BindingElement, why not use that or create an alias for it?

I wanted to use BindingElement["parent"]["parent"] so it stays in sync with changes to both binding elements' types and their parents' types - so an alias it is.

sandersn · 2017-12-15T18:13:54Z

            case SyntaxKind.Parameter:
+                return visitNodes(cbNode, cbNodes, node.decorators) ||
+                    visitNodes(cbNode, cbNodes, node.modifiers) ||
+                    visitNode(cbNode, (<ParameterDeclaration>node).dotDotDotToken) ||


why can't these be shared any more? Are there order conflicts in the current, shared version?

The union won't allow access to properties that don't exist on every node. Also, this is less polymorphic, so should have slightly better performance.

sandersn · 2017-12-15T18:14:48Z

-        type?: TypeNode;
-        initializer?: Expression;
-    }
+    export type VariableLikeDeclaration =


performance? I remember doing this (or similar) and found it added quite a bit of time to our build.

There's no noticeable difference in self-compilation (maybe slightly better? The difference is in the noise.).

sandersn · 2017-12-15T18:16:03Z

    interface JSDocContainer {
    }
    type HasJSDoc = ParameterDeclaration | CallSignatureDeclaration | ConstructSignatureDeclaration | MethodSignature | PropertySignature | ArrowFunction | ParenthesizedExpression | SpreadAssignment | ShorthandPropertyAssignment | PropertyAssignment | FunctionExpression | LabeledStatement | ExpressionStatement | VariableStatement | FunctionDeclaration | ConstructorDeclaration | MethodDeclaration | PropertyDeclaration | AccessorDeclaration | ClassLikeDeclaration | InterfaceDeclaration | TypeAliasDeclaration | EnumMember | EnumDeclaration | ModuleDeclaration | ImportEqualsDeclaration | IndexSignatureDeclaration | FunctionTypeNode | ConstructorTypeNode | JSDocFunctionType | EndOfFileToken;
+    type HasType = SignatureDeclaration | VariableDeclaration | ParameterDeclaration | PropertySignature | PropertyDeclaration | TypePredicateNode | ParenthesizedTypeNode | TypeOperatorNode | MappedTypeNode | AssertionExpression | TypeAliasDeclaration | JSDocTypeExpression | JSDocNonNullableType | JSDocNullableType | JSDocOptionalType | JSDocVariadicType;


are these aliases meant to be public? I guess maybe they are required for export to services/

I wanted to make them public (like HasJSDoc), because knowing what nodes have a .type or a .initializer is a useful set of nodes, IMO. It can be internal, since I don't think any public functions use them?

I don't care much either way, just checking.

sandersn · 2017-12-15T18:16:56Z


  /** @type {string|undefined} */
  foo: undefined,
->foo : string | undefined


this change seems incorrect

fix'd since this comment was first submitted

sandersn · 2017-12-15T18:22:01Z


 for ({name: nameA } of robots) {
 >{name: nameA } : { name: string; }
->name : Robot


this change is confusing. Either the baseline was wrong before or it's wrong now. I think it was wrong before.

It was wrong before.

sandersn · 2017-12-15T18:27:57Z

 verify.referenceGroups(r4, [
    { definition: "(property) I.property1: number", ranges: [r0, r1, r2, r3] },
-    { definition: "(property) property1: I", ranges: [r4] }
+    { definition: "(property) property1: any", ranges: [r4] }


This is confusing. What is correct here? any shouldn't be it, but I don't think I is correct either.

I was very wrong. any is at least the type of p2, which is bound to the same symbol as that property because the object literal at this location isn't being correctly interpreted as a binding pattern in all locations. This PR is like a partial fix for that (now the type isn't completely incorrect, at least), but the symbols still need to be bound up correctly. IMO, the root cause is object literal/binding pattern confusion causing us to not handle binding patterns correctly in all the places we need to. I'd like to do a more directed fix at making binding patterns actually appear in expression positions instead of object literals when they're actually binding patterns, and use that to fix this.

For now, this is much better, though.

sandersn · 2017-12-15T18:38:38Z

                    case SyntaxKind.Parameter:
                    case SyntaxKind.VariableDeclaration:
-                        return node === (<VariableLikeDeclaration>parent).type;
+                        return node === (parent as Node & { type: Node }).type;


:( parsing for humans (looks like (parent as Node) & { type: Node } at first glance)

So wouldn't parent as HasType work too? It would be easier to read.

Or just parent as TypeContainer.

sandersn · 2017-12-15T18:43:03Z

+    /** True if has type node attached to it. */
+    /* @internal */
+    export function hasType(node: Node): node is HasType {
+        return !!(node as TypeContainer).type;


I would drop TypeContainer and just use any.

weswigham · 2017-12-15T22:36:09Z

@sandersn Have I addressed all of your comments?

Fix get symbol at location to behave correctly got parameter assignme…

fdded50

…nts and jsx attributes

weswigham requested review from DanielRosenwasser and sandersn December 14, 2017 22:36

weswigham added 4 commits December 14, 2017 17:52

Handle all those edge cases that weren;t explicitly handled

fa85c9b

Fix part of bug WRT getTypeAtLocation and for loops

f53c795

Merge branch 'master' into get-symbol-at-location-fix

f9e67be

Baseline corrections pursuant to microsoft#20710

7405a9c

weswigham force-pushed the get-symbol-at-location-fix branch from cafb3c5 to 7405a9c Compare December 15, 2017 18:17

weswigham added 2 commits December 15, 2017 10:33

Restore jsdoc tag interpretation

be49853

Clean up some code, revert fourslash tests

f947fbf

sandersn requested changes Dec 15, 2017

View reviewed changes

Cleanup types used by PR feedback

07c7c6d

sandersn approved these changes Dec 15, 2017

View reviewed changes

weswigham merged commit dd933f4 into microsoft:master Dec 15, 2017

weswigham deleted the get-symbol-at-location-fix branch December 15, 2017 23:50

This was referenced Dec 16, 2017

Fix devtools test #20731

Merged

Add method signature handler to getTypeOfVariableOrParameterOrProperty #20825

Merged

microsoft locked and limited conversation to collaborators Jun 14, 2018

Conversation

weswigham commented Dec 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sandersn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weswigham commented Dec 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

weswigham commented Dec 14, 2017 •

edited

Loading