Fix #134: Do not break when the behaviour is a macro (#135)

* Fix #134: Do not break when the behaviour is a macro * back version
AdRoll · May 26, 2021 · bb8f699 · bb8f699
1 parent 76e0ab5
commit bb8f699
Show file tree

Hide file tree

Showing 4 changed files with 28 additions and 17 deletions.
diff --git a/src/rules/unnecessary_function_arguments.erl b/src/rules/unnecessary_function_arguments.erl
@@ -55,16 +55,15 @@ analyze(FilesAndASTs, _Context) ->
 %% 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
+%%    If that call fails, send empty list.
 -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 ->
-                                  {_, BehaviourMod} = erl_syntax_lib:analyze_wild_attribute(Node),
-                                  FileCallbacks ++ behaviour_callbacks(BehaviourMod);
+                                  FileCallbacks ++ behaviour_callbacks(Node);
                               _ ->
                                   FileCallbacks
                           end
@@ -81,15 +80,23 @@ callback_usage(FilesAndASTs) ->
                 #{},
                 FilesAndASTs).
 
-%% @doc Returns the behaviour's callback list if the given behaviour is a "known behaviour",
-%%      it is an OTP behaviour without "dynamic" callbacks.
-%%      If this is not satisfied, throws an exception.
--spec behaviour_callbacks(atom()) -> [atom()].
-behaviour_callbacks(BehaviourMod) ->
-    case lists:member(BehaviourMod, ?KNOWN_BEHAVIOURS) of
-        true ->
-            BehaviourMod:behaviour_info(callbacks);
-        false ->
+%% @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) ->
+    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)
+            end
+    catch
+        _:syntax_error ->
+            %% There is a macro, then raise ignore
             throw(ignore)
     end.
 

diff --git a/test/files/unnecessary_function_arguments/a_behaviour_imp.erl b/test/files/unnecessary_function_arguments/a_behaviour_imp.erl
@@ -13,6 +13,6 @@ a_kind_of_magic(_) ->
     % this function won't be warned since it's a callback
     implemented.
 
-%% this will warn
+%% this won't warn because the whole file implementing a local behaviour is ignored
 function_with_ignored_arg(_, Value) ->
     Value.
diff --git a/test/files/unnecessary_function_arguments/macro_behaviour_imp.erl b/test/files/unnecessary_function_arguments/macro_behaviour_imp.erl
@@ -1,7 +1,7 @@
 %% @doc This module is a nasty case, it implements a behaviour inside a macro
 -module(macro_behaviour_imp).
 
--define(WHY_DO_YOU_DO_THIS_TO_ME, a_behaviour)
+-define(WHY_DO_YOU_DO_THIS_TO_ME, a_behaviour).
 
 -behaviour(?WHY_DO_YOU_DO_THIS_TO_ME).
 

diff --git a/test/unnecessary_function_arguments_SUITE.erl b/test/unnecessary_function_arguments_SUITE.erl
@@ -21,7 +21,6 @@ with_warnings(_Config) ->
     FileB = "warnings_B.erl",
     FileC = "a_behaviour.erl",
     FileD = "gen_server_imp.erl",
-    FileE = "a_behaviour_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">>},
@@ -31,13 +30,18 @@ with_warnings(_Config) ->
      #{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, FileE, "a_behaviour_imp.erl"]),
+        analyze([FileA, FileB, FileC, FileD, "a_behaviour_imp.erl", "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"]),
+    [] =
+        analyze(["weird.erl",
+                 "clean.erl",
+                 "nifs.erl",
+                 "a_behaviour_imp.erl",
+                 "macro_behaviour_imp.erl"]),
     ok.
 
 %% @doc Macros as function names should work