Skip to content

Private-named instance fields #32

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Apr 3, 2019
Merged

Private-named instance fields #32

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b0b5d44
Move class property transformation into new transformer.
Sep 20, 2018
256bb91
Parse Private Names
mheiber Jul 8, 2018
159d511
Fix display of private names in language server
Oct 9, 2018
2902c73
Accept baseline changes
Oct 9, 2018
72978c8
Private Name Support in the Checker (#5)
mheiber Jan 28, 2019
3f6b68f
Add private named instance field transformation (#31)
joeywatts Mar 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 44 additions & 22 deletions src/compiler/binder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,16 @@ namespace ts {
Debug.assert(isWellKnownSymbolSyntactically(nameExpression));
return getPropertyNameForKnownSymbolName(idText((<PropertyAccessExpression>nameExpression).name));
}
if (isPrivateName(name)) {
// containingClass exists because private names only allowed inside classes
const containingClass = getContainingClass(name.parent);
if (!containingClass) {
// we're in a case where there's a private name outside a class (invalid)
return undefined;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that this is the only explicit return value of undefined. Any other undefined would have to come from an inner method call. I see some existing calling code that does things like !getDeclarationName(...). Does this undefined value here mean the same thing as any other potential undefined values? If not, maybe it's better to throw here (explicitly or implicitly) instead?

Copy link
Author

@mheiber mheiber Apr 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Neuroboy23 these changes were reviewed internally months ago and also given a peruse by Daniel and Ron. I'm open to revisiting this after we open the PR against the MS TS repo.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re this specific question:

  • a private name outside a class body is a parse error and will be caught by the parser. If the user has noEmitOnError: false in their compilerOptions then we have to do best-effort on the emit, even though the emit almost certainly bad. In this case, I'm not sure whether it would be better to return node or undefined.

}
const containingClassSymbol = containingClass.symbol;
return getPropertyNameForPrivateNameDescription(containingClassSymbol, name.escapedText);
}
return isPropertyNameLiteral(name) ? getEscapedTextOfIdentifierOrLiteral(name) : undefined;
}
switch (node.kind) {
Expand Down Expand Up @@ -334,6 +344,10 @@ 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);

Expand Down Expand Up @@ -386,11 +400,6 @@ 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
Expand Down Expand Up @@ -1458,7 +1467,7 @@ namespace ts {
}
if (node.expression.kind === SyntaxKind.PropertyAccessExpression) {
const propertyAccess = <PropertyAccessExpression>node.expression;
if (isNarrowableOperand(propertyAccess.expression) && isPushOrUnshiftIdentifier(propertyAccess.name)) {
if (isIdentifier(propertyAccess.name) && isNarrowableOperand(propertyAccess.expression) && isPushOrUnshiftIdentifier(propertyAccess.name)) {
currentFlow = createFlowArrayMutation(currentFlow, node);
}
}
Expand Down Expand Up @@ -1855,6 +1864,18 @@ namespace ts {
return Diagnostics.Identifier_expected_0_is_a_reserved_word_in_strict_mode;
}

// 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) {
if (node.escapedText === "#constructor") {
// Report error only if there are no parse errors in file
if (!file.parseDiagnostics.length) {
file.bindDiagnostics.push(createDiagnosticForNode(node,
Diagnostics.constructor_is_a_reserved_word, declarationNameToString(node)));
}
}
}

function checkStrictModeBinaryExpression(node: BinaryExpression) {
if (inStrictMode && isLeftHandSideExpression(node.left) && isAssignmentOperator(node.operatorToken.kind)) {
// ECMA 262 (Annex C) The identifier eval or arguments may not appear as the LeftHandSideExpression of an
Expand Down Expand Up @@ -2127,6 +2148,8 @@ namespace ts {
node.flowNode = currentFlow;
}
return checkStrictModeIdentifier(<Identifier>node);
case SyntaxKind.PrivateName:
return checkPrivateName(node as PrivateName);
case SyntaxKind.PropertyAccessExpression:
case SyntaxKind.ElementAccessExpression:
if (currentFlow && isNarrowableReference(<Expression>node)) {
Expand Down Expand Up @@ -2424,7 +2447,7 @@ namespace ts {
if (!setCommonJsModuleIndicator(node)) {
return;
}
const symbol = forEachIdentifierInEntityName(node.arguments[0], /*parent*/ undefined, (id, symbol) => {
const symbol = forEachIdentifierOrPrivateNameInEntityName(node.arguments[0], /*parent*/ undefined, (id, symbol) => {
if (symbol) {
addDeclarationToSymbol(symbol, id, SymbolFlags.Module | SymbolFlags.Assignment);
}
Expand All @@ -2443,7 +2466,7 @@ namespace ts {
return;
}
const lhs = node.left as PropertyAccessEntityNameExpression;
const symbol = forEachIdentifierInEntityName(lhs.expression, /*parent*/ undefined, (id, symbol) => {
const symbol = forEachIdentifierOrPrivateNameInEntityName(lhs.expression, /*parent*/ undefined, (id, symbol) => {
if (symbol) {
addDeclarationToSymbol(symbol, id, SymbolFlags.Module | SymbolFlags.Assignment);
}
Expand Down Expand Up @@ -2613,7 +2636,7 @@ namespace ts {
// make symbols or add declarations for intermediate containers
const flags = SymbolFlags.Module | SymbolFlags.Assignment;
const excludeFlags = SymbolFlags.ValueModuleExcludes & ~SymbolFlags.Assignment;
namespaceSymbol = forEachIdentifierInEntityName(entityName, namespaceSymbol, (id, symbol, parent) => {
namespaceSymbol = forEachIdentifierOrPrivateNameInEntityName(entityName, namespaceSymbol, (id, symbol, parent) => {
if (symbol) {
addDeclarationToSymbol(symbol, id, flags);
return symbol;
Expand Down Expand Up @@ -2701,15 +2724,15 @@ namespace ts {
}
}

function forEachIdentifierInEntityName(e: EntityNameExpression, parent: Symbol | undefined, action: (e: Identifier, symbol: Symbol | undefined, parent: Symbol | undefined) => Symbol | undefined): Symbol | undefined {
function forEachIdentifierOrPrivateNameInEntityName(e: EntityNameExpression, parent: Symbol | undefined, action: (e: Identifier | PrivateName, 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 = forEachIdentifierInEntityName(e.expression, parent, action);
const s = forEachIdentifierOrPrivateNameInEntityName(e.expression, parent, action);
if (!s || !s.exports) return Debug.fail();
return action(e.name, s.exports.get(e.name.escapedText), s);
}
Expand Down Expand Up @@ -3231,8 +3254,7 @@ namespace ts {
// A ClassDeclaration is ES6 syntax.
transformFlags = subtreeFlags | TransformFlags.AssertES2015;

// A class with a parameter property assignment, property initializer, computed property name, or decorator is
// TypeScript syntax.
// A class with a parameter property assignment or decorator is TypeScript syntax.
// An exported declaration may be TypeScript syntax, but is handled by the visitor
// for a namespace declaration.
if ((subtreeFlags & TransformFlags.ContainsTypeScriptClassSyntax)
Expand All @@ -3249,8 +3271,7 @@ namespace ts {
// A ClassExpression is ES6 syntax.
let transformFlags = subtreeFlags | TransformFlags.AssertES2015;

// A class with a parameter property assignment, property initializer, or decorator is
// TypeScript syntax.
// A class with a parameter property assignment or decorator is TypeScript syntax.
if (subtreeFlags & TransformFlags.ContainsTypeScriptClassSyntax
|| node.typeParameters) {
transformFlags |= TransformFlags.AssertTypeScript;
Expand Down Expand Up @@ -3340,7 +3361,6 @@ namespace ts {
|| hasModifier(node, ModifierFlags.TypeScriptModifier)
|| node.typeParameters
|| node.type
|| (node.name && isComputedPropertyName(node.name)) // While computed method names aren't typescript, the TS transform must visit them to emit property declarations correctly
|| !node.body) {
transformFlags |= TransformFlags.AssertTypeScript;
}
Expand Down Expand Up @@ -3371,7 +3391,6 @@ namespace ts {
if (node.decorators
|| hasModifier(node, ModifierFlags.TypeScriptModifier)
|| node.type
|| (node.name && isComputedPropertyName(node.name)) // While computed accessor names aren't typescript, the TS transform must visit them to emit property declarations correctly
|| !node.body) {
transformFlags |= TransformFlags.AssertTypeScript;
}
Expand All @@ -3386,12 +3405,15 @@ namespace ts {
}

function computePropertyDeclaration(node: PropertyDeclaration, subtreeFlags: TransformFlags) {
// A PropertyDeclaration is TypeScript syntax.
let transformFlags = subtreeFlags | TransformFlags.AssertTypeScript;
let transformFlags = subtreeFlags;

// Decorators, TypeScript-specific modifiers, and type annotations are TypeScript syntax.
if (some(node.decorators) || hasModifier(node, ModifierFlags.TypeScriptModifier) || node.type) {
transformFlags |= TransformFlags.AssertTypeScript;
}

// If the PropertyDeclaration has an initializer or a computed name, we need to inform its ancestor
// so that it handle the transformation.
if (node.initializer || isComputedPropertyName(node.name)) {
// Hoisted variables related to class properties should live within the TypeScript class wrapper.
if (isComputedPropertyName(node.name) || (hasStaticModifier(node) && node.initializer)) {
transformFlags |= TransformFlags.ContainsTypeScriptClassSyntax;
}

Expand Down
Loading