Fix #69: Rename unnecessary_function_arguments rule (#78)

* Fix #69: Rename unnecessary_function_arguments rule

* Fix #79: NIF check crashing

* A bit of cleanup

priv/test_files/unnecessary_function_arguments/clean.erl

@@ -0,0 +1,34 @@
+-module(clean).
+
+-define(FC(X, Y), {X, Y}).
+-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])
+
+single_fun(Arg1, Arg2) ->
+    Arg1 * Arg2.
+
+multi_fun(_Arg1, Arg2, Arg3) when Arg2 > 1 ->
+    [Arg2, Arg3];
+multi_fun(Arg1, _, Arg3) ->
+    [Arg1, Arg3].
+
+function_call() ->
+    function_call(not_a_nif).
+
+function_call(UsedArg1) ->
+    F = function_call,
+    F(UsedArg1, two).
+
+function_call(UsedArg1, UsedArg2) ->
+    ?FC(UsedArg1, UsedArg2).
+
+function_call(UsedArg1, UsedArg2, vars) ->
+    UsedArg2:UsedArg2(UsedArg1);
+function_call(UsedArg1, UsedArg2, macros) ->
+    ?MODULE:function_call(UsedArg1, UsedArg2);
+function_call(UsedArg1, UsedArg2, records) ->
+    (UsedArg2#r.fc)(UsedArg1);
+function_call(UsedArg1, UsedArg2, weird_stuff) ->
+    begin io:format("~p~n", [UsedArg1]), function_call end(UsedArg2).

rebar.config

@@ -15,7 +15,7 @@
 
 {minimum_otp_vsn, "21"}.
 
-{deps, [{katana_code, "1.1.1"}]}.
+{deps, [{katana_code, "~> 1.1.0"}]}.
 
 {project_plugins, [{rebar3_format, "~> 0.10.0"}, {rebar3_lint, "~> 0.3.2"}, rebar3_hex]}.
 

rebar.lock

@@ -1,8 +1,8 @@
 {"1.2.0",
-[{<<"katana_code">>,{pkg,<<"katana_code">>,<<"1.1.1">>},0}]}.
+[{<<"katana_code">>,{pkg,<<"katana_code">>,<<"1.1.2">>},0}]}.
 [
 {pkg_hash,[
- {<<"katana_code">>, <<"7EFFD69078AD6263F54C0E02231387A292A4FBD80BF79E5F62D666410D798038">>}]},
+ {<<"katana_code">>, <<"4336743263236C3213FF1B979ECAE263940B9084ACE90E9434838F3F98CCA578">>}]},
 {pkg_hash_ext,[
- {<<"katana_code">>, <<"94F4C9DF74F09B8944C5B04902F7902A3C5A1B289E45CFC1A83A462A669359FA">>}]}
+ {<<"katana_code">>, <<"E7E6162A44E826A03F68B503B7D92981B894BF834C1EF0E647783F7D6688021C">>}]}
 ].

src/hank_utils.erl

@@ -4,10 +4,9 @@
 %% To allow erl_syntax:syntaxTree/0 type spec
 -elvis([{elvis_style, atom_naming_convention, #{regex => "^([a-zA-Z][a-z0-9]*_?)*$"}}]).
 
--export([macro_arity/1, macro_name/1, parse_macro_name/1, macro_definition_name/1,
-         function_description/1, application_node_to_mfa/1, attr_name/1, ast_has_attrs/2,
-         node_has_attrs/2, attr_args/3, attr_args/2, attr_args_concrete/2, implements_behaviour/1,
-         node_line/1, paths_match/2, format_text/2]).
+-export([macro_arity/1, macro_name/1, macro_definition_name/1, function_description/1,
+         application_node_to_mfa/1, attr_name/1, node_has_attrs/2, attr_args_concrete/2,
+         implements_behaviour/1, node_line/1, paths_match/2, format_text/2]).
 
 %% @doc Get the macro arity of given Node
 -spec macro_arity(erl_syntax:syntaxTree()) -> none | pos_integer().
@@ -22,16 +21,18 @@ macro_arity(Node) ->
 %% @doc Get the parsed macro name of given Node
 -spec macro_name(erl_syntax:syntaxTree()) -> string().
 macro_name(Node) ->
-    parse_macro_name(erl_syntax:macro_name(Node)).
+    parse_node_name(erl_syntax:macro_name(Node)).
 
-%% @doc Parse the given Node macro name
--spec parse_macro_name(erl_syntax:syntaxTree()) -> string().
-parse_macro_name(Node) ->
+%% @doc Parse the given Node name
+-spec parse_node_name(erl_syntax:syntaxTree()) -> string().
+parse_node_name(Node) ->
     case erl_syntax:type(Node) of
         variable ->
             erl_syntax:variable_literal(Node);
         atom ->
-            erl_syntax:atom_name(Node)
+            erl_syntax:atom_name(Node);
+        macro ->
+            parse_node_name(erl_syntax:macro_name(Node))
     end.
 
 %% @doc Get the macro definition name and arity of a given Macro Node.
@@ -41,7 +42,7 @@ macro_definition_name(Node) ->
     case erl_syntax:type(MacroNameNode) of
         application ->
             Operator = erl_syntax:application_operator(MacroNameNode),
-            MacroName = parse_macro_name(Operator),
+            MacroName = parse_node_name(Operator),
             MacroArity = length(erl_syntax:application_arguments(MacroNameNode)),
             {MacroName, MacroArity};
         variable ->
@@ -66,16 +67,27 @@ function_description(Node) ->
 
 %% @doc Returns a MFA tuple for given application node
 -spec application_node_to_mfa(erl_syntax:syntaxTree()) ->
-                                 undefined | {string(), string(), [erl_syntax:syntaxTree()]}.
+                                 undefined |
+                                 {string(), string(), [erl_syntax:syntaxTree()]} |
+                                 {string(), [erl_syntax:syntaxTree()]}.
 application_node_to_mfa(Node) ->
     case erl_syntax:type(Node) of
         application ->
             Operator = erl_syntax:application_operator(Node),
-            Module = erl_syntax:module_qualifier_argument(Operator),
-            Function = erl_syntax:module_qualifier_body(Operator),
-            {erl_syntax:atom_name(Module),
-             erl_syntax:atom_name(Function),
-             erl_syntax:application_arguments(Node)};
+            case erl_syntax:type(Operator) of
+                module_qualifier ->
+                    Module = erl_syntax:module_qualifier_argument(Operator),
+                    Function = erl_syntax:module_qualifier_body(Operator),
+                    {parse_node_name(Module),
+                     parse_node_name(Function),
+                     erl_syntax:application_arguments(Node)};
+                atom ->
+                    {erl_syntax:atom_name(Operator), erl_syntax:application_arguments(Node)};
+                variable ->
+                    {erl_syntax:variable_literal(Operator), erl_syntax:application_arguments(Node)};
+                _ ->
+                    undefined
+            end;
         _ ->
             undefined
     end.
@@ -91,14 +103,6 @@ attr_name(Node) ->
             N
     end.
 
-%% @doc Whether the given AST nodes list
-%%      has defined the given AttrNames attribute names or not
--spec ast_has_attrs(erl_syntax:forms(), atom() | [atom()]) -> boolean().
-ast_has_attrs(AST, AttrName) when not is_list(AttrName) ->
-    ast_has_attrs(AST, [AttrName]);
-ast_has_attrs(AST, AttrNames) ->
-    lists:any(fun(Node) -> node_has_attrs(Node, AttrNames) end, AST).
-
 %% @doc Whether the given Node node
 %%      has defined the given AttrNames attribute names or not
 -spec node_has_attrs(erl_syntax:syntaxTree(), atom() | [atom()]) -> boolean().
@@ -118,11 +122,6 @@ attr_args(AST, AttrNames, MapFunc) ->
         node_has_attrs(Node, AttrNames),
         AttrArg <- erl_syntax:attribute_arguments(Node)].
 
-%% @doc Same as attr_args/3 but with a dummy default MapFunc
--spec attr_args(erl_syntax:forms(), atom() | [atom()]) -> [term()].
-attr_args(AST, AttrName) ->
-    attr_args(AST, AttrName, fun(AttrArg) -> AttrArg end).
-
 %% @doc Same as attr_args/3 but calling erl_syntax:concrete/1 for each element
 -spec attr_args_concrete(erl_syntax:forms(), atom() | [atom()]) -> [term()].
 attr_args_concrete(AST, AttrName) ->
@@ -131,7 +130,7 @@ attr_args_concrete(AST, AttrName) ->
 %% @doc Whether the given AST nodes list has behaviours implemented or not
 -spec implements_behaviour(erl_syntax:forms()) -> boolean().
 implements_behaviour(AST) ->
-    ast_has_attrs(AST, [behaviour, behavior]).
+    lists:any(fun(Node) -> node_has_attrs(Node, [behaviour, behavior]) end, AST).
 
 %% @doc Returns the line number of the given node
 -spec node_line(erl_syntax:syntaxTree()) -> non_neg_integer().

src/rules/unused_ignored_function_params.erl → src/rules/unnecessary_function_arguments.erl

@@ -1,10 +1,10 @@
-%% @doc A rule to detect unused function parameters.
+%% @doc A rule to detect unneded function parameters.
 %%      <p>The rule emits a warning for each function parameter that is consistently
 %%      ignored in all function clauses.</p>
 %%      <p>To avoid this warning, remove the unused parameter(s).</p>
 %%      <p><b>Note:</b> This rule will not emit a warning if the function
-%%      implements a behaviour callback.</p>
--module(unused_ignored_function_params).
+%%      implements a behaviour callback or a NIF call.</p>
+-module(unnecessary_function_arguments).
 
 -behaviour(hank_rule).
 
@@ -111,5 +111,5 @@ check_computed_results(FuncDesc, Line, Results) ->
     Errors.
 
 set_error(Line, Pos, FuncDesc) ->
-    Text = hank_utils:format_text("Param #~p is not used at ~ts", [Pos, FuncDesc]),
+    Text = hank_utils:format_text("~ts doesn't need its #~p argument", [FuncDesc, Pos]),
     {error, Line, Text}.

test/unused_ignored_function_params_SUITE.erl → test/unnecessary_function_arguments_SUITE.erl

@@ -1,4 +1,4 @@
--module(unused_ignored_function_params_SUITE).
+-module(unnecessary_function_arguments_SUITE).
 
 -export([all/0, init_per_testcase/2, end_per_testcase/2]).
 -export([with_warnings/1, without_warnings/1, macros/1]).
@@ -7,7 +7,7 @@ all() ->
     [with_warnings, without_warnings, macros].
 
 init_per_testcase(_, Config) ->
-    hank_test_utils:init_per_testcase(Config, "unused_ignored_function_params").
+    hank_test_utils:init_per_testcase(Config, "unnecessary_function_arguments").
 
 end_per_testcase(_, Config) ->
     hank_test_utils:end_per_testcase(Config).
@@ -20,19 +20,19 @@ with_warnings(_Config) ->
     FileB = "warnings_B.erl",
     [#{file := FileA,
        line := 6,
-       text := <<"Param #2 is not used at single_fun/2">>},
+       text := <<"single_fun/2 doesn't need its #2 argument">>},
      #{file := FileA,
        line := 10,
-       text := <<"Param #3 is not used at multi_fun/3">>},
+       text := <<"multi_fun/3 doesn't need its #3 argument">>},
      #{file := FileA,
        line := 18,
-       text := <<"Param #1 is not used at unicode_αβåö/1"/utf8>>},
+       text := <<"unicode_αβåö/1 doesn't need its #1 argument"/utf8>>},
      #{file := FileA,
        line := 21,
-       text := <<"Param #1 is not used at with_nif_stub/2">>},
+       text := <<"with_nif_stub/2 doesn't need its #1 argument">>},
      #{file := FileB,
        line := 6,
-       text := <<"Param #1 is not used at underscore/3">>}] =
+       text := <<"underscore/3 doesn't need its #1 argument">>}] =
         analyze([FileA, FileB]),
     ok.
 
@@ -47,8 +47,8 @@ macros(_Config) ->
     ct:comment("Macros as function names should not crash hank"),
     [#{file := "macros.erl",
        line := 4,
-       text := <<"Param #1 is not used at ?MODULE/1">>}] =
+       text := <<"?MODULE/1 doesn't need its #1 argument">>}] =
         analyze(["macros.erl"]).
 
 analyze(Files) ->
-    hank_test_utils:analyze_and_sort(Files, [unused_ignored_function_params]).
+    hank_test_utils:analyze_and_sort(Files, [unnecessary_function_arguments]).