Do not create multiple copies of comments when cloning nodes#14551
Do not create multiple copies of comments when cloning nodes#14551nicolo-ribaudo merged 3 commits intobabel:mainfrom
Conversation
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/51946/ |
| if (!node) return node; | ||
|
|
||
| const isTop = !commentsCache.get("inCloning"); | ||
| if (isTop) commentsCache.set("inCloning", true); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
}| node: T, | ||
| deep: boolean = true, | ||
| withoutLoc: boolean = false, | ||
| commentsCache: Map<t.Comment, t.Comment> | null, |
There was a problem hiding this comment.
I'm worried someone is using (like me😳) cloneIfNodeOrArray.
There was a problem hiding this comment.
Well, cloneIfNodeOrArray is not exported 😅
There was a problem hiding this comment.
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. :)
There was a problem hiding this comment.
How are you using Object.keys? module.exports has a single property, which is cloneNode. It's impossible to access cloneIfNodeOrArray from this file 🤔
There was a problem hiding this comment.
I tested it and you are right!
May be it was available in a few years old version.
I will remove this compatibility.
|
Also, I found that deep cloning is not complete. For example comments, loc, extra properties in nodes. |
aa86cad to
d67e7f9
Compare
d67e7f9 to
fa214d0
Compare
|
Unfortunately, there are still issues with comment handling at the moment. 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);
}); |
|
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. |
|
Yes, the remaining issues are relatively minor and we can fix them in the future. |
5493bba to
81f540c
Compare
| deep: boolean = true, | ||
| withoutLoc: boolean = false, | ||
| ): T { | ||
| return cloneNodeInternal(node, deep, withoutLoc, new Map()); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
This issue was introduced in #13178.
Due to extremely complex compatibility needs, I can't find a perfect way to solve this problem.