Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add MasgnNode class for masgn nodes #203

Merged
merged 1 commit into from
Oct 29, 2024
Merged

Conversation

dvandersluis
Copy link
Member

Follows #201. I actually ended up needing masgn to have its own class with expression defined in order to do the refactoring I wanted to, so this adds it.

Because masgn nodes are more involved than the other assignment types, this class has a different interface than the other assignments, except for expression which works the same. Rather than name that the other assignment classes have, MasgnNode has names (since there will inherently be more than one). It also is able to handle nested mlhses.

@dvandersluis
Copy link
Member Author

Sorry I missed this in my previous PR @marcandre 😅 🙌

Copy link
Contributor

@marcandre marcandre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

masgn is complicated, isn't it? 😅

PR looks like a good start, but you seem to have overlooked 2 important cases: foo.bar, foo[:baz], ... = ....
Please describe results of your proposed methods and add tests.

Also:

  • each_descendant should only be used on the first child. Please add a test for x, y = 1, z+=2 for example. Even then, some corner cases could be problematic: foo[x+=1], bar = ...

  • I doubt each_assignment is necessary. Performance of enumerators is typically quite poor, especially for short lists

@dvandersluis
Copy link
Member Author

I knew there were things I wasn't thinking of, thanks @marcandre I'll fix up for those cases!

@dvandersluis
Copy link
Member Author

dvandersluis commented Aug 13, 2021

@marcandre updated now.

I have now introduced a MlhsNode as well, which the assignments logic has been moved to (and each_assignment has been removed). MlhsNode#assignments takes care of finding nested assignments (in splats or other mlhs nodes), and also now returns send nodes to cover your edge cases. I also have a tests covering casgn nodes now, which were previously missing.

MasgnNode#names is now able to handle send nodes as well (by just returning the source, I don't think there's a better solution for foo.bar or foo[:bar], etc.).

I also added values and array? to make it easier to process the values in the expression.

For symmetry, MasgnNode now has lhs (returns the mlhs node) and rhs (alias of expression) methods.

Please let me know your thoughts!

@dvandersluis
Copy link
Member Author

The "modern" spec was failing on indexasgn nodes so that's handled now too.

spec/rubocop/ast/masgn_node_spec.rb Outdated Show resolved Hide resolved
spec/rubocop/ast/masgn_node_spec.rb Outdated Show resolved Hide resolved
spec/rubocop/ast/masgn_node_spec.rb Outdated Show resolved Hide resolved
spec/rubocop/ast/masgn_node_spec.rb Outdated Show resolved Hide resolved
lib/rubocop/ast/node/masgn_node.rb Outdated Show resolved Hide resolved
spec/rubocop/ast/masgn_node_spec.rb Show resolved Hide resolved
lib/rubocop/ast/node/masgn_node.rb Outdated Show resolved Hide resolved
@dvandersluis
Copy link
Member Author

@marcandre it's been a long time but I updated this PR and tried to address your concerns/simplify some things.

@marcandre
Copy link
Contributor

That's great @dvandersluis , good work.

So a decision remains as to what we return for the LHS in the different cases:

  • setting a variable a => :a (probably)
  • calling []= => ?
  • calling a= => ?

@dvandersluis
Copy link
Member Author

dvandersluis commented Oct 29, 2024

I think for simplicity with names, if a LHS value is a send node, using method_name unadulterated would suffice given your suggestions.

a, b.c, d[e] = z => [:a, :c=, :[]=]

Is this what you had in mind @marcandre?

@marcandre
Copy link
Contributor

I think for simplicity with names, if a LHS value is a send node, using method_name unadulterated would suffice given your suggestions.

a, b.c, d[e] = z => [:a, :c=, :[]=]

Is this what you had in mind @marcandre?

Yes, I agree that's probably best 👍

@dvandersluis
Copy link
Member Author

Updated #names and #expression tests as per review.

end
alias rhs expression

# @return [Array<Node>] values being assigned on the RHS of the multiple assignment
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be worth explaining the subtle difference between expression and values, in particular that [1, 2] vs 1, 2 (which expression considers the same but values doesn't)

@marcandre
Copy link
Contributor

So quick! A bit of extra doc and this is good to go. It's 2am here in Barcelona, so I'll look at it tomorrow 😅

@dvandersluis
Copy link
Member Author

dvandersluis commented Oct 29, 2024

Thanks as always for your quick reviews @marcandre! I updated with some more documentation on expression and values; let me know if there is more you want there.

@marcandre marcandre merged commit 122ed4b into rubocop:master Oct 29, 2024
19 checks passed
@marcandre
Copy link
Contributor

Very nice, thank you ❤️

@dvandersluis dvandersluis deleted the masgn branch October 29, 2024 17:10
@marcandre
Copy link
Contributor

Released v1.13.0

bquorning added a commit to bquorning/rubocop that referenced this pull request Nov 4, 2024
Now that rubocop-ast has a `MasgnNode` and a `MlhsNode`, we can remove
some node destructuring from our code base.

See rubocop/rubocop-ast#203 for the
implementation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants