Merge pull request #878 from gomoripeti/bound_var_pattern_diag

Add diagnostics highlighting already bound variables in patterns

priv/code_navigation/src/diagnostics_bound_var_in_pattern.erl

@@ -0,0 +1,20 @@
+-module(diagnostics_bound_var_in_pattern).
+
+-export([f/1, g/1, h/2]).
+
+f(Var1) ->
+  Var1 = 1.
+
+g(Var2) ->
+  case a:b() of
+    {ok, Var2} -> ok;
+    _ -> error
+  end.
+
+h(Var3, Var4) ->
+  try a:b() of
+    {New, Var3} ->
+      New
+  catch Var4 ->
+      error
+  end.

src/els_bound_var_in_pattern_diagnostics.erl

@@ -0,0 +1,137 @@
+%%==============================================================================
+%% Diagnostics detecting already bound variables in patterns
+%%==============================================================================
+-module(els_bound_var_in_pattern_diagnostics).
+
+%%==============================================================================
+%% Exports
+%%==============================================================================
+-behaviour(els_diagnostics).
+
+-export([ is_default/0
+        , run/1
+        , source/0
+        ]).
+
+%%==============================================================================
+%% Includes
+%%==============================================================================
+-include("erlang_ls.hrl").
+-include_lib("kernel/include/logger.hrl").
+
+%%==============================================================================
+%% Type Definitions
+%%==============================================================================
+
+%%==============================================================================
+%% Callback Functions
+%%==============================================================================
+
+-spec is_default() -> boolean().
+is_default() ->
+  false.
+
+-spec run(uri()) -> [els_diagnostics:diagnostic()].
+run(Uri) ->
+  case filename:extension(Uri) of
+    <<".erl">> ->
+      BoundVarsInPatterns = find_vars(Uri),
+      [make_diagnostic(POI) || POI <- BoundVarsInPatterns];
+    _ ->
+      []
+  end.
+
+-spec source() -> binary().
+source() ->
+  <<"BoundVarInPattern">>.
+
+%%==============================================================================
+%% Internal Functions
+%%==============================================================================
+
+-spec find_vars(uri()) -> [poi()].
+find_vars(Uri) ->
+  {ok, #{text := Text}} = els_utils:lookup_document(Uri),
+  {ok, Forms} = parse_file(Text),
+  lists:flatmap(fun find_vars_in_form/1, Forms).
+
+-spec find_vars_in_form(erl_syntax:forms()) -> [poi()].
+find_vars_in_form(Form) ->
+  case erl_syntax:type(Form) of
+    function ->
+      AnnotatedForm = erl_syntax_lib:annotate_bindings(Form, []),
+      %% There are no bound variables in function heads or guards
+      %% so lets decend straight into the bodies
+      Clauses = erl_syntax:function_clauses(AnnotatedForm),
+      ClauseBodies = lists:map(fun erl_syntax:clause_body/1, Clauses),
+      fold_subtrees(ClauseBodies, []);
+    _ ->
+      []
+  end.
+
+-spec fold_subtrees([[tree()]], [poi()]) -> [poi()].
+fold_subtrees(Subtrees, Acc) ->
+  erl_syntax_lib:foldl_listlist(fun find_vars_in_tree/2, Acc, Subtrees).
+
+-spec find_vars_in_tree(tree(), [poi()]) -> [poi()].
+find_vars_in_tree(Tree, Acc) ->
+  case erl_syntax:type(Tree) of
+    match_expr ->
+      Pattern = erl_syntax:match_expr_pattern(Tree),
+      NewAcc = fold_pattern(Pattern, Acc),
+      find_vars_in_tree(erl_syntax:match_expr_body(Tree), NewAcc);
+    clause ->
+      Patterns = erl_syntax:clause_patterns(Tree),
+      NewAcc = fold_pattern_list(Patterns, Acc),
+      fold_subtrees([erl_syntax:clause_body(Tree)], NewAcc);
+    _ ->
+      fold_subtrees(erl_syntax:subtrees(Tree), Acc)
+  end.
+
+-spec fold_pattern(tree(), [poi()]) -> [poi()].
+fold_pattern(Pattern, Acc) ->
+  erl_syntax_lib:fold(fun find_vars_in_pattern/2, Acc, Pattern).
+
+-spec fold_pattern_list([tree()], [poi()]) -> [poi()].
+fold_pattern_list(Patterns, Acc) ->
+  lists:foldl(fun fold_pattern/2, Acc, Patterns).
+
+-spec find_vars_in_pattern(tree(), [poi()]) -> [poi()].
+find_vars_in_pattern(Tree, Acc) ->
+  case erl_syntax:type(Tree) of
+    variable ->
+      Var = erl_syntax:variable_name(Tree),
+      Anno = erl_syntax:get_ann(Tree),
+      case lists:keyfind(free, 1, Anno) of
+        {free, Free} when Free =:= [Var] ->
+          %% Using already bound variable in pattern
+          [variable(Tree) | Acc];
+        _ ->
+          Acc
+      end;
+    _ ->
+      Acc
+  end.
+
+-spec parse_file(binary()) -> {ok, erl_syntax:forms()}.
+parse_file(Text) ->
+  IoDevice = els_io_string:new(Text),
+  {ok, Forms} = els_dodger:parse(IoDevice, {1, 1}),
+  ok = file:close(IoDevice),
+  {ok, Forms}.
+
+-spec variable(tree()) -> poi().
+variable(Tree) ->
+  Id = erl_syntax:variable_name(Tree),
+  Pos = erl_syntax:get_pos(Tree),
+  Range = els_range:range(Pos, variable, Id, undefined),
+  els_poi:new(Range, variable, Id, undefined).
+
+-spec make_diagnostic(poi()) -> els_diagnostics:diagnostic().
+make_diagnostic(#{id := Id, range := POIRange}) ->
+  Range = els_protocol:range(POIRange),
+  VariableName = atom_to_binary(Id, utf8),
+  Message = <<"Bound variable in pattern: ", VariableName/binary>>,
+  Severity = ?DIAGNOSTIC_HINT,
+  Source = source(),
+  els_diagnostics:make_diagnostic(Range, Message, Severity, Source).

src/els_diagnostics.erl

@@ -57,7 +57,8 @@
 
 -spec available_diagnostics() -> [diagnostic_id()].
 available_diagnostics() ->
-  [ <<"compiler">>
+  [ <<"bound_var_in_pattern">>
+  , <<"compiler">>
   , <<"crossref">>
   , <<"dialyzer">>
   , <<"elvis">>

test/els_diagnostics_SUITE.erl

@@ -11,7 +11,8 @@
         ]).
 
 %% Test cases
--export([ compiler/1
+-export([ bound_var_in_pattern/1
+        , compiler/1
         , compiler_with_behaviour/1
         , compiler_with_broken_behaviour/1
         , compiler_with_custom_macros/1
@@ -71,6 +72,11 @@ end_per_suite(Config) ->
   els_test_utils:end_per_suite(Config).
 
 -spec init_per_testcase(atom(), config()) -> config().
+init_per_testcase(bound_var_in_pattern, Config) ->
+  meck:new(els_bound_var_in_pattern_diagnostics, [passthrough, no_link]),
+  meck:expect(els_bound_var_in_pattern_diagnostics, is_default, 0, true),
+  els_mock_diagnostics:setup(),
+  els_test_utils:init_per_testcase(bound_var_in_pattern, Config);
 init_per_testcase(TestCase, Config) when TestCase =:= code_reload orelse
                                          TestCase =:= code_reload_sticky_mod ->
   mock_rpc(),
@@ -121,6 +127,11 @@ init_per_testcase(TestCase, Config) ->
   els_test_utils:init_per_testcase(TestCase, Config).
 
 -spec end_per_testcase(atom(), config()) -> ok.
+end_per_testcase(bound_var_in_pattern, Config) ->
+  meck:unload(els_bound_var_in_pattern_diagnostics),
+  els_test_utils:end_per_testcase(bound_var_in_pattern, Config),
+  els_mock_diagnostics:teardown(),
+  ok;
 end_per_testcase(TestCase, Config) when TestCase =:= code_reload orelse
                                         TestCase =:= code_reload_sticky_mod ->
   unmock_rpc(),
@@ -161,6 +172,42 @@ end_per_testcase(TestCase, Config) ->
 %%==============================================================================
 %% Testcases
 %%==============================================================================
+-spec bound_var_in_pattern(config()) -> ok.
+bound_var_in_pattern(Config) ->
+  Uri = ?config(diagnostics_bound_var_in_pattern_uri, Config),
+  els_mock_diagnostics:subscribe(),
+  ok = els_client:did_save(Uri),
+  Diagnostics = els_mock_diagnostics:wait_until_complete(),
+  Expected =
+    [ #{message => <<"Bound variable in pattern: Var1">>,
+        range =>
+          #{'end' => #{character => 6, line => 5},
+            start => #{character => 2, line => 5}},
+        severity => 4,
+        source => <<"BoundVarInPattern">>},
+      #{message => <<"Bound variable in pattern: Var2">>,
+        range =>
+          #{'end' => #{character => 13, line => 9},
+            start => #{character => 9, line => 9}},
+        severity => 4,
+        source => <<"BoundVarInPattern">>},
+      #{message => <<"Bound variable in pattern: Var3">>,
+        range =>
+          #{'end' => #{character => 14, line => 15},
+            start => #{character => 10, line => 15}},
+        severity => 4,
+        source => <<"BoundVarInPattern">>},
+      #{message => <<"Bound variable in pattern: Var4">>,
+        range =>
+          #{'end' => #{character => 12, line => 17},
+            start => #{character => 8, line => 17}},
+        severity => 4,
+        source => <<"BoundVarInPattern">>}
+    ],
+  F = fun(#{message := M1}, #{message := M2}) -> M1 =< M2 end,
+  ?assertEqual(Expected, lists:sort(F, Diagnostics)),
+  ok.
+
 -spec compiler(config()) -> ok.
 compiler(Config) ->
   Uri = ?config(diagnostics_uri, Config),

test/els_test_utils.erl

@@ -151,6 +151,7 @@ sources() ->
   , completion_caller
   , completion_snippets
   , diagnostics
+  , diagnostics_bound_var_in_pattern
   , diagnostics_behaviour
   , diagnostics_behaviour_impl
   , diagnostics_macros