Support NamedEvaluation for decorated anonymous class expression by JLHwung · Pull Request #15122 · babel/babel

JLHwung · 2022-11-04T00:04:37Z

Q	A
Fixed Issues?	Ensures `context.name` is correct for decorated anonymous class expression
Patch: Bug Fix?	Yes
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?
License	MIT

This PR ensures that the context.name is correctly assigned when an anonymous class expression is decorated. Note that current main already works in the following case:

function dec(value, context) {
  context.name // <-- context.name should be "a"
}
var a = @dec class {}

This PR supports more cases (See testcases for a complete set):

a = @dec class {}
{ a: @dec class {} }

To support computed key and the default export, a new helper setFunctionName is introduced, which is very similar to the TS compiler utility __setFunctionName:

https://github.com/microsoft/TypeScript/blob/c837d78d7ee5bab2ab6f2c3c7c24882ca9c31dcf/src/compiler/factory/emitHelpers.ts#L899-L909

This PR also fixes that context.name is not a string when decorating a computed numerical key:

function dec(value, context) {
  context.name // <-- previously 1n, now "1"
}
class C { @dec 1n }

Limit

~~This PR currently does not support NamedEvaluation for auto-accessors:~~

~~class { static accessor key = @dec class {}~~

~~The context.name available in dec will be the backed private key storage name. Per spec, it should be the accessor name key.~~

This PR does not support preserving name after es2015 transpilation.

JLHwung · 2022-11-04T00:06:09Z

packages/babel-plugin-proposal-decorators/src/transformer-2022-03.ts

-      path.parentPath.node.type === "VariableDeclarator" &&
-      path.parentPath.node.id.type === "Identifier"
-    ) {
-      className = path.parentPath.node.id.name;


Here is how we previously handle the namedEvaluation in the case var c = @dec class {}.

JLHwung · 2022-11-04T00:08:07Z

packages/babel-plugin-proposal-decorators/src/transformer-2022-03.ts

+}
+
+// Todo: unify name references logic with helper-function-name
+function NamedEvaluationVisitoryFactory(


At first I tried to extract logic from helper-function-name but it turns out that helper is out-of-dated. Since any exports in helpers must await a new minor release, I end up implementing a new namedEvaluationVisitor.

If we were to put this in helper-function-name waiting for a minor, would we need to keep both implementations or can we just update the old one?

babel-bot · 2022-11-04T00:08:13Z

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/53477/

JLHwung · 2022-11-04T00:12:33Z

packages/babel-plugin-proposal-decorators/src/transformer-2022-03.ts

+    },
+    // We listen on ObjectExpression so that we don't have to visit
+    // the object properties under object patterns
+    ObjectExpression(path, state) {


Guess I have to handle RecordExpression here as well.

A record expression can not contain function as value anyway, so we can ignore that.

@nicolo-ribaudo The current record spec requires NamedEvaluation for the initializer. https://tc39.es/proposal-record-tuple/#sec-record-initializer-runtime-semantics-recordpropertydefinitionevaluation Is that observable now that function / class can not be a record value?

JLHwung · 2022-11-04T01:03:17Z

packages/babel-helpers/src/helpers.ts

 `;
+
+helpers.setFunctionName = helper("7.20.1")`
+  export default function _setFunctionName(f, n) {


To mirror https://tc39.es/ecma262/#sec-setfunctionname, the helper should support prefix used by getters / setters, though it is currently not used by this plugin.

packages/babel-helpers/src/helpers/applyDecs.js

JLHwung · 2022-11-04T15:53:37Z

packages/babel-helpers/src/helpers/toPropertyKey.js

+  var key = toPrimitive(arg, "string");
+  return typeof key === "symbol" ? key : String(key);


Further assumption idea: _toPropertyKey can be simplified to typeof key === "symbol" ? key : "".concat(key) (aka TS's __propKey) if [Symbol.toPrimitive] method does not return a symbol.

nicolo-ribaudo · 2022-11-05T14:13:37Z

We could consider adding extra.name to anonymous functions&classes at the parser level, which is easier to track than making sure to name the function before transforming the code around it.

JLHwung · 2022-11-07T21:22:13Z

We could consider adding extra.name to anonymous functions&classes at the parser level, which is easier to track than making sure to name the function before transforming the code around it.

While engines like V8 retains such information in their AST ScopeInfo, the Babel's challenge here is that we are not handling the final JS code. The inferred function name can be easily out-of-sync because of any AST changes, and historically we are not good at syncing other scope info e.g. bindings, too. So I think it is fine to do it at the plugin level: we infer the function name only when we need such info.

JLHwung · 2022-11-15T20:52:23Z

...-proposal-decorators/test/fixtures/2021-12-accessors--to-es2015/undecorated-public/output.js

    return babelHelpers.classPrivateFieldGet(this, _C);
  }
-  set ['c'](v) {
+  set [_computedKey](v) {


The new output aligns to the decorated variant of this test case.

JLHwung · 2022-11-15T20:53:40Z

packages/babel-plugin-proposal-decorators/src/transformer-2022-03.ts

+
+      // Recrawl the scope to make sure new identifiers are properly synced
+      path.scope.crawl();
+    }


They are copied from the epilogue of this transform.

Also fixed non-memoised computed key for undecorated accessor

JLHwung · 2022-11-16T16:26:27Z

packages/babel-plugin-proposal-decorators/src/transformer-2022-03.ts

-      const valueNode = value ? t.cloneNode(value) : undefined;
-
-      const newField = generateClassProperty(newId, valueNode, isStatic);
+      const newField = generateClassProperty(newId, value, isStatic);


Here we avoid cloning the accessor property's initializer because

cloning is unnecessary since the initializer is moved to the proxy setter body

If the initializer has been transformed by propertyVisitor.ClassAccessorProperty (e.g. the case of nested decorator @noop accessor foo = @dec class {}), the cloned node will result to a different NodePath, which bypasses the VISITED check and gets the inner class @dec class {} transformed twice.

JLHwung · 2022-11-16T16:30:15Z

packages/babel-plugin-proposal-decorators/src/transformer-2022-03.ts

+      }
      hasElementDecorators = true;
    } else if (element.node.type === "ClassAccessorProperty") {
+      // @ts-expect-error todo: propertyVisitor.ClassAccessorProperty should be callable. Improve typings.


Help wanted: I don't know how to make the visitor callable.

I tried this, but I also failed.😅

JLHwung · 2022-11-16T21:30:58Z

...abel-plugin-proposal-decorators/test/fixtures/2022-03-accessors/undecorated-public/output.js

    return this.#C;
  }
-  set ['c'](v) {
+  set [_computedKey](v) {


This test captured the bug that computed key in a non-decorated accessor is not properly memoised.

liuxingbaoyu · 2022-11-26T23:19:19Z

packages/babel-plugin-proposal-decorators/src/transformer-2022-03.ts

+      let newKey = key;
+      if (computed && !scopeParent.isStatic(key)) {
+        newKey = memoiseExpression(key as t.Expression, "computedKey");
+      }
+      addProxyAccessorsFor(newPath, newKey, newId, computed);


Suggested change

let newKey = key;

if (computed && !scopeParent.isStatic(key)) {

newKey = memoiseExpression(key as t.Expression, "computedKey");

}

addProxyAccessorsFor(newPath, newKey, newId, computed);

addProxyAccessorsFor(

newPath,

computed && !scopeParent.isStatic(key)

? memoiseExpression(key as t.Expression, "computedKey")

: key,

newId,

computed,

);

Would this be better?

liuxingbaoyu · 2022-11-26T23:20:29Z

packages/babel-plugin-proposal-decorators/src/transformer-2022-03.ts

+    const id = path.node.id;
+    const className = id.name;


Suggested change

const id = path.node.id;

const className = id.name;

const className = path.node.id.name;

It seems that id is only used once?

nicolo-ribaudo · 2023-11-28T10:49:26Z

packages/babel-helpers/src/helpers.ts

+helpers.setFunctionName = helper("7.20.1")`
+  export default function _setFunctionName(f, n) {
+    if (typeof n === "symbol") n = n.description ? "[" + n.description + "]" : "";
+    return Object.defineProperty(f, "name", { configurable: true, value: n + "" });


In some older browsers .name was non-configurable, so we should wrap this in try/catch.

nicolo-ribaudo · 2023-11-28T10:49:56Z

packages/babel-helpers/src/helpers/toPrimitive.js

+/* @minVersion 7.1.5 */
+
+// https://tc39.es/ecma262/#sec-toprimitive
+export default function _toPrimitive(


Can we inline this in the toPropertyKey helper?

This helper has been exposed since 7.1.5, so probably not in the patch release. Here I extracted toPrimitive and toPropertyKey into single files.

nicolo-ribaudo · 2023-11-28T10:52:53Z

packages/babel-plugin-proposal-decorators/src/transformer-2022-03.ts

+}
+
+// Todo: unify name references logic with helper-function-name
+function NamedEvaluationVisitoryFactory(


If we were to put this in helper-function-name waiting for a minor, would we need to keep both implementations or can we just update the old one?

JLHwung · 2023-11-28T16:34:16Z

This PR is a rebase nightmare. I will try to extract some commits into separate PRs.

nicolo-ribaudo · 2023-11-28T16:51:16Z

Ok, sorry for not reviewing it earlier 😅

JLHwung · 2023-11-28T16:54:01Z

@nicolo-ribaudo No problem. Maybe the one-year-older me can find new bugs hidden in this PR. 😄

JLHwung added PR: Bug Fix 🐛 A type of pull request used for our changelog categories Spec: Decorators labels Nov 4, 2022

JLHwung commented Nov 4, 2022

View reviewed changes

packages/babel-helpers/src/helpers/applyDecs.js Outdated Show resolved Hide resolved

JLHwung commented Nov 4, 2022

View reviewed changes

liuxingbaoyu self-requested a review November 9, 2022 21:50

JLHwung force-pushed the named-evaluation-decorated-anonymous-class-expression branch from 7ce9211 to 699b31f Compare November 9, 2022 22:09

JLHwung added 8 commits November 14, 2022 17:44

refactor: add NamedEvaluationVisitoryFactory

d862a9e

fix: allow BigIntLiteral in ClassMethod.key

5e7e3f5

fix: expand namedEvaluationVisitor

eab6ee5

exeucte context-name tests on node 16.11+

c383946

fix: apply toProperty before assigning context name

7e95277

update package exports

16b82ba

test: execute context-name-es2015 tests on Node 14.6+

013235c

add parameter declaration test cases

da24744

JLHwung force-pushed the named-evaluation-decorated-anonymous-class-expression branch from 699b31f to da24744 Compare November 14, 2022 22:45

JLHwung commented Nov 15, 2022

View reviewed changes

fix: support named evaluation in accessor property initializer

920073e

Also fixed non-memoised computed key for undecorated accessor

JLHwung force-pushed the named-evaluation-decorated-anonymous-class-expression branch from 4839e26 to 920073e Compare November 15, 2022 20:59

fix: support named evaluation in decorated class properties

be9293f

JLHwung force-pushed the named-evaluation-decorated-anonymous-class-expression branch from ad89112 to be9293f Compare November 15, 2022 21:19

add passing ES2015 test cases

248fb76

JLHwung force-pushed the named-evaluation-decorated-anonymous-class-expression branch from 7e2b9dd to 248fb76 Compare November 15, 2022 22:40

JLHwung commented Nov 16, 2022

View reviewed changes

liuxingbaoyu reviewed Nov 26, 2022

View reviewed changes

liuxingbaoyu approved these changes Nov 26, 2022

View reviewed changes

JLHwung mentioned this pull request Jan 25, 2023

feat: Generate sourcemaps of friendly call frames #15022

Merged

evanw mentioned this pull request Nov 23, 2023

[Bug]: Stage 3 decorators behavior differences from TypeScript #16117

Closed

7 tasks

nicolo-ribaudo reviewed Nov 28, 2023

View reviewed changes

This was referenced Nov 28, 2023

Apply toPropertyKey on decorator context name #16139

Merged

Set function name for decorated private non-field elements #16144

Merged

Support named evaluation for decorated anonymous class exp #16148

Merged

JLHwung closed this in #16148 Dec 4, 2023

nicolo-ribaudo deleted the named-evaluation-decorated-anonymous-class-expression branch December 4, 2023 18:03

github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Mar 5, 2024

github-actions bot locked as resolved and limited conversation to collaborators Mar 5, 2024

		var key = toPrimitive(arg, "string");
		return typeof key === "symbol" ? key : String(key);

	const id = path.node.id;
	const className = id.name;
	const className = path.node.id.name;

Uh oh!

Conversation

JLHwung commented Nov 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Limit

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

babel-bot commented Nov 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JLHwung Nov 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolo-ribaudo commented Nov 5, 2022

Uh oh!

JLHwung commented Nov 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JLHwung commented Nov 28, 2023

Uh oh!

nicolo-ribaudo commented Nov 28, 2023

Uh oh!

JLHwung commented Nov 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JLHwung commented Nov 4, 2022 •

edited

Loading

babel-bot commented Nov 4, 2022 •

edited

Loading

JLHwung Nov 4, 2022 •

edited

Loading