Get name of declaration uniformly, even for JS-style assignment declarations. #15594

sandersn · 2017-05-04T23:16:31Z

JS-style assignment declarations have a name, but it's not stored on the name property of the BinaryExpression. This commit adds getNameOfDeclaration to uniformly get the name of a declaration.

It also reworks the declaration of Declaration so that accessing name is an error unless the type does not include BinaryExpression.

The implementation is crude, especially the reworking of the type definitions. Ideas for improvement are welcome.

Improves the fix for #15586

JS-style assignment declarations have a name, but it's not stored on the name property of the BinaryExpression. This commit adds `getNameOfDeclaration` to uniformly get the name of a declaration. It also reworks the declaration of `Declaration` so that accessing `name` is an error unless the type does *not* include BinaryExpression.

mhegazy · 2017-05-04T23:37:25Z

src/compiler/checker.ts

                    return;
                }

+                let errorNode: Node;
+                if (propDeclaration && propDeclaration.kind === SyntaxKind.BinaryExpression) {


i would say it is safe to assume that if it is a BinarayExression, that it is a SpecialPropertyAssignment

yeah, good point. It's unlikely that any other declaration (even a special ModuleExport) would get in here by mistake.

mhegazy · 2017-05-04T23:38:38Z

src/compiler/types.ts

@@ -603,12 +603,15 @@ namespace ts {

    export type DeclarationName = Identifier | StringLiteral | NumericLiteral | ComputedPropertyName | BindingPattern;

-    export interface Declaration extends Node {
+    export interface RealDeclaration extends Node {


can we find a better name for this...

I'm not certain I'm a big fan of renaming this at all.

rbuckton · 2017-05-05T01:11:54Z

src/compiler/types.ts

@@ -603,12 +603,15 @@ namespace ts {

    export type DeclarationName = Identifier | StringLiteral | NumericLiteral | ComputedPropertyName | BindingPattern;

-    export interface Declaration extends Node {
+    export interface RealDeclaration extends Node {


I'm not certain I'm a big fan of renaming this at all.

rbuckton · 2017-05-05T01:14:32Z

src/compiler/utilities.ts

@@ -11,6 +11,36 @@ namespace ts {
        isTypeReferenceDirective?: boolean;
    }

+    export function getNameOfDeclaration(declaration: Declaration): DeclarationName {


Can't we just use the type Declaration | BinaryExpression here? If not I would rather we have a DeclarationLike alias for that rather than renaming Declaration to RealDeclaration.

rbuckton · 2017-05-05T01:15:26Z

src/compiler/utilities.ts

@@ -1994,7 +2024,7 @@ namespace ts {
     *      Symbol.
     */
    export function hasDynamicName(declaration: Declaration): boolean {
-        return declaration.name && isDynamicName(declaration.name);
+        return getNameOfDeclaration(declaration) && isDynamicName(getNameOfDeclaration(declaration));


Inefficient to look this up twice.

rbuckton · 2017-05-05T01:16:32Z

src/compiler/types.ts

        _declarationBrand: any;
        name?: DeclarationName;
    }

-    export interface DeclarationStatement extends Declaration, Statement {
+    // Binary expressions can be declarations if they are 'exports.foo = bar' expressions in JS files
+    export type Declaration = RealDeclaration | BinaryExpression;


Why not name this DeclarationLike?

rbuckton · 2017-05-05T01:18:15Z

src/compiler/checker.ts

@@ -18106,17 +18110,18 @@ namespace ts {

                    forEach(overloads, o => {
                        const deviation = getEffectiveDeclarationFlags(o, flagsToCheck) ^ canonicalFlags;
+                        const name = getNameOfDeclaration(o);


Despite the repetition, calling getNameOfDeclaration(o) everywhere we had o.name is more efficient since we don't look up the name if there are no deviations.

Thanks! Fixed.

rbuckton · 2017-05-05T22:05:16Z

src/compiler/types.ts

+    export type Declaration =
+        | ArrowFunction
+        // Binary expressions can be declarations if they are 'exports.foo = bar' expressions in JS files
+        | BinaryExpression


What do you think about:

export interface JsExportsIdentifier extends Identifier { text: "exports"; } export interface JsExportsPropertyAccess extends PropertyAccessExpression { expression: JsExportsIdentifier; } export interface JsExportsElementAccess extends ElementAccessExpression { expression: JsExportsIdentifier; } export interface JsExportExpression extends AssignmentExpression<Token<SyntaxKind.EqualsToken>>, Declaration { left: JsExportsPropertyAccess | JsExportsElementAccess; }

Then we have a type that represents a specific shape for a BinaryExpression that represents a declaration, rather than a large discriminated union?

This approach would require separate types for this.x = 12 and Foo.x = 12 and Foo.prototype.x assignments, wouldn't it?

For the last two, there's no way to change the inherited type of text, since it can vary, though I think JsPropertyPropertyAccess could just have expression: Identifier.

So this is for more than just 'exports.foo = bar' as per the comment? If that's the case, then disregard. I generally haven't been a fan of us tagging BinaryExpression, PropertyAccessExpression, and CallExpression and NewExpression as Declaration, but that ship has sailed.

rbuckton · 2017-05-05T22:05:22Z

src/compiler/types.ts

-    export type Declaration = RealDeclaration | BinaryExpression;
-
-    export interface DeclarationStatement extends RealDeclaration, Statement {
+    export type Declaration =


How does this large discriminated union affect the compile time of the compiler itself?

Based on a couple of before/after runs just timed using time, the time goes from 46.1 to 47.7 seconds, or 3.3% slower. I think that's OK — @mhegazy do you have an opinion?

rbuckton · 2017-05-05T22:09:19Z

src/compiler/types.ts

@@ -1780,12 +1863,30 @@ namespace ts {
        kind: SyntaxKind.ClassExpression;
    }

-    export interface ClassElement extends RealDeclaration {
+    export interface ClassElement extends DeclarationBase {


I have wanted to make ClassElement and TypeElement discriminated unions (and replace ObjectLiteralElement with ObjectLiteralElementLike), but I've held off due to performance-related issues with respect to discriminated unions.

I will try it and see.

Another nearly-3% slowdown for a combined 106.4% slowdown. On my machine compilation went from 46.1 → 47.7 → 49.0 seconds

I think we should steer clear from large discriminated unions, including this one for Declaration. Is there another way we can structure it? Something like:

interface Declaration extends Node { _declarationBrand: any; } interface DeclarationWithName extends Declaration { name?: DeclarationName; }

Have the expressions inherit from Declaration and have other named declarations inherit from DeclarationWithName?

rbuckton · 2017-05-05T22:09:46Z

src/compiler/types.ts

+            | SyntaxKind.GetAccessor
+            | SyntaxKind.SetAccessor
+            | SyntaxKind.IndexSignature
+            | SyntaxKind.MissingDeclaration;
        _classElementBrand: any;


Could we feasibly remove the brand with this change?

rbuckton · 2017-05-05T22:10:00Z

src/compiler/types.ts

+            | SyntaxKind.IndexSignature
+            | SyntaxKind.MissingDeclaration
+            | SyntaxKind.JSDocPropertyTag
+            | SyntaxKind.JSDocRecordMember;
        _typeElementBrand: any;


Could we feasibly remove the brand with this change?

sandersn · 2017-05-08T15:45:00Z

Switching to a union for Declaration slows the compiler's build time down 3%, and switching all of Declaration, ClassElement and TypeElement slows build time down 6%.

For example, on my machine the build went from 46.1 → 47.7 → 49.0 seconds.

I'm not sure whether either of those steps are worthwhile. Let's discuss with @mhegazy in person today.

Slows down compilation another 3%, so I'm not sure we'll want to take this change.

sandersn · 2017-05-08T18:39:32Z

Sorry @rbuckton I didn't see your earlier reply in the comment thread. Your inheritance-based suggestion seems like a good one.

This reverts commit 22f353a.

This reverts commit b39c319.

This reverts commit 573040f.

This PR included a fix that I already checked in this morning.

sandersn · 2017-05-08T22:41:13Z

OK, the discriminated union is gone in favour of a deeper interface hierarchy.

rbuckton · 2017-05-08T23:31:05Z

src/compiler/types.ts

@@ -1417,7 +1419,7 @@ namespace ts {
    export type EntityNameExpression = Identifier | PropertyAccessEntityNameExpression | ParenthesizedExpression;
    export type EntityNameOrEntityNameExpression = EntityName | EntityNameExpression;

-    export interface PropertyAccessExpression extends MemberExpression, Declaration {
+    export interface PropertyAccessExpression extends MemberExpression, NamedDeclaration {


Why is this a NamedDeclaration?

This one actually does have a name property, so I'll leave it alone.

rbuckton · 2017-05-08T23:31:10Z

src/compiler/types.ts

@@ -1449,7 +1451,7 @@ namespace ts {
        | SuperElementAccessExpression
        ;

-    export interface CallExpression extends LeftHandSideExpression, Declaration {
+    export interface CallExpression extends LeftHandSideExpression, NamedDeclaration {


Why is this a NamedDeclaration?

rbuckton · 2017-05-08T23:31:14Z

src/compiler/types.ts

@@ -1468,7 +1470,7 @@ namespace ts {
        typeArguments?: NodeArray<TypeNode>;
    }

-    export interface NewExpression extends PrimaryExpression, Declaration {
+    export interface NewExpression extends PrimaryExpression, NamedDeclaration {


Why is this a NamedDeclaration?

rbuckton · 2017-05-08T23:31:25Z

src/compiler/types.ts

@@ -1403,7 +1405,7 @@ namespace ts {
     * JSXAttribute or JSXSpreadAttribute. ObjectLiteralExpression, on the other hand, can only have properties of type
     * ObjectLiteralElement (e.g. PropertyAssignment, ShorthandPropertyAssignment etc.)
     */
-    export interface ObjectLiteralExpressionBase<T extends ObjectLiteralElement> extends PrimaryExpression, Declaration {
+    export interface ObjectLiteralExpressionBase<T extends ObjectLiteralElement> extends PrimaryExpression, NamedDeclaration {


Why is this a NamedDeclaration?

No reason! I'll change all the types without a name property to extend Declaration instead.

sandersn · 2017-05-09T17:50:55Z

Node types with no name now correctly extend Declaration directly instead of claiming that they might have a node.

Note that getNameOfDeclaration doesn't handle these node kinds differently. It just returns node.name which will be undefined for them.

mhegazy · 2017-05-10T01:08:40Z

We should port this to release-2.3 as well.

sandersn · 2017-05-10T16:24:55Z

This is now on release-2.3 as well.

ghost · 2017-05-19T02:23:52Z

❗
The public Declaration.name was removed and replaced with an internal getNameOfDeclaration.
This is a breaking change and there's no clear path forward for users.

mhegazy · 2017-05-19T04:30:40Z

@sandersn can we make getNameOfDeclaration part of the public utilities.

sandersn · 2017-05-19T17:06:53Z

Will do. In the meantime, people can still cast Declaration down to NamedDeclaration to get the name property.

mhegazy · 2017-05-19T18:11:42Z

Added note for the breaking change in https://github.com/Microsoft/TypeScript/wiki/API-Breaking-Changes#typescript-23

sandersn added 2 commits May 4, 2017 16:12

Update baselines

aceadeb

msftclas added the cla-not-required label May 4, 2017

mhegazy reviewed May 4, 2017

View reviewed changes

mhegazy requested a review from rbuckton May 4, 2017 23:38

rbuckton requested changes May 5, 2017

View reviewed changes

sandersn added 3 commits May 5, 2017 13:47

Convert Declaration to union, not type hierarchy

573040f

Fix lint

b39c319

Address comments: simplify, dedupe and clean up

6a17532

rbuckton reviewed May 5, 2017

View reviewed changes

Turn ClassElement and TypeElement into unions too

22f353a

Slows down compilation another 3%, so I'm not sure we'll want to take this change.

sandersn mentioned this pull request May 8, 2017

Exception in getSpecialPropertyAssignmentKind when handling || in const value #15616

Closed

sandersn added 3 commits May 8, 2017 14:16

Revert "Turn ClassElement and TypeElement into unions too"

dd94ad2

This reverts commit 22f353a.

Revert "Fix lint"

e81733b

This reverts commit b39c319.

Revert "Convert Declaration to union, not type hierarchy"

acec74b

This reverts commit 573040f.

This was referenced May 8, 2017

Add Javascript declarations to index constraint check error reporting #15586

Merged

Exception in checkIndexConstraintForProperty when handling assignment to property in constructor #15681

Closed

sandersn added 2 commits May 8, 2017 15:07

Switch to Declaration/NamedDeclaration hierarchy

9392342

Merge branch 'master' into get-name-of-declaration-wrapper

0080c5f

This PR included a fix that I already checked in this morning.

rbuckton requested changes May 8, 2017

View reviewed changes

Unnamed nodes now extend Declaration, not NamedDeclaration

157b777

rbuckton approved these changes May 9, 2017

View reviewed changes

sandersn merged commit 3768dae into master May 10, 2017

sandersn deleted the get-name-of-declaration-wrapper branch May 10, 2017 15:08

andy-hanson mentioned this pull request May 19, 2017

Fix failures on typescript@next palantir/tslint#2789

Merged

4 tasks

mhegazy added Breaking Change Would introduce errors in existing code API Relates to the public API for TypeScript labels May 19, 2017

ghost mentioned this pull request May 22, 2017

Clarify that some node types never have name defined. #15567

Closed

microsoft locked and limited conversation to collaborators Jun 14, 2018

Get name of declaration uniformly, even for JS-style assignment declarations. #15594

Get name of declaration uniformly, even for JS-style assignment declarations. #15594

Conversation

sandersn commented May 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn commented May 8, 2017

sandersn commented May 8, 2017

sandersn commented May 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn commented May 9, 2017

mhegazy commented May 10, 2017

sandersn commented May 10, 2017

ghost commented May 19, 2017

mhegazy commented May 19, 2017

sandersn commented May 19, 2017

mhegazy commented May 19, 2017