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

Find references for variables #645

Merged
merged 4 commits into from
Mar 19, 2024
Merged

Find references for variables #645

merged 4 commits into from
Mar 19, 2024

Conversation

scohen
Copy link
Collaborator

@scohen scohen commented Mar 12, 2024

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.

@scohen scohen requested review from zachallaun and scottming March 12, 2024 21:32
@scohen scohen force-pushed the find_references_variables branch 3 times, most recently from a78e3d2 to d1a7393 Compare March 13, 2024 03:04
@scottming
Copy link
Collaborator

scottming commented Mar 13, 2024

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.😂

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:

```elixir
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.
@scohen scohen force-pushed the find_references_variables branch from d1a7393 to a015889 Compare March 13, 2024 17:08
@scohen
Copy link
Collaborator Author

scohen commented Mar 13, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator Author

scohen commented Mar 14, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator Author

scohen commented Mar 18, 2024

@scottming added a special case for tests.

Copy link
Collaborator

@scottming scottming left a comment

Choose a reason for hiding this comment

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

I think we can merge it now and further optimize accuracy when we want to implement the rename variable feature in the future.

@scohen
Copy link
Collaborator Author

scohen commented Mar 19, 2024

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

@scohen scohen force-pushed the find_references_variables branch from 4db70b3 to 14310cc Compare March 19, 2024 17:17
@scohen scohen merged commit 64ff5dc into main Mar 19, 2024
9 checks passed
@scohen scohen deleted the find_references_variables branch March 19, 2024 17:26
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