Set shorthand: false when renaming an identifier inside an object property

[coderaiser]

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:

• putoutjs/printer@e494ffb)
• coderaiser/putout@f2e01f9

[babel-bot]

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

[nicolo-ribaudo]

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

[liuxingbaoyu]

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]

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]

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

[coderaiser]

@nicolo-ribaudo @liuxingbaoyu just applied update

[nicolo-ribaudo]

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

[coderaiser]

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.

[coderaiser]

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]

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

[liuxingbaoyu]

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]

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

[nicolo-ribaudo]

Uh yes, shorthand properties cannot be computed.

[coderaiser]

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

[nicolo-ribaudo]

@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]

@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.

[nicolo-ribaudo]

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.

[liuxingbaoyu]

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.🤔

[nicolo-ribaudo]

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.

[liuxingbaoyu]

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!

[nicolo-ribaudo]

Feel free to go ahead :)