review: feature: fail when element is added twice

[pvojtechovsky]

There is basic spoon concept: each element of AST has 0 or 1 parent and is in children collection of exactly one parent.

When some element which is already part of AST is added into another part of AST, then this rule concept is not true and Spoon can behave unpredictable.

This PR assures that element add will fail in such situation.

[monperrus]

There is basic spoon concept: each element of AST has 0 or 1 parent and is in children collection of exactly one parent.

I agree with this contract.

This PR is not the only solution for this.

This other solution to maintain this contract is to automatically do call delete before any call to setParent. This would be much more backward compatible.

WDYT?

[pvojtechovsky]

it is backward compatible, but it doesn't notifies client that s/he is probably doing something bad. It is much better to fail with exception, comparing to silient delete of node from origin tree and adding into new tree. Therefore we can make it configurable. By default fail and optionally it can delete the node from the origin tree automatically.

Another correct solution is to clone the node.... but I still think that throwing exception is best for new code - client is notified and can quickly understand what is wrong. For the old code we can have switch which will automatically remove node from the origin tree.

Note: thanks to this check this PR found real bug in Spoon sources, where element was added into new subtree but we forgot to clone it before, so the origin AST was broken.

[ashutosh1598]

I think it is better to throw an exception rather than silently deleting the node or cloning it. Two days ago I had to spend some time to debug because I forgot to clone a CtBlock.

[monperrus]

thanks Pavel!