This is a great start!

I have one or two issues at the moment. The biggest issue is that I think I didn't design the#copy interface well. If you passnil as the value of one of the fields, it uses the value that's already present.

e.g., if you were to runIfNode#copy and passpredicate: nil, then it wouldn't change the tree at all.

In this case I think one of the precursors to this PR could be changing the copy methods to instead use the current values as the default values, as in change:

defcopy(predicate:nil,statements:nil,consequent:nil,location:nil)node=IfNode.new(predicate:predicate ||self.predicate,statements:statements ||self.statements,consequent:consequent ||self.consequent,location:location ||self.location)node.comments.concat(comments.map(&:copy))nodeend

to

defcopy(predicate:self.predicate,statements:self.statements,consequent:self.consequent,location:self.location)node=IfNode.new(predicate:predicate,statements:statements,consequent:consequent,location:location)node.comments.concat(comments.map(&:copy))nodeend

At the moment most of the places where you're removing nodes is going to result in not actually removing them because of this.

The other thing I have an issue with is that removing nodes from the tree could easily break the tree's expectations.

For example, in your example you have it removing a vcall withdo_more_work as the value. If that were the predicate of an if node, as in:if do_more_work and the predicate went away, then you now can't format the tree at all.

For these reasons, I think we need aRemovedNode that you put in place of the existing node when you replace it. ThatRemovedNode should have the same API as the other nodes in the tree so that it can be queried as normal.