Skip to content

Fix CJS local binding emit for ES decorators #53725

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 2 commits into from
Apr 10, 2023
Merged

Fix CJS local binding emit for ES decorators #53725

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c7008df
Fix CJS local binding emit for ES decorators
rbuckton Apr 10, 2023
27948b1
Add test for class+namespace merging
rbuckton Apr 10, 2023
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
11 changes: 10 additions & 1 deletion src/compiler/transformers/esDecorators.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ import {
getAllDecoratorsOfClassElement,
getCommentRange,
getEffectiveBaseTypeNode,
getEmitScriptTarget,
getFirstConstructorWithBody,
getHeritageClause,
getNonAssignmentOperatorForCompoundAssignment,
Expand Down Expand Up @@ -279,6 +280,9 @@ export function transformESDecorators(context: TransformationContext): (x: Sourc
hoistVariableDeclaration,
} = context;

const compilerOptions = context.getCompilerOptions();
const languageVersion = getEmitScriptTarget(compilerOptions);

let top: LexicalEnvironmentStackEntry | undefined;
let classInfo: ClassInfo | undefined;
let classThis: Identifier | undefined;
Expand Down Expand Up @@ -1003,7 +1007,12 @@ export function transformESDecorators(context: TransformationContext): (x: Sourc
Debug.assertIsDefined(node.name, "A class declaration that is not a default export must have a name.");
const iife = transformClassLike(node, factory.createStringLiteralFromNode(node.name));
const modifiers = visitNodes(node.modifiers, modifierVisitor, isModifier);
const varDecl = factory.createVariableDeclaration(node.name, /*exclamationToken*/ undefined, /*type*/ undefined, iife);
// When we transform to ES5/3 this will be moved inside an IIFE and should reference the name
// without any block-scoped variable collision handling
const declName = languageVersion <= ScriptTarget.ES2015 ?
factory.getInternalName(node, /*allowComments*/ false, /*allowSourceMaps*/ true) :
factory.getLocalName(node, /*allowComments*/ false, /*allowSourceMaps*/ true);
const varDecl = factory.createVariableDeclaration(declName, /*exclamationToken*/ undefined, /*type*/ undefined, iife);
const varDecls = factory.createVariableDeclarationList([varDecl], NodeFlags.Let);
const statement = factory.createVariableStatement(modifiers, varDecls);
setOriginalNode(statement, node);
Expand Down
4 changes: 2 additions & 2 deletions ...rence/esDecorators-classDeclaration-commentPreservation(module=commonjs,target=es2015).js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ let C = (() => {
Object.defineProperty(exports, "__esModule", { value: true });
exports.D = void 0;
/*34*/
exports.D = (() => {
let D = exports.D = (() => {
Copy link
Member

Choose a reason for hiding this comment

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

If this uses a let, will code like:

class Foo {}
namespace Foo {
    export test = 1234;
}

Work correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should. The merging behavior occurs in the ts transform, before decorators are processed.

Copy link
Member

Choose a reason for hiding this comment

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

Is there a test we could add to verify?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That behavior is already covered by tests/cases/conformance/internalModules/DeclarationMerging/ClassAndModuleThatMerge*.ts. Nothing I changed here would have any impact on declaration merging since that happens long before this transform runs.

Copy link
Member

Choose a reason for hiding this comment

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

Right, I get that class/namespace merging is covered, but skimming the tests you link, none of them export and use CJS, so it doesn't seem like this particular bug + namespace merging is covered.

Am I missing a test that would exhibit both behaviors? I would assume it doesn't exist, otherwise the original bug would have also been caught?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think class/namespace merging has anything to do with the original bug, but I've added a test anyways.

let _classDecorators = [dec, dec];
let _classDescriptor;
let _classExtraInitializers = [];
Expand Down Expand Up @@ -243,7 +243,7 @@ exports.default = (() => {
Object.defineProperty(exports, "__esModule", { value: true });
exports.F = void 0;
/*40*/
exports.F = (() => {
let F = exports.F = (() => {
let _classDecorators = [dec, dec];
let _classDescriptor;
let _classExtraInitializers = [];
Expand Down
4 changes: 2 additions & 2 deletions ...rence/esDecorators-classDeclaration-commentPreservation(module=commonjs,target=es2022).js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ let C = (() => {
Object.defineProperty(exports, "__esModule", { value: true });
exports.D = void 0;
/*34*/
exports.D = (() => {
let D = exports.D = (() => {
let _classDecorators = [dec, dec];
let _classDescriptor;
let _classExtraInitializers = [];
Expand Down Expand Up @@ -238,7 +238,7 @@ exports.default = (() => {
Object.defineProperty(exports, "__esModule", { value: true });
exports.F = void 0;
/*40*/
exports.F = (() => {
let F = exports.F = (() => {
let _classDecorators = [dec, dec];
let _classDescriptor;
let _classExtraInitializers = [];
Expand Down
71 changes: 71 additions & 0 deletions tests/baselines/reference/esDecorators-classDeclaration-commonjs-classNamespaceMerge.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
//// [esDecorators-classDeclaration-commonjs-classNamespaceMerge.ts]
declare var deco: any;

@deco
export class Example {
static foo() {}
}

export namespace Example {
export const x = 1;
}

Example.foo();

//// [esDecorators-classDeclaration-commonjs-classNamespaceMerge.js]
"use strict";
var __esDecorate = (this && this.__esDecorate) || function (ctor, descriptorIn, decorators, contextIn, initializers, extraInitializers) {
function accept(f) { if (f !== void 0 && typeof f !== "function") throw new TypeError("Function expected"); return f; }
var kind = contextIn.kind, key = kind === "getter" ? "get" : kind === "setter" ? "set" : "value";
var target = !descriptorIn && ctor ? contextIn["static"] ? ctor : ctor.prototype : null;
var descriptor = descriptorIn || (target ? Object.getOwnPropertyDescriptor(target, contextIn.name) : {});
var _, done = false;
for (var i = decorators.length - 1; i >= 0; i--) {
var context = {};
for (var p in contextIn) context[p] = p === "access" ? {} : contextIn[p];
for (var p in contextIn.access) context.access[p] = contextIn.access[p];
context.addInitializer = function (f) { if (done) throw new TypeError("Cannot add initializers after decoration has completed"); extraInitializers.push(accept(f || null)); };
var result = (0, decorators[i])(kind === "accessor" ? { get: descriptor.get, set: descriptor.set } : descriptor[key], context);
if (kind === "accessor") {
if (result === void 0) continue;
if (result === null || typeof result !== "object") throw new TypeError("Object expected");
if (_ = accept(result.get)) descriptor.get = _;
if (_ = accept(result.set)) descriptor.set = _;
if (_ = accept(result.init)) initializers.push(_);
}
else if (_ = accept(result)) {
if (kind === "field") initializers.push(_);
else descriptor[key] = _;
}
}
if (target) Object.defineProperty(target, contextIn.name, descriptor);
done = true;
};
var __runInitializers = (this && this.__runInitializers) || function (thisArg, initializers, value) {
var useValue = arguments.length > 2;
for (var i = 0; i < initializers.length; i++) {
value = useValue ? initializers[i].call(thisArg, value) : initializers[i].call(thisArg);
}
return useValue ? value : void 0;
};
Object.defineProperty(exports, "__esModule", { value: true });
exports.Example = void 0;
let Example = exports.Example = (() => {
let _classDecorators = [deco];
let _classDescriptor;
let _classExtraInitializers = [];
let _classThis;
var Example = class {
static {
__esDecorate(null, _classDescriptor = { value: this }, _classDecorators, { kind: "class", name: this.name }, null, _classExtraInitializers);
Example = _classThis = _classDescriptor.value;
__runInitializers(_classThis, _classExtraInitializers);
}
static foo() { }
};
return Example = _classThis;
})();
(function (Example) {
Example.x = 1;
})(Example = exports.Example || (exports.Example = {}));
Example.foo();
26 changes: 26 additions & 0 deletions tests/baselines/reference/esDecorators-classDeclaration-commonjs-classNamespaceMerge.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
=== tests/cases/conformance/esDecorators/classDeclaration/esDecorators-classDeclaration-commonjs-classNamespaceMerge.ts ===
declare var deco: any;
>deco : Symbol(deco, Decl(esDecorators-classDeclaration-commonjs-classNamespaceMerge.ts, 0, 11))

@deco
>deco : Symbol(deco, Decl(esDecorators-classDeclaration-commonjs-classNamespaceMerge.ts, 0, 11))

export class Example {
>Example : Symbol(Example, Decl(esDecorators-classDeclaration-commonjs-classNamespaceMerge.ts, 0, 22), Decl(esDecorators-classDeclaration-commonjs-classNamespaceMerge.ts, 5, 1))

static foo() {}
>foo : Symbol(Example.foo, Decl(esDecorators-classDeclaration-commonjs-classNamespaceMerge.ts, 3, 22))
}

export namespace Example {
>Example : Symbol(Example, Decl(esDecorators-classDeclaration-commonjs-classNamespaceMerge.ts, 0, 22), Decl(esDecorators-classDeclaration-commonjs-classNamespaceMerge.ts, 5, 1))

export const x = 1;
>x : Symbol(x, Decl(esDecorators-classDeclaration-commonjs-classNamespaceMerge.ts, 8, 16))
}

Example.foo();
>Example.foo : Symbol(Example.foo, Decl(esDecorators-classDeclaration-commonjs-classNamespaceMerge.ts, 3, 22))
>Example : Symbol(Example, Decl(esDecorators-classDeclaration-commonjs-classNamespaceMerge.ts, 0, 22), Decl(esDecorators-classDeclaration-commonjs-classNamespaceMerge.ts, 5, 1))
>foo : Symbol(Example.foo, Decl(esDecorators-classDeclaration-commonjs-classNamespaceMerge.ts, 3, 22))

28 changes: 28 additions & 0 deletions tests/baselines/reference/esDecorators-classDeclaration-commonjs-classNamespaceMerge.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
=== tests/cases/conformance/esDecorators/classDeclaration/esDecorators-classDeclaration-commonjs-classNamespaceMerge.ts ===
declare var deco: any;
>deco : any

@deco
>deco : any

export class Example {
>Example : Example

static foo() {}
>foo : () => void
}

export namespace Example {
>Example : typeof Example

export const x = 1;
>x : 1
>1 : 1
}

Example.foo();
>Example.foo() : void
>Example.foo : () => void
>Example : typeof Example
>foo : () => void

64 changes: 64 additions & 0 deletions tests/baselines/reference/esDecorators-classDeclaration-commonjs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
//// [esDecorators-classDeclaration-commonjs.ts]
declare var deco: any;

@deco
export class Example {
static foo() {}
}

Example.foo();

//// [esDecorators-classDeclaration-commonjs.js]
"use strict";
var __esDecorate = (this && this.__esDecorate) || function (ctor, descriptorIn, decorators, contextIn, initializers, extraInitializers) {
function accept(f) { if (f !== void 0 && typeof f !== "function") throw new TypeError("Function expected"); return f; }
var kind = contextIn.kind, key = kind === "getter" ? "get" : kind === "setter" ? "set" : "value";
var target = !descriptorIn && ctor ? contextIn["static"] ? ctor : ctor.prototype : null;
var descriptor = descriptorIn || (target ? Object.getOwnPropertyDescriptor(target, contextIn.name) : {});
var _, done = false;
for (var i = decorators.length - 1; i >= 0; i--) {
var context = {};
for (var p in contextIn) context[p] = p === "access" ? {} : contextIn[p];
for (var p in contextIn.access) context.access[p] = contextIn.access[p];
context.addInitializer = function (f) { if (done) throw new TypeError("Cannot add initializers after decoration has completed"); extraInitializers.push(accept(f || null)); };
var result = (0, decorators[i])(kind === "accessor" ? { get: descriptor.get, set: descriptor.set } : descriptor[key], context);
if (kind === "accessor") {
if (result === void 0) continue;
if (result === null || typeof result !== "object") throw new TypeError("Object expected");
if (_ = accept(result.get)) descriptor.get = _;
if (_ = accept(result.set)) descriptor.set = _;
if (_ = accept(result.init)) initializers.push(_);
}
else if (_ = accept(result)) {
if (kind === "field") initializers.push(_);
else descriptor[key] = _;
}
}
if (target) Object.defineProperty(target, contextIn.name, descriptor);
done = true;
};
var __runInitializers = (this && this.__runInitializers) || function (thisArg, initializers, value) {
var useValue = arguments.length > 2;
for (var i = 0; i < initializers.length; i++) {
value = useValue ? initializers[i].call(thisArg, value) : initializers[i].call(thisArg);
}
return useValue ? value : void 0;
};
Object.defineProperty(exports, "__esModule", { value: true });
exports.Example = void 0;
let Example = exports.Example = (() => {
let _classDecorators = [deco];
let _classDescriptor;
let _classExtraInitializers = [];
let _classThis;
var Example = class {
static {
__esDecorate(null, _classDescriptor = { value: this }, _classDecorators, { kind: "class", name: this.name }, null, _classExtraInitializers);
Example = _classThis = _classDescriptor.value;
__runInitializers(_classThis, _classExtraInitializers);
}
static foo() { }
};
return Example = _classThis;
})();
Example.foo();
19 changes: 19 additions & 0 deletions tests/baselines/reference/esDecorators-classDeclaration-commonjs.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
=== tests/cases/conformance/esDecorators/classDeclaration/esDecorators-classDeclaration-commonjs.ts ===
declare var deco: any;
>deco : Symbol(deco, Decl(esDecorators-classDeclaration-commonjs.ts, 0, 11))

@deco
>deco : Symbol(deco, Decl(esDecorators-classDeclaration-commonjs.ts, 0, 11))

export class Example {
>Example : Symbol(Example, Decl(esDecorators-classDeclaration-commonjs.ts, 0, 22))

static foo() {}
>foo : Symbol(Example.foo, Decl(esDecorators-classDeclaration-commonjs.ts, 3, 22))
}

Example.foo();
>Example.foo : Symbol(Example.foo, Decl(esDecorators-classDeclaration-commonjs.ts, 3, 22))
>Example : Symbol(Example, Decl(esDecorators-classDeclaration-commonjs.ts, 0, 22))
>foo : Symbol(Example.foo, Decl(esDecorators-classDeclaration-commonjs.ts, 3, 22))

20 changes: 20 additions & 0 deletions tests/baselines/reference/esDecorators-classDeclaration-commonjs.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
=== tests/cases/conformance/esDecorators/classDeclaration/esDecorators-classDeclaration-commonjs.ts ===
declare var deco: any;
>deco : any

@deco
>deco : any

export class Example {
>Example : Example

static foo() {}
>foo : () => void
}

Example.foo();
>Example.foo() : void
>Example.foo : () => void
>Example : typeof Example
>foo : () => void

15 changes: 15 additions & 0 deletions ...Decorators/classDeclaration/esDecorators-classDeclaration-commonjs-classNamespaceMerge.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// @target: es2022
// @module: commonjs

declare var deco: any;

@deco
export class Example {
static foo() {}
}

export namespace Example {
export const x = 1;
}

Example.foo();
11 changes: 11 additions & 0 deletions ...cases/conformance/esDecorators/classDeclaration/esDecorators-classDeclaration-commonjs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// @target: es2022
// @module: commonjs

declare var deco: any;

@deco
export class Example {
static foo() {}
}

Example.foo();