Set `shorthand: false` when renaming an identifier inside an object property by coderaiser · Pull Request #15649 · babel/babel

coderaiser · 2023-05-25T12:16:33Z

Q	A
Fixed Issues?	Fixes #15648
Patch: Bug Fix?	Y
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?
License	MIT

Add ability to handle shorthand property of ObjectProperty inside ObjectPattern when path.scope.rename() called, so recast and any other parser can relay on this property only instead of using some workarounds:

…ObjectProperty (babel#15648)

babel-bot · 2023-05-25T12:18:39Z

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

nicolo-ribaudo · 2023-05-25T13:16:25Z

packages/babel-traverse/test/scope.js

+            path.node.declarations[0].id.properties[0].shorthand,
+          ).toBeFalsy();
+        },
+      });


Could:

Move the assertion from inside the visitor to after the program.traverse call, to be 100% sure that it runs (I know you copied this pattern from the test above, but we can do better 🙂)

Replace .toBeFalsy() with .toBe(false) (since we expect false and not any falsy value)

Add an expect(program.toString()).toMatchInlineSnapshot() at the end, which helps when reading the tests to more easily see what's happening

Also, it would be good to have the following tests:

const a = 1, obj = { a };, renaming a

const { a } = 1; { const { b } = 2 } renaming a to b

I did another fix locally.
I don't think it's necessary to traverse twice to fix this, just add the ObjectProperty visitor.
At the same time I also found a strange thing, we also have an extra.shorthand in the AST, I don't understand why.🤔

liuxingbaoyu · 2023-05-25T13:33:28Z

Thank you for your PR!
Since you didn't select Would you like to work on a fix? initially, I've prepared a fix locally.
You can use it however you want!

diff --git a/packages/babel-traverse/src/scope/lib/renamer.ts b/packages/babel-traverse/src/scope/lib/renamer.ts
index 67663c6717..d826e22fc6 100644
--- a/packages/babel-traverse/src/scope/lib/renamer.ts
+++ b/packages/babel-traverse/src/scope/lib/renamer.ts
@@ -38,6 +38,14 @@ const renameVisitor: Visitor<Renamer> = {
       if (name === state.oldName) ids[name].name = state.newName;
     }
   },
+  ObjectProperty(path) {
+    const { node } = path;
+    // @ts-expect-error if shorthand === true, key and value must be identifiers
+    if (node.shorthand && node.key.name !== node.value.name) {
+      node.shorthand = false;
+      if (node.extra?.shorthand) node.extra.shorthand = false;
+    }
+  },
 };

 export default class Renamer {
diff --git a/packages/babel-traverse/test/scope.js b/packages/babel-traverse/test/scope.js
index 5a15dfae58..c6e07088a5 100644
--- a/packages/babel-traverse/test/scope.js
+++ b/packages/babel-traverse/test/scope.js
@@ -1038,5 +1038,58 @@ describe("scope", () => {
         },
       });
     });
+
+    it(".shorthand after renaming `ObjectProperty ` in `ObjectPattern`", () => {
+      const program = getPath(`
+        const { a } = b;
+        ({ a } = b);
+      `);
+
+      program.traverse({
+        Identifier(path) {
+          if (path.node.name !== "a") return;
+
+          path.scope.rename("a");
+        },
+      });
+
+      expect(
+        t.cloneDeepWithoutLoc(
+          program.node.body[0].declarations[0].id.properties[0],
+        ),
+      ).toMatchInlineSnapshot(`
+        Object {
+          "computed": false,
+          "extra": Object {
+            "shorthand": false,
+          },
+          "key": Object {
+            "loc": null,
+            "name": "a",
+            "type": "Identifier",
+          },
+          "loc": null,
+          "shorthand": false,
+          "type": "ObjectProperty",
+          "value": Object {
+            "extra": Object {},
+            "loc": null,
+            "name": "_a",
+            "type": "Identifier",
+          },
+        }
+      `);
+      expect(
+        t.cloneDeepWithoutLoc(program.node.body[0].declarations[1]),
+      ).toMatchInlineSnapshot(`undefined`);
+      expect(program + "").toMatchInlineSnapshot(`
+        "const {
+          a: _a
+        } = b;
+        ({
+          a: _a
+        } = b);"
+      `);
+    });
   });
 });

coderaiser · 2023-05-25T14:10:07Z

@liuxingbaoyu I'm OK, if we use your fix

coderaiser · 2023-05-26T11:13:56Z

@nicolo-ribaudo @liuxingbaoyu just applied update

nicolo-ribaudo · 2023-05-26T15:46:23Z

packages/babel-preset-env/test/fixtures/preset-options/destructuring-edge/output.js

 ((a, {
  b: _b = 0,
-  c: _c = 3
+  c = 3


This code should be transformed, because Edge 15 doesn't support default values in destructuring in parameters of arrow functions.

I don't understand why the first property is still transformed, but the second one isn't 🤔

The source code of this transform is https://github.com/babel/preset-modules/blob/master/src/plugins/transform-edge-default-parameters/index.js

Looks like the reason is setting:

path.parent.shorthand = false; (path.parent.extra || {}).shorthand = false;

And if shorthand set to false no fix for ObjectProperty occurred.

Also first version of my PR didn't affect additional fixtures, should I revert to previous version?

@liuxingbaoyu have you test this case locally? My tests with "additional traversing" worked good.

nicolo-ribaudo · 2023-05-26T15:53:49Z

packages/babel-traverse/src/scope/lib/renamer.ts

      }
    }
  },
+  ObjectProperty(path: NodePath<ObjectProperty>) {


In this function we should also check if value.name === state.oldName, because we only want to modify the nodes that we are renaming.

Maybe it doesn't matter? Because this should not happen normally in AST.

Also technically, even value.name === state.oldName is not necessarily the node we just renamed. So I tend not to check it.

liuxingbaoyu · 2023-05-26T16:10:49Z

packages/babel-traverse/src/scope/lib/renamer.ts

+    if (computed) {
+      return;
+    }
+
+    if (!shorthand) {
+      return;
+    }


When shorthand===true, must computed===false? I'm not entirely sure about this, any other reviewers know? Thanks!

Uh yes, shorthand properties cannot be computed.

coderaiser · 2023-05-29T08:36:00Z

@nicolo-ribaudo @liuxingbaoyu could you please clarify should I make any additional changes?

nicolo-ribaudo · 2023-05-29T09:44:56Z

@coderaiser I pushed a fix, sorry for the confusion. The problem was #15649 (comment): we were marking all the shorthand properties as non-shorthand, even those that we weren't actually renaming.

liuxingbaoyu · 2023-05-29T09:58:33Z

packages/babel-traverse/src/scope/lib/renamer.ts

-      path.node.shorthand = false;
-      if (extra?.shorthand) extra.shorthand = false;
+  ObjectProperty({ node }: NodePath<ObjectProperty>, state) {
+    if (node.shorthand && (node.value as Identifier).name === state.oldName) {


@nicolo-ribaudo

I remember they were renamed here, so actually in the ObjectProperty visitor it has been renamed. (I was troubled for a long time at that time😂)

Also sorry I didn't understand what you meant, can you explain?
According to my understanding, the shorthand for key!==value is true is illegal ast, we should have no problem marking them as false.

I think what's happening is that some visitors are running interspersed, so the AST is temporarily in invalid states while some transforms are running.

Personally I'd still prefer to use the original fix, and fix another plugin that relies on shorthand properties, plugins shouldn't depend on the wrong shorthand property across different visitors.🤔

I think renaming a variable a should only touch that variable, and it's not responsible for generically fixing other parts of the AST. However, if we find where we generate the invalid AST we should fix it in that place.

Yes, I agree with should only touch that variable! It's just that I thought it would be faster to use the original fix here. Renaming should be a relatively common function.

By the way, do you have a plan to find that place that generates invalid AST? If not I'd love to try it!

Feel free to go ahead :)

feature: babel-traverse: set shorthand when rename Identifier inside …

e4d4746

…ObjectProperty (babel#15648)

nicolo-ribaudo added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label May 25, 2023

nicolo-ribaudo reviewed May 25, 2023

View reviewed changes

feature: babel-traverse: shorthand of ObjectProperty (babel#15648)

d6c01d3

coderaiser requested review from liuxingbaoyu and nicolo-ribaudo May 26, 2023 11:13

test: babel-preset-env: update fixture

961233c

coderaiser force-pushed the feature/set-shorthand-when-rename-identifier-inside-object-property branch from 33a3668 to 961233c Compare May 26, 2023 13:43

nicolo-ribaudo reviewed May 26, 2023

View reviewed changes

liuxingbaoyu reviewed May 26, 2023

View reviewed changes

coderaiser requested review from liuxingbaoyu and nicolo-ribaudo May 29, 2023 08:37

nicolo-ribaudo force-pushed the feature/set-shorthand-when-rename-identifier-inside-object-property branch from 7afa628 to cf066e6 Compare May 29, 2023 09:53

liuxingbaoyu reviewed May 29, 2023

View reviewed changes

Fix bug

e039d8e

nicolo-ribaudo force-pushed the feature/set-shorthand-when-rename-identifier-inside-object-property branch from cf066e6 to e039d8e Compare May 29, 2023 10:25

misc

a7728f2

liuxingbaoyu approved these changes May 29, 2023

View reviewed changes

nicolo-ribaudo merged commit 4018739 into babel:main May 29, 2023

nicolo-ribaudo changed the title ~~feature: babel-traverse: set shorthand when rename Identifier inside ObjectProperty~~ Set shorthand: false when renaming an identifier inside an object property May 29, 2023

coderaiser deleted the feature/set-shorthand-when-rename-identifier-inside-object-property branch May 29, 2023 12:48

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

github-actions bot locked as resolved and limited conversation to collaborators Aug 29, 2023

Uh oh!

Conversation

coderaiser commented May 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

babel-bot commented May 25, 2023 • 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!

liuxingbaoyu May 25, 2023 • 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!

liuxingbaoyu commented May 25, 2023

Uh oh!

coderaiser commented May 25, 2023

Uh oh!

coderaiser commented May 26, 2023

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!

liuxingbaoyu May 26, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coderaiser commented May 29, 2023

Uh oh!

nicolo-ribaudo commented May 29, 2023

Uh oh!

liuxingbaoyu May 29, 2023 • 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coderaiser commented May 25, 2023 •

edited

Loading

babel-bot commented May 25, 2023 •

edited

Loading

liuxingbaoyu May 25, 2023 •

edited

Loading

liuxingbaoyu May 26, 2023 •

edited

Loading

liuxingbaoyu May 29, 2023 •

edited

Loading