lexical-lsp / lexical

Lexical is a next-generation elixir language server
874 stars 80 forks source link

Find references for variables #645

Closed scohen closed 6 months ago

scohen commented 6 months ago

This PR implements find references for variables and fixes a couple bugs and missed cases in the variable indexer.

There's a notable edge case present in the implementation. A rebound variable that's assigned to a block will effectively shadow the variable of that name inside the block. I believe this is a limitation of the information we get back from the indexer, so another approach might be necessary in the future if this becomes an issue. An example follows:

name = "Stinky"
name = if some_condition() do
  "Smelly"
else
  name
end

Searching for references for the name variable on the first line will return no results, but searching for references to the rebind on the second line will return the reference in the else clause in the if.

scottming commented 6 months ago

There's a notable edge case present in the implementation. A rebound variable that's assigned to a block will effectively shadow the variable of that name inside the block. I believe this is a limitation of the information we get back from the indexer, so another approach might be necessary in the future if this becomes an issue. An example follows:

I took a closer look at the implementation of that algorithm. If I understand correctly, the main reason is that by the time it gets to the second definition, it has already been shadowed, so it cannot further collect references.

Also, when the cursor is on a reference, this algorithm doesn't check whether the identically named definition before the cursor is the real definition for that reference. Maybe adding this check could solve the problem, but it seems tricky because there are too many ways of defining things.

For example, like in this case:

code =
  ~q{
    entries = [1, 2, 3]
    entries =
      if something() do
        [4 | entries]
      else
        |entries # __CURSOR__ 
      end
  }

{position, document} = pop_cursor(code, as: :document)
quoted = document |> Ast.analyze()
cursor_path = Ast.cursor_path(quoted, position)
#Here we will find the second line, so we exclude this line and continue searching upward for the real definition.
Macro.path(cursor_path, &match?({:=, _, _}, &1)) 

This is just a simple idea, hoping to provide some food for thought rather than adding more burden.😂

scohen commented 6 months ago

Also, when the cursor is on a reference, this algorithm doesn't check whether the identically named definition before the cursor is the definition for that reference.

It should, at least that's the intention of what it does. It first backtracks to the nearest definition of a variable with the same name, then moves forward to find all references. That's kind of the problem above, the nearest previous definition with the same name is the wrong one.

It's important to note that we're not using the AST at this point, instead, we're using the results of indexing, which returns a flat list of "things", and those things might be somewhat ambiguous, and I think the above issue is caused by that ambiguity.

I also think an AST based approach will be difficult as well.

scottming commented 6 months ago

Today, while using it at work, I realized we missed some test-related cases.

For example, variables within the context of a single test - some of them should be variables defined in setup.


describe "test something" do
  setup do
    v = 1
    %{v: v}
  end

  test "case 1", %{v: v} do
    v
  end
end
scohen commented 6 months ago

I disagree, that variable is defined in the test declaration, there can be multiple setup blocks, and later ones can override previous ones. That will be impossible to figure out.

scottming commented 6 months ago

I disagree, that variable is defined in the test declaration, there can be multiple setup blocks, and later ones can override previous ones. That will be impossible to figure out.

In that case, then I think we can first ignore the approach of finding definitions in setup, and only focus on the context and test block, something like this:

test "case 1", %{n: n} do # definition
    v + 1 # ref1
    assert v == 1 #ref2
end
scohen commented 6 months ago

@scottming added a special case for tests.

scohen commented 6 months ago

Yeah, that's what I'm hoping. This is good, but could be better.