Open overlookmotel opened 4 months ago
@Dunqing Please don't use this API from now on 😁 .
Use move_xxx
APIs instead.
I don't think it's necessarily high priority to fix this. I just wanted to request that we don't introduce more uses of it now, as it will make it more work to remove it later.
@overlookmotel Can we use the CloneIn
trait over this?
I think CloneIn
is something different. Usually ast.copy
is used when we want to extract a node from AST, and insert it somewhere else. Cloning would be very expensive if the node is the tip of a deep tree, and isn't necessary.
I think
CloneIn
is something different. Usuallyast.copy
is used when we want to extract a node from AST, and insert it somewhere else. Cloning would be very expensive if the node is the tip of a deep tree, and isn't necessary.
If we just want to take the sub-tree out and put it somewhere else, We should be able to use the move method instead. For all other cases if we don't clone the sub-tree and actually copy it in 2 places how can we prevent aliasing?
Yes, that's true, if you want to duplicate the sub-tree and put it in AST in 2 places, it must be cloned or it's UB. But all the places I saw ast.copy
being used, it was to take it out of one place, and put it back in another.
So what is stopping us from replacing all of our usages with the move method? Wouldn't it be better if we deprecate this unsound method?
So what is stopping us from replacing all of our usages with the move method?
Nothing! We just haven't done it yet.
Unfortunately, using move methods is quite unergonomic - but such is the ways of Rust.
I would be really keen to see the back of this unsound method. I remain quite anxious about the soundness of Traverse
, and would like to run Miri over the transformer to test it. But right now there's no point doing that because Miri is likely to go ape over all the ast.copy
s, and that'll obscure any other problem.
https://github.com/oxc-project/oxc/blob/7f7b5ea9e83f37ff666d6f33fda7a9e27ea2aa07/crates/oxc_ast/src/ast_builder.rs#L71-L79
This method can be used correctly when a node is being removed from AST anyway, but there are multiple ways in which it can lead to unsound situations:
Aliasing violation:
Mutation through immutable ref:
The double-free which #3481 fixes was likely caused by using
copy
.We could make the method
unsafe
and caller will have to ensure it's only used when safe to do so. However, personally I think we should remove the API entirely because (1) it is difficult to reason about when it's safe to use and is therefore a footgun and (2) the performance hit from using safe alternatives is very minimal.Unfortunately the alternatives are likely
take
ormem::replace
which are not so ergonomic.