Add MutationVisitor#remove API to remove nodes from the tree

[nvasilevski]

This PR allows extends MutationVisitor to allow removing nodes from the tree.
The API we are going with is having remove helper method along with mutate to define patterns for nodes to be removed from the tree.
It's not a necessity but provides a nicer API compared to:

visitor.mutate("<pattern for node to be removed>") do |node|
   RemovedNode.new
end

allowing for visitor.remove("<pattern for node to be removed>") to be used.

Under the hood we are mutating the tree by substituting nodes that match the removal pattern with an instance of RemovedNode which purpose is to keep the tree valid and be formatted as an empty string during tree dumping.

[kddnewton]

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 pass nil as the value of one of the fields, it uses the value that's already present.

e.g., if you were to run IfNode#copy and pass predicate: 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:

def copy(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))
  node
end

to

def copy(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))
  node
end

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 with do_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 a RemovedNode that you put in place of the existing node when you replace it. That RemovedNode should have the same API as the other nodes in the tree so that it can be queried as normal.

[nvasilevski]

Hey @kddnewton, sorry for a delay, I finally have found some time to make changes on the PR

For these reasons, I think we need a RemovedNode that you put in place of the existing node when you replace it.

That's what I end up doing, could you have another look? Thanks!

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

Correct me if I'm wrong but I don't think this is a concern anymore as long as we go with a "truthy" RemovedNode instances. I'm willing to create a separate PR to change the design of copy methods but I don't think it's a strict requirement for the #remove API to be shipped

[nvasilevski]

I wasn't sure about exact API of the RemovedNode. I think we may want RemovedNode to point to an instance of the node it substitutes but I at the moment I don't have use-cases for it so I decided not to overcomplicate. Perhaps I should have simplified it even further and dropped the comments and location properties but some existing code expected every node to respond to comments so I decided to avoid breaking this expectation

[kddnewton]

I actually like this API a lot, but am going to close in favor of prism. But if you want to open a similar API for prism I would be down! Sorry it took forever to review.