Bump elixir_sense and use ElixirSense.references/3 #82
Conversation
| %{ | ||
| "uri" => SourceFile.path_to_uri(file), | ||
| "uri" => SourceFile.path_to_uri(reference.uri), |
There was a problem hiding this comment.
For references to variables in the current text file uri will be nil. Will it work?
There was a problem hiding this comment.
Ah, good catch. I'm able to reproduce that locally:
06:59:52.672 [error] Process #PID<0.1010.0> raised an exception
** (FunctionClauseError) no function clause matching in IO.chardata_to_string/1
(elixir) lib/io.ex:557: IO.chardata_to_string(nil)
(elixir) lib/path.ex:680: Path.expand_home/1
(elixir) lib/path.ex:154: Path.expand/1
(language_server) lib/language_server/source_file.ex:54: ElixirLS.LanguageServer.SourceFile.path_to_uri/1
(language_server) lib/language_server/providers/references.ex:41: ElixirLS.LanguageServer.Providers.References.build_loc/1
(elixir) lib/enum.ex:1336: Enum."-map/2-lists^map/1-0-"/2
(kernel) global.erl:425: :global.trans/4
(language_server) lib/language_server/server.ex:355: anonymous fn/5 in ElixirLS.LanguageServer.Server.handle_request/2
@lukaszsamson if the reference.uri returned from ElixirSense is nil can we assume that the uri is the current file? If not could we change ElixirSense to return :nofile (or similar) to make the case more explicit?
There was a problem hiding this comment.
I'm afraid we cannot assume that. IIRC nil will be returned also for macro generated fun calls, builtins etc. I think we should return either :nofile or :current_buffer to clearly discriminate those cases.
Edit:
It will be nil only in the case of current buffer reference. In other cases it will be either file uri or :nofile
There was a problem hiding this comment.
Yeah :current_buffer is the case that we really need. Where :current_buffer means the reference was found in the text that was passed in.
Based on your "Edit" are you saying that we can make an assumption? (or perhaps I'm reading that incorrectly)
There was a problem hiding this comment.
@lukaszsamson do you have an example that would generate a :nofile response? I've tried with a macro generated function but I just get back an empty list from ElixirSense.references/3 when hovering over the macro_a_func() call with these modules:
defmodule ElixirLS.Test.MacroA do
defmacro __using__(_) do
quote do
def macro_a_func do
:ok
end
end
end
end
defmodule ElixirLS.Test.UsesMacroA do
use ElixirLS.Test.MacroA
def my_fun do
macro_a_func()
end
def my_other_fun do
macro_a_func()
end
end
|
Yes, we can. Sorry for the confusion.
… On 6 Dec 2019, at 06:54, Jason Axelson ***@***.***> wrote:
@axelson commented on this pull request.
In apps/language_server/lib/language_server/providers/references.ex:
> %{
- "uri" => SourceFile.path_to_uri(file),
+ "uri" => SourceFile.path_to_uri(reference.uri),
Yeah :current_buffer is the case that we really need. Where :current_buffer means the reference was found in the text that was passed in.
Based on your "Edit" are you saying that we can make an assumption? (or perhaps I'm reading that incorrectly)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
I don’t have an example right now but I remember seeing it somewhere. It could have been with builtin erlang types.
… On 6 Dec 2019, at 18:23, Jason Axelson ***@***.***> wrote:
@axelson commented on this pull request.
In apps/language_server/lib/language_server/providers/references.ex:
> %{
- "uri" => SourceFile.path_to_uri(file),
+ "uri" => SourceFile.path_to_uri(reference.uri),
@lukaszsamson do you have an example that would generate a :nofile response? I've tried with a macro generated function but I just get back an empty list from ElixirSense.references/3 when hovering over the macro_a_func() call with these modules:
defmodule ElixirLS.Test.MacroA do
defmacro __using__(_) do
quote do
def macro_a_func do
:ok
end
end
end
end
defmodule ElixirLS.Test.UsesMacroA do
use ElixirLS.Test.MacroA
def my_fun do
macro_a_func()
end
def my_other_fun do
macro_a_func()
end
end
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
Haven't had any luck trying to trigger that case with builtin erlang types, but finished up the mapping code to handle |
|
@axelson I also tried with no luck. I must have seen it somewhere deeper in the internals of |
|
Okay, that makes sense. But I'm not ready to merge this yet because I'm not occasionally getting runaway memory usage in ElixirLS that I believe may be related/triggered by the ElixirSense updates. When it happens I see my OS memory usage grow from 2GB to 8GB plus 4GB swap (and it goes back down to 2GB when I kill ElixirLS). Haven't had a chance to track it down further yet. |
Now that elixir-lsp/elixir_sense#42 is fixed (via elixir-lsp/elixir_sense#61) and umbrella support is added via elixir-lsp/elixir_sense#46 we no longer need to maintain a separate implementation of `ElixirSense.references/3` and can instead use the implementation provided by `ElixirSense`. This is good because now we're using less of the private internals of `ElixirSense`.
|
Okay, this is what my runaway memory usage looks like in observer: But it doesn't appear to be a single process using all of the memory (although I'm a little suspicious of this chart): |
|
@axelson Unfortunately this does not help much. What do System and Memory allocators tabs show? Is it binaries, atoms or sth else?. Under what kind of workload are you experiencing this? I experienced something similar when developing ElixirSense under vs code with elixir-ls plugin. Elixir-ls was compiling modules ElixirSense modules while I was editing & saving them and started calling them (it seemed they were shadowing ElixirSense modules from deps of elixir-ls). I ended up disabling the plugin for ElixirSense and elixir-ls projects. I haven't noticed anything similar happening on other projects. |
|
@lukaszsamson Okay, was finally able to capture a stacktrace before my system became totally unresponsive. Filed the details in elixir-lsp/elixir_sense#67 And yes developing ElixirLS or ElixirSense with elixir-ls is a bit of a pain because the modules you are editing will immediately be loaded and start to run. It kind of happens on other projects but because those modules aren't used by ElixirLS or ElixirSense there is no real effect (except for the |
|
@axelson Great job finding the root root cause. Could you please try the fix from latest master? |
|
Thanks for looking into it. I'm now running the latest version and if I don't encounter any other issues will get this merged down shortly! |
|
Okay, this has been working well for me. Merging! |
Now that elixir-lsp/elixir_sense#42 is fixed (via elixir-lsp/elixir_sense#61) and umbrella support is added via elixir-lsp/elixir_sense#46 we no longer need to maintain a separate implementation of
ElixirSense.references/3and can instead use the implementation provided by ElixirSense. This is good because now we're using less of the private internals of ElixirSense.Fixes #68
Fixes #39