-
-
Notifications
You must be signed in to change notification settings - Fork 83
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
Conversation
a78e3d2
to
d1a7393
Compare
apps/remote_control/test/lexical/remote_control/code_intelligence/variable_test.exs
Show resolved
Hide resolved
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.
d1a7393
to
a015889
Compare
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. |
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 |
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:
|
@scottming added a special case for tests. |
There was a problem hiding this 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.
Yeah, that's what I'm hoping. This is good, but could be better. |
4db70b3
to
14310cc
Compare
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:
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 theelse
clause in the if.