Introduce references tracer #80
Conversation
lukaszsamson
force-pushed
the
ls-fix-references-1.10
branch
from
049de4b
to
020e6c8
Compare
| pos_integer, | ||
| ElixirSense.Core.References.Tracer.call_trace_t() | nil | ||
| ) :: [References.reference_info()] | ||
| def references(code, line, column, trace \\ nil) do |
There was a problem hiding this comment.
I think that requiring passing in a trace (that was generated by ElixirSense.Core.References.Tracer which isn't marked private, although it probably should be) to a public api could be improved because now callers of ElixirSense.references/4 need to call a potentially private api.
Instead perhaps we should collect and store the compiler trace within ElixirSense itself, although that would require ElixirSense to have it's own supervision hierarchy. Any thoughts on the tradeoffs involved?
There was a problem hiding this comment.
Obviously the tracer API needs to be public.
Regarding setting up a supervision hierarchy in ElixirSense, it would be a major breaking change for the users. ElixirSense is currently a stateless library. For it to work It requires the client to compile and load all the modules, as it is in the case of elixir-LS. In my opinion it is easier to setup and manage the tracer process there.
There was a problem hiding this comment.
Well changing ElixirSense to have it's own supervision hierarchy should generally be transparent to end-users, although I can see an argument that managing the tracer process would be easier from ElixirLS 🤔
There was a problem hiding this comment.
I agree with @axelson, ElixirSense could have its own supervision tree that end-users would not have to know about, especially since ElixirSense is largely obscured behind ElixirLS. Supervision trees are simple to set up, and I could make a PR to add just that to ElixirSense (that I imagine would just supervise the TCP server at this stage).
Alternatively, could we use :ets for storage of the traces and not have to manually manage supervision?
Wandering perhaps too far into the philosophical; Is there a reason other than historical for why ElixirSense and ElixirLS are separate codebases? Wouldn't changes like these be easier to think about if they were unified?
There was a problem hiding this comment.
@lukaszsamson this is fantastic work! The only thing I think we should do differently is to move any version dependent code to the core/normalized folder. Anything that has different implementations for different version should be isolated and kept there. It makes easier for us to keep track of those differences and, when the time comes, decide how to deprecate/remove support for older versions.
So, maybe we could have something like ElixirSense.Core.Normalized.References.calls(...) with the minimum non-generic code. You could even don't expose the tracer to the public API for now and initialize/use it internally inside that function. As far as I could see we can only have a single named tracer process at a time and we don't have concurrent calls so we should be fine with this approach for now.
In the future, as the elixir core team adds more features to the tracer, we can consider replacing our parser with a much simpler version that relies on the tracer. Then we can review the whole approach. WDYT?
|
@msaraiva I fully agree on moving environment dependant code to |
|
I wanted to make a quick note here that in addition to storing the trace in memory, we will want to store it on disk as well. That way every time that ElixirLS starts up we don't need to clear and recompile the user's entire code base. Boundary is the best code base to look at for an example of this. Here is the line where the manifest is read back from a file: https://github.com/sasa1977/boundary/blob/b3ddb21a8ff3fb9da6c6f7d4927f291a6c985a2e/lib/boundary/mix/xref.ex#L73 |
|
@axelson Should we account for the possibility that the codebase was changed while ElixirLS was not running? In that case, the manifest written to disk will be stale. It would be nice if Elixir provided some kind of compilation hash. |
|
Yes that is an important consideration, not sure off the top of my head how to best account for that |
|
Tracers are broken in elixir 1.11.1 (not sure about 1.11.0) - see elixir-lang/elixir#10451 |
There was a problem hiding this comment.
Since this has been open for a while, I thought I would try to rekindle interest with a few suggestions and insights.
Not strictly relevant to this PR, but it occurred to me that we could have multiple reference "providers". i.e. There would be a ElixirSense.References behaviour that could be implemented by one or more modules. With such a system, we could have both a references implementation built on Mix.Tasks.Xref for backwards compatibility and another build on compilation tracers for future compatibility.
| pos_integer, | ||
| ElixirSense.Core.References.Tracer.call_trace_t() | nil | ||
| ) :: [References.reference_info()] | ||
| def references(code, line, column, trace \\ nil) do |
There was a problem hiding this comment.
I agree with @axelson, ElixirSense could have its own supervision tree that end-users would not have to know about, especially since ElixirSense is largely obscured behind ElixirLS. Supervision trees are simple to set up, and I could make a PR to add just that to ElixirSense (that I imagine would just supervise the TCP server at this stage).
Alternatively, could we use :ets for storage of the traces and not have to manually manage supervision?
Wandering perhaps too far into the philosophical; Is there a reason other than historical for why ElixirSense and ElixirLS are separate codebases? Wouldn't changes like these be easier to think about if they were unified?
| }) | ||
|
|
||
| :ok | ||
| end |
There was a problem hiding this comment.
Do we also need a trace function head for {:local_function, ...} for local (in-module) function calls?
|
@lukaszsamson since we're removing the built-in server and probably other stuff that is not necessary anymore, maybe it's time to consider dropping support for versions < |
Keeps them in line with our Version Support Guidance: https://github.com/elixir-lsp/elixir-ls/blob/be0af9dadb2c4cceeb0893fe71d8380debe33f08/DEVELOPMENT.md#version-support-guidelines Also by moving the minimum version of Elixir to 1.10 we can finally use compilation tracers (initial PR at elixir-lsp/elixir_sense#80)
|
FYI, I just opened elixir-lsp/elixir-ls#561 to drop support for Elixir < 1.10.0 (as per the ElixirLS Version Support Guidelines: https://github.com/elixir-lsp/elixir-ls/blob/206240a5d3a116ab84ef1abf16bee4512ac16381/DEVELOPMENT.md#version-support-guidelines) So once that is merged I think we can consider this unblocked and we can move forward on it. |
* Update minimum versions to Elixir 1.10 and OTP 22 Keeps them in line with our Version Support Guidance: https://github.com/elixir-lsp/elixir-ls/blob/be0af9dadb2c4cceeb0893fe71d8380debe33f08/DEVELOPMENT.md#version-support-guidelines Also by moving the minimum version of Elixir to 1.10 we can finally use compilation tracers (initial PR at elixir-lsp/elixir_sense#80) * Fix formatting
* Update minimum versions to Elixir 1.10 and OTP 22 Keeps them in line with our Version Support Guidance: https://github.com/elixir-lsp/elixir-ls/blob/be0af9dadb2c4cceeb0893fe71d8380debe33f08/DEVELOPMENT.md#version-support-guidelines Also by moving the minimum version of Elixir to 1.10 we can finally use compilation tracers (initial PR at elixir-lsp/elixir_sense#80) * Fix formatting
|
Since we dropped support for anything older than 1.10 this can now be addressed |
|
Closing. I opened an new one as github for some reason is not picking up changes in the original branch. Tracked in #160 |
Addresses #75