Add private instance field transformation, pr feedback

Implements private instance fields on top of the class properties refactor.

This commit also includes incorporation of PR feedback on the
checker, parser, and transformer in order to make the rebase
manageable.

Co-Authored-By: Max Heiber <[email protected]>
Co-Authored-By: Ron Buckton <[email protected]>

Signed-off-by: Joey Watts <[email protected]>
Signed-off-by: Max Heiber <[email protected]>

Incorporate PR feedback

Fix checker crash with new block scoped bindings

Add classExpressionInLoop test

Update baselines for private name errors

Apply suggestions from code review

Fix privateNameFieldCallExpression test

Remove unnecessary comment

Fix PropertyAccessEntityNameExpression type

Remove PrivateName from PropertyNameLiteral

Add createPrivateName

Remove PrivateName type from textSourceNode

Add Debug asserts for invalid syntax kinds

Don't output private name syntax when undeclared

Make PrivateName extend Node

Update baselines for public API

Fix completions in language server

Fix fourslash test crash

intern private name descriptions

undo expensive node.name.parent assignment

Back the way things were on `master`: only
assign node.name.parent when we need to

Add tests for private names and JSDoc

Patch hoverOverPrivateName fourslash test

Fix goToDefinition for private-named fields

remove Debug.fail for private name outside class

Remove Debug.fail in binder.ts::`getDeclarationName`.
It turns out this code path *does* get hit (intentionally).

For best error messages, return `undefined` and rely
on the parser generating a good error message

"Private names are not allowed outside class bodies"

Add rbuckton test cases for private names

These test cases specifically exercise where
we needed to use name-mangling. They are
cases where private names have the same
description.

- private names no conflict when inheriting
- private names distinct even when
the two classes have the same name

check dup instance+static private identifiers

class A {
    #foo;
    static #foo;
}

not allowed because static and private names
share the same lexical scope
https://tc39.es/proposal-class-fields/#prod-ClassBody

refactor getPropertyForPrivateName, rel spans

refactor getPropertyForPrivateName so
it is easier to read (use findAncestor instead
of loop).

Fix bugs in getPropertyForPrivateName:
- make it work with deeply-nested classes with
and without shadowing
- make it catch shadowing when the conflict is
between static and instance
private name descriptions (these can actually
clash)

And add related spans to diagnostics
for getPropertyForPrivateName

catch conflicts between static and instance
private identifiers:
- cannot have an instance and static private identifier
  with the same spelling, as there is only one namespace
  for private names

rename 'PrivateName' to 'PrivateIdentifier'

to match the change of wording in the spec
prposal:

https://tc39.es/proposal-class-fields/#sec-syntax

The rename in the spec was to avoid confusion
between the AST Node PrivateIdentifier
and the internal spec type PrivateName.

So one uses the [[Description]] of a
PrivateIdentifier to look up the PrivateName
for a given scope.

This corresponds closely to our implementation
in the binder and checker:
- we mangle PrivateIdentifier.escapedText to
get a `key` which we use to get the symbol
for a property

f

getLiteralTypeFromProperty-check privateIdentifier

rename and simplify privateNameAndAny test case

make it clearer that all this test is showing is
that we allow accessing arbitrary private identifiers
on `any`.

Note: we could have something more sound here by
checking that there is a private-named field declaration
in a class scope above the property access.

Discussion:
https://github.com/microsoft/TypeScript/pull/30829/files#r302760015

Fix typo in checker

s/identifer/identifier

remove accidental change

patch fourslash test broken by isPrivateIdentifier

just needed to add a check to see if the symbol
.name is defined

extract reportUnmatchedProperty

per @nsandersn feedback

propertiesRelatedTo was getting to long

pull out the unmatchedProperty reporting into
a seprate function

fix typo in private names test

Fix type soundness with private names

Remove PrivateIdentifier from emit with Expr hint

Fixup helpers and set function name for privates

remove accidentally-committed baselines

These baselines somehow ended up in this pr,
though they have nothing to do with the changes

Revert "getLiteralTypeFromProperty-check privateIdentifier"

This reverts commit bd1155c.

reason: the check for isIdentifier in
getLiteralTypeFromProperty is superfluous because
we do this check in getLiteralTypeFromPropertyName

Update comment in private name uniqueness test 3

I think the comments were not accurate and that we
export the error on `this.#x = child.#x` because:
- private names are lexically scoped: the code in question is not in a
lexical scope under the definition of Child's #x.
- private names are private to their containing class: never inherited

This expected behavior matches that of Chrome Canary and
my understanding of the spec

test private names use before def, circular ref

test private names use before def, circular ref

update diagnosticMessages s/delete/'delete'

per @DanielRosenwasser and @sandersn guidance,
use this style in diagnostic messages:

"operand of a 'delete' operator" (single quotes)

rather than old style:

"operand of a delete operator" (single quotes)

This is for consistency, as we use the new
style in the privateIdentifiers error messages
and it is consistent with our messages about
other operators

incorporate private names early exit feedback

and code style change to use a ternary
instead of if so we can 'const'

require private-named fields to be declared in JS

All fields must be declared in TS files.

In JS files, we typically do not have this requirement.

So special-case private fields, which must always
be declared (even in JS files, per spec)

update debug failure for es2015 private identifier

Co-Authored-By: Ron Buckton <[email protected]>

fix checker handling of private name in subclasse

update checker and tests to account for the
following ts features:

- private names do not participate in
the prototype chain, but *are* assigned
in the parent class' constructor. So
parent can access its *own* private fields
on instances of the subclass

Add more tests for private-named fields in JS

add test to hit symbolToExpression w private names

symbolToExpression knows about private names
add a test to exercise this code path

ban private-named static methods (not supported yet)

ban private-named methods (not supported yet)

ban private-named accessors (not supported yet)

fix privateIdentifier fourslash test

change assertion throw to return

Co-Authored-By: Ron Buckton <[email protected]>

Update comment in checker.ts re reserved members

Remove case for privateIdentifier in EntityNameExpr

Remove case for privateIdentifier in
EntityNameExpr. That code path is never hit,
and privateIdnetifiers cannot be entity names.

remove unnecessary parentheses

Ban private identifier in enum

As the new test, 'privateNameEnumNoEmit',
shows, the checker now correctly makes
a diagnostic for private identifiers in enums.

However, when noEmit is false we
hit this assertion in the transformer.

This assertion will have to be removed
so that we have a diagnostic here instead
of an assertion error.

When the assertion is removed,
the 'privateNameEnum' baseline
will have to be updated

Fix destructuring assignment, use createCallBinding, remove unneeded helper

Add class private field helpers to external helpers

Remove private identifier enum assertion, use missing identifiers

Fix hash map inefficiency by only using get

Update baselines with empty identifier change

Add privateNameEnum test baselines

Fix private identifier destructuring (array assignment defaults, unique names)

Use createPrivateIdentifierAssignment in destructuring transform

Fix lint error

Separate destructuring target visitor from regular visitor

Fix destructuring assignment with this bindings

Fix destructuring assignment with this bindings

Fix syntax error with destructuring assignment output

Disallow private identifiers in property signatures

remove duplicate test baselines

Add tests for undeclarated private identifiers

remove unnecessary cast

Nicer error message for mis-placed hashbang

Workaround v8 bug with destructured assignments

Optimize private identifier stack lookup

Avoid the cost of performing an array lookup to look at the top of the
private identifier environment stack.

Change function name to be more specific

Changes "getOperatorForCompoundAssignment" to
"getNonAssignmentOperatorForCompoundAssignment" now that this
function is accessible to the entire compiler.

Improve test case for private name assignment

Adds a compound assignment test case for a class with private names
being declared on the left-hand-side of the assignment expression.

Remove extra non-private-field test

Remove isPrivateIdentifierAssignmentExpression helper

Don't transform private names in ESNext target

Preserve private fields initialized to functions

Move function expressions to outer scope for efficiency

Add WeakMap collision check

Modify potential WeakMap collision condition

Fix this property assignment binding with private names

Add correct error message for WeakMap collision

Add error for statements before super with private identifiers

Refactor getPropertyForPrivateIdentifier

Add test for private identifier fields initialized to class exprs

Fix shebang errors

Fix private errors on index signatures

Add codefix for missing private property

Update error messages for unsupported private features

Fix inheritance-related errors

Fixes inheritance-related errors with private identifiers by resolving
properties from base classes. Private identifiers do not show up as
properties on a union type, so those do not type-check.

Add test for interface extending class with private access

Remove debugging log

Remove name assignment from private named functions

Rename to getPrivateIdentifierPropertyOfType

Fix index signature test comment

Fix test target syntax error

Change error messages

patch private identifiers outside class bodies

Add test for private identifier with ooo super

Add test for a class with a private identifier
with a non-preambly (for example, not a comment)
statement before 'super':

should error, saying 'super' must come first

Fix nits

incorporate PR feedback

Incorporate checker feedback

- reorganize if statements in checkFunctionOrConstructorSymbol
- remove overload for getPrivateIdentifierPropertyOfType

reorganize if statements in checkFunctionOrConstructorSymbol

test for private names with JSX

use getPropertyOftype in getPropertyForPrivateIdentifier

getPrivateIdentifierPropertyOfType error on synthetic

make getPrivateIdentifierPropertyOfType  error
if given a node that is not from the parse tree

Simplify checkPrivateIdentifierPropertyAccess

use getPropertiesOfType instead of
rehashing that logic

test for private identifiers w decl merging

fix test target for privateNameDeclarationMerging

update baselines

Fix private identifier ++/-- numeric coercion

Remove 'downleveled' from super error

Fix bad comments in helper call emit

Error on private identifiers in JSX tag names

Add more private identifier tests

Add es2015 target for private name destructured binding test

Add privateNameConstructorSignature test

Add test for navigation bar w private identifier

Remove spurious line from test

// in js file
class A {
    exports.#foo = 3; // should error
}

The above line failed to produce an error
when run using the test harness.

When using tsc or the language server,
we got the expected error message.

Removing the troublesome line, as it
seems to say more about the test runner
than about the code it is testing.

Fix inefficient constructor generation

dts: don't emit type for private-identified field

Do not emit types for private-identified fields
when generating declaration files.

// example.ts
export class A {
    #foo: number;
}

// example.d.ts

export declare class A {
    #foo;
}

**This is not ideal!**

The following would be better:

// example.d.ts

export declare unique class A {
    #foo;
}

See discussion:

microsoft#33038 (comment)

notice when private-identified field unused

Notice when private-identified fields are unused,
and implement the same behavior as for unused
private-modified fields.

This is used in the language server to make things
grayed out.

This case generates an error when --noUnusedLocals
flag is passed to tsc:
    - "<name> is declared but never used"

accept baselines

Revert "dts: don't emit type for private-identified field"

This reverts commit e50305d.

Instead of just excluding the type from private identifier
emit, only emit a single private identifier
per class.

This accomplishes nominality while
hiding implementation detail that
is irrelevant to .d.ts consumers

only emit a single private identifier in dts

In dts emit, emit at most one private identifier,
and rename it to `#private`.

refactor getPrivateIdentifierPropertyOfType

- safer check for wehther is parse tree node
- return undefined when not found (instead of
a Debug.fail)

Incorporate PR feedback

Don't rely on parent pointers in transform

Passes context about whether the postfix unary expression value is
discarded down the tree into the visitPostfixUnaryExpression function.

Remove orphaned test baseline files

remove unreachable if

Check `any`-typed private identified fields

Update private identifier incompatible modifier checks

- disallow 'abstract' with private identifiers
- s/private-named-property/private identifier

Add additional call expression test cases

Fix disallow 'abstract' with private identifier

Static private identifiers not inherited

Including this in the PR for private
instance fields even though static
private identifiers are banned.

Reason: this change
improves quality of error messages,
see test case.

Thanks Ron!

Simplifiy private identifier 'any' type handling

Error on private identifier declarations for ES5/ES3

Bind `this` for private identifier property tagged template literals

Fix target for jsdocPrivateName1 test

Update getPrivateIdentifierPropertyOfType API

Make it easier to use by accepting a string
and location, rather than a PrivateIdentifier.

Thanks Ron!

remove orphaned tests

rename test

remove duplicate tests

Remove unrelated error from test

update diagnostic message 'private identifier'

The nodes for hash private fields are now
called 'private identifier'. Update one last
diagnostic message to use the new terminology.

refine solution for static private identifier fields

- use `continue` instead of `filter` for perf
- better naming
- make it clear the current solution will
need to be altered when we lift the ban on
static private identifier fields, including
a test case that should change in the future

Fix display of private identifiers in lang server

Fix bug where private identifiers in completion
tooltips in the playground were showing up
as the symbol table entries (with underscores and such).

microsoft#30829 (comment)

Author: joeywatts

src/compiler/binder.ts

@@ -356,15 +356,15 @@ namespace ts {
                 if (isWellKnownSymbolSyntactically(name)) {
                     return getPropertyNameForKnownSymbolName(idText(name.name));
                 }
-                if (isPrivateName(name)) {
+                if (isPrivateIdentifier(name)) {
                     // containingClass exists because private names only allowed inside classes
-                    const containingClass = getContainingClass(name.parent);
+                    const containingClass = getContainingClass(node);
                     if (!containingClass) {
-                        // we're in a case where there's a private name outside a class (invalid)
+                        // we can get here in cases where there is already a parse error.
                         return undefined;
                     }
                     const containingClassSymbol = containingClass.symbol;
-                    return getPropertyNameForPrivateNameDescription(containingClassSymbol, name.escapedText);
+                    return getSymbolNameForPrivateIdentifier(containingClassSymbol, name.escapedText);
                 }
                 return isPropertyNameLiteral(name) ? getEscapedTextOfIdentifierOrLiteral(name) : undefined;
             }
@@ -422,10 +422,6 @@ namespace ts {
 
             const isDefaultExport = hasModifier(node, ModifierFlags.Default);
 
-            // need this before getDeclarationName
-            if (isNamedDeclaration(node)) {
-                node.name.parent = node;
-            }
             // The exported symbol for an export default function/class node is always named "default"
             const name = isDefaultExport && parent ? InternalSymbolName.Default : getDeclarationName(node);
 
@@ -478,6 +474,10 @@ namespace ts {
                         symbolTable.set(name, symbol = createSymbol(SymbolFlags.None, name));
                     }
                     else if (!(includes & SymbolFlags.Variable && symbol.flags & SymbolFlags.Assignment)) {
+                        // Assignment declarations are allowed to merge with variables, no matter what other flags they have.
+                        if (isNamedDeclaration(node)) {
+                            node.name.parent = node;
+                        }
                         // Report errors every position with duplicate declaration
                         // Report errors on previous encountered declarations
                         let message = symbol.flags & SymbolFlags.BlockScopedVariable
@@ -2079,7 +2079,7 @@ namespace ts {
 
         // The binder visits every node, so this is a good place to check for
         // the reserved private name (there is only one)
-        function checkPrivateName(node: PrivateName) {
+        function checkPrivateIdentifier(node: PrivateIdentifier) {
             if (node.escapedText === "#constructor") {
                 // Report error only if there are no parse errors in file
                 if (!file.parseDiagnostics.length) {
@@ -2361,8 +2361,8 @@ namespace ts {
                         node.flowNode = currentFlow;
                     }
                     return checkStrictModeIdentifier(<Identifier>node);
-                case SyntaxKind.PrivateName:
-                    return checkPrivateName(node as PrivateName);
+                case SyntaxKind.PrivateIdentifier:
+                    return checkPrivateIdentifier(node as PrivateIdentifier);
                 case SyntaxKind.PropertyAccessExpression:
                 case SyntaxKind.ElementAccessExpression:
                     const expr = node as PropertyAccessExpression | ElementAccessExpression;
@@ -2664,7 +2664,7 @@ namespace ts {
             if (!setCommonJsModuleIndicator(node)) {
                 return;
             }
-            const symbol = forEachIdentifierOrPrivateNameInEntityName(node.arguments[0], /*parent*/ undefined, (id, symbol) => {
+            const symbol = forEachIdentifierInEntityName(node.arguments[0] as any, /*parent*/ undefined, (id, symbol) => {
                 if (symbol) {
                     addDeclarationToSymbol(symbol, id, SymbolFlags.Module | SymbolFlags.Assignment);
                 }
@@ -2682,7 +2682,8 @@ namespace ts {
             if (!setCommonJsModuleIndicator(node)) {
                 return;
             }
-            const symbol = forEachIdentifierOrPrivateNameInEntityName(node.left.expression, /*parent*/ undefined, (id, symbol) => {
+
+            const symbol = forEachIdentifierInEntityName(node.left.expression, /*parent*/ undefined, (id, symbol) => {
                 if (symbol) {
                     addDeclarationToSymbol(symbol, id, SymbolFlags.Module | SymbolFlags.Assignment);
                 }
@@ -2719,6 +2720,12 @@ namespace ts {
 
         function bindThisPropertyAssignment(node: BindablePropertyAssignmentExpression | PropertyAccessExpression | LiteralLikeElementAccessExpression) {
             Debug.assert(isInJSFile(node));
+            // private identifiers *must* be declared (even in JS files)
+            const hasPrivateIdentifier = (isBinaryExpression(node) && isPropertyAccessExpression(node.left) && isPrivateIdentifier(node.left.name))
+                || (isPropertyAccessExpression(node) && isPrivateIdentifier(node.name));
+            if (hasPrivateIdentifier) {
+                return;
+            }
             const thisContainer = getThisContainer(node, /*includeArrowFunctions*/ false);
             switch (thisContainer.kind) {
                 case SyntaxKind.FunctionDeclaration:
@@ -2885,7 +2892,7 @@ namespace ts {
                 // make symbols or add declarations for intermediate containers
                 const flags = SymbolFlags.Module | SymbolFlags.Assignment;
                 const excludeFlags = SymbolFlags.ValueModuleExcludes & ~SymbolFlags.Assignment;
-                namespaceSymbol = forEachIdentifierOrPrivateNameInEntityName(entityName, namespaceSymbol, (id, symbol, parent) => {
+                namespaceSymbol = forEachIdentifierInEntityName(entityName, namespaceSymbol, (id, symbol, parent) => {
                     if (symbol) {
                         addDeclarationToSymbol(symbol, id, flags);
                         return symbol;
@@ -3009,16 +3016,21 @@ namespace ts {
             }
         }
 
-        function forEachIdentifierOrPrivateNameInEntityName(e: BindableStaticNameExpression, parent: Symbol | undefined, action: (e: Declaration, symbol: Symbol | undefined, parent: Symbol | undefined) => Symbol | undefined): Symbol | undefined {
+        function forEachIdentifierInEntityName(e: BindableStaticNameExpression, parent: Symbol | undefined, action: (e: Declaration, symbol: Symbol | undefined, parent: Symbol | undefined) => Symbol | undefined): Symbol | undefined {
             if (isExportsOrModuleExportsOrAlias(file, e)) {
                 return file.symbol;
             }
             else if (isIdentifier(e)) {
                 return action(e, lookupSymbolForPropertyAccess(e), parent);
             }
             else {
-                const s = forEachIdentifierOrPrivateNameInEntityName(e.expression, parent, action);
-                return action(getNameOrArgument(e), s && s.exports && s.exports.get(getElementOrPropertyAccessName(e)), s);
+                const s = forEachIdentifierInEntityName(e.expression, parent, action);
+                const name = getNameOrArgument(e);
+                // unreachable
+                if (isPrivateIdentifier(name)) {
+                    Debug.fail("unexpected PrivateIdentifier");
+                }
+                return action(name, s && s.exports && s.exports.get(getElementOrPropertyAccessName(e)), s);
             }
         }
 
@@ -4182,6 +4194,10 @@ namespace ts {
             case SyntaxKind.BreakStatement:
                 transformFlags |= TransformFlags.ContainsHoistedDeclarationOrCompletion;
                 break;
+
+            case SyntaxKind.PrivateIdentifier:
+                transformFlags |= TransformFlags.ContainsClassFields;
+                break;
         }
 
         node.transformFlags = transformFlags | TransformFlags.HasComputedFlags;