Skip to content

leave bad code in for #constructor and duplicate private names #58

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 3 commits into from
Mar 19, 2021
Merged

leave bad code in for #constructor and duplicate private names #58

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
682906f
leave bad code in for #constructor
mkubilayk Mar 19, 2021
d911f26
abort transform if there are duplicate private names
mkubilayk Mar 19, 2021
daab990
do not abort early, leave invalid declarations untransformed
mkubilayk Mar 19, 2021
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
90 changes: 63 additions & 27 deletions src/compiler/transformers/classFields.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ namespace ts {
* Stores if the identifier is static or not
*/
isStatic: boolean;
/**
* Stores if the identifier declaration is valid or not. Reserved names (e.g. #constructor)
* or duplicate identifiers are considered invalid.
*/
isValid: boolean;
}
interface PrivateIdentifierAccessorInfo extends PrivateIdentifierInfoBase {
kind: PrivateIdentifierKind.Accessor;
Expand Down Expand Up @@ -260,6 +265,13 @@ namespace ts {
return visitEachChild(node, classElementVisitor, context);
}

// leave invalid code untransformed
const info = accessPrivateIdentifier(node.name);
Debug.assert(info, "Undeclared private name for property declaration.");
if (!info.isValid) {
return node;
}

const functionName = getHoistedFunctionName(node);
if (functionName) {
getPendingExpressions().push(
Expand Down Expand Up @@ -303,17 +315,27 @@ namespace ts {

function visitPropertyDeclaration(node: PropertyDeclaration) {
Debug.assert(!some(node.decorators));
if (!shouldTransformPrivateElements && isPrivateIdentifier(node.name)) {
// Initializer is elided as the field is initialized in transformConstructor.
return factory.updatePropertyDeclaration(
node,
/*decorators*/ undefined,
visitNodes(node.modifiers, visitor, isModifier),
node.name,
/*questionOrExclamationToken*/ undefined,
/*type*/ undefined,
/*initializer*/ undefined
);

if (isPrivateIdentifier(node.name)) {
if (!shouldTransformPrivateElements) {
// Initializer is elided as the field is initialized in transformConstructor.
return factory.updatePropertyDeclaration(
node,
/*decorators*/ undefined,
visitNodes(node.modifiers, visitor, isModifier),
node.name,
/*questionOrExclamationToken*/ undefined,
/*type*/ undefined,
/*initializer*/ undefined
);
}

// leave invalid code untransformed
const info = accessPrivateIdentifier(node.name);
Debug.assert(info, "Undeclared private name for property declaration.");
if (!info.isValid) {
return node;
}
}
// Create a temporary variable to store a computed property name (if necessary).
// If it's not inlineable, then we emit an expression after the class which assigns
Expand Down Expand Up @@ -1162,55 +1184,62 @@ namespace ts {
const env = getPrivateIdentifierEnvironment();
const { weakSetName, classConstructor } = env;
const assignmentExpressions: Expression[] = [];

const privateName = node.name.escapedText;
const previousInfo = env.identifiers.get(privateName);
const isValid = !isReservedPrivateName(node.name) && previousInfo === undefined;

if (hasStaticModifier(node)) {
Debug.assert(classConstructor, "weakSetName should be set in private identifier environment");
if (isPropertyDeclaration(node)) {
const variableName = createHoistedVariableForPrivateName(text, node);
env.identifiers.set(node.name.escapedText, {
env.identifiers.set(privateName, {
kind: PrivateIdentifierKind.Field,
variableName,
brandCheckIdentifier: classConstructor,
isStatic: true,
isValid,
});
}
else if (isMethodDeclaration(node)) {
const functionName = createHoistedVariableForPrivateName(text, node);
env.identifiers.set(node.name.escapedText, {
env.identifiers.set(privateName, {
kind: PrivateIdentifierKind.Method,
methodName: functionName,
brandCheckIdentifier: classConstructor,
isStatic: true,
isValid,
});
}
else if (isGetAccessorDeclaration(node)) {
const getterName = createHoistedVariableForPrivateName(text + "_get", node);
const previousInfo = env.identifiers.get(node.name.escapedText);
if (previousInfo?.kind === PrivateIdentifierKind.Accessor) {
if (previousInfo?.kind === PrivateIdentifierKind.Accessor && previousInfo.isStatic && !previousInfo.getterName) {
previousInfo.getterName = getterName;
}
else {
env.identifiers.set(node.name.escapedText, {
env.identifiers.set(privateName, {
kind: PrivateIdentifierKind.Accessor,
getterName,
setterName: undefined,
brandCheckIdentifier: classConstructor,
isStatic: true,
isValid,
});
}
}
else if (isSetAccessorDeclaration(node)) {
const setterName = createHoistedVariableForPrivateName(text + "_set", node);
const previousInfo = env.identifiers.get(node.name.escapedText);
if (previousInfo?.kind === PrivateIdentifierKind.Accessor) {
if (previousInfo?.kind === PrivateIdentifierKind.Accessor && previousInfo.isStatic && !previousInfo.setterName) {
previousInfo.setterName = setterName;
}
else {
env.identifiers.set(node.name.escapedText, {
env.identifiers.set(privateName, {
kind: PrivateIdentifierKind.Accessor,
getterName: undefined,
setterName,
brandCheckIdentifier: classConstructor,
isStatic: true,
isValid,
});
}
}
Expand All @@ -1220,11 +1249,12 @@ namespace ts {
}
else if (isPropertyDeclaration(node)) {
const weakMapName = createHoistedVariableForPrivateName(text, node);
env.identifiers.set(node.name.escapedText, {
env.identifiers.set(privateName, {
kind: PrivateIdentifierKind.Field,
brandCheckIdentifier: weakMapName,
isStatic: false,
variableName: undefined
variableName: undefined,
isValid,
});

assignmentExpressions.push(factory.createAssignment(
Expand All @@ -1239,46 +1269,48 @@ namespace ts {
else if (isMethodDeclaration(node)) {
Debug.assert(weakSetName, "weakSetName should be set in private identifier environment");

env.identifiers.set(node.name.escapedText, {
env.identifiers.set(privateName, {
kind: PrivateIdentifierKind.Method,
methodName: createHoistedVariableForPrivateName(text, node),
brandCheckIdentifier: weakSetName,
isStatic: false,
isValid,
});
}
else if (isAccessor(node)) {
Debug.assert(weakSetName, "weakSetName should be set in private identifier environment");
const previousInfo = env.identifiers.get(node.name.escapedText);

if (isGetAccessor(node)) {
const getterName = createHoistedVariableForPrivateName(text + "_get", node);

if (previousInfo?.kind === PrivateIdentifierKind.Accessor) {
if (previousInfo?.kind === PrivateIdentifierKind.Accessor && !previousInfo.isStatic && !previousInfo.getterName) {
previousInfo.getterName = getterName;
}
else {
env.identifiers.set(node.name.escapedText, {
env.identifiers.set(privateName, {
kind: PrivateIdentifierKind.Accessor,
getterName,
setterName: undefined,
brandCheckIdentifier: weakSetName,
isStatic: false,
isValid,
});
}
}
else {
const setterName = createHoistedVariableForPrivateName(text + "_set", node);

if (previousInfo?.kind === PrivateIdentifierKind.Accessor) {
if (previousInfo?.kind === PrivateIdentifierKind.Accessor && !previousInfo.isStatic && !previousInfo.setterName) {
previousInfo.setterName = setterName;
}
else {
env.identifiers.set(node.name.escapedText, {
env.identifiers.set(privateName, {
kind: PrivateIdentifierKind.Accessor,
getterName: undefined,
setterName,
brandCheckIdentifier: weakSetName,
isStatic: false,
isValid,
});
}
}
Expand Down Expand Up @@ -1475,4 +1507,8 @@ namespace ts {
[receiver]
);
}

function isReservedPrivateName(node: PrivateIdentifier) {
return node.escapedText === "#constructor";
}
}
3 changes: 2 additions & 1 deletion tests/baselines/reference/privateNameConstructorReserved.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,6 @@ class A {
constructor() {
_A_instances.add(this);
}
#constructor() { } // Error: `#constructor` is a reserved word.
}
_A_instances = new WeakSet(), _A_constructor = function _A_constructor() { };
_A_instances = new WeakSet();
Loading