Skip to content

Fix other environment cases #177

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

Merged

Conversation

vinistock
Copy link
Contributor

I found a few issues in the implementation related to aref_field, method_add_block and nested arguments.

ARefField

This boils down to a var_ref, so we were actually counting them twice. Also, visit_aref_field would be unnecessarily complicated if the collection was a Call, which would need special treatment. Simply removing that visit makes it so that we only go through var_refs once and works when the collection is a Call

MethodAddBlock

These nodes have two parts: the call and the block. Only the block requires a fresh environment block, the call occurs in the same environment that the node is found. E.g.:

object = MyObject.new
# object is inside the call part of method_add_block, but it happens in the current environment
object["attributes"].find do |a|
  # only inside this block do we need a fresh environment
  a["field"] == "expected"
end

Nested arguments

We weren't handling the case where required positional arguments were "nested". E.g.:

[].each do |one, (two, (three, four))|
end

I used recursion to fix this one because (to my surprise) it seems that Ruby allows you to have as many layers as desired. I assume most cases will only have one level, but better to model what Ruby allows you to do.

ARefField nodes end up with regular VarRef nodes inside, so we were
actually counting them twice
The call part of a MethodAddBlock node occurs in the same environment.
Only the block portion of it occurs in a fresh environment.
Handle arguments of type
    [].each do |one, (two, three)|
    end
when declaring arguments
@kddnewton kddnewton merged commit 100e0a8 into ruby-syntax-tree:main Oct 18, 2022
@vinistock vinistock deleted the vs/fix_other_environment_cases branch October 18, 2022 18:39
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.

None yet

2 participants