Parse Private Names #4

mheiber · 2018-07-25T22:08:37Z

Description of Changes

Parser and Scanner:

Add new SyntaxKind: SyntaxKind.PrivateName
Edit baseline reference files to use ¬ (hook) instead of # (hash) to test parser error recovery.
Test for correct parsing
"Property _ does not exist on type _" error should still happen with private names even if the class has an index signature (new test case)

Steps to Test

Build the compiler and services (npm run build:compiler)
Tests should pass. Please review the new test files in the privateNames directory carefully, the rest are minor tweaks that I hope don't matter.
Check the AST. Assuming you have a file in the right place with a private name declaration and access, the AST should have private name nodes in both places.

node -e "ts = require('./built/local/tsserver');visit = node => {console.log(node); ts.forEachChild(node, visit)};visit(ts.createSourceFile('f', fs.readFileSync('../tssample/src/1.ts', 'utf8')))

The binder and checker work for private names in this PR is very primitive, just a start

joeywatts · 2018-07-30T18:58:50Z

src/compiler/factory.ts

@@ -138,6 +138,10 @@ namespace ts {
        : node;
    }

+    export function updatePrivateName(node: PrivateName): PrivateName {


What's the purpose of this function? It doesn't seem to be making a modified PrivateName.

joeywatts · 2018-07-30T19:02:54Z

src/compiler/visitor.ts

@@ -221,6 +221,8 @@ namespace ts {

            case SyntaxKind.Identifier:
                return updateIdentifier(<Identifier>node, nodesVisitor((<Identifier>node).typeArguments, visitor, isTypeNodeOrTypeParameterDeclaration));
+            case SyntaxKind.PrivateName:
+                return updatePrivateName(node as PrivateName);


If no modification is needed here, we can probably remove this case, right?

will fix, good catch

joeywatts · 2018-07-30T19:09:46Z

src/compiler/parser.ts

@@ -1386,6 +1389,17 @@ namespace ts {
            return finishNode(node);
        }

+        function createPrivateName(): PrivateName {


Can we just call this parsePrivateName and get rid of the function below?

following the pattern set by other functions such as createIdentifier() and parseIdentifier()

RyanCavanaugh · 2018-07-30T21:09:35Z

src/compiler/types.ts

@@ -741,6 +742,13 @@ namespace ts {
        expression: Expression;
    }

+    export interface PrivateName extends Declaration {


Should extend Node not Declaration

thanks @RyanCavanaugh ! I'll check this out.

@RyanCavanaugh in binder.ts, I think I need to be able to pass instances of PrivateName as the node parameter to addDeclarationToSymbol(symbol: Symbol, node: Declaration, symbolFlags: SymbolFlags). In order to do this, doesn't PrivateName have to be an identifier?

It's likely I'm missing what a Declaration is and why Identifier extends Declaration.

Ah, I was mistaken on this one. extends Declaration is fine

RyanCavanaugh · 2018-07-30T21:10:07Z

src/compiler/types.ts

@@ -1176,7 +1184,7 @@ namespace ts {

    export interface StringLiteral extends LiteralExpression {
        kind: SyntaxKind.StringLiteral;
-        /* @internal */ textSourceNode?: Identifier | StringLiteralLike | NumericLiteral; // Allows a StringLiteral to get its text from another node (used by transforms).
+        /* @internal */ textSourceNode?: Identifier | PrivateName | StringLiteralLike | NumericLiteral; // Allows a StringLiteral to get its text from another node (used by transforms).


I don't think this case should occur (?)

@RyanCavanaugh thanks for noticing this, I wasn't sure what to do here.
Without the change above (allowing textSourceNode values to be PrivateNames) the factory function for creating literals from nodes doesn't compile:

https://github.com/bloomberg/TypeScript/pull/4/files#diff-9c914fcc05c72a72db62c0a023b15f3fR110

should createLiteralFromNode work for PropertyNameLiterals that are private names?

ping @RyanCavanaugh

createLiteralFromNode shouldn't accept a PrivateName node - unlike property names (whose names are meaningful in isolation), private names can't be used anywhere a literal is needed.

Any place where createLiteral is being called today with a node that is sourced from a property name, we need to add logic to do the right thing for PrivateNames. This should just be mostly asserts to make sure we don't e.g. try to rewrite x.#foo with x["#foo"] (something we currently treat as being always OK).

I incorporated this feedback and disallowed creating literals from private names.

I think the code does what we want now, with these caveats:

added this type alias, but it helped avoid making big changes to the type hierarchy in types.d.ts.

small new TODO in the transformer.

ping @RyanCavanaugh would you be able to take another look?

RyanCavanaugh · 2018-07-30T21:13:33Z

src/compiler/utilities.ts

@@ -2641,8 +2653,8 @@ namespace ts {
        return node.kind === SyntaxKind.Identifier && (<Identifier>node).escapedText === "Symbol";
    }

-    export function isPushOrUnshiftIdentifier(node: Identifier) {
-        return node.escapedText === "push" || node.escapedText === "unshift";
+    export function isPushOrUnshiftIdentifier(node: Identifier | PrivateName) {


Shouldn't be legal to invoke this with a PrivateName

RyanCavanaugh · 2018-07-30T21:16:20Z

src/compiler/utilities.ts

@@ -770,12 +771,21 @@ namespace ts {

    export function entityNameToString(name: EntityNameOrEntityNameExpression): string {
        switch (name.kind) {
+            // fall through


Seems to be have left in unintentionally

RyanCavanaugh · 2018-07-30T21:29:01Z

Possible bug (haven't tried it but there should be a testcase):

class foo {
    [k: string]: any;
    b() {
        // Should error 
        this.#x = 10;
    }
}

mheiber · 2018-08-02T01:48:16Z

@RyanCavanaugh you were right that this doesn't error (but should):

class foo {
    [k: string]: any;
    b() {
        // Should error 
        this.#x = 10;
    }
}

Do you have guidance on what is causing this problem? It seems that we'll need to encode the information that private names are not accessible via indexing with a string.
Since this PR is just for getting private names to parse and the checker work is started in a follow-on PR, what do you think of deferring it? If so, how can I add a "skip" test case?

RyanCavanaugh · 2018-08-02T03:57:14Z

@mheiber you'd probably just want to add a baseline test, which doesn't directly encode any "this should happen" information, so I think it would make sense to just add the testcase and then let the next PR update its baseline outputs

RyanCavanaugh · 2018-08-02T04:05:03Z

As for behavior, in checker.ts see checkPropertyAccessExpressionOrQualifiedName:

            const prop = getPropertyOfType(apparentType, right.escapedText);
            if (isIdentifier(left) && parentSymbol && !(prop && isConstEnumOrConstEnumOnlyModule(prop))) {
                markAliasReferenced(parentSymbol, node);
            }
            if (!prop) {
HERE        ^^^^^^^^^^^^
                const indexInfo = getIndexInfoOfType(apparentType, IndexKind.String);
                if (!(indexInfo && indexInfo.type)) {
                    if (right.escapedText && !checkAndReportErrorForExtendingInterface(node)) {
                        reportNonexistentProperty(right, leftType.flags & TypeFlags.TypeParameter && (leftType as TypeParameter).isThisType ? apparentType : leftType);
                    }
                    return errorType;
                }
                if (indexInfo.isReadonly && (isAssignmentTarget(node) || isDeleteTarget(node))) {
                    error(node, Diagnostics.Index_signature_in_type_0_only_permits_reading, typeToString(apparentType));
                }
                propType = indexInfo.type;
            }

Here, we get prop by looking up the dotted name. If we don't find anything, then there's no declared property with this name. When that happens, we check for an index signature, and if there's a suitable string index signature, we act as if there's a property with that name of the type of the index signature.

The diff should be approximately as simple as

if (!prop && /* the property name isn't a private name */) {
          +++++++++++++++++++++++++++++++++++++++++++++++

joeywatts · 2018-08-16T19:15:57Z

src/compiler/transformers/destructuring.ts

@@ -535,7 +535,8 @@ namespace ts {
        const propertyNames: Expression[] = [];
        let computedTempVariableOffset = 0;
        for (let i = 0; i < elements.length - 1; i++) {
-            const propertyName = getPropertyNameOfBindingOrAssignmentElement(elements[i]);
+            // can safely assume property name is not a PrivateName because PrivateNames are nto allowed in object literals


typo, "PrivateNames are not allowed in object literals"

Neuroboy23 · 2018-08-17T18:33:55Z

src/compiler/utilities.ts

@@ -2642,7 +2653,7 @@ namespace ts {
    }

    export function isPushOrUnshiftIdentifier(node: Identifier) {
-        return node.escapedText === "push" || node.escapedText === "unshift";
+        return isIdentifier(node) && node.escapedText === "push" || node.escapedText === "unshift";


Isn't isIdentifier(node) always true here? If not, this expression is not correct. It should be isIdentifier(node) && (node.escapedText === "push" || node.escapedText === "unshift).

nice catch. True, the node will always be an identifier, fixed

joeywatts · 2018-08-21T18:42:38Z

src/compiler/factory.ts

+    /**
+     * accessor declaration that can be converted to an expression (`name` field cannot be a `PrivateName`)
+     */
+    type ExpressionableAccessorDeclaration = AccessorDeclaration & {name: Exclude<PropertyName, PrivateName>};


Might be more consistent to define like this:

interface ExpressionableAccessorDeclaration extends AccessorDeclaration { name: Exclude<PropertyName, PrivateName>; }

Yours is easier on the eyes, for sure. But since AccessorDeclaration is a type it seems more consistent to me for ExpressionableAccessorDeclaration to be a type.

and check that private names not used in parameters Signed-off-by: Max Heiber <max.heiber@gmail.com>

mheiber · 2018-08-27T00:51:31Z

src/compiler/binder.ts

@@ -266,6 +266,9 @@ namespace ts {
                    Debug.assert(isWellKnownSymbolSyntactically(nameExpression));
                    return getPropertyNameForKnownSymbolName(idText((<PropertyAccessExpression>nameExpression).name));
                }
+                if (isPrivateName(node)) {
+                    return nodePosToString(node) as __String;
+                }


ignore this: it's a mistake and to be removed in #5. Actually has no effect, since there is no way the node can be a private name here anyway.

mheiber · 2018-08-27T00:53:55Z

src/compiler/checker.ts

@@ -21876,6 +21878,9 @@ namespace ts {
            checkGrammarDecoratorsAndModifiers(node);

            checkVariableLikeDeclaration(node);
+            if (node.name && isIdentifier(node.name) && node.name.originalKeywordKind === SyntaxKind.PrivateName) {
+                error(node, Diagnostics.Private_names_cannot_be_used_as_parameters);


we plan to undo this change is in #5

mheiber · 2018-08-27T00:54:13Z

src/compiler/checker.ts

@@ -29288,6 +29293,10 @@ namespace ts {
                checkESModuleMarker(node.name);
            }

+            if (isIdentifier(node.name) && node.name.originalKeywordKind === SyntaxKind.PrivateName) {
+                return grammarErrorOnNode(node.name, Diagnostics.Private_names_are_not_allowed_in_variable_declarations);
+            }


we plan to undo this change in #5

mheiber · 2018-08-27T00:56:22Z

src/compiler/transformers/destructuring.ts

@@ -535,7 +535,8 @@ namespace ts {
        const propertyNames: Expression[] = [];
        let computedTempVariableOffset = 0;
        for (let i = 0; i < elements.length - 1; i++) {
-            const propertyName = getPropertyNameOfBindingOrAssignmentElement(elements[i]);
+            // can safely assume property name is not a PrivateName because PrivateNames are not allowed in object literals
+            const propertyName = getPropertyNameOfBindingOrAssignmentElement(elements[i]) as Exclude<PropertyName, PrivateName>;


should I add a Debug.assert here? Don't want this to be a bug vector if private names are eventually allowed in object literals

Note that these annotations are parsed by the tool in Microsoft/vscode-gdpr-tooling; the associated PR #4 adds Typescript to what the tool processes.

* Add baselines * Update baselines Signed-off-by: Joseph Watts <jwatts43@bloomberg.net>

* wip: add types * wip * Add cases * Add some case * Add more check * accept baseline * add abstract abd declare method * add override in declaration * accept baseline * add property override * Fix decalre modifier * update baseline * Add more cases * make lint happy * make lint happy * Update description * Add codefix * simplify code * accept baseline * Update desc * Accept baseline * Add override completions * filter out implements field in override context * fix tests * Add parameter property check * Accept baseline * acept baseline * Add parameter property to declaration code action * Add quickfix for override parameter property * fix code style * Add override with interface tests * Add more cases about modifier position * rename flag * rename flags * Added tests. * Accepted baselines. * Always issue errors for unnecessary 'override' modifiers. * Accepted baselines. * Override perf (#4) * try cache check result * pre check for override * Do not issue error if implement abstract * Add abstract-spec check * Avoid override dead lock * Add more case * Add codefix for new error * Fix error message * Add jsdoc override tag (#6) * Override jsdoc tag (#7) * accept baseline * Disallow codefix in js * update baseline * Omit override in d.ts * Add more case in js * Accept baseline * fix override js test * fix crlf * Revert merge conflict changes * Accept baseline * Avoid space * Fix CR issues * Accept baseline * Fix typo and add more check * Fix error name Co-authored-by: Daniel Rosenwasser <Daniel.Rosenwasser@microsoft.com>

mheiber force-pushed the syntaxkind branch from 5dd45c6 to 9af9da8 Compare July 25, 2018 22:13

mheiber requested review from joeywatts, molisani, robpalme, Neuroboy23 and ramin25 July 25, 2018 22:15

joeywatts suggested changes Jul 30, 2018

View reviewed changes

mheiber force-pushed the syntaxkind branch 5 times, most recently from 0fda1ac to b385921 Compare July 30, 2018 20:29

RyanCavanaugh reviewed Jul 30, 2018

View reviewed changes

mheiber force-pushed the syntaxkind branch 2 times, most recently from 8051b35 to 84bc7e9 Compare August 3, 2018 01:33

mheiber force-pushed the syntaxkind branch from a3cc9da to e28bfb4 Compare August 15, 2018 20:00

joeywatts suggested changes Aug 16, 2018

View reviewed changes

Neuroboy23 reviewed Aug 17, 2018

View reviewed changes

mheiber force-pushed the syntaxkind branch 3 times, most recently from 7a27449 to 684c955 Compare August 21, 2018 16:22

Neuroboy23 previously approved these changes Aug 21, 2018

View reviewed changes

joeywatts reviewed Aug 21, 2018

View reviewed changes

joeywatts previously approved these changes Aug 21, 2018

View reviewed changes

Neuroboy23 previously approved these changes Aug 23, 2018

View reviewed changes

mheiber dismissed stale reviews from Neuroboy23 and joeywatts via 065af59 August 23, 2018 19:48

mheiber force-pushed the syntaxkind branch 2 times, most recently from 065af59 to 1ad69a0 Compare August 23, 2018 19:52

mheiber changed the title ~~Parse Private Names~~ [WIP] Parse Private Names Aug 23, 2018

mheiber closed this Aug 23, 2018

mheiber reopened this Aug 23, 2018

mheiber force-pushed the syntaxkind branch 5 times, most recently from f754933 to a3a4098 Compare August 23, 2018 20:23

Parse Private Names

548c9f8

and check that private names not used in parameters Signed-off-by: Max Heiber <max.heiber@gmail.com>

mheiber force-pushed the syntaxkind branch from a3a4098 to 548c9f8 Compare August 23, 2018 20:24

mheiber changed the title ~~[WIP] Parse Private Names~~ Parse Private Names Aug 23, 2018

Neuroboy23 approved these changes Aug 23, 2018

View reviewed changes

joeywatts approved these changes Aug 23, 2018

View reviewed changes

mheiber requested a review from joeywatts August 23, 2018 21:36

joeywatts approved these changes Aug 23, 2018

View reviewed changes

Neuroboy23 merged commit 0b8c723 into bloomberg:es-private-fields Aug 23, 2018

mheiber mentioned this pull request Aug 23, 2018

All Bloomberg Changes for Private Fields and Methods #6

Closed

mheiber commented Aug 27, 2018

View reviewed changes

mheiber pushed a commit that referenced this pull request Sep 20, 2018

Add GDPR annotations (microsoft#27217)

80045ca

Note that these annotations are parsed by the tool in Microsoft/vscode-gdpr-tooling; the associated PR #4 adds Typescript to what the tool processes.

mheiber pushed a commit that referenced this pull request Nov 15, 2018

Baseline changes for ESNext Class Property Transformation (#4)

861936e

* Add baselines * Update baselines Signed-off-by: Joseph Watts <jwatts43@bloomberg.net>

Parse Private Names #4

Parse Private Names #4

Uh oh!

Conversation

mheiber commented Jul 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of Changes

Steps to Test

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!

mheiber Jul 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

mheiber Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

mheiber Aug 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

RyanCavanaugh commented Jul 30, 2018

Uh oh!

mheiber commented Aug 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RyanCavanaugh commented Aug 2, 2018

Uh oh!

RyanCavanaugh commented Aug 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neuroboy23 Aug 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

mheiber Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

mheiber commented Jul 25, 2018 •

edited

Loading

mheiber Jul 30, 2018 •

edited

Loading

mheiber Jul 31, 2018 •

edited

Loading

mheiber Aug 5, 2018 •

edited

Loading

mheiber commented Aug 2, 2018 •

edited

Loading

Neuroboy23 Aug 17, 2018 •

edited

Loading

mheiber Aug 22, 2018 •

edited

Loading