Skip to content
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.

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

Fix class name references #55262

Merged
merged 2 commits into from
Aug 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
43 changes: 12 additions & 31 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -846,7 +846,6 @@ import {
NodeCheckFlags,
NodeFlags,
nodeHasName,
nodeIsDecorated,
nodeIsMissing,
nodeIsPresent,
nodeIsSynthesized,
Expand Down Expand Up @@ -1070,7 +1069,7 @@ import {
WhileStatement,
WideningContext,
WithStatement,
YieldExpression,
YieldExpression
Comment on lines -1073 to +1072
Copy link
Member

Choose a reason for hiding this comment

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

Do you know what you ran which did this? Was it organize imports? Something else? Hoping to get to the bottom of these so we can be consistent (or, I guess formatting will prevent that).

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably organize imports.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, just tested running organize on checker.ts and this didn't happen, even when reordering things, really odd.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it was a result of running Organize Imports to remove an unused import. I just checked locally and that does remove the trailing ,.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
YieldExpression
YieldExpression,

Well, in any case, probably best to leave it until we can figure out what's happening. This is of course a nit (still looking at the PR)

} from "./_namespaces/ts";
import * as moduleSpecifiers from "./_namespaces/ts.moduleSpecifiers";
import * as performance from "./_namespaces/ts.performance";
Expand Down Expand Up @@ -27964,38 +27963,20 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {

let declaration = localOrExportSymbol.valueDeclaration;
if (declaration && localOrExportSymbol.flags & SymbolFlags.Class) {
// Due to the emit for class decorators, any reference to the class from inside of the class body
Copy link
Member

Choose a reason for hiding this comment

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

To check my understanding: before, the _a reference was only generated for ClassExpressions and decorated classes. Now it happens for any class declaration/expression that references itself in its body. Is that correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

We won't always generate the reference, but now we will always set the flag. It's just that the cases where we need the flag set were no longer restricted to the cases mentioned in the comment.

// must instead be rewritten to point to a temporary variable to avoid issues with the double-bind
// behavior of class names in ES6.
if (declaration.kind === SyntaxKind.ClassDeclaration
&& nodeIsDecorated(legacyDecorators, declaration as ClassDeclaration)) {
let container = getContainingClass(node);
while (container !== undefined) {
if (container === declaration && container.name !== node) {
getNodeLinks(declaration).flags |= NodeCheckFlags.ClassWithConstructorReference;
getNodeLinks(node).flags |= NodeCheckFlags.ConstructorReferenceInClass;
break;
}

container = getContainingClass(container);
}
}
else if (declaration.kind === SyntaxKind.ClassExpression) {
// When we emit a class expression with static members that contain a reference
// to the constructor in the initializer, we will need to substitute that
// binding with an alias as the class name is not in scope.
// When we downlevel classes we may emit some code outside of the class body. Due to the fact the
// class name is double-bound, we must ensure we mark references to the class name so that we can
// emit an alias to the class later.
if (isClassLike(declaration) && declaration.name !== node) {
let container = getThisContainer(node, /*includeArrowFunctions*/ false, /*includeClassComputedPropertyName*/ false);
while (container.kind !== SyntaxKind.SourceFile) {
if (container.parent === declaration) {
if (isPropertyDeclaration(container) && isStatic(container) || isClassStaticBlockDeclaration(container)) {
getNodeLinks(declaration).flags |= NodeCheckFlags.ClassWithConstructorReference;
getNodeLinks(node).flags |= NodeCheckFlags.ConstructorReferenceInClass;
}
break;
}

while (container.kind !== SyntaxKind.SourceFile && container.parent !== declaration) {
Copy link
Member

Choose a reason for hiding this comment

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

as I read it, this while loop is semantically equivalent to the old one. Am I missing anything?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, they are equivalent, but I feel this is a bit easier to read and reason over.

Copy link
Member

Choose a reason for hiding this comment

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

Agree, the new one is much easier to read.

container = getThisContainer(container, /*includeArrowFunctions*/ false, /*includeClassComputedPropertyName*/ false);
}

if (container.kind !== SyntaxKind.SourceFile) {
getNodeLinks(declaration).flags |= NodeCheckFlags.ContainsConstructorReference;
getNodeLinks(container).flags |= NodeCheckFlags.ContainsConstructorReference;
getNodeLinks(node).flags |= NodeCheckFlags.ConstructorReference;
}
}
}

Expand Down
16 changes: 13 additions & 3 deletions src/compiler/transformers/classFields.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1718,6 +1718,9 @@ export function transformClassFields(context: TransformationContext): (x: Source
}
else if (isPrivateIdentifierClassElementDeclaration(member)) {
containsInstancePrivateElements = true;
if (resolver.getNodeCheckFlags(member) & NodeCheckFlags.ContainsConstructorReference) {
facts |= ClassFacts.NeedsClassConstructorReference;
}
}
else if (isPropertyDeclaration(member)) {
containsPublicInstanceFields = true;
Expand Down Expand Up @@ -1849,6 +1852,7 @@ export function transformClassFields(context: TransformationContext): (x: Source
}
}

const isClassWithConstructorReference = resolver.getNodeCheckFlags(node) & NodeCheckFlags.ContainsConstructorReference;
const isExport = hasSyntacticModifier(node, ModifierFlags.Export);
const isDefault = hasSyntacticModifier(node, ModifierFlags.Default);
let modifiers = visitNodes(node.modifiers, modifierVisitor, isModifier);
Expand Down Expand Up @@ -1887,6 +1891,12 @@ export function transformClassFields(context: TransformationContext): (x: Source
));
}

const alias = getClassLexicalEnvironment().classConstructor;
if (isClassWithConstructorReference && alias) {
enableSubstitutionForClassAliases();
classAliases[getOriginalNodeId(node)] = alias;
}

const classDecl = factory.updateClassDeclaration(
node,
modifiers,
Expand Down Expand Up @@ -1918,7 +1928,8 @@ export function transformClassFields(context: TransformationContext): (x: Source
// these statements after the class expression variable statement.
const isDecoratedClassDeclaration = !!(facts & ClassFacts.ClassWasDecorated);
const staticPropertiesOrClassStaticBlocks = getStaticPropertiesAndClassStaticBlock(node);
const isClassWithConstructorReference = resolver.getNodeCheckFlags(node) & NodeCheckFlags.ClassWithConstructorReference;
const classCheckFlags = resolver.getNodeCheckFlags(node);
const isClassWithConstructorReference = classCheckFlags & NodeCheckFlags.ContainsConstructorReference;

let temp: Identifier | undefined;
function createClassTempVar() {
Expand All @@ -1935,7 +1946,6 @@ export function transformClassFields(context: TransformationContext): (x: Source
return getClassLexicalEnvironment().classConstructor = node.emitNode.classThis;
}

const classCheckFlags = resolver.getNodeCheckFlags(node);
const requiresBlockScopedVar = classCheckFlags & NodeCheckFlags.BlockScopedBindingInLoop;
const temp = factory.createTempVariable(requiresBlockScopedVar ? addBlockScopedVariable : hoistVariableDeclaration, /*reservedInNestedScopes*/ true);
getClassLexicalEnvironment().classConstructor = factory.cloneNode(temp);
Expand Down Expand Up @@ -3236,7 +3246,7 @@ export function transformClassFields(context: TransformationContext): (x: Source

function trySubstituteClassAlias(node: Identifier): Expression | undefined {
if (enabledSubstitutions & ClassPropertySubstitutionFlags.ClassAliases) {
if (resolver.getNodeCheckFlags(node) & NodeCheckFlags.ConstructorReferenceInClass) {
if (resolver.getNodeCheckFlags(node) & NodeCheckFlags.ConstructorReference) {
// Due to the emit for class decorators, any reference to the class from inside of the class body
// must instead be rewritten to point to a temporary variable to avoid issues with the double-bind
// behavior of class names in ES6.
Expand Down
4 changes: 2 additions & 2 deletions src/compiler/transformers/legacyDecorators.ts
Original file line number Diff line number Diff line change
Expand Up @@ -756,7 +756,7 @@ export function transformLegacyDecorators(context: TransformationContext): (x: S
* double-binding semantics for the class name.
*/
function getClassAliasIfNeeded(node: ClassDeclaration) {
if (resolver.getNodeCheckFlags(node) & NodeCheckFlags.ClassWithConstructorReference) {
if (resolver.getNodeCheckFlags(node) & NodeCheckFlags.ContainsConstructorReference) {
enableSubstitutionForClassAliases();
const classAlias = factory.createUniqueName(node.name && !isGeneratedIdentifier(node.name) ? idText(node.name) : "default");
classAliases[getOriginalNodeId(node)] = classAlias;
Expand Down Expand Up @@ -805,7 +805,7 @@ export function transformLegacyDecorators(context: TransformationContext): (x: S

function trySubstituteClassAlias(node: Identifier): Expression | undefined {
if (classAliases) {
if (resolver.getNodeCheckFlags(node) & NodeCheckFlags.ConstructorReferenceInClass) {
if (resolver.getNodeCheckFlags(node) & NodeCheckFlags.ConstructorReference) {
// Due to the emit for class decorators, any reference to the class from inside of the class body
// must instead be rewritten to point to a temporary variable to avoid issues with the double-bind
// behavior of class names in ES6.
Expand Down
16 changes: 7 additions & 9 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6022,15 +6022,13 @@ export const enum NodeCheckFlags {
ContainsCapturedBlockScopeBinding = 1 << 13, // Part of a loop that contains block scoped variable captured in closure
CapturedBlockScopedBinding = 1 << 14, // Block-scoped binding that is captured in some function
BlockScopedBindingInLoop = 1 << 15, // Block-scoped binding with declaration nested inside iteration statement
ClassWithBodyScopedClassBinding = 1 << 16, // Decorated class that contains a binding to itself inside of the class body.
BodyScopedClassBinding = 1 << 17, // Binding to a decorated class inside of the class's body.
Comment on lines -6025 to -6026
Copy link
Member

Choose a reason for hiding this comment

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

Should we shift the other bits down or is it no big deal to leave them empty until later?

Copy link
Member

Choose a reason for hiding this comment

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

this is a driveby cleanup, right? I don't see any removal of BodyScopedClassBinding in the rest of the PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

All references to BodyScopedClassBinding were removed in a prior PR, but I neglected to remove the flag. I can fix up the bits used as well.

NeedsLoopOutParameter = 1 << 18, // Block scoped binding whose value should be explicitly copied outside of the converted loop
AssignmentsMarked = 1 << 19, // Parameter assignments have been marked
ClassWithConstructorReference = 1 << 20, // Class that contains a binding to its constructor inside of the class body.
ConstructorReferenceInClass = 1 << 21, // Binding to a class constructor inside of the class's body.
ContainsClassWithPrivateIdentifiers = 1 << 22, // Marked on all block-scoped containers containing a class with private identifiers.
ContainsSuperPropertyInStaticInitializer = 1 << 23, // Marked on all block-scoped containers containing a static initializer with 'super.x' or 'super[x]'.
InCheckIdentifier = 1 << 24,
NeedsLoopOutParameter = 1 << 16, // Block scoped binding whose value should be explicitly copied outside of the converted loop
AssignmentsMarked = 1 << 17, // Parameter assignments have been marked
ContainsConstructorReference = 1 << 18, // Class or class element that contains a binding that references the class constructor.
ConstructorReference = 1 << 29, // Binding to a class constructor inside of the class's body.
ContainsClassWithPrivateIdentifiers = 1 << 20, // Marked on all block-scoped containers containing a class with private identifiers.
ContainsSuperPropertyInStaticInitializer = 1 << 21, // Marked on all block-scoped containers containing a static initializer with 'super.x' or 'super[x]'.
InCheckIdentifier = 1 << 22,
}

/** @internal */
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/classBlockScoping.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,10 @@ function f(b) {
function Foo() {
}
Foo.x = function () {
new Foo();
new _a();
};
Foo.prototype.m = function () {
new Foo();
new _a();
};
return Foo;
}()),
Expand Down
44 changes: 44 additions & 0 deletions tests/baselines/reference/classNameReferencesInStaticElements.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
//// [tests/cases/compiler/classNameReferencesInStaticElements.ts] ////

//// [classNameReferencesInStaticElements.ts]
// https://github.com/microsoft/TypeScript/issues/54607
class Foo {
static { console.log(this, Foo) }
static x = () => { console.log(this, Foo) }
static y = function(this: unknown) { console.log(this, Foo) }

#x() { console.log(Foo); }
x() { this.#x(); }
}

const oldFoo = Foo;
(Foo as any) = null;
oldFoo.x();
oldFoo.y();
new oldFoo().x();

//// [classNameReferencesInStaticElements.js]
var __classPrivateFieldGet = (this && this.__classPrivateFieldGet) || function (receiver, state, kind, f) {
if (kind === "a" && !f) throw new TypeError("Private accessor was defined without a getter");
if (typeof state === "function" ? receiver !== state || !f : !state.has(receiver)) throw new TypeError("Cannot read private member from an object whose class did not declare it");
return kind === "m" ? f : kind === "a" ? f.call(receiver) : f ? f.value : state.get(receiver);
};
var _Foo_instances, _a, _Foo_x;
// https://github.com/microsoft/TypeScript/issues/54607
class Foo {
constructor() {
_Foo_instances.add(this);
}
x() { __classPrivateFieldGet(this, _Foo_instances, "m", _Foo_x).call(this); }
}
_a = Foo, _Foo_instances = new WeakSet(), _Foo_x = function _Foo_x() { console.log(_a); };
(() => {
console.log(_a, _a);
})();
Foo.x = () => { console.log(_a, _a); };
Foo.y = function () { console.log(this, _a); };
const oldFoo = Foo;
Foo = null;
oldFoo.x();
oldFoo.y();
new oldFoo().x();
2 changes: 1 addition & 1 deletion tests/baselines/reference/classStaticBlock12.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,5 @@ class C {
_a = C;
_C_x = { value: 1 };
(() => {
__classPrivateFieldGet(C, _a, "f", _C_x);
__classPrivateFieldGet(_a, _a, "f", _C_x);
})();
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,11 @@ var __classPrivateFieldGet = (this && this.__classPrivateFieldGet) || function (
var _a, _C_x;
class C {
foo() {
return __classPrivateFieldGet(C, _a, "f", _C_x);
return __classPrivateFieldGet(_a, _a, "f", _C_x);
}
}
_a = C;
_C_x = { value: 123 };
(() => {
console.log(__classPrivateFieldGet(C, _a, "f", _C_x));
console.log(__classPrivateFieldGet(_a, _a, "f", _C_x));
})();
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ for (let i = 0; i < 10; ++i) {
let _b, _c;
array.push((_c = class C {
constructor() {
this[_b] = () => C;
this[_b] = () => _c;
}
},
_b = i,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ var _loop_1 = function (i) {
var _b = void 0, _c = void 0;
array.push((_c = /** @class */ (function () {
function C() {
this[_b] = function () { return C; };
this[_b] = function () { return _c; };
}
return C;
}()),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,5 +24,5 @@ let Testing123 = Testing123_1 = class Testing123 {
exports.Testing123 = Testing123;
Testing123.prop1 = Testing123_1.prop0;
exports.Testing123 = Testing123 = Testing123_1 = __decorate([
Something({ v: () => Testing123_1 })
Something({ v: () => Testing123 })
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand how this diff happened. I assume it's because of the first block in the checker getting deleted, but I thought the edit to the second block would cause the reference to still get generated as Testing123_1.

Copy link
Member Author

Choose a reason for hiding this comment

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

Testing123_1 represents the class binding inside the class body, however a class decorator is evaluated outside of the class body, so it was pointing to the wrong reference prior to this change.

], Testing123);
7 changes: 3 additions & 4 deletions tests/baselines/reference/decoratedClassExportsCommonJS2.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,11 @@ var __decorate = (this && this.__decorate) || function (decorators, target, key,
else for (var i = decorators.length - 1; i >= 0; i--) if (d = decorators[i]) r = (c < 3 ? d(r) : c > 3 ? d(target, key, r) : d(target, key)) || r;
return c > 3 && r && Object.defineProperty(target, key, r), r;
};
var Testing123_1;
Object.defineProperty(exports, "__esModule", { value: true });
exports.Testing123 = void 0;
let Testing123 = Testing123_1 = class Testing123 {
let Testing123 = class Testing123 {
};
exports.Testing123 = Testing123;
exports.Testing123 = Testing123 = Testing123_1 = __decorate([
Something({ v: () => Testing123_1 })
exports.Testing123 = Testing123 = __decorate([
Something({ v: () => Testing123 })
], Testing123);
2 changes: 1 addition & 1 deletion tests/baselines/reference/decoratedClassExportsSystem1.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ System.register([], function (exports_1, context_1) {
exports_1("Testing123", Testing123);
Testing123.prop1 = Testing123_1.prop0;
exports_1("Testing123", Testing123 = Testing123_1 = __decorate([
Something({ v: () => Testing123_1 })
Something({ v: () => Testing123 })
], Testing123));
}
};
Expand Down
8 changes: 4 additions & 4 deletions tests/baselines/reference/decoratedClassExportsSystem2.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,16 @@ System.register([], function (exports_1, context_1) {
else for (var i = decorators.length - 1; i >= 0; i--) if (d = decorators[i]) r = (c < 3 ? d(r) : c > 3 ? d(target, key, r) : d(target, key)) || r;
return c > 3 && r && Object.defineProperty(target, key, r), r;
};
var Testing123_1, Testing123;
var Testing123;
var __moduleName = context_1 && context_1.id;
return {
setters: [],
execute: function () {
Testing123 = Testing123_1 = class Testing123 {
Testing123 = class Testing123 {
};
exports_1("Testing123", Testing123);
exports_1("Testing123", Testing123 = Testing123_1 = __decorate([
Something({ v: () => Testing123_1 })
exports_1("Testing123", Testing123 = __decorate([
Something({ v: () => Testing123 })
], Testing123));
}
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,14 @@ class BulkEditPreviewProvider {
}

//// [decoratorOnClassConstructorParameter5.js]
let BulkEditPreviewProvider = class BulkEditPreviewProvider {
var BulkEditPreviewProvider_1;
let BulkEditPreviewProvider = BulkEditPreviewProvider_1 = class BulkEditPreviewProvider {
constructor(_modeService) {
this._modeService = _modeService;
}
};
BulkEditPreviewProvider.Schema = 'vscode-bulkeditpreview';
BulkEditPreviewProvider.emptyPreview = { scheme: BulkEditPreviewProvider.Schema };
BulkEditPreviewProvider = __decorate([
BulkEditPreviewProvider.emptyPreview = { scheme: BulkEditPreviewProvider_1.Schema };
BulkEditPreviewProvider = BulkEditPreviewProvider_1 = __decorate([
__param(0, IFoo)
], BulkEditPreviewProvider);
6 changes: 2 additions & 4 deletions tests/baselines/reference/decoratorReferences.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,14 @@ var __param = (this && this.__param) || function (paramIndex, decorator) {
var C = /** @class */ (function () {
function C() {
}
C_1 = C;
C.prototype.method = function (x, y) { }; // <-- decorator y should be resolved at the class declaration, not the parameter.
var C_1;
__decorate([
y(null) // <-- y should resolve to the function declaration, not the parameter; T should resolve to the type parameter of the class
,
__param(0, y)
], C.prototype, "method", null);
C = C_1 = __decorate([
y(1, C_1) // <-- T should be resolved to the type alias, not the type parameter of the class; C should resolve to the class
C = __decorate([
y(1, C) // <-- T should be resolved to the type alias, not the type parameter of the class; C should resolve to the class
], C);
return C;
}());
4 changes: 2 additions & 2 deletions tests/baselines/reference/privateNameAndObjectRestSpread.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,9 @@ class C {
__classPrivateFieldGet(obj, _C_prop, "f");
const rest = __rest(other, []);
__classPrivateFieldGet(rest, _C_prop, "f");
const statics = Object.assign({}, C);
const statics = Object.assign({}, _a);
__classPrivateFieldGet(statics, _a, "f", _C_propStatic);
const sRest = __rest(C, []);
const sRest = __rest(_a, []);
__classPrivateFieldGet(sRest, _a, "f", _C_propStatic);
}
}
Expand Down