Add VarNode class for lvar, ivar, cvar and gvar node types

[dvandersluis]

Adds node classes for variables, to have symmetry with the assignment nodes. eg. a lvasgn and lvar for the same variable should have the same value for the #name method.

[dvandersluis]

CasgnNode and Const should have a symmetrical interface so that they can be compared (eg. in Lint/SelfAssignment).

[marcandre]

Sounds good, but maybe the best would be to simply move all the implementation of Const to a module and include this here too?

[dvandersluis]

@marcandre if you can take another look, I have extracted the methods from ConstNode and included them in CasgnNode as well. I called the mixin ConstantNode but happy for a suggestion for a better name if you'd like.

This adds some new methods to CasgnNode to mirror ConstNode, so I've added tests for those as well.

[dvandersluis]

@marcandre CI is failing on https://github.com/rubocop/rubocop/blob/345be1d50c6f44b8ae1a880f490d5faa1f368945/spec/rubocop/cop/metrics/utils/abc_size_calculator_spec.rb#L345-L360

because of

https://github.com/rubocop/rubocop/blob/2fe4b1a6faca23adff13e1bbff6ecf5b66c6447b/lib/rubocop/cop/metrics/utils/repeated_attribute_discount.rb#L129

Now lvar returns a VarNode, but RepeatedAttributeDiscount uses s(:lvar, ...) which returns a Node, and then it's not considered correctly.

Any ideas?

[marcandre]

Any ideas?

Sorry, that's my code 😅 .
It could be adapted so that it creates the right type of node, base on the existence of VarNode or not...?
Or change the test to compare [type, name] instead of the nodes directly.

[dvandersluis]

Is there a way we can update s to return the right class instead of Node for all node types? That probably would be the "best" solution IMO.

It could be adapted so that it creates the right type of node, base on the existence of VarNode or not...?

Do you mean something like:

if defined?(RuboCop::AST::VarNode) ? RuboCop::AST::VarNode.new(...) : s(...)

Or change the test to compare [type, name] instead of the nodes directly.

I don't think we can do this because I think the issue is this fetch: https://github.com/rubocop/rubocop/blob/2fe4b1a6faca23adff13e1bbff6ecf5b66c6447b/lib/rubocop/cop/metrics/utils/repeated_attribute_discount.rb#L114

I've had a little trouble tracing what's happening in that class though so I may be offbase.

[bbatsov]

@dvandersluis Let me know if you're interested in reviving this PR, so we can drive it to the finish line.

[dvandersluis]

Sure, I'll take another look.

[dvandersluis]

So it looks like in the interim since this was last touched, the s() method was updated to return a specific node type instead of the generic Node, which fixes the issues I described above.

@marcandre @bbatsov if you're able please take a look 🙌

[marcandre]

Looks great, thank you 💪

[marcandre]

Let me know if you need a release quickly for this, otherwise this will be released whenever the other upcoming issues & PR are resolved / soonish

[dvandersluis]

No rush on my end! Thanks @marcandre!