Disallow PrivateIdentifier in Optional Chains

mheiber · mheiber · commit c85ae74a255c · 2019-10-25T13:10:47.000+01:00
diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
@@ -22920,6 +22920,10 @@ namespace ts {
             const assignmentKind = getAssignmentTargetKind(node);
             const apparentType = getApparentType(assignmentKind !== AssignmentKind.None || isMethodAccessForCall(node) ? getWidenedType(leftType) : leftType);
             if (isPrivateIdentifier(right)) {
+                if (isOptionalChain(node)) {
+                    grammarErrorOnNode(right, Diagnostics.An_optional_chain_cannot_contain_private_identifiers);
+                    return anyType;
+                }
                 checkExternalEmitHelpers(node, ExternalEmitHelpers.ClassPrivateFieldGet);
             }
             const isAnyLike = isTypeAny(apparentType) || apparentType === silentNeverType;
@@ -23301,9 +23305,14 @@ namespace ts {
                 return true;
             }
             const prop = getPropertyOfType(type, propertyName);
-            return prop ? checkPropertyAccessibility(node, isSuper, type, prop)
-                // In js files properties of unions are allowed in completion
-                : isInJSFile(node) && (type.flags & TypeFlags.Union) !== 0 && (<UnionType>type).types.some(elementType => isValidPropertyAccessWithType(node, isSuper, propertyName, elementType));
+            if (prop) {
+                if (isOptionalChain(node) && isPrivateIdentifierPropertyDeclaration(prop.valueDeclaration)) {
+                    return false;
+                }
+                return checkPropertyAccessibility(node, isSuper, type, prop);
+            }
+            // In js files properties of unions are allowed in completion
+            return isInJSFile(node) && (type.flags & TypeFlags.Union) !== 0 && (<UnionType>type).types.some(elementType => isValidPropertyAccessWithType(node, isSuper, propertyName, elementType));
         }
 
         /**
diff --git a/src/compiler/diagnosticMessages.json b/src/compiler/diagnosticMessages.json
@@ -5366,5 +5366,9 @@
     "Private identifiers are only available when targeting ECMAScript 2015 and higher.": {
         "category": "Error",
         "code": 18028
+    },
+    "An optional chain cannot contain private identifiers.": {
+        "category": "Error",
+        "code": 18029
     }
 }
diff --git a/src/compiler/factory.ts b/src/compiler/factory.ts
@@ -115,7 +115,7 @@ namespace ts {
         return node;
     }
 
-    function createLiteralFromNode(sourceNode: PropertyNameLiteral): StringLiteral {
+    function createLiteralFromNode(sourceNode: Exclude<PropertyNameLiteral, PrivateIdentifier>): StringLiteral {
         const node = createStringLiteral(getTextOfIdentifierOrLiteral(sourceNode));
         node.textSourceNode = sourceNode;
         return node;
diff --git a/src/compiler/parser.ts b/src/compiler/parser.ts
@@ -4655,7 +4655,8 @@ namespace ts {
             const propertyAccess = <PropertyAccessExpression>createNode(SyntaxKind.PropertyAccessExpression, expression.pos);
             propertyAccess.expression = expression;
             propertyAccess.questionDotToken = questionDotToken;
-            propertyAccess.name = parseRightSideOfDot(/*allowIdentifierNames*/ true, /*allowPrivateIdentifiers*/ !questionDotToken);
+            // checker will error on private identifiers in optional chains, so don't have to catch them here
+            propertyAccess.name = parseRightSideOfDot(/*allowIdentifierNames*/ true, /*allowPrivateIdentifiers*/ true);
             if (questionDotToken || expression.flags & NodeFlags.OptionalChain) {
                 propertyAccess.flags |= NodeFlags.OptionalChain;
             }
diff --git a/src/compiler/transformers/esnext.ts b/src/compiler/transformers/esnext.ts
@@ -42,8 +42,13 @@ namespace ts {
         function flattenChain(chain: OptionalChain) {
             const links: OptionalChain[] = [chain];
             while (!chain.questionDotToken && !isTaggedTemplateExpression(chain)) {
-                chain = cast(chain.expression, isOptionalChain);
-                links.unshift(chain);
+                if (isOptionalChain(chain.expression)) {
+                    chain = chain.expression;
+                    links.unshift(chain);
+                }
+                else {
+                    return { expression: chain.expression, chain: links };
+                }
             }
             return { expression: chain.expression, chain: links };
         }
diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts
@@ -2957,10 +2957,11 @@ namespace ts {
         }
     }
 
-    export type PropertyNameLiteral = Identifier | StringLiteralLike | NumericLiteral;
+    export type PropertyNameLiteral = Identifier | PrivateIdentifier | StringLiteralLike | NumericLiteral;
     export function isPropertyNameLiteral(node: Node): node is PropertyNameLiteral {
         switch (node.kind) {
             case SyntaxKind.Identifier:
+            case SyntaxKind.PrivateIdentifier:
             case SyntaxKind.StringLiteral:
             case SyntaxKind.NoSubstitutionTemplateLiteral:
             case SyntaxKind.NumericLiteral:
@@ -2970,11 +2971,23 @@ namespace ts {
         }
     }
     export function getTextOfIdentifierOrLiteral(node: PropertyNameLiteral): string {
-        return node.kind === SyntaxKind.Identifier ? idText(node) : node.text;
+        switch (node.kind) {
+            case SyntaxKind.Identifier:
+            case SyntaxKind.PrivateIdentifier:
+                return idText(node);
+            default:
+                return node.text;
+        }
     }
 
     export function getEscapedTextOfIdentifierOrLiteral(node: PropertyNameLiteral): __String {
-        return node.kind === SyntaxKind.Identifier ? node.escapedText : escapeLeadingUnderscores(node.text);
+        switch(node.kind) {
+            case SyntaxKind.Identifier:
+            case SyntaxKind.PrivateIdentifier:
+                return node.escapedText;
+            default:
+                return escapeLeadingUnderscores(node.text);
+        }
     }
 
     export function getPropertyNameForUniqueESSymbol(symbol: Symbol): __String {
diff --git a/tests/baselines/reference/privateIdentifierChain.1.errors.txt b/tests/baselines/reference/privateIdentifierChain.1.errors.txt
@@ -0,0 +1,28 @@
+tests/cases/conformance/expressions/optionalChaining/privateIdentifierChain/privateIdentifierChain.1.ts(8,15): error TS18029: An optional chain cannot contain private identifiers.
+tests/cases/conformance/expressions/optionalChaining/privateIdentifierChain/privateIdentifierChain.1.ts(9,9): error TS2532: Object is possibly 'undefined'.
+tests/cases/conformance/expressions/optionalChaining/privateIdentifierChain/privateIdentifierChain.1.ts(9,17): error TS18029: An optional chain cannot contain private identifiers.
+tests/cases/conformance/expressions/optionalChaining/privateIdentifierChain/privateIdentifierChain.1.ts(10,22): error TS18029: An optional chain cannot contain private identifiers.
+
+
+==== tests/cases/conformance/expressions/optionalChaining/privateIdentifierChain/privateIdentifierChain.1.ts (4 errors) ====
+    class A {
+        a?: A
+        #b?: A;
+        getA(): A {
+            return new A();
+        }
+        constructor() {
+            this?.#b;           // Error
+                  ~~
+!!! error TS18029: An optional chain cannot contain private identifiers.
+            this?.a.#b;         // Error
+            ~~~~~~~
+!!! error TS2532: Object is possibly 'undefined'.
+                    ~~
+!!! error TS18029: An optional chain cannot contain private identifiers.
+            this?.getA().#b;    // Error
+                         ~~
+!!! error TS18029: An optional chain cannot contain private identifiers.
+        }
+    }
+    
diff --git a/tests/baselines/reference/privateIdentifierChain.1.js b/tests/baselines/reference/privateIdentifierChain.1.js
@@ -0,0 +1,28 @@
+//// [privateIdentifierChain.1.ts]
+class A {
+    a?: A
+    #b?: A;
+    getA(): A {
+        return new A();
+    }
+    constructor() {
+        this?.#b;           // Error
+        this?.a.#b;         // Error
+        this?.getA().#b;    // Error
+    }
+}
+
+
+//// [privateIdentifierChain.1.js]
+"use strict";
+class A {
+    constructor() {
+        this?.#b; // Error
+        this?.a.#b; // Error
+        this?.getA().#b; // Error
+    }
+    #b;
+    getA() {
+        return new A();
+    }
+}
diff --git a/tests/baselines/reference/privateIdentifierChain.1.symbols b/tests/baselines/reference/privateIdentifierChain.1.symbols
@@ -0,0 +1,35 @@
+=== tests/cases/conformance/expressions/optionalChaining/privateIdentifierChain/privateIdentifierChain.1.ts ===
+class A {
+>A : Symbol(A, Decl(privateIdentifierChain.1.ts, 0, 0))
+
+    a?: A
+>a : Symbol(A.a, Decl(privateIdentifierChain.1.ts, 0, 9))
+>A : Symbol(A, Decl(privateIdentifierChain.1.ts, 0, 0))
+
+    #b?: A;
+>#b : Symbol(A.#b, Decl(privateIdentifierChain.1.ts, 1, 9))
+>A : Symbol(A, Decl(privateIdentifierChain.1.ts, 0, 0))
+
+    getA(): A {
+>getA : Symbol(A.getA, Decl(privateIdentifierChain.1.ts, 2, 11))
+>A : Symbol(A, Decl(privateIdentifierChain.1.ts, 0, 0))
+
+        return new A();
+>A : Symbol(A, Decl(privateIdentifierChain.1.ts, 0, 0))
+    }
+    constructor() {
+        this?.#b;           // Error
+>this : Symbol(A, Decl(privateIdentifierChain.1.ts, 0, 0))
+
+        this?.a.#b;         // Error
+>this?.a : Symbol(A.a, Decl(privateIdentifierChain.1.ts, 0, 9))
+>this : Symbol(A, Decl(privateIdentifierChain.1.ts, 0, 0))
+>a : Symbol(A.a, Decl(privateIdentifierChain.1.ts, 0, 9))
+
+        this?.getA().#b;    // Error
+>this?.getA : Symbol(A.getA, Decl(privateIdentifierChain.1.ts, 2, 11))
+>this : Symbol(A, Decl(privateIdentifierChain.1.ts, 0, 0))
+>getA : Symbol(A.getA, Decl(privateIdentifierChain.1.ts, 2, 11))
+    }
+}
+
diff --git a/tests/baselines/reference/privateIdentifierChain.1.types b/tests/baselines/reference/privateIdentifierChain.1.types
@@ -0,0 +1,37 @@
+=== tests/cases/conformance/expressions/optionalChaining/privateIdentifierChain/privateIdentifierChain.1.ts ===
+class A {
+>A : A
+
+    a?: A
+>a : A | undefined
+
+    #b?: A;
+>#b : A | undefined
+
+    getA(): A {
+>getA : () => A
+
+        return new A();
+>new A() : A
+>A : typeof A
+    }
+    constructor() {
+        this?.#b;           // Error
+>this?.#b : any
+>this : this
+
+        this?.a.#b;         // Error
+>this?.a.#b : any
+>this?.a : A | undefined
+>this : this
+>a : A | undefined
+
+        this?.getA().#b;    // Error
+>this?.getA().#b : any
+>this?.getA() : A
+>this?.getA : () => A
+>this : this
+>getA : () => A
+    }
+}
+
diff --git a/tests/cases/conformance/expressions/optionalChaining/privateIdentifierChain/privateIdentifierChain.1.ts b/tests/cases/conformance/expressions/optionalChaining/privateIdentifierChain/privateIdentifierChain.1.ts
@@ -0,0 +1,15 @@
+// @strict: true
+// @target: esnext
+
+class A {
+    a?: A
+    #b?: A;
+    getA(): A {
+        return new A();
+    }
+    constructor() {
+        this?.#b;           // Error
+        this?.a.#b;         // Error
+        this?.getA().#b;    // Error
+    }
+}
diff --git a/tests/cases/fourslash/completionAfterQuestionDot.ts b/tests/cases/fourslash/completionAfterQuestionDot.ts
@@ -1,20 +1,43 @@
 /// <reference path="fourslash.ts" />
 // @strict: true
 
-//// interface User {
+//// class User {
+////     #foo: User;
+////     bar: User;
 ////     address?: {
 ////         city: string;
 ////         "postal code": string;
+////     };
+////     constructor() {
+////         this.address[|?./*1*/|];
+////         this[|?./*2*/|];
+////         this?.bar[|?./*3*/|];
 ////     }
 //// };
-//// declare const user: User;
-//// user.address[|?./**/|]
 
 verify.completions({
-    marker: "",
+    marker: "1",
     exact: [
         { name: "city", text: "(property) city: string" },
         { name: "postal code", text: "(property) \"postal code\": string", insertText: "?.[\"postal code\"]", replacementSpan: test.ranges()[0] }
     ],
     preferences: { includeInsertTextCompletions: true },
 });
+
+verify.completions({
+    marker: "2",
+    exact: [
+        { name: "bar" },
+        { name: "address" }
+    ],
+    preferences: { includeInsertTextCompletions: true },
+});
+
+verify.completions({
+    marker: "3",
+    exact: [
+        { name: "bar" },
+        { name: "address" }
+    ],
+    preferences: { includeInsertTextCompletions: true },
+});

Original file line number	Diff line number	Diff line change
`@@ -5366,5 +5366,9 @@`
`5366`	`5366`	`"Private identifiers are only available when targeting ECMAScript 2015 and higher.": {`
`5367`	`5367`	`"category": "Error",`
`5368`	`5368`	`"code": 18028`
	`5369`	`+ },`
	`5370`	`+ "An optional chain cannot contain private identifiers.": {`
	`5371`	`+ "category": "Error",`
	`5372`	`+ "code": 18029`
`5369`	`5373`	`}`
`5370`	`5374`	`}`
Original file line number	Diff line number	Diff line change
`@@ -115,7 +115,7 @@ namespace ts {`
`115`	`115`	`return node;`
`116`	`116`	`}`
`117`	`117`
`118`		`- function createLiteralFromNode(sourceNode: PropertyNameLiteral): StringLiteral {`
	`118`	`+ function createLiteralFromNode(sourceNode: Exclude<PropertyNameLiteral, PrivateIdentifier>): StringLiteral {`
`119`	`119`	`const node = createStringLiteral(getTextOfIdentifierOrLiteral(sourceNode));`
`120`	`120`	`node.textSourceNode = sourceNode;`
`121`	`121`	`return node;`