Skip to content

Fix decorator emit crash #60224

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

Merged
merged 1 commit into from
Oct 14, 2024
Merged
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion src/compiler/transformers/esDecorators.ts
Original file line number Diff line number Diff line change
@@ -686,7 +686,7 @@ export function transformESDecorators(context: TransformationContext): (x: Sourc
let shouldTransformPrivateStaticElementsInClass = false;

// 1. Class decorators are evaluated outside of the private name scope of the class.
const classDecorators = transformAllDecoratorsOfDeclaration(getAllDecoratorsOfClass(node));
const classDecorators = transformAllDecoratorsOfDeclaration(getAllDecoratorsOfClass(node, /*useLegacyDecorators*/ false));
if (classDecorators) {
// - Since class decorators don't have privileged access to private names defined inside the class,
// they must be evaluated outside of the class body.
2 changes: 1 addition & 1 deletion src/compiler/transformers/legacyDecorators.ts
Original file line number Diff line number Diff line change
@@ -679,7 +679,7 @@ export function transformLegacyDecorators(context: TransformationContext): (x: S
* @param node The class node.
*/
function generateConstructorDecorationExpression(node: ClassExpression | ClassDeclaration) {
const allDecorators = getAllDecoratorsOfClass(node);
const allDecorators = getAllDecoratorsOfClass(node, /*useLegacyDecorators*/ true);
const decoratorExpressions = transformAllDecoratorsOfDeclaration(allDecorators);
if (!decoratorExpressions) {
return undefined;
18 changes: 9 additions & 9 deletions src/compiler/transformers/utilities.ts
Original file line number Diff line number Diff line change
@@ -679,9 +679,9 @@ function getDecoratorsOfParameters(node: FunctionLikeDeclaration | undefined) {
*
* @internal
*/
export function getAllDecoratorsOfClass(node: ClassLikeDeclaration): AllDecorators | undefined {
export function getAllDecoratorsOfClass(node: ClassLikeDeclaration, useLegacyDecorators: boolean): AllDecorators | undefined {
const decorators = getDecorators(node);
const parameters = getDecoratorsOfParameters(getFirstConstructorWithBody(node));
const parameters = useLegacyDecorators ? getDecoratorsOfParameters(getFirstConstructorWithBody(node)) : undefined;
Copy link
Member

Choose a reason for hiding this comment

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

Should getDecoratorsOfParameters take the parameter and return undefined instead of copying the ternary to all call sites?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Branching is cheaper at runtime than invocation, so probably not.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe a bit overoptimized but alright.

Copy link
Contributor Author

@rbuckton rbuckton Oct 14, 2024

Choose a reason for hiding this comment

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

In this case specifically, just passing the argument to getDecoratorsOfParameters means iterating over all class elements to find a constructor, which is unnecessary work.

if (!some(decorators) && !some(parameters)) {
return undefined;
}
@@ -705,12 +705,12 @@ export function getAllDecoratorsOfClassElement(member: ClassElement, parent: Cla
case SyntaxKind.GetAccessor:
case SyntaxKind.SetAccessor:
if (!useLegacyDecorators) {
return getAllDecoratorsOfMethod(member as AccessorDeclaration);
return getAllDecoratorsOfMethod(member as AccessorDeclaration, /*useLegacyDecorators*/ false);
}
return getAllDecoratorsOfAccessors(member as AccessorDeclaration, parent);
return getAllDecoratorsOfAccessors(member as AccessorDeclaration, parent, /*useLegacyDecorators*/ true);

case SyntaxKind.MethodDeclaration:
return getAllDecoratorsOfMethod(member as MethodDeclaration);
return getAllDecoratorsOfMethod(member as MethodDeclaration, useLegacyDecorators);

case SyntaxKind.PropertyDeclaration:
return getAllDecoratorsOfProperty(member as PropertyDeclaration);
@@ -726,7 +726,7 @@ export function getAllDecoratorsOfClassElement(member: ClassElement, parent: Cla
* @param parent The class node that contains the accessor.
* @param accessor The class accessor member.
*/
function getAllDecoratorsOfAccessors(accessor: AccessorDeclaration, parent: ClassExpression | ClassDeclaration): AllDecorators | undefined {
function getAllDecoratorsOfAccessors(accessor: AccessorDeclaration, parent: ClassExpression | ClassDeclaration, useLegacyDecorators: boolean): AllDecorators | undefined {
if (!accessor.body) {
return undefined;
}
@@ -741,7 +741,7 @@ function getAllDecoratorsOfAccessors(accessor: AccessorDeclaration, parent: Clas
}

const decorators = getDecorators(firstAccessorWithDecorators);
const parameters = getDecoratorsOfParameters(setAccessor);
const parameters = useLegacyDecorators ? getDecoratorsOfParameters(setAccessor) : undefined;
if (!some(decorators) && !some(parameters)) {
return undefined;
}
@@ -759,13 +759,13 @@ function getAllDecoratorsOfAccessors(accessor: AccessorDeclaration, parent: Clas
*
* @param method The class method member.
*/
function getAllDecoratorsOfMethod(method: MethodDeclaration | AccessorDeclaration): AllDecorators | undefined {
function getAllDecoratorsOfMethod(method: MethodDeclaration | AccessorDeclaration, useLegacyDecorators: boolean): AllDecorators | undefined {
if (!method.body) {
return undefined;
}

const decorators = getDecorators(method);
const parameters = getDecoratorsOfParameters(method);
const parameters = useLegacyDecorators ? getDecoratorsOfParameters(method) : undefined;
if (!some(decorators) && !some(parameters)) {
return undefined;
}
14 changes: 14 additions & 0 deletions tests/baselines/reference/parameterDecoratorsEmitCrash.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
parameterDecoratorsEmitCrash.ts(6,17): error TS1206: Decorators are not valid here.


==== parameterDecoratorsEmitCrash.ts (1 errors) ====
// https://github.com/microsoft/TypeScript/issues/58269
declare var dec: any;

export class C {
@dec x: any;
constructor(@dec x: any) {}
~
!!! error TS1206: Decorators are not valid here.
}

71 changes: 71 additions & 0 deletions tests/baselines/reference/parameterDecoratorsEmitCrash.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
//// [tests/cases/compiler/parameterDecoratorsEmitCrash.ts] ////

//// [parameterDecoratorsEmitCrash.ts]
// https://github.com/microsoft/TypeScript/issues/58269
declare var dec: any;

export class C {
@dec x: any;
constructor(@dec x: any) {}
}


//// [parameterDecoratorsEmitCrash.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.unshift(_);
}
else if (_ = accept(result)) {
if (kind === "field") initializers.unshift(_);
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.C = void 0;
var C = function () {
var _a;
var _x_decorators;
var _x_initializers = [];
var _x_extraInitializers = [];
return _a = /** @class */ (function () {
function C(x) {
this.x = __runInitializers(this, _x_initializers, void 0);
__runInitializers(this, _x_extraInitializers);
}
return C;
}()),
(function () {
var _metadata = typeof Symbol === "function" && Symbol.metadata ? Object.create(null) : void 0;
_x_decorators = [dec];
__esDecorate(null, null, _x_decorators, { kind: "field", name: "x", static: false, private: false, access: { has: function (obj) { return "x" in obj; }, get: function (obj) { return obj.x; }, set: function (obj, value) { obj.x = value; } }, metadata: _metadata }, _x_initializers, _x_extraInitializers);
if (_metadata) Object.defineProperty(_a, Symbol.metadata, { enumerable: true, configurable: true, writable: true, value: _metadata });
})(),
_a;
}();
exports.C = C;
9 changes: 9 additions & 0 deletions tests/cases/compiler/parameterDecoratorsEmitCrash.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// @noTypesAndSymbols: true

// https://github.com/microsoft/TypeScript/issues/58269
declare var dec: any;

export class C {
@dec x: any;
constructor(@dec x: any) {}
}