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

Provide a Gradualizer diagnostic #1117

Merged
merged 23 commits into from
Oct 22, 2021
Merged

Conversation

erszcz
Copy link
Contributor

@erszcz erszcz commented Oct 15, 2021

This implements a basic Gradualizer diagnostic for Erlang LS we, i.e. @garazdawi, Ferenc Pesti and I, built during an Erlang Solutions internal hackathon early this year. Some example screenshots of this integration in action can be seen here - josefs/Gradualizer#344 (comment).

I'm creating this as a draft, since I'm not sure what's the best way to express the dependency on Gradualizer - currently even the newly added test won't pass, since I haven't added Gradualizer to Rebar3 deps. What's the best approach for adding dependencies for optional diagnostics?

Important: Gradualizer, while being continuously improved, is still a work in progress and doesn't successfully self-gradualize yet. It's already useful, but it might also be misleading at times. That being said, I think having the integration in place is worth the effort, as it makes trying out the project and contributing easier.

@robertoaloi
Copy link
Member

Thanks for this, amazing stuff! I asked in our Slack channel what's the Community view on this. I will also take it with the other core maintainers and come back to you shortly :)

@robertoaloi
Copy link
Member

For visibility, copying from Slack:

We briefly discussed this internally. The current take would be to keep it simple as much as we can (e.g. just include gradualizer and have it disabled by default), until we decide on an eventual plugin-based structure

@robertoaloi
Copy link
Member

Also, I briefly tried the Gradualizer on the Erlang LS project itself and I noticed a couple of things:

  • A number of warnings appear from Gradualizer about "undefined functions"
  • Some messages seem truncated. For example:
{
      "source": "Gradualizer",
      "severity": 2,
      "range": {
        "start": {
          "line": 145,
          "character": 0
        },
        "end": {
          "line": 146,
          "character": 0
        }
      },
      "message": "The pattern #{config := #{entries := Entries, task := Task} = Config,"
    },

@erszcz
Copy link
Contributor Author

erszcz commented Oct 16, 2021

Thanks for the feedback!

  • A number of warnings appear from Gradualizer about "undefined functions"

I haven't seen this happening, but I'm not saying it's not - could you provide an example?

  • Some messages seem truncated. For example:

Good catch! Fixed now.

I've added the dependency on Gradualizer, but maybe it's worth waiting for josefs/Gradualizer#358 before merging this one.

@erszcz
Copy link
Contributor Author

erszcz commented Oct 19, 2021

The warnings cleanup in Gradualizer is merged, so I'm clicking "Ready for review" :)

@erszcz erszcz marked this pull request as ready for review October 19, 2021 09:13
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 great, just a couple of questions and nits that can eventually be addressed as follow ups.

apps/els_lsp/src/els_gradualizer_diagnostics.erl Outdated Show resolved Hide resolved
-spec start_and_load() -> boolean().
start_and_load() ->
try application:ensure_all_started(gradualizer) of
{ok, [gradualizer]} ->
Copy link
Member

Choose a reason for hiding this comment

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

In principle this could fail if, for example, gradualizer had an additional dependency which was not started. Maybe relax the matching to an {ok, _} or similar?

Copy link
Member

Choose a reason for hiding this comment

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

Also, I wonder if we shouldn't execute this only once during the negotiation phase between client and server.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gradualizer is written with no runtime dependencies other than OTP libs. This check, together with the second clause {ok, []}, effectively makes this code run only the first time start_and_load is called.

apps/els_lsp/src/els_gradualizer_diagnostics.erl Outdated Show resolved Hide resolved
apps/els_lsp/src/els_gradualizer_diagnostics.erl Outdated Show resolved Hide resolved
FmtError = gradualizer_fmt:format_type_error(Error, FmtOpts),
case re:run(FmtError, "([0-9]+):([0-9]+:)? (.*)",
[{capture, all_but_first, binary}, dotall]) of
{match, [BinLine, _BinCol, Msg]} ->
Copy link
Member

Choose a reason for hiding this comment

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

Can't the column be used for the diagnostic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We had some trouble making this work reliably, but I don't remember the details. I think this is something worth working out in the future, but is not critical for getting the warnings - they're just reported at line granularity for now.

@erszcz
Copy link
Contributor Author

erszcz commented Oct 20, 2021

Having applied the fixes and added Gradualizer as a Rebar3 dependency, I'm getting an unexpected warning that

can't find include lib "gradualizer/include/gradualizer.hrl"

for -include_lib("gradualizer/include/gradualizer.hrl"). in files open in the editor. This header file contains macros for manually adding type annotations to bits of code.

Please wait with the merge until I resolve that. EDIT: resolved, please see the comment below.

@erszcz
Copy link
Contributor Author

erszcz commented Oct 20, 2021

Ok, problem solved. This was an Erlang LS config mistake on my side. It should be:

include_dirs:
  - "_build/default/lib/"

NOT:

include_dirs:
  - "_build/default/lib/*/include"

And if Gradualizer is also added as a project dependency, then -include_lib("gradualizer/include/gradualizer.hrl"). works as expected.

@robertoaloi
Copy link
Member

Thanks!

@robertoaloi robertoaloi merged commit 705f2c0 into erlang-ls:main Oct 22, 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.

3 participants