Inconsistency between `RescueNode` and `EnsureNode`

[dvandersluis]

ruby-parse -e '
begin
  foo
  bar
rescue StandardError => e
  baz
ensure
  quux
end
'
(kwbegin
  (ensure
    (rescue
      (begin                                <---------- A
        (send nil :foo)
        (send nil :bar))
      (resbody
        (array
          (const nil :StandardError))
        (lvasgn :e)
        (send nil :baz)) nil)
    (send nil :quux)))                      <---------- B

RescueNode.body returns the body of the begin...end block (marked A above). It has resbody_branches and branches to access the bodies of each rescue branch.

EnsureNode.body returns the body of the ensure branch (marked B above). It has currently no method to return the begin...end block, which may or may not be inside a rescue node.

I'd like to have an EnsureNode.body method that mirrors RescueNode.body (ie. returns the code before rescue/ensure), but the method name is already in use for something inconsistent.

1. Would it be okay to rename EnsureNode.body? I'd update rubocop/extensions to match once released.
2. If not, what could the method to return A be called on EnsureNode?

[marcandre]

Interesting. I agree it is not ideal. Just to be clear: you are proposing that body be either the first child, or if the first child is a rescue node then that child's first child, right?

[dvandersluis]

Yes. I'm proposing that the body in all cases be the same, whether there's a rescue node or not. That will allow EnsureNode#body, RescueNode#body and (eventually) KeywordBeginNode#body to all be consistent.

I've already added EnsureNode#rescue_node in #333, but I'd extract it to this change. That'll allow the rescue node to be easily accessible (and @bquorning already indicated he has need for it).

It'd also mean we'd need a new name for the current EnsureNode#body.

[marcandre]

KeywordBeginNode#body => we're thinking of that being an array... Did you want to also return an array for EnsureNode#body and RescueNode#body and "wrap" the unique node if it's not a begin??

[dvandersluis]

I actually would rather not use an array because that won't have location information and will be harder to work with IMO. If the body is multiple lines and wrapped in an ensure or a rescue, a begin node is added. I'm assuming none is added for kwbegin because the kwbegin node itself acts like a begin.

[marcandre]

Right. So "EnsureNode#body, RescueNode#body and (eventually) KeywordBeginNode#body to all be consistent." is not accurate in that sense, the first two would always return a node (possible a BeginNode), but the latter would always return an array of Nodes, correct?

[dvandersluis]

I'd rather not return an array in KeywordBeginNode either. I am thinking that KeywordBeginNode#body returns self if it has multiple lines but does not have an inner rescue or ensure. In that way there'll be consistency as all three would return a single node from body. Does that make sense for you?

[marcandre]

Ah, yes, I misunderstood. You are right, this is the way.

So I agree with the program. Let's merge KeywordBeginNode asap, and this PR should be part of the upcoming major release