Fix #143: Analyze private function on unknown/dynamic behavior implem…

…entations (#147)

* Fix #143: Analyze private function on unknown/dynamic behavior implementations

* Clarification

* Update test/files/unnecessary_function_arguments/a_behaviour_imp.erl

Co-authored-by: Pablo Brud <[email protected]>

Co-authored-by: Pablo Brud <[email protected]>

.github/workflows/erlang.yml

@@ -27,4 +27,4 @@ jobs:
     - name: Format check
       run: rebar3 format --verify
     - name: Run tests and verifications
-      run: rebar3 dialyzer || rebar3 test # first dialyzer run always complains and 'test' alias runs dialyzer
+      run: rebar3 test || rebar3 test # first dialyzer run always complains and 'test' alias runs dialyzer

src/rules/unnecessary_function_arguments.erl

@@ -38,7 +38,7 @@
 -elvis([{elvis_style, invalid_dynamic_call, disable},
         {elvis_style, atom_naming_convention, #{regex => "^([a-zA-Z][a-z0-9]*_?)*$"}}]).
 
--type imp_callbacks() :: #{File :: string() => [tuple()] | ignore}.
+-type imp_callbacks() :: #{File :: string() => [tuple()] | syntax_error}.
 
 %% @private
 -spec analyze(hank_rule:asts(), hank_context:t()) -> [hank_rule:result()].
@@ -47,25 +47,24 @@ analyze(FilesAndASTs, _Context) ->
     [Result
      || {File, AST} <- FilesAndASTs,
         not hank_utils:is_old_test_suite(File),
-        not is_unrecognized_behaviour(File, ImpCallbacks),
+        is_parseable(File, ImpCallbacks),
         Node <- AST,
         erl_syntax:type(Node) == function,
         not is_callback(Node, File, ImpCallbacks),
         Result <- analyze_function(File, Node)].
 
 %% @doc Constructs a map with the callbacks of all the files.
 %% 1. collect all the behaviors that the file implements.
-%% 2. for each one of them evaluate with BehaviourMod:behaviour_info(callbacks)
-%%    and keep the tuples in a single list.
-%%    If that call fails, send empty list.
+%% 2. for each one of them, build the list of their possible callbacks.
+%% 3. if that list could not be built (usually because of macros), adds 'syntax_error' instead.
 -spec callback_usage(hank_rule:asts()) -> imp_callbacks().
 callback_usage(FilesAndASTs) ->
     lists:foldl(fun({File, AST}, Result) ->
                    FoldFun =
                        fun(Node, FileCallbacks) ->
                           case hank_utils:node_has_attrs(Node, [behaviour, behavior]) of
                               true ->
-                                  FileCallbacks ++ behaviour_callbacks(Node);
+                                  FileCallbacks ++ behaviour_callbacks(Node, AST);
                               _ ->
                                   FileCallbacks
                           end
@@ -74,8 +73,8 @@ callback_usage(FilesAndASTs) ->
                        try
                            erl_syntax_lib:fold(FoldFun, [], erl_syntax:form_list(AST))
                        catch
-                           ignore ->
-                               ignore
+                           syntax_error ->
+                               syntax_error
                        end,
                    maps:put(File, ResultsForFile, Result)
                 end,
@@ -85,23 +84,48 @@ callback_usage(FilesAndASTs) ->
 %% @doc Returns the behaviour's callback list if the given behaviour Node is a "known behaviour",
 %%      this means it is an OTP behaviour without "dynamic" callbacks.
 %%      If this is not satisfied or the behaviour attribute contains a macro,
-%%      throws an ignore exception.
--spec behaviour_callbacks(erl_syntax:syntaxTree()) -> [atom()].
-behaviour_callbacks(Node) ->
+%%      this function returns the whole list of functions that exported from the file.
+%%      That's because, for dynamic behaviors, any exported function can be the implementation
+%%      of a callback.
+-spec behaviour_callbacks(erl_syntax:syntaxTree(), erl_syntax:forms()) ->
+                             [{atom(), non_neg_integer()}].
+behaviour_callbacks(Node, AST) ->
     try erl_syntax_lib:analyze_wild_attribute(Node) of
         {_, BehaviourMod} ->
             case lists:member(BehaviourMod, ?KNOWN_BEHAVIOURS) of
                 true ->
                     BehaviourMod:behaviour_info(callbacks);
                 false ->
-                    throw(ignore)
+                    module_exports(AST)
             end
     catch
         _:syntax_error ->
-            %% There is a macro, then raise ignore
-            throw(ignore)
+            %% There is a macro, then return all its exports, just in case
+            module_exports(AST)
     end.
 
+-spec module_exports(erl_syntax:forms()) -> [{atom(), non_neg_integer()}].
+module_exports(AST) ->
+    FoldFun =
+        fun(Node, Exports) ->
+           case erl_syntax:type(Node) of
+               attribute ->
+                   try erl_syntax_lib:analyze_attribute(Node) of
+                       {export, NewExports} ->
+                           Exports ++ NewExports;
+                       _ ->
+                           Exports
+                   catch
+                       _:syntax_error ->
+                           %% Probably macros, we can't parse this module
+                           throw(syntax_error)
+                   end;
+               _ ->
+                   Exports
+           end
+        end,
+    erl_syntax_lib:fold(FoldFun, [], erl_syntax:form_list(AST)).
+
 %% @doc It will check if arguments are ignored in all function clauses:
 %%      [(_a, b, _c), (_x, b, c)]
 %%      [[1, 0, 1], [1, 0, 0]] => [1, 0, 0] => warning 1st param!
@@ -160,18 +184,14 @@ is_clause_a_nif_stub(Clause) ->
 %% @doc Checks if the given function node implements a callback
 -spec is_callback(erl_syntax:syntaxTree(), string(), imp_callbacks()) -> boolean().
 is_callback(FunctionNode, File, ImpCallbacks) ->
-    case maps:get(File, ImpCallbacks, []) of
-        Callbacks when is_list(Callbacks) ->
-            lists:member(
-                hank_utils:function_tuple(FunctionNode), Callbacks);
-        _ ->
-            false
-    end.
+    lists:member(
+        hank_utils:function_tuple(FunctionNode), maps:get(File, ImpCallbacks, [])).
 
-%% @doc Returns true if the file is an unrecongized_behaviour
--spec is_unrecognized_behaviour(string(), imp_callbacks()) -> boolean().
-is_unrecognized_behaviour(File, ImpCallbacks) ->
-    maps:get(File, ImpCallbacks, []) =:= ignore.
+%% @doc Returns true if hank could parse the file.
+%%      Otherwise the file is ignored and no warnings are reported for it
+-spec is_parseable(string(), imp_callbacks()) -> boolean().
+is_parseable(File, ImpCallbacks) ->
+    maps:get(File, ImpCallbacks, []) =/= syntax_error.
 
 %% @doc Computes position by position (multiply/and)
 %%      Will be 1 only when an argument is unused over all the function clauses

test/files/unnecessary_function_arguments/a_behaviour_imp.erl

@@ -1,5 +1,6 @@
 %% @doc This module implements a local behaviour:
-%%      The unnecessary_function_arguments rule will be ignored for the whole file.
+%%      The unnecessary_function_arguments rule will be ignored for all
+%%      exported functions since we can't tell which ones are dynamic callbacks.
 -module(a_behaviour_imp).
 
 -behaviour(a_behaviour).
@@ -13,6 +14,11 @@ a_kind_of_magic(_) ->
     % this function won't be warned since it's a callback
     implemented.
 
-%% this won't warn because the whole file implementing a local behaviour is ignored
+%% this exported function won't warn because the module implements a local behaviour
 function_with_ignored_arg(_, Value) ->
+    non_exported_function_with(ignored_arg, Value).
+
+%% this should produce a warning since, being a non-exported function, it can't
+%% be a callback implementation.
+non_exported_function_with(_IgnoredArg, Value) ->
     Value.

test/files/unnecessary_function_arguments/clean.erl

@@ -5,8 +5,10 @@
 -record(r, {fc = function_call}).
 
 -export([single_fun/2, multi_fun/3]).
+-export([function_call/0, function_call/1, function_call/2, function_call/3]).
 
--export( [ function_call/ 0 , function_call/ 1 , function_call/ 2 , function_call/ 3 ] ) single_fun( Arg1 , Arg2 ) -> Arg1 * Arg2 .
+single_fun(Arg1, Arg2) ->
+    Arg1 * Arg2.
 
 multi_fun(_Arg1, Arg2, Arg3) when Arg2 > 1 ->
     [Arg2, Arg3];

test/unnecessary_function_arguments_SUITE.erl

@@ -20,28 +20,25 @@ with_warnings(_Config) ->
     FileA = "warnings_A.erl",
     FileB = "warnings_B.erl",
     FileC = "a_behaviour.erl",
-    FileD = "gen_server_imp.erl",
+    FileD = "a_behaviour_imp.erl",
+    FileE = "gen_server_imp.erl",
     [#{file := FileC,
        text := <<"a_function_from_the_behaviour/1 doesn't need its #1 argument">>},
-     #{file := FileD, text := <<"my_function/1 doesn't need its #1 argument">>},
-     #{file := FileD, text := <<"my_other_function/2 doesn't need its #2 argument">>},
+     #{file := FileD, text := <<"non_exported_function_with/2 doesn't need its #1 argument">>},
+     #{file := FileE, text := <<"my_function/1 doesn't need its #1 argument">>},
+     #{file := FileE, text := <<"my_other_function/2 doesn't need its #2 argument">>},
      #{file := FileA, text := <<"single_fun/2 doesn't need its #2 argument">>},
      #{file := FileA, text := <<"multi_fun/3 doesn't need its #3 argument">>},
      #{file := FileA, text := <<"unicode_αβåö/1 doesn't need its #1 argument"/utf8>>},
      #{file := FileA, text := <<"with_nif_stub/2 doesn't need its #1 argument">>},
      #{file := FileB, text := <<"underscore/3 doesn't need its #1 argument">>}] =
-        analyze([FileA, FileB, FileC, FileD, "a_behaviour_imp.erl", "macro_behaviour_imp.erl"]),
+        analyze([FileA, FileB, FileC, FileD, FileE, "macro_behaviour_imp.erl"]),
     ok.
 
 %% @doc Hank finds nothing!
 without_warnings(_Config) ->
     ct:comment("Should not detect anything since the files are clean from warnings"),
-    [] =
-        analyze(["weird.erl",
-                 "clean.erl",
-                 "nifs.erl",
-                 "a_behaviour_imp.erl",
-                 "macro_behaviour_imp.erl"]),
+    [] = analyze(["weird.erl", "clean.erl", "nifs.erl", "macro_behaviour_imp.erl"]),
     ok.
 
 %% @doc Macros as function names should work