RefactorErl Diagnostics #1137

robertfiko · 2021-11-22T21:23:26Z

Description

As you (@robertoaloi) asked, I cleaned the code and created this PR with implementation for unused macro and unsecure call detection with RefactorErl backed.

gomoripeti

nice initiative

gomoripeti · 2021-11-23T13:50:06Z

apps/els_lsp/src/els_referl_unsecure_calls_diagnostics.erl

+        Node -> 
+          case rpc:call(Node, ri, add, [binary_to_list(els_uri:path(Uri))], ?TIME_OUT) of
+            {badrpc, _} ->
+              els_server:send_notification(<<"window/showMessage">>, #{ type => ?MESSAGE_TYPE_ERROR, message => <<"Refactor Erl node is down {badrpc}!">> }),


@robertoaloi would it make sense to add shortcuts to client notifications like els_utils:show_msg(info | warning | error, Fmt :: string | binary [, Args :: list()])

Sure, but we can probably take care of that outside of this PR.

of course 👍

gomoripeti · 2021-11-23T13:53:38Z

apps/els_lsp/src/els_referl_unsecure_calls_diagnostics.erl

+
+
+-spec referl_node() -> atom(). % If called with no args, then try to get it from config
+referl_node() ->


looks like some common referl functionality could be moved to a library module like els_referl_util, eg referl_node/0/1, error handling of rpc call to the referl node, processing common referl responses like [{Option, Tuple, Atom, DataList}] with a fun etc

@gomoripeti I just started an els_referl module for the "common parts" should be finished by the end of the week. Also cleaned out refactor erl node detection and error management will be added.

I agree. It would be nice to decouple the diagnostic backend itself from a module which deals with interfacing with RefactorErl. These could be a single els_refactorerl_diagnostics.erl (which covers all, configurable, diagnostic types) and els_refactorerl.erl

Alright, that's a great tougth. I wrote a plan about this a few comments above.

robertoaloi · 2021-11-23T14:05:02Z

apps/els_core/src/els_config.erl

@@ -55,7 +55,8 @@
               | elvis_config_path
               | indexing_enabled
               | bsp_enabled
-               | compiler_telemetry_enabled.
+               | compiler_telemetry_enabled
+               | refactorerl.


Considering that someone else is trying a similar approach with Wrangler (at least so I've heard), I wonder if the config should be a bit more generic, as in:

external_refactoring_tool: name: refactorerl | wrangler

Or similar.

I checked with Melinda, for this time we wan't integrate the diagnostic and analystic features of RefactorErl intp ELS. So the config file would look like something like this:

refactorerl: node: referl@host diagnostics: - unsecure_call - unused_macros

Where the user can specify which analytic and diagnostic tools they wan't to use (there will be a documentation page on ELS website, where they can find a list of features and how to use). Of course if no refactorerl entry is added to the config file, then it won't do anything.
What are your toughts on this?

The proposal looks good to me.

robertoaloi · 2021-11-23T14:11:12Z

apps/els_lsp/src/els_diagnostics.erl

@@ -64,6 +64,8 @@ available_diagnostics() ->
  , <<"elvis">>
  , <<"unused_includes">>
  , <<"unused_macros">>
+  , <<"referl_unused_macros">>


In my mind, it could make more sense to have <<"refactorerl">> as a single diagnostics backend, instead of one per feature. This would also help removing some of the existing duplication between the two newly introduced modules. What do you think?

Sure thing.
The main conception would be to have:

els_refactorerl_utils for the utility functions and RPC calls and others... and will define the interface with RefactorErl

els_refactorerl_diagnostic for the all of the diagnostics (unused, unsecure, and others ...)
other "groups" of functionalities may be added as single modules in the future.
What do you think?

robertoaloi · 2021-11-23T14:11:56Z

apps/els_lsp/src/els_referl_unsecure_calls_diagnostics.erl

+            _ ->
+              ModuleName = filename:rootname(filename:basename(binary_to_list(els_uri:path(Uri)))),
+              ReferlResult = rpc:call(Node, refusr_sq, run, [[{positions, linecol}, {output, msg}], [], "mods[name=" ++ ModuleName ++ "].funs.unsecure_calls"]),
+              Pois = convertToPoi(ReferlResult),


Nit: we are not using camel case for function names.

Of course, sorry I will change it.

robertoaloi · 2021-11-23T14:17:22Z

apps/els_lsp/src/els_referl_unsecure_calls_diagnostics.erl

+            {badrpc, _} ->
+              els_server:send_notification(<<"window/showMessage">>, #{ type => ?MESSAGE_TYPE_ERROR, message => <<"Refactor Erl node is down {badrpc}!">> }),
+              [];
+            error ->


What's the scenario where a retry can actually help? Also, not sure if we want to really pop a message every second. Maybe use logs instead?

Once the user switches between buffers, then it may occur that the execution of one diagnostic may not be finished on the first module, while another module is added to RefactorErl. The adding may change/refresh RefactorErl's database, so it will refuse the adding. Thus, the retry will be neccessary.

About the messages: of course we do not wan't pop them every execution.

robertoaloi · 2021-11-23T14:19:13Z

apps/els_lsp/src/els_referl_unsecure_calls_diagnostics.erl

+              ModuleName = filename:rootname(filename:basename(binary_to_list(els_uri:path(Uri)))),
+              ReferlResult = rpc:call(Node, refusr_sq, run, [[{positions, linecol}, {output, msg}], [], "mods[name=" ++ ModuleName ++ "].funs.unsecure_calls"]),
+              Pois = convertToPoi(ReferlResult),
+              [make_diagnostic(Poi) || Poi <- Pois]


What's the reason to convert the RefactorErl output to a POI first and then to a diagnostic?

Sure, it can be done in single iteration. I will implement it.

robertoaloi · 2021-11-23T14:22:45Z

apps/els_lsp/src/els_referl_unsecure_calls_diagnostics.erl

+
+
+-spec referl_node() -> atom(). % If called with no args, then try to get it from config
+referl_node() ->


I agree. It would be nice to decouple the diagnostic backend itself from a module which deals with interfacing with RefactorErl. These could be a single els_refactorerl_diagnostics.erl (which covers all, configurable, diagnostic types) and els_refactorerl.erl

robertoaloi · 2021-11-23T14:30:36Z

Definitely a nice addition @robertfiko , thanks! I left a few comments here and there, but from my side the main thing would be to:

Use a single els_refactorerl_diagnostics backend which covers all diagnostics returned by RefactorErl (which should be configurable)
Introduce a separate els_refactorerl module which contains all the logic to interact with the external RefactorErl node

Before this can be merged, we would also need to:

Add tests (probably mocking RefactorErl)
Add an explanation to the user on how to install/configure RefactorErl

The real challenge with RefactorErl, as previously mentioned by @alanz , will be to find a way to provide input to the tool, since this is not really supported by the LSP protocol. We may propose an extension, but that would require custom implementations for all clients. Any suggestion in that sense is more than welcome.

robertfiko · 2021-11-24T10:22:04Z

Definitely a nice addition @robertfiko , thanks! I left a few comments here and there, but from my side the main thing would be to:

Use a single els_refactorerl_diagnostics backend which covers all diagnostics returned by RefactorErl (which should be configurable)

Introduce a separate els_refactorerl module which contains all the logic to interact with the external RefactorErl node

Before this can be merged, we would also need to:

Add tests (probably mocking RefactorErl)

Add an explanation to the user on how to install/configure RefactorErl

The real challenge with RefactorErl, as previously mentioned by @alanz , will be to find a way to provide input to the tool, since this is not really supported by the LSP protocol. We may propose an extension, but that would require custom implementations for all clients. Any suggestion in that sense is more than welcome.

Thank you for your tougths and suggesrtions @robertoaloi!

I will reorded and repurpose the modules as I wrote in a comment.
I will write a documentation page for everything that is needed for the setup. As I can see, I can also contribute to the website, so I will write it there. (and will expand the docs when more functionalites will be added)
I will look into how to add tests to theese functionalities
A done a quick search on the "input" topic, I found something in the MS LSP Doc: https://microsoft.github.io/language-server-protocol/specification#workspace_executeCommand I do not know if that can work, but for now I will clear out things related to this two features, as this will define the interface. After it I will look deeper into the topic.

Thank you for everything, I will add some new commits to the PR in a few days.

Upgrade LS source code, to published 0.21 (LS 0.31)

This is needed to avoid confusion between disabled which should means, that the node is disabled *on purpose of bad behavior* from notconfigured which self-descriptive

Heavy documentation added and new notification utility function The syntax of Node recognition is re-taught to make more sense

Fixing the issues that were described in the PR earlier

robertfiko · 2022-01-28T08:30:47Z

Main changes

I hope I covered everything, that we talked about, if not, let me know!

Documented the code the get more understandable, for you, for me, for everyone
Merged the logic behind the two diagnostics (unsecured calls and unused macros)
The RefactorErl node can be configured in the config file, like this:

refactorerl:
  node: "name@host"

Also, the diagnostics which the user want to use can be added here, instead of burning it into the source code. Kind of like you are doing currently with diagnostics!
Merged the make diagnostic and convert to poi logics into one, so we don't iterate twice.

I hope covered all major changes.

robertoaloi

Nice progress!

I have a couple of suggestions so that we can merge this one in and then iterate.

The extension is somewhat experimental, so it should be off by default
We need some basic tests for this. Those tests could be mocking the RPC
The intermediate POI generation seems un-necessary and could be removed
I would not include error handling as a start and assume the code is up and running when the configuration is present. That should simplify this PR significantly.

apps/els_lsp/src/els_refactorerl_diagnostics.erl

apps/els_lsp/src/els_refactorerl_utils.erl

robertoaloi · 2022-01-30T17:19:18Z

apps/els_lsp/src/els_refactorerl_utils.erl

+check_node(Node) ->
+  Response = rpc:call(Node, ri, ls, [], 10000),
+  case Response of
+    {{ok, _}, {error, _}} ->


What kind of errors do we expect here?

@robertfiko

robertoaloi · 2022-01-30T17:19:57Z

apps/els_lsp/src/els_refactorerl_utils.erl

+      Poi = els_poi:new(Range, application, Id, Name),
+      [ Poi | convert_to_poi(Tail) ];
+    _ ->
+        [] %TODO Robi notify


Maybe write a log message here?

This is also changed, yet if you still think it would be nice to write logs somewhere, let me know

robertoaloi · 2022-01-30T17:21:47Z

apps/els_lsp/src/els_refactorerl_utils.erl

+%% The RefactorErl format is how the refusr_sq:run\3 returns
+convert_to_poi(ReferlResult) ->
+case ReferlResult of
+    [{_, _, _, DataList}] ->


Given that this function takes a generic any() as an input (which is fine, since the type is defined outside of the Erlang LS codebase), it's probably wise to name these variables, to indicate what they mean (e.g. _Message vs _).

That is also changed a bit, as the request to the RefactorErl node was not so stable. I will describe this in a comment when I merge to this PR. But this indication is great idea, I added to the code where I think it needs clarification, if you need it more, please let me know

robertoaloi · 2022-01-30T17:23:33Z

apps/els_lsp/src/els_refactorerl_utils.erl

+      end
+  end.
+
+-spec convert_to_poi(any()) -> poi().


Given the type is defined outside of the Erlang LS code base, I would probably define an alias and add a comment about the expected type:

%% For the type definition, please refer to [LINK HERE] %% -type refactorerl_result() :: [{..., ...}] | ... -type refactorerl_result() :: any().

Yes, that's a great idea I also taught about something similar. I'll check my possibilities on that one

robertoaloi · 2022-01-30T17:27:48Z

apps/els_lsp/src/els_refactorerl_utils.erl

+    [{{_, {FromLine, FromCol}, {ToLine, ToCol}}, Name} | Tail] ->
+      Range = #{ from => {FromLine, FromCol}, to => {ToLine, ToCol} },
+      Id = refactorerl_poi, %"{module(), 'atom()', 'arity()''}",
+      Poi = els_poi:new(Range, application, Id, Name),


Any reason why we generate an intermediate POI here? Why not to generate diagnostics directly from RefactorErl results?

Yes I also merged this, it will come soon to this PR ;-)

This function clause has an importance, when a diagnostic has no result

This is due to preparing to new diagnostics

Rebasing from 'main', upstream

robertoaloi

Left a couple of comments, but nothing blocking. If CI passes, let's get this one in!

robertoaloi · 2022-03-31T10:33:52Z

apps/els_lsp/test/els_diagnostics_SUITE.erl

+  meck:expect(els_refactorerl_diagnostics, is_default, 0, true).
+
+
+unmock_refactoerl() ->


Typo: refactorerl

robertoaloi · 2022-03-31T10:34:18Z

apps/els_lsp/test/els_diagnostics_SUITE.erl

@@ -818,3 +857,29 @@ src_path(Module) ->

 include_path(Header) ->
  filename:join(["code_navigation", "include", Header]).
+


These could be moved to a separate module.

robertoaloi · 2022-03-31T10:35:19Z

apps/els_lsp/src/els_refactorerl_diagnostics.erl

+enabled_diagnostics() ->
+  case els_config:get(refactorerl) of
+    #{"diagnostics" := List} ->
+      [list_to_atom(Element) || Element <- List];


Nit: It would be good to avoid the list_to_atom here.

robertoaloi · 2022-03-31T10:36:06Z

apps/els_lsp/src/els_refactorerl_utils.erl

+    {ok, Node} ->
+      %% returns error | ok
+      rpc:call(Node, referl_els, run_diagnostics, [DiagnosticAliases, Module]);
+    _ -> % In this case there was probably error.


You probably want to log a message here.

robertoaloi · 2022-04-01T13:24:54Z

Thanks!

robertfiko added 7 commits November 22, 2021 21:31

Configuration for Refactor Erl Node

e5de42f

Referl unused macros with basic node detect

82dfd3b

source() formatted

10f88b8

Unsecure Calls by RefactorErl

78c8a94

Behaviour basic approach

70ba798

RefactorErl conversion module

4c0f14c

Resolved Merge

c9e7521

gomoripeti reviewed Nov 23, 2021

View reviewed changes

robertoaloi reviewed Nov 23, 2021

View reviewed changes

robertfiko and others added 14 commits November 26, 2021 00:36

Nameing conventions

539b392

Changed to map, to extend confiureability

222ac75

Better node detection, utility module

9e86bc5

Error management unifying. Code reorganisation

cf4eaa7

First run for a common diagnostic module

c10e70f

Merge pull request #1 from robertfiko/referl_vscode_0_21

6ff2301

Upgrade LS source code, to published 0.21 (LS 0.31)

Minor fix

b4f1302

Commond diagnostic backend

44ead24

Styling conventions

5c9e80e

Change 'disabled' to 'notconfigured'

ddc2975

This is needed to avoid confusion between disabled which should means, that the node is disabled *on purpose of bad behavior* from notconfigured which self-descriptive

Documentation and utility functions, fixes

a402ea6

Heavy documentation added and new notification utility function The syntax of Node recognition is re-taught to make more sense

Documentation added

b3ca287

Line length changes

4401ef5

Merge pull request #2 from robertfiko/referl_diagnostic_common_poi2

e7ddf1d

Fixing the issues that were described in the PR earlier

robertoaloi requested changes Jan 30, 2022

View reviewed changes

robertfiko added 3 commits February 2, 2022 19:30

Minor changes

cb0c5fa

New, recieve based communication is working

aa5c787

Working with double diagnositc processing

d523dd4

robertfiko and others added 27 commits March 27, 2022 20:06

make_diagnostic no result clause

88aaccf

This function clause has an importance, when a diagnostic has no result

Diagnostic description made

84e436a

Coding conventions

f5b7518

a few dyalizer errors

6527f1e

Case clause

9aa18f9

Function spec

ce85588

POI removal

5268394

Diagnostic descriptor change

fc7fc1f

This is due to preparing to new diagnostics

Retry removal, smaller fixes

26bc7ad

Working logic, cleaning out...

9ef5ceb

Cleaned out

5cdde72

Linting

694c47a

Test drafts

65842d6

Rebase conflict

cc8b487

Tests

167e6a9

Passing tests, draft

d9f89e2

rebase conflict resolve

1a0e7b8

Test failing due to it is not configured

8628705

Test

c4c8a3f

@doc tag fix

0fb3ba6

doc fix2

a0e65bd

Diagnostic config option

1389a30

Code clearing and diagnostic config

faf0504

rebase conflict resolve

d5e490d

lint

e9b8dfa

Merge branch 'referl_diagnostics' into referl_diag_conf2

d8ca672

Merge pull request #7 from robertfiko/referl_diag_conf2

6c56b4d

Rebasing from 'main', upstream

robertoaloi approved these changes Mar 31, 2022

View reviewed changes

robertoaloi merged commit b87b84d into erlang-ls:main Apr 1, 2022



		-spec referl_node() -> atom(). % If called with no args, then try to get it from config
		referl_node() ->

		meck:expect(els_refactorerl_diagnostics, is_default, 0, true).


		unmock_refactoerl() ->

		@@ -818,3 +857,29 @@ src_path(Module) ->

		include_path(Header) ->
		filename:join(["code_navigation", "include", Header]).

RefactorErl Diagnostics #1137

RefactorErl Diagnostics #1137

Conversation

robertfiko commented Nov 22, 2021

Description

gomoripeti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertfiko Nov 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertoaloi commented Nov 23, 2021

robertfiko commented Nov 24, 2021

robertfiko commented Jan 28, 2022

Main changes

robertoaloi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertoaloi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertoaloi commented Apr 1, 2022

robertfiko Nov 23, 2021 •

edited

Loading