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

Can't find references of the contents of an inline function #184

Closed
mhanberg opened this issue Aug 15, 2023 · 22 comments · Fixed by #430
Closed

Can't find references of the contents of an inline function #184

mhanberg opened this issue Aug 15, 2023 · 22 comments · Fixed by #430
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@mhanberg
Copy link
Collaborator

mhanberg commented Aug 15, 2023

Description

Trying to find references to the GenServer module located at the cursor will not work, as it thinks you are trying to find references to the function definition of Cache.get/0

    defmodule Cache do
      use GenServer

      def get(), do: GenServer.call(__MODULE__, :get)
      #                 ^
    end

Acceptance Criteria

it should find references to the symbol under the cursor

@mhanberg mhanberg added bug Something isn't working good first issue Good for newcomers labels Aug 15, 2023
@TwistingTwists
Copy link

I am willing to take this up. Can you please point to which files to look at?

@mhanberg
Copy link
Collaborator Author

@TwistingTwists hi!

I believe someone else has started on this, but I'll confirm with them if they've made progress.

@mhanberg
Copy link
Collaborator Author

mhanberg commented Aug 15, 2023

@apoorv-2204 can I assign this to you?

@mhanberg mhanberg assigned mhanberg and apoorv-2204 and unassigned mhanberg Aug 15, 2023
@mhanberg
Copy link
Collaborator Author

mhanberg commented Sep 9, 2023

@apoorv-2204 you should be able to use the code snippet above and follow the instructions in order to reproduce

@mhanberg
Copy link
Collaborator Author

@TwistingTwists I think you can tackle this if you're still interested!

@TwistingTwists
Copy link

I am willing to take this up. Can you please point to which files to look at?

Aye aye captain. Point me to the files I need to look !

@apoorv-2204
Copy link

I am willing to take this up. Can you please point to which files to look at?

Aye aye captain. Point me to the files I need to look !

maybe its is generating that code, as for cache , whenever it is going through any code base.
or either when ls is reading any new repo or code, its not recognising the call.

@mhanberg
Copy link
Collaborator Author

I am willing to take this up. Can you please point to which files to look at?

Aye aye captain. Point me to the files I need to look !

lib/next_ls.ex

@mhanberg
Copy link
Collaborator Author

@TwistingTwists i think the primary problem is somewhere a query is being made or something that is only accounting for the line number and not also the column number

@TwistingTwists
Copy link

That is a good head start for me. I will familiarise my self with the codebase and ask more questions as I explore more

@mhanberg
Copy link
Collaborator Author

Awesome, thanks!

@crbelaus
Copy link
Contributor

Is this still a problem? In my case it seems to work well as it is looking references for Genserver instead of the get function.

This looks for references of `Genserver` image
This looks for references of `get` image

@mhanberg
Copy link
Collaborator Author

Interesting, let me check locally

@mhanberg
Copy link
Collaborator Author

mhanberg commented Oct 21, 2023

Oh, I think VScode is giving you those hints.

Add a reference to the get function somewhere and try again.

@crbelaus
Copy link
Contributor

Oh I see now. You are right!

I think that I know how to fix this. Let me check if I'm correct and open a PR.

@crbelaus
Copy link
Contributor

crbelaus commented Oct 21, 2023

When we check what's under the cursor we always get the function that's defined in that line because, as you point out, we are not taking the cursor column into account (link).

My first thought was to modify the query to take the line into account just like we do when querying the references table. Unfortunately there seems to be another issue in the symbols table which is that every definition has the correct line but they column is always 1.

I think that we should:

  1. Update the symbols table to have a start_column and end_column just like the references table.
  2. Update the previously linked query to take the start_column and end_column so the language server knows that the cursor is not under the function definition but on a different function call.

Next question is how to proceed for a migration in the symbols table? And how could we get the line column information? It looks like the Sidecar gets the line but not the column.

@mhanberg
Copy link
Collaborator Author

To get the column information, we'll need to do some AST analyzing or ask José on the forum how to get the column info in the compilation tracer event.

I prefer the latter, as it'll be the easiest.

If it needs a fix to elixir core, then I'm okay with fixing it and just having it be a progressive enhancement as folks update their elixir versions.

Re: José, it's tempting to just mention him here on GitHub, but I prefer to go through his preferred channels (the forum) for questions, to respect his time.

I can post a question when I get home.

@crbelaus
Copy link
Contributor

I agree with you @mhanberg. Asking José sounds like the best course of action.
I took a look at the AST but it did not seem clear how to extract the column information from there.

@mhanberg
Copy link
Collaborator Author

@mhanberg
Copy link
Collaborator Author

elixir-lang/elixir#13029

@mhanberg
Copy link
Collaborator Author

José fixed this upstream, so let's get that fixed and just have it be a progressive enhancement as folks upgrade their elixir versions.

To implement this @crbelaus, you'll have to locally develop with main elixir (i believe you can just do asdf install elixir ref:main) and then we can test that it works.

It might be valuable to add a CI job that tests with the main elixir branch so that we can avoid regressions and be alerted to upstream changes before they get released, but that is orthogonal to this ticket.

@crbelaus
Copy link
Contributor

Cool I believe we can get the available column information for newest Elixir versions and just fall back to line 1 like we do now for older versions 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
4 participants