Do not create multiple copies of comments when cloning nodes

[liuxingbaoyu]

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]

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

[nicolo-ribaudo]

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.

[nicolo-ribaudo]

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]

When will it be null?

[liuxingbaoyu]

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

[nicolo-ribaudo]

Well, cloneIfNodeOrArray is not exported 😅

[liuxingbaoyu]

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. :)

[nicolo-ribaudo]

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

[liuxingbaoyu]

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

[liuxingbaoyu]

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

[nicolo-ribaudo]

Thanks!

[liuxingbaoyu]

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]

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]

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]

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.

[liuxingbaoyu]

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.