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

[WIP]: Sheldon diagnostics #1148

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

vkatsuba
Copy link

No description provided.

@vkatsuba vkatsuba force-pushed the sheldon_diagnostics branch 5 times, most recently from 79f3cac to 5fe3acd Compare December 1, 2021 16:48
apps/els_lsp/src/els_diagnostics.erl Outdated Show resolved Hide resolved
apps/els_lsp/src/els_sheldon_diagnostics.erl Outdated Show resolved Hide resolved
apps/els_lsp/src/els_sheldon_diagnostics.erl Outdated Show resolved Hide resolved
apps/els_lsp/src/els_sheldon_diagnostics.erl Outdated Show resolved Hide resolved
elvis.config Outdated Show resolved Hide resolved
rebar.config Outdated Show resolved Hide resolved
@vkatsuba vkatsuba changed the title [WIP]: Sheldon diagnostics Sheldon diagnostics Dec 1, 2021
@vkatsuba vkatsuba marked this pull request as ready for review December 1, 2021 17:13
* return map instead of list in diagnostic sheldon
@vkatsuba vkatsuba force-pushed the sheldon_diagnostics branch 2 times, most recently from f3c3852 to dedc7dd Compare December 2, 2021 14:36
Copy link
Member

@robertoaloi robertoaloi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I left a couple of final comments. After that, we should be good to go.

apps/els_lsp/src/els_diagnostics.erl Outdated Show resolved Hide resolved
apps/els_lsp/src/els_sheldon_diagnostics.erl Outdated Show resolved Hide resolved
apps/els_lsp/src/els_sheldon_diagnostics.erl Show resolved Hide resolved
apps/els_lsp/src/els_sheldon_diagnostics.erl Outdated Show resolved Hide resolved
apps/els_lsp/src/els_sheldon_diagnostics.erl Show resolved Hide resolved
@vkatsuba vkatsuba force-pushed the sheldon_diagnostics branch 6 times, most recently from 9839af8 to 6c0a23b Compare December 10, 2021 20:09
@robertoaloi
Copy link
Member

robertoaloi commented Dec 11, 2021

Hi @vkatsuba ! I gave this a try, but after enabling the sheldon diagnostics backend via the erlang_ls.config file I see some crashes:

  1. The sheldon functions are undef. I suspect the app is not included in the escript correctly (it probably misses in the .app.src dependency chain.
  2. The sheldon/priv/lang folder required by the backend also doesn't seem to be available, so even after patching 1, the sheldon_sup crashes.

Also, it looks like sheldon comes with a 5MB english dictionary. Including it would make Erlang LS more than double its size.
I wonder if, instead, we shouldn't provide the user the ability to point to a custom dictionary via the config file.
Finally, I suspect we will have to move the ast part from the rebar plugin to sheldon itself. The fact is that, by adding rebar3_sheldon as a dependency in the .app.src causes some rebar3 logic to trigger. In our internal project (1.5 million lines of code) this seems to trigger compilation for the whole project, which we cannot afford.

@vkatsuba
Copy link
Author

vkatsuba commented Dec 11, 2021

Hi @robertoaloi!

The sheldon functions are undef.

Maybe the logic of init function or approach is wrong. Because we load sheldon in init behavior. Could you try do it the same with ref 652bc5830d640309aacfa44a723f7043956c1e02? I suppose this commit should be works.

The sheldon functions are undef. I suspect the app is not included in the escript correctly (it probably misses in the .app.src dependency chain.

We cannot add sheldon to .app.src because sheldon will try run and load and store dictionary by default. While it is expected that Sheldon can only start at a certain moment. Most likely you need to extend the plugin and add an API to run Sheldon separately.

The sheldon/priv/lang folder required by the backend also doesn't seem to be available, so even after patching 1, the sheldon_sup crashes.

This folder is a part of sheldon

Finally, I suspect we will have to move the ast part from the rebar plugin to sheldon itself.

If we do it, it will be no make sense to integrate rebar3_sheldon.

this seems to trigger compilation for the whole project, which we cannot afford.

The rebar3_sheldon works mostly the same as rebar3_format which is also added as a dependency to the current project. I changed the API on purpose of rebar3_sheldon for current integration for call functions for analysis of AST.

@robertoaloi
Copy link
Member

So @vkatsuba if this doesn't work out of the box, how are you managing to get the diagnostics yourself?

@vkatsuba
Copy link
Author

@robertoaloi I was focused on common tests only and didn't try it in some IDE.

@vkatsuba vkatsuba marked this pull request as draft December 11, 2021 18:21
@vkatsuba vkatsuba changed the title Sheldon diagnostics [WIP]: Sheldon diagnostics Dec 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants