Skip to content
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

Do not create multiple copies of comments when cloning nodes #14551

Merged
merged 3 commits into from May 17, 2022

Conversation

liuxingbaoyu
Copy link
Contributor

@liuxingbaoyu liuxingbaoyu commented May 14, 2022

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

This issue was introduced in #13178.

Due to extremely complex compatibility needs, I can't find a perfect way to solve this problem.

@babel-bot
Copy link
Collaborator

@babel-bot babel-bot commented May 14, 2022

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

@nicolo-ribaudo nicolo-ribaudo added the PR: Bug Fix 🐛 label May 16, 2022
@@ -32,6 +33,9 @@ export default function cloneNode<T extends t.Node>(
): T {
if (!node) return node;

const isTop = !commentsCache.get("inCloning");
if (isTop) commentsCache.set("inCloning", true);
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo May 16, 2022

Choose a reason for hiding this comment

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

Instead of a special string key, we can just default commentsCache to null and do

const isTop = !commentsCache;
if (isTop) commentsCache = new Map();

and replace commentsCache.clear()with commentsCache = null.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo May 16, 2022

Choose a reason for hiding this comment

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

Or well actually, even better we can avoid relying on a global map:

export default function cloneNode<T extends t.Node>(
  node: T,
  deep: boolean = true,
  withoutLoc: boolean = false,
): T {
  return cloneNodeInternal(node, deep, withoutLoc, new Map());
}

function cloneNodeInternal<T extends t.Node>(
  node: T,
  deep: boolean,
  withoutLoc: boolean,
  commentsCache: Map<t.Comment, t.Comment>
): T {
  // original cloneNode implementation, recursively passing `commentsCache` to nested `cloneNodeInternal` calls
}

@nicolo-ribaudo nicolo-ribaudo changed the title fix: duplicate generated annotations after deep cloning Do not duplicate comments when deep cloning May 16, 2022
node: T,
deep: boolean = true,
withoutLoc: boolean = false,
commentsCache: Map<t.Comment, t.Comment> | null,
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo May 16, 2022

Choose a reason for hiding this comment

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

When will it be null?

Copy link
Contributor Author

@liuxingbaoyu liuxingbaoyu May 16, 2022

Choose a reason for hiding this comment

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

I'm worried someone is using (like me😳) cloneIfNodeOrArray.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo May 16, 2022

Choose a reason for hiding this comment

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

Well, cloneIfNodeOrArray is not exported 😅

Copy link
Contributor Author

@liuxingbaoyu liuxingbaoyu May 16, 2022

Choose a reason for hiding this comment

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

Yes, I just found out....😅
Because the type declaration of babel in vs code was not complete before, I have been using Object.keys to get all the methods.
It doesn't look like there's an extra cost to keep it compatible though. :)

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo May 16, 2022

Choose a reason for hiding this comment

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

How are you using Object.keys? module.exports has a single property, which is cloneNode. It's impossible to access cloneIfNodeOrArray from this file 🤔

Copy link
Contributor Author

@liuxingbaoyu liuxingbaoyu May 16, 2022

Choose a reason for hiding this comment

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

I tested it and you are right!
May be it was available in a few years old version.
I will remove this compatibility.

@liuxingbaoyu
Copy link
Contributor Author

@liuxingbaoyu liuxingbaoyu commented May 16, 2022

Also, I found that deep cloning is not complete. For example comments, loc, extra properties in nodes.

@liuxingbaoyu liuxingbaoyu force-pushed the fix-cloneNode-with-comments branch from aa86cad to d67e7f9 Compare May 16, 2022
@liuxingbaoyu liuxingbaoyu force-pushed the fix-cloneNode-with-comments branch from d67e7f9 to fa214d0 Compare May 16, 2022
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Thanks!

@nicolo-ribaudo nicolo-ribaudo changed the title Do not duplicate comments when deep cloning Do not create multiple copies of comments when cloning nodes May 16, 2022
@liuxingbaoyu
Copy link
Contributor Author

@liuxingbaoyu liuxingbaoyu commented May 16, 2022

Unfortunately, there are still issues with comment handling at the moment.
Because the generator relies on judging whether the comment objects are equal.
When an expression next to a comment is deep copied alone, a new comment is generated.

example:

it("should same code after deep cloning2", function () {
  let code = `//test1
  /*test2*/var/*test3*/ a = 1/*test4*/;//test5
  //test6
  var b;
  `;
  code = new CodeGenerator(parse(code), { retainLines: true }).generate()
    .code;

  const ast = parse(code);

  traverse.default(ast, {
    exit: path => {
      path.replaceWith(
        t.cloneNode(path.node, /* deep */ true, /* withoutLoc */ false),
      );
      path.skip();
    },
  });

  const newCode = new CodeGenerator(ast, { retainLines: true }).generate()
    .code;

  expect(newCode).toBe(code);
});

@nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented May 16, 2022

It's not obvious that in that case the comment should not be duplicated, since you are explicitly creating a new comment node using a deep clone that includes comments.

Let's focus on the bug solved by this PR, since the correct behavior is more straightforward.

@liuxingbaoyu
Copy link
Contributor Author

@liuxingbaoyu liuxingbaoyu commented May 16, 2022

Yes, the remaining issues are relatively minor and we can fix them in the future.
In addition, I fixed the typo in this pr by the way.

@JLHwung JLHwung force-pushed the fix-cloneNode-with-comments branch from 5493bba to 81f540c Compare May 17, 2022
@@ -29,6 +29,15 @@ export default function cloneNode<T extends t.Node>(
node: T,
deep: boolean = true,
withoutLoc: boolean = false,
): T {
return cloneNodeInternal(node, deep, withoutLoc, new Map());
Copy link
Contributor

@JLHwung JLHwung May 17, 2022

Choose a reason for hiding this comment

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

Should it be a WeakMap? If a comment node has been removed from AST, I don't think the cache should prevent it from being GC'ed.

Copy link
Contributor Author

@liuxingbaoyu liuxingbaoyu May 17, 2022

Choose a reason for hiding this comment

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

This Map will be destroyed after the function returns.

Because a comment in ast will be referenced by the two expressions before and after respectively, and the generator judges whether it is the same by comparing the comment object instance.
After deep cloning, it will become two different comment objects, resulting in repeated generation.

Use comment caching to avoid duplicate comments that are deeply cloned together.
If it is to be more perfect, it needs to be refactored in the future.

@nicolo-ribaudo nicolo-ribaudo merged commit a3a63d2 into babel:main May 17, 2022
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: types PR: Bug Fix 🐛
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants