Skip to content

Commit 53c0a91

Browse files
joeywattsmheiber
authored andcommitted
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. Incorporate PR feedback Signed-off-by: Joey Watts <[email protected]> Fix checker crash with new block scoped bindings Signed-off-by: Joey Watts <[email protected]> Add classExpressionInLoop test Signed-off-by: Joey Watts <[email protected]> Update baselines for private name errors Signed-off-by: Joey Watts <[email protected]> Apply suggestions from code review Co-Authored-By: Ron Buckton <[email protected]> Fix privateNameFieldCallExpression test Signed-off-by: Joey Watts <[email protected]> Remove unnecessary comment Signed-off-by: Joey Watts <[email protected]> Fix PropertyAccessEntityNameExpression type Signed-off-by: Joey Watts <[email protected]> Remove PrivateName from PropertyNameLiteral Signed-off-by: Joey Watts <[email protected]> Add createPrivateName Signed-off-by: Joey Watts <[email protected]> Remove PrivateName type from textSourceNode Signed-off-by: Joey Watts <[email protected]> Add Debug asserts for invalid syntax kinds Signed-off-by: Joey Watts <[email protected]> Don't output private name syntax when undeclared Signed-off-by: Joey Watts <[email protected]> Make PrivateName extend Node Signed-off-by: Joey Watts <[email protected]> Update baselines for public API Signed-off-by: Joey Watts <[email protected]> Fix completions in language server Signed-off-by: Joey Watts <[email protected]> Fix fourslash test crash Signed-off-by: Joey Watts <[email protected]> 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 Signed-off-by: Joey Watts <[email protected]> Remove PrivateIdentifier from emit with Expr hint Signed-off-by: Joey Watts <[email protected]> Fixup helpers and set function name for privates Signed-off-by: Joey Watts <[email protected]> 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 Signed-off-by: Joey Watts <[email protected]> Optimize private identifier stack lookup Avoid the cost of performing an array lookup to look at the top of the private identifier environment stack. Signed-off-by: Joey Watts <[email protected]> Change function name to be more specific Changes "getOperatorForCompoundAssignment" to "getNonAssignmentOperatorForCompoundAssignment" now that this function is accessible to the entire compiler. Signed-off-by: Joey Watts <[email protected]> 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. Signed-off-by: Joey Watts <[email protected]> Remove extra non-private-field test Signed-off-by: Joey Watts <[email protected]> Remove isPrivateIdentifierAssignmentExpression helper Signed-off-by: Joey Watts <[email protected]> Don't transform private names in ESNext target Signed-off-by: Joey Watts <[email protected]> Preserve private fields initialized to functions Signed-off-by: Joey Watts <[email protected]> Move function expressions to outer scope for efficiency Signed-off-by: Joey Watts <[email protected]> Add WeakMap collision check Signed-off-by: Joey Watts <[email protected]> Modify potential WeakMap collision condition Signed-off-by: Joey Watts <[email protected]> Fix this property assignment binding with private names Signed-off-by: Joey Watts <[email protected]> Add correct error message for WeakMap collision Signed-off-by: Joey Watts <[email protected]> Add error for statements before super with private identifiers Signed-off-by: Joey Watts <[email protected]> Refactor getPropertyForPrivateIdentifier Signed-off-by: Joey Watts <[email protected]> Add test for private identifier fields initialized to class exprs Signed-off-by: Joey Watts <[email protected]> Fix shebang errors Signed-off-by: Joey Watts <[email protected]> Fix private errors on index signatures Signed-off-by: Joey Watts <[email protected]> Add codefix for missing private property Signed-off-by: Joey Watts <[email protected]> Update error messages for unsupported private features Signed-off-by: Joey Watts <[email protected]> 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. Signed-off-by: Joey Watts <[email protected]> Add test for interface extending class with private access Signed-off-by: Joey Watts <[email protected]> Remove debugging log Signed-off-by: Joey Watts <[email protected]> Remove name assignment from private named functions Signed-off-by: Joey Watts <[email protected]> Rename to getPrivateIdentifierPropertyOfType Signed-off-by: Joey Watts <[email protected]> Fix index signature test comment Signed-off-by: Joey Watts <[email protected]> Fix test target syntax error Signed-off-by: Joey Watts <[email protected]> Change error messages Signed-off-by: Joey Watts <[email protected]> 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 Signed-off-by: Joey Watts <[email protected]> incorporate PR feedback Signed-off-by: Max Heiber <[email protected]> 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 Signed-off-by: Max Heiber <[email protected]> update baselines Fix private identifier ++/-- numeric coercion Signed-off-by: Joey Watts <[email protected]> Remove 'downleveled' from super error Signed-off-by: Joey Watts <[email protected]> Fix bad comments in helper call emit Signed-off-by: Joey Watts <[email protected]> Error on private identifiers in JSX tag names Signed-off-by: Joey Watts <[email protected]> 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 Signed-off-by: Joey Watts <[email protected]> Signed-off-by: Max Heiber <[email protected]> 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 Signed-off-by: Joey Watts <[email protected]> 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 Signed-off-by: Joey Watts <[email protected]> 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 Signed-off-by: Joey Watts <[email protected]> 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 Signed-off-by: Joey Watts <[email protected]> Bind `this` for private identifier property tagged template literals Signed-off-by: Joey Watts <[email protected]> Fix target for jsdocPrivateName1 test Signed-off-by: Joey Watts <[email protected]> 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)
1 parent 37ce931 commit 53c0a91

File tree

394 files changed

+8727
-1914
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

394 files changed

+8727
-1914
lines changed

src/compiler/binder.ts

Lines changed: 33 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -356,15 +356,15 @@ namespace ts {
356356
if (isWellKnownSymbolSyntactically(name)) {
357357
return getPropertyNameForKnownSymbolName(idText(name.name));
358358
}
359-
if (isPrivateName(name)) {
359+
if (isPrivateIdentifier(name)) {
360360
// containingClass exists because private names only allowed inside classes
361-
const containingClass = getContainingClass(name.parent);
361+
const containingClass = getContainingClass(node);
362362
if (!containingClass) {
363-
// we're in a case where there's a private name outside a class (invalid)
363+
// we can get here in cases where there is already a parse error.
364364
return undefined;
365365
}
366366
const containingClassSymbol = containingClass.symbol;
367-
return getPropertyNameForPrivateNameDescription(containingClassSymbol, name.escapedText);
367+
return getSymbolNameForPrivateIdentifier(containingClassSymbol, name.escapedText);
368368
}
369369
return isPropertyNameLiteral(name) ? getEscapedTextOfIdentifierOrLiteral(name) : undefined;
370370
}
@@ -422,10 +422,6 @@ namespace ts {
422422

423423
const isDefaultExport = hasModifier(node, ModifierFlags.Default);
424424

425-
// need this before getDeclarationName
426-
if (isNamedDeclaration(node)) {
427-
node.name.parent = node;
428-
}
429425
// The exported symbol for an export default function/class node is always named "default"
430426
const name = isDefaultExport && parent ? InternalSymbolName.Default : getDeclarationName(node);
431427

@@ -478,6 +474,10 @@ namespace ts {
478474
symbolTable.set(name, symbol = createSymbol(SymbolFlags.None, name));
479475
}
480476
else if (!(includes & SymbolFlags.Variable && symbol.flags & SymbolFlags.Assignment)) {
477+
// Assignment declarations are allowed to merge with variables, no matter what other flags they have.
478+
if (isNamedDeclaration(node)) {
479+
node.name.parent = node;
480+
}
481481
// Report errors every position with duplicate declaration
482482
// Report errors on previous encountered declarations
483483
let message = symbol.flags & SymbolFlags.BlockScopedVariable
@@ -2082,7 +2082,7 @@ namespace ts {
20822082

20832083
// The binder visits every node, so this is a good place to check for
20842084
// the reserved private name (there is only one)
2085-
function checkPrivateName(node: PrivateName) {
2085+
function checkPrivateIdentifier(node: PrivateIdentifier) {
20862086
if (node.escapedText === "#constructor") {
20872087
// Report error only if there are no parse errors in file
20882088
if (!file.parseDiagnostics.length) {
@@ -2364,8 +2364,8 @@ namespace ts {
23642364
node.flowNode = currentFlow;
23652365
}
23662366
return checkStrictModeIdentifier(<Identifier>node);
2367-
case SyntaxKind.PrivateName:
2368-
return checkPrivateName(node as PrivateName);
2367+
case SyntaxKind.PrivateIdentifier:
2368+
return checkPrivateIdentifier(node as PrivateIdentifier);
23692369
case SyntaxKind.PropertyAccessExpression:
23702370
case SyntaxKind.ElementAccessExpression:
23712371
const expr = node as PropertyAccessExpression | ElementAccessExpression;
@@ -2665,7 +2665,7 @@ namespace ts {
26652665
if (!setCommonJsModuleIndicator(node)) {
26662666
return;
26672667
}
2668-
const symbol = forEachIdentifierOrPrivateNameInEntityName(node.arguments[0], /*parent*/ undefined, (id, symbol) => {
2668+
const symbol = forEachIdentifierInEntityName(node.arguments[0] as any, /*parent*/ undefined, (id, symbol) => {
26692669
if (symbol) {
26702670
addDeclarationToSymbol(symbol, id, SymbolFlags.Module | SymbolFlags.Assignment);
26712671
}
@@ -2683,7 +2683,8 @@ namespace ts {
26832683
if (!setCommonJsModuleIndicator(node)) {
26842684
return;
26852685
}
2686-
const symbol = forEachIdentifierOrPrivateNameInEntityName(node.left.expression, /*parent*/ undefined, (id, symbol) => {
2686+
2687+
const symbol = forEachIdentifierInEntityName(node.left.expression, /*parent*/ undefined, (id, symbol) => {
26872688
if (symbol) {
26882689
addDeclarationToSymbol(symbol, id, SymbolFlags.Module | SymbolFlags.Assignment);
26892690
}
@@ -2719,6 +2720,12 @@ namespace ts {
27192720

27202721
function bindThisPropertyAssignment(node: BindablePropertyAssignmentExpression | PropertyAccessExpression | LiteralLikeElementAccessExpression) {
27212722
Debug.assert(isInJSFile(node));
2723+
// private identifiers *must* be declared (even in JS files)
2724+
const hasPrivateIdentifier = (isBinaryExpression(node) && isPropertyAccessExpression(node.left) && isPrivateIdentifier(node.left.name))
2725+
|| (isPropertyAccessExpression(node) && isPrivateIdentifier(node.name));
2726+
if (hasPrivateIdentifier) {
2727+
return;
2728+
}
27222729
const thisContainer = getThisContainer(node, /*includeArrowFunctions*/ false);
27232730
switch (thisContainer.kind) {
27242731
case SyntaxKind.FunctionDeclaration:
@@ -2881,7 +2888,7 @@ namespace ts {
28812888
// make symbols or add declarations for intermediate containers
28822889
const flags = SymbolFlags.Module | SymbolFlags.Assignment;
28832890
const excludeFlags = SymbolFlags.ValueModuleExcludes & ~SymbolFlags.Assignment;
2884-
namespaceSymbol = forEachIdentifierOrPrivateNameInEntityName(entityName, namespaceSymbol, (id, symbol, parent) => {
2891+
namespaceSymbol = forEachIdentifierInEntityName(entityName, namespaceSymbol, (id, symbol, parent) => {
28852892
if (symbol) {
28862893
addDeclarationToSymbol(symbol, id, flags);
28872894
return symbol;
@@ -3005,16 +3012,21 @@ namespace ts {
30053012
}
30063013
}
30073014

3008-
function forEachIdentifierOrPrivateNameInEntityName(e: BindableStaticNameExpression, parent: Symbol | undefined, action: (e: Declaration, symbol: Symbol | undefined, parent: Symbol | undefined) => Symbol | undefined): Symbol | undefined {
3015+
function forEachIdentifierInEntityName(e: BindableStaticNameExpression, parent: Symbol | undefined, action: (e: Declaration, symbol: Symbol | undefined, parent: Symbol | undefined) => Symbol | undefined): Symbol | undefined {
30093016
if (isExportsOrModuleExportsOrAlias(file, e)) {
30103017
return file.symbol;
30113018
}
30123019
else if (isIdentifier(e)) {
30133020
return action(e, lookupSymbolForPropertyAccess(e), parent);
30143021
}
30153022
else {
3016-
const s = forEachIdentifierOrPrivateNameInEntityName(e.expression, parent, action);
3017-
return action(getNameOrArgument(e), s && s.exports && s.exports.get(getElementOrPropertyAccessName(e)), s);
3023+
const s = forEachIdentifierInEntityName(e.expression, parent, action);
3024+
const name = getNameOrArgument(e);
3025+
// unreachable
3026+
if (isPrivateIdentifier(name)) {
3027+
Debug.fail("unexpected PrivateIdentifier");
3028+
}
3029+
return action(name, s && s.exports && s.exports.get(getElementOrPropertyAccessName(e)), s);
30183030
}
30193031
}
30203032

@@ -4178,6 +4190,10 @@ namespace ts {
41784190
case SyntaxKind.BreakStatement:
41794191
transformFlags |= TransformFlags.ContainsHoistedDeclarationOrCompletion;
41804192
break;
4193+
4194+
case SyntaxKind.PrivateIdentifier:
4195+
transformFlags |= TransformFlags.ContainsClassFields;
4196+
break;
41814197
}
41824198

41834199
node.transformFlags = transformFlags | TransformFlags.HasComputedFlags;

0 commit comments

Comments
 (0)