feat: find references #139

crbelaus · 2023-08-05T08:07:24Z

This PR is meant to implement the Find References Request.

The strategy is as follows:

Identify what kind of symbol we looking for
1. Check the symbols table to see if we are in a definition: defmodule, def, defp, defmacro or defstruct
2. If we are not in a definition, check the references table to see if we are in a function call or in a module alias.
Once we know what kind of symbol we are looking for (either a module or a function), check the references table to list all known references.

I've been using this today at $DAYJOB, which is a pretty big codebase, and everything seems to work just fine. I've also added two automated tests to confirm that the functionality works for finding function and module references.

Closes #43

mhanberg · 2023-08-05T14:17:56Z

Extract the "find references" logic to a separate module just like the rest of actions such as NextLS.Definition.

I think it's probably fine to leave it "unextracted" for now. I probably want these in the same module anyway, but will name it something different. Can do it later.

Make the implementation work for modules and struct. At the moment it only works for functions.

You can do this in a later PR if you want, would be better to get any version of this working and released so that the general infrastructure can be used and tested. (same thing i did with go to def basically)

See how to handle aliases. For example if we want to find references of DB where we have a previous alias NextLS.DB.

The reference stored in the database should be the expanded alias (I think). You can double check by querying the db with the sqlite3 cli or using a GUI client. So it should "just work" for aliases.

See if I can get the information in a single query instead of two.

We can optimize this later, feel free to not worry about it for now

lib/next_ls.ex

mhanberg · 2023-08-05T14:21:47Z

Feel free to post a screen recording if you get a chance, it's always fun to see it in action 😎

mhanberg · 2023-08-05T14:24:04Z

lib/next_ls.ex

+          SELECT file, start_line, end_line, start_column, end_column
+          FROM "references" as refs
+          WHERE refs.identifier = ?
+            AND refs.arity = ?
+            AND refs.type = ?
+            AND refs.module = ?


in the case that we are finding references for a reference (meaning, a usage of the function, not the definition), we'll want to exclude the reference we're on from the result set.

I think at least, would be valuable to see what other LSPs like gopls and rust analyzer do

ElixirLS doesn't exclude the current reference but we can definitely do it. I'll definitely check what other LSPs do.

mhanberg · 2023-08-05T15:29:31Z

I know you are still in progress, but i'll drop a couple of screen recordings of some odd behavior i'm seeing as of now

Unrelated results for private function

CleanShot.2023-08-05.at.11.24.29.mp4

client side exception when finding references of a function definition

CleanShot.2023-08-05.at.11.26.59.mp4

unrelated results for a remote function

CleanShot.2023-08-05.at.11.28.36.mp4

crbelaus · 2023-08-05T23:10:07Z

Thanks for early the feedback @mhanberg 🙇

I've just pushed another commit that makes the "Find references" functionality work from module/function definitions as well as references (previously it was only working from function references).

I thought that I would need Code.Fragment.surround_context/3 but it looks that we have all the information that we need in the references and symbols tables. The code is very messy at the moment as I am still trying to get the right information out and verify if we are going in the right direction.

Here are a few videos from VSCode with the latest changes:

Find references from a function call

Screen.Recording.2023-08-06.at.01.04.46.mov

Find references from a function definition

Screen.Recording.2023-08-06.at.01.07.33.mov

Find references from a module reference

Screen.Recording.2023-08-06.at.01.06.37.mov

Find references from a module definition

Screen.Recording.2023-08-06.at.01.08.24.mov

mhanberg · 2023-08-05T23:32:13Z

🤯

Beast mode!

crbelaus · 2023-08-06T13:18:09Z

I've just pushed another commit to extract and improve the symbol information query. We now check if we are in a symbol definition first and fallback to check if we are in a function/module reference otherwise.

This logic has been moved into the symbol_info/4 function. I've kept this private function in the same file as I am still unsure what would be the right place for it.

I plan to test this tomorrow for the entire day in my $DAYJOB, which is a pretty big codebase. If I don't find any stability issues or obvious problems I will update the PR description, remove the draft status and open it for review if that's okay for you @mhanberg

mhanberg · 2023-08-06T14:29:23Z

@crbelaus you are a legend!

Thank you so much for this, I'll do a thorough review once you mark as ready, but when I looked yesterday I was already pretty happy.

crbelaus · 2023-08-07T15:58:52Z

I've removed the draft state and this is now ready for review. I've also added a few automated tests to ensure that this works well and doesn't break with future changes.

mhanberg · 2023-08-07T16:58:29Z

test/next_ls_test.exs

+                        }
+                      }
+                    ],
+                    500


I have the timeout for these globally set to 30 seconds, so you can remove all of these custom timeouts, as it's actually making it shorter 😂

https://github.com/elixir-tools/next-ls/blob/main/test/test_helper.exs#L9

I've just pushed a commit that removes those explicit timeouts.

mhanberg · 2023-08-07T18:13:14Z

lib/next_ls.ex

+
+    locations =
+      dispatch(lsp.assigns.registry, :databases, fn databases ->
+        for {database, _} <- databases do


If you are going to flatten it below, you might as well use Enum.flat_map here instead of a comprehension

You are absolutely right! Much better to do it in a single operation since it doesn't hurt readability either.

I've just pushed a commit that replaces the for comprehension with Enum.flat_map as you suggested.

mhanberg · 2023-08-07T18:15:01Z

Just had that one comment, i will test this manually after work and hopefully merge and release before i present at the Denver Elixir Meetup about NextLS, so I can show off your hard work 💪.

I assume you'll be asleep at that time but I'll give you the loudest of shoutouts 👏.

This reverts commit 5a3b530.

* Revert "chore: format sql" This reverts commit b25688a. * Revert "fix(references): ignore references to elixir source code" This reverts commit 6ff4c17. * Revert "fix(references): clamp line and column numbers" This reverts commit 55ead79. * Revert "feat: find references (#139)" This reverts commit 5a3b530.

mhanberg reviewed Aug 5, 2023

View reviewed changes

lib/next_ls.ex Outdated Show resolved Hide resolved

mhanberg reviewed Aug 5, 2023

View reviewed changes

lib/next_ls.ex Outdated Show resolved Hide resolved

mhanberg reviewed Aug 5, 2023

View reviewed changes

crbelaus changed the title ~~WIP: Find references~~ Find references Aug 7, 2023

crbelaus marked this pull request as ready for review August 7, 2023 16:06

mhanberg reviewed Aug 7, 2023

View reviewed changes

mhanberg changed the title ~~Find references~~ feat: find references Aug 7, 2023

mhanberg reviewed Aug 7, 2023

View reviewed changes

crbelaus added 9 commits August 7, 2023 14:26

WIP: find references for functions

fdcfe89

TODO notes

222f928

Find references from function/module definitions

85a57e6

Minor refactor

864f7f2

Handle defmacro

aef66d5

Add tests

2e9c6bd

Handle includeDeclaration option

4ccab8e

Remove timeouts in tests

8a31b86

Use Enum.flat_map instead of for+List.flatten

b1ba45e

mhanberg force-pushed the find-references branch from 2abd08e to b1ba45e Compare August 7, 2023 18:26

mhanberg merged commit 5a3b530 into elixir-tools:main Aug 7, 2023
10 checks passed

github-actions bot mentioned this pull request Aug 7, 2023

chore(main): release 0.7.0 #140

Merged

crbelaus deleted the find-references branch August 8, 2023 05:46

mhanberg added a commit that referenced this pull request Aug 8, 2023

Revert "feat: find references (#139)"

1774440

This reverts commit 5a3b530.

github-actions bot mentioned this pull request Sep 21, 2023

chore(main): release 0.13.0 #237

Closed

This was referenced Sep 30, 2023

chore(main): release 0.13.0 #250

Closed

chore(main): release 0.14.0 #255

Closed

chore(main): release 0.14.0 #256

Closed

github-actions bot mentioned this pull request Jan 18, 2024

chore(main): release 0.16.0 #356

Merged

github-actions bot mentioned this pull request Jun 14, 2024

chore(main): release 1.0.0 #521

Closed

This was referenced Jul 3, 2024

chore(main): release 0.24.0 #528

Closed

chore(main): release 0.24.0 #529

Closed

chore(main): release 0.24.0 #530

Closed

github-actions bot mentioned this pull request Aug 22, 2024

chore(main): release 0.23.2 #542

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: find references #139

feat: find references #139

crbelaus commented Aug 5, 2023 •

edited by mhanberg

Loading

mhanberg commented Aug 5, 2023

mhanberg commented Aug 5, 2023

mhanberg Aug 5, 2023

crbelaus Aug 5, 2023

mhanberg commented Aug 5, 2023

crbelaus commented Aug 5, 2023 •

edited

Loading

mhanberg commented Aug 5, 2023 •

edited

Loading

crbelaus commented Aug 6, 2023 •

edited

Loading

mhanberg commented Aug 6, 2023

crbelaus commented Aug 7, 2023

mhanberg Aug 7, 2023

crbelaus Aug 7, 2023

mhanberg Aug 7, 2023

mhanberg Aug 7, 2023

crbelaus Aug 7, 2023

mhanberg commented Aug 7, 2023

+                                      }
+                                    }
+                                  ],
+

feat: find references #139

feat: find references #139

Conversation

crbelaus commented Aug 5, 2023 • edited by mhanberg Loading

mhanberg commented Aug 5, 2023

mhanberg commented Aug 5, 2023

mhanberg Aug 5, 2023

Choose a reason for hiding this comment

crbelaus Aug 5, 2023

Choose a reason for hiding this comment

mhanberg commented Aug 5, 2023

crbelaus commented Aug 5, 2023 • edited Loading

mhanberg commented Aug 5, 2023 • edited Loading

crbelaus commented Aug 6, 2023 • edited Loading

mhanberg commented Aug 6, 2023

crbelaus commented Aug 7, 2023

mhanberg Aug 7, 2023

Choose a reason for hiding this comment

crbelaus Aug 7, 2023

Choose a reason for hiding this comment

mhanberg Aug 7, 2023

Choose a reason for hiding this comment

mhanberg Aug 7, 2023

Choose a reason for hiding this comment

crbelaus Aug 7, 2023

Choose a reason for hiding this comment

mhanberg commented Aug 7, 2023

crbelaus commented Aug 5, 2023 •

edited by mhanberg

Loading

crbelaus commented Aug 5, 2023 •

edited

Loading

mhanberg commented Aug 5, 2023 •

edited

Loading

crbelaus commented Aug 6, 2023 •

edited

Loading