BreakNode / ReturnNode / NextNode: Multiple fixes

[marcandre]

This PR:

• fixes return by not including MethodDispatchNode (since it is not a method dispatch). Most methods would return nonsense (e.g. method_call could be nil or whatever the second argument was)
• same for break, plus fixes argument handling
• adds NextNode

next, break and return are very similar, the only difference being how far back they leave the current scope.

[marcandre]

Alternative would be to have #receiver return nil and #method_name return :return / :break / :next, like we do for yield, super & defined?.

My first impression is that while yield, super and defined? are keywords, they act more like method call. return/break/next change the control flow so I don't feel that they should be considered similarly (same as retry), but those with more experience writing cops can chime in.

[Drenmi]

Alternative would be to have #receiver return nil and #method_name return :return / :break / :next, like we do for yield, super & defined?.

My first impression is that while yield, super and defined? are keywords, they act more like method call. return/break/next change the control flow so I don't feel that they should be considered similarly (same as retry), but those with more experience writing cops can chime in.

Generally, when I made these decisions, the starting point was always in RuboCop itself. I would notice that for some cops it would be useful to have polymorphism between e.g. super and send nodes. (By useful I mean it simplified the code of the cop significantly.)

[marcandre]

Generally, when I made these decisions, the starting point was always in RuboCop itself. I would notice that for some cops it would be useful to have polymorphism between e.g. super and send nodes. (By useful I mean it simplified the code of the cop significantly.)

Sorry if I wasn't clear: super and send being similar is great. I completely agree with this, both on a theoretical point of view and a practical one.

Not so for break, next and return, and the polymorphism is not used by any cop because it was never actually implemented (missing node_parts method), so I think it's best to simply remove it, but if you want to implement it, I'm not strongly against it either.

[Drenmi]

Not so for break, next and return, and the polymorphism is not used by any cop because it was never actually implemented (missing node_parts method), so I think it's best to simply remove it, but if you want to implement it, I'm not strongly against it either.

I'm okay with us being pragmatic about it, and not making it polymorphic until that seems useful. That approach has worked well until now. 🙂