Fix variable references

[sheldak]

I've made an attempt to fix the reference provider (Issue #124). I've described part of the changes below since they needed to be applied in several places. After that, the provider should cover many cases that are handled incorrectly in the current implementation.

---

1. A new scope for a function

State.new_func_vars_scope had a different behavior than State.new_vars_scope - the first one didn't increase the scope counter what I've changed. Previously all variables from all function headers were in the same scope. Now, it is changed and every function creates two new scopes:

• one for the function header
• one for the function body (to avoid handling do ... end differently for the functions; this part was already there)

2. Preserving variables when leaving a scope

I created a new function State.maybe_move_vars_to_outer_scope. It is called in these three places:

• MetadataBuilder.post_scope_keyword - after processing :for, :fn and :with keywords
• MetadataBuilder.post_block_keyword - after processing :do, :else, :rescue, :catch and :after keywords
• MetadataBuilder.post_clause - after processing ->

so whenever a scope is deleted and it may be needed to move part of the variables to the outer scope to extract all the references later. The function takes all variables in the scope we are deleting, filters out the ones that were defined inside the scope, and puts the rest of them into the outer scope.

3. Vars info per scope id

Calculating vars_info_per_scope_id field in the State struct needed to be changed to keep all variables that are not only defined, but also available in the particular scope. Thus, it was needed to consider all outer scopes during the update of vars_info_per_scope_id field.

4. Redefining variables

Variables defined in different scopes must be treated as different ones. E.g.

def my_func do
  x = 1
  if x  == 1 do
     x = 2
  end
  x # here x is still equal to 1
end

To make it more consistent I decided to extend the above to treat every reassignment as creating a new variable. E.g.

def my_func(x) do
  x = 1 + x # variables in this line are treated as different ones
  x # definition of this variable is in the line right above
end

Also, if we consider renaming based on references, we may obtain a tool more powerful than using just regex.

This required differentiating between variables with the same names so in many places variables started to be differentiated by their positions. This propagated to ElixirSense.Providers.References.find and ElixirSense.Providers.Definition.find functions - finding proper definitions and references required checking the position of the variable as well. That led to extending the parameters of these functions by a line and a column.

5. Assignments

Since we can have many variables with the same name, I rely on their order in vars and scope_vars fields in the State struct. The variables are "reduced" by merging the ones with the same name until getting one with is_definition: true. Then, the next occurrence of a variable with the same name will be treated as a separate variable. Reduction is done from the newest to the oldest occurrences of variables. However, in the case of the assignments (:= and :<-), we want to add a new definition of the variable after processing the statement. Let's consider an example:

def my_func(x) do
  x = 1 + x
end

We don't want to treat the first x in the second line as the definition of the second x in that line. Thus, I moved processing the left side of assignments to the post function.

6. Guards

Previously guards were omitted in the AST traversal so variables in them were not tracked. I've changed that.

---

TODO

The pin operator (^) is not taken into consideration so variables can be wrongly marked as definitions. E.g.

def my_fun(a, b) do
  case a do
    ^b -> b
  end
end

variable b will be treated as redefined in the case clause, but in fact, it is the same unchanged variable as the one defined in the function header. I suggest fixing it in a follow-up PR.

---

I'm ready to work more on that in case I omitted or broke something, so I'm waiting for the feedback.

[lukaszsamson]

Excellent work @sheldak. I think you nailed most of the issues here. I once tried to fix #124 but got lost in the amount of things that were not correctly handled.

1. A new scope for a function
2. Preserving variables when leaving a scope

Yes, IIRC I had a similar idea back then

4. Redefining variables
5. Assignments

I didn't initially see that as necessary but you are right. SSA

6. Guards

Nice catch

[lukaszsamson]

Nice

[lukaszsamson]

I added that test in 1037edf but looking at it now it was a workaround instead of a fix

[lukaszsamson]

I hope that's easily switchable to Code.Fragment when it's time to merge #170

[sheldak]

If Code.Fragment returns the same line and column, it should be fine

[lukaszsamson]

I'm ready to work more on that in case I omitted or broke something, so I'm waiting for the feedback.

@sheldak Great work, I didn't find any issues. I hope you are still interested in fixing the pin issue

[sheldak]

@sheldak Great work, I didn't find any issues. I hope you are still interested in fixing the pin issue

Thanks, that's great. Yes, but it will probably take me a few weeks since I can only work part-time on this.