From ca8fb52a35fe6678dd2c873c91e45af76da45a7d Mon Sep 17 00:00:00 2001 From: Trevor Brown Date: Fri, 16 Dec 2022 14:59:38 -0500 Subject: [PATCH 1/5] Add test to assert NestedFunctionCalls does not warn for calls in a pipeline --- .../nested_function_calls_test.exs | 34 ++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/test/credo/check/readability/nested_function_calls_test.exs b/test/credo/check/readability/nested_function_calls_test.exs index e0e68d4aa..a6b09ad1f 100644 --- a/test/credo/check/readability/nested_function_calls_test.exs +++ b/test/credo/check/readability/nested_function_calls_test.exs @@ -94,7 +94,22 @@ defmodule Credo.Check.Readability.NestedFunctionCallsTest do |> refute_issues() end - test "it should NOT two nested functions calls when the inner function call takes no arguments" do + test "it should NOT report nested function calls when the outer function is already in a pipeline" do + """ + defmodule CredoSampleModule do + def some_code do + [1,2,3,4] + |> Test.test() + |> Enum.map(SomeMod.some_fun(argument)) + end + end + """ + |> to_source_file() + |> run_check(NestedFunctionCalls) + |> refute_issues() + end + + test "it should NOT report two nested functions calls when the inner function call takes no arguments" do """ defmodule CredoSampleModule do def some_code do @@ -158,4 +173,21 @@ defmodule Credo.Check.Readability.NestedFunctionCallsTest do |> run_check(NestedFunctionCalls, min_pipeline_length: 3) |> refute_issues() end + + test "it should report nested function calls inside a pipeline when the inner function calls could be a pipeline of their own" do + """ + defmodule CredoSampleModule do + def some_code do + [1,2,3,4] + |> Test.test() + |> Enum.map(fn(item) -> + SomeMod.some_fun(another_fun(item)) + end) + end + end + """ + |> to_source_file() + |> run_check(NestedFunctionCalls) + |> assert_issue() + end end From 772ede8b6010933fad4a3af70b87f814e337339d Mon Sep 17 00:00:00 2001 From: Trevor Brown Date: Fri, 16 Dec 2022 14:59:46 -0500 Subject: [PATCH 2/5] Extract valid_chain_start?/3 pipe helper function into helper module --- lib/credo/check/pipe_helpers.ex | 232 ++++++++++++++++++ lib/credo/check/refactor/pipe_chain_start.ex | 233 +------------------ 2 files changed, 234 insertions(+), 231 deletions(-) create mode 100644 lib/credo/check/pipe_helpers.ex diff --git a/lib/credo/check/pipe_helpers.ex b/lib/credo/check/pipe_helpers.ex new file mode 100644 index 000000000..2dd835359 --- /dev/null +++ b/lib/credo/check/pipe_helpers.ex @@ -0,0 +1,232 @@ +defmodule Credo.Check.PipeHelpers do + @elixir_custom_operators [ + :<-, + :|||, + :&&&, + :<<<, + :>>>, + :<<~, + :~>>, + :<~, + :~>, + :<~>, + :"<|>", + :"^^^", + :"~~~", + :"..//" + ] + + def valid_chain_start?( + {:__block__, _, [single_ast_node]}, + excluded_functions, + excluded_argument_types + ) do + valid_chain_start?( + single_ast_node, + excluded_functions, + excluded_argument_types + ) + end + + for atom <- [ + :%, + :%{}, + :.., + :<<>>, + :@, + :__aliases__, + :unquote, + :{}, + :&, + :<>, + :++, + :--, + :&&, + :||, + :+, + :-, + :*, + :/, + :>, + :>=, + :<, + :<=, + :==, + :for, + :with, + :not, + :and, + :or + ] do + def valid_chain_start?( + {unquote(atom), _meta, _arguments}, + _excluded_functions, + _excluded_argument_types + ) do + true + end + end + + for operator <- @elixir_custom_operators do + def valid_chain_start?( + {unquote(operator), _meta, _arguments}, + _excluded_functions, + _excluded_argument_types + ) do + true + end + end + + # anonymous function + def valid_chain_start?( + {:fn, _, [{:->, _, [_args, _body]}]}, + _excluded_functions, + _excluded_argument_types + ) do + true + end + + # function_call() + def valid_chain_start?( + {atom, _, []}, + _excluded_functions, + _excluded_argument_types + ) + when is_atom(atom) do + true + end + + # function_call(with, args) and sigils + def valid_chain_start?( + {atom, _, arguments} = ast, + excluded_functions, + excluded_argument_types + ) + when is_atom(atom) and is_list(arguments) do + sigil?(atom) || + valid_chain_start_function_call?( + ast, + excluded_functions, + excluded_argument_types + ) + end + + # map[:access] + def valid_chain_start?( + {{:., _, [Access, :get]}, _, _}, + _excluded_functions, + _excluded_argument_types + ) do + true + end + + # Module.function_call() + def valid_chain_start?( + {{:., _, _}, _, []}, + _excluded_functions, + _excluded_argument_types + ), + do: true + + # Elixir <= 1.8.0 + # '__#{val}__' are compiled to String.to_charlist("__#{val}__") + # we want to consider these charlists a valid pipe chain start + def valid_chain_start?( + {{:., _, [String, :to_charlist]}, _, [{:<<>>, _, _}]}, + _excluded_functions, + _excluded_argument_types + ), + do: true + + # Elixir >= 1.8.0 + # '__#{val}__' are compiled to String.to_charlist("__#{val}__") + # we want to consider these charlists a valid pipe chain start + def valid_chain_start?( + {{:., _, [List, :to_charlist]}, _, [[_ | _]]}, + _excluded_functions, + _excluded_argument_types + ), + do: true + + # Module.function_call(with, parameters) + def valid_chain_start?( + {{:., _, _}, _, _} = ast, + excluded_functions, + excluded_argument_types + ) do + valid_chain_start_function_call?( + ast, + excluded_functions, + excluded_argument_types + ) + end + + def valid_chain_start?(_, _excluded_functions, _excluded_argument_types), do: true + + def valid_chain_start_function_call?( + {_atom, _, arguments} = ast, + excluded_functions, + excluded_argument_types + ) do + function_name = to_function_call_name(ast) + + found_argument_types = + case arguments do + [nil | _] -> [:atom] + x -> x |> List.first() |> argument_type() + end + + Enum.member?(excluded_functions, function_name) || + Enum.any?( + found_argument_types, + &Enum.member?(excluded_argument_types, &1) + ) + end + + defp sigil?(atom) do + atom + |> to_string + |> String.match?(~r/^sigil_[a-zA-Z]$/) + end + + defp to_function_call_name({_, _, _} = ast) do + {ast, [], []} + |> Macro.to_string() + |> String.replace(~r/\.?\(.*\)$/s, "") + end + + @alphabet_wo_r ~w(a b c d e f g h i j k l m n o p q s t u v w x y z) + @all_sigil_chars Enum.flat_map(@alphabet_wo_r, &[&1, String.upcase(&1)]) + @matchable_sigils Enum.map(@all_sigil_chars, &:"sigil_#{&1}") + + for sigil_atom <- @matchable_sigils do + defp argument_type({unquote(sigil_atom), _, _}) do + [unquote(sigil_atom)] + end + end + + defp argument_type({:sigil_r, _, _}), do: [:sigil_r, :regex] + defp argument_type({:sigil_R, _, _}), do: [:sigil_R, :regex] + + defp argument_type({:fn, _, _}), do: [:fn] + defp argument_type({:%{}, _, _}), do: [:map] + defp argument_type({:{}, _, _}), do: [:tuple] + defp argument_type(nil), do: [] + + defp argument_type(v) when is_atom(v), do: [:atom] + defp argument_type(v) when is_binary(v), do: [:binary] + defp argument_type(v) when is_bitstring(v), do: [:bitstring] + defp argument_type(v) when is_boolean(v), do: [:boolean] + + defp argument_type(v) when is_list(v) do + if Keyword.keyword?(v) do + [:keyword, :list] + else + [:list] + end + end + + defp argument_type(v) when is_number(v), do: [:number] + + defp argument_type(v), do: [:credo_type_error, v] +end diff --git a/lib/credo/check/refactor/pipe_chain_start.ex b/lib/credo/check/refactor/pipe_chain_start.ex index 6a0e65039..c8bc739c1 100644 --- a/lib/credo/check/refactor/pipe_chain_start.ex +++ b/lib/credo/check/refactor/pipe_chain_start.ex @@ -32,22 +32,7 @@ defmodule Credo.Check.Refactor.PipeChainStart do ] ] - @elixir_custom_operators [ - :<-, - :|||, - :&&&, - :<<<, - :>>>, - :<<~, - :~>>, - :<~, - :~>, - :<~>, - :"<|>", - :"^^^", - :"~~~", - :"..//" - ] + alias Credo.Check.PipeHelpers @doc false @impl true @@ -82,7 +67,7 @@ defmodule Credo.Check.Refactor.PipeChainStart do excluded_functions, excluded_argument_types ) do - if valid_chain_start?(lhs, excluded_functions, excluded_argument_types) do + if PipeHelpers.valid_chain_start?(lhs, excluded_functions, excluded_argument_types) do {ast, issues} else {ast, issues ++ [issue_for(issue_meta, meta[:line], "TODO")]} @@ -99,220 +84,6 @@ defmodule Credo.Check.Refactor.PipeChainStart do {ast, issues} end - defp valid_chain_start?( - {:__block__, _, [single_ast_node]}, - excluded_functions, - excluded_argument_types - ) do - valid_chain_start?( - single_ast_node, - excluded_functions, - excluded_argument_types - ) - end - - for atom <- [ - :%, - :%{}, - :.., - :<<>>, - :@, - :__aliases__, - :unquote, - :{}, - :&, - :<>, - :++, - :--, - :&&, - :||, - :+, - :-, - :*, - :/, - :>, - :>=, - :<, - :<=, - :==, - :for, - :with, - :not, - :and, - :or - ] do - defp valid_chain_start?( - {unquote(atom), _meta, _arguments}, - _excluded_functions, - _excluded_argument_types - ) do - true - end - end - - for operator <- @elixir_custom_operators do - defp valid_chain_start?( - {unquote(operator), _meta, _arguments}, - _excluded_functions, - _excluded_argument_types - ) do - true - end - end - - # anonymous function - defp valid_chain_start?( - {:fn, _, [{:->, _, [_args, _body]}]}, - _excluded_functions, - _excluded_argument_types - ) do - true - end - - # function_call() - defp valid_chain_start?( - {atom, _, []}, - _excluded_functions, - _excluded_argument_types - ) - when is_atom(atom) do - true - end - - # function_call(with, args) and sigils - defp valid_chain_start?( - {atom, _, arguments} = ast, - excluded_functions, - excluded_argument_types - ) - when is_atom(atom) and is_list(arguments) do - sigil?(atom) || - valid_chain_start_function_call?( - ast, - excluded_functions, - excluded_argument_types - ) - end - - # map[:access] - defp valid_chain_start?( - {{:., _, [Access, :get]}, _, _}, - _excluded_functions, - _excluded_argument_types - ) do - true - end - - # Module.function_call() - defp valid_chain_start?( - {{:., _, _}, _, []}, - _excluded_functions, - _excluded_argument_types - ), - do: true - - # Elixir <= 1.8.0 - # '__#{val}__' are compiled to String.to_charlist("__#{val}__") - # we want to consider these charlists a valid pipe chain start - defp valid_chain_start?( - {{:., _, [String, :to_charlist]}, _, [{:<<>>, _, _}]}, - _excluded_functions, - _excluded_argument_types - ), - do: true - - # Elixir >= 1.8.0 - # '__#{val}__' are compiled to String.to_charlist("__#{val}__") - # we want to consider these charlists a valid pipe chain start - defp valid_chain_start?( - {{:., _, [List, :to_charlist]}, _, [[_ | _]]}, - _excluded_functions, - _excluded_argument_types - ), - do: true - - # Module.function_call(with, parameters) - defp valid_chain_start?( - {{:., _, _}, _, _} = ast, - excluded_functions, - excluded_argument_types - ) do - valid_chain_start_function_call?( - ast, - excluded_functions, - excluded_argument_types - ) - end - - defp valid_chain_start?(_, _excluded_functions, _excluded_argument_types), do: true - - defp valid_chain_start_function_call?( - {_atom, _, arguments} = ast, - excluded_functions, - excluded_argument_types - ) do - function_name = to_function_call_name(ast) - - found_argument_types = - case arguments do - [nil | _] -> [:atom] - x -> x |> List.first() |> argument_type() - end - - Enum.member?(excluded_functions, function_name) || - Enum.any?( - found_argument_types, - &Enum.member?(excluded_argument_types, &1) - ) - end - - defp sigil?(atom) do - atom - |> to_string - |> String.match?(~r/^sigil_[a-zA-Z]$/) - end - - defp to_function_call_name({_, _, _} = ast) do - {ast, [], []} - |> Macro.to_string() - |> String.replace(~r/\.?\(.*\)$/s, "") - end - - @alphabet_wo_r ~w(a b c d e f g h i j k l m n o p q s t u v w x y z) - @all_sigil_chars Enum.flat_map(@alphabet_wo_r, &[&1, String.upcase(&1)]) - @matchable_sigils Enum.map(@all_sigil_chars, &:"sigil_#{&1}") - - for sigil_atom <- @matchable_sigils do - defp argument_type({unquote(sigil_atom), _, _}) do - [unquote(sigil_atom)] - end - end - - defp argument_type({:sigil_r, _, _}), do: [:sigil_r, :regex] - defp argument_type({:sigil_R, _, _}), do: [:sigil_R, :regex] - - defp argument_type({:fn, _, _}), do: [:fn] - defp argument_type({:%{}, _, _}), do: [:map] - defp argument_type({:{}, _, _}), do: [:tuple] - defp argument_type(nil), do: [] - - defp argument_type(v) when is_atom(v), do: [:atom] - defp argument_type(v) when is_binary(v), do: [:binary] - defp argument_type(v) when is_bitstring(v), do: [:bitstring] - defp argument_type(v) when is_boolean(v), do: [:boolean] - - defp argument_type(v) when is_list(v) do - if Keyword.keyword?(v) do - [:keyword, :list] - else - [:list] - end - end - - defp argument_type(v) when is_number(v), do: [:number] - - defp argument_type(v), do: [:credo_type_error, v] - defp issue_for(issue_meta, line_no, trigger) do format_issue( issue_meta, From d85be666af366c5cec20c40a55f9f858b0133273 Mon Sep 17 00:00:00 2001 From: Trevor Brown Date: Fri, 16 Dec 2022 15:00:03 -0500 Subject: [PATCH 3/5] Fix bug in NestedFunctionCalls logic by using existing valid_chain_start?/3 function --- .../readability/nested_function_calls.ex | 60 ++++++++----------- 1 file changed, 26 insertions(+), 34 deletions(-) diff --git a/lib/credo/check/readability/nested_function_calls.ex b/lib/credo/check/readability/nested_function_calls.ex index 1749cc121..025ca0f3b 100644 --- a/lib/credo/check/readability/nested_function_calls.ex +++ b/lib/credo/check/readability/nested_function_calls.ex @@ -33,6 +33,7 @@ defmodule Credo.Check.Readability.NestedFunctionCalls do ] ] + alias Credo.Check.PipeHelpers alias Credo.Code.Name @doc false @@ -42,54 +43,53 @@ defmodule Credo.Check.Readability.NestedFunctionCalls do min_pipeline_length = Params.get(params, :min_pipeline_length, __MODULE__) - {_continue, issues} = + {_min_pipeline_length, issues} = Credo.Code.prewalk( source_file, - &traverse(&1, &2, issue_meta, min_pipeline_length), - {true, []} + &traverse(&1, &2, issue_meta), + {min_pipeline_length, []} ) issues end - # A call with no arguments - defp traverse({{:., _loc, _call}, _meta, []} = ast, {_, issues}, _, min_pipeline_length) do - {ast, {min_pipeline_length, issues}} + # A call in a pipeline + defp traverse({:|>, _meta, [pipe_input, {{:., _meta2, _fun}, _meta3, args}]}, accumulator, _issue_meta) do + {[pipe_input, args], accumulator} + end + + # A fully qualified call with no arguments + defp traverse({{:., _meta, _call}, _meta2, []} = ast, accumulator, _issue_meta) do + {ast, accumulator} end - # A call with arguments + # Any call defp traverse( - {{:., _loc, call}, meta, args} = ast, - {_, issues}, - issue_meta, - min_pipeline_length + {{_name, _loc, call}, meta, args} = ast, + {min_pipeline_length, issues} = acc, + issue_meta ) do if valid_chain_start?(ast) do - {ast, {min_pipeline_length, issues}} + {ast, acc} else case length_as_pipeline(args) + 1 do potential_pipeline_length when potential_pipeline_length >= min_pipeline_length -> - {ast, - {min_pipeline_length, issues ++ [issue_for(issue_meta, meta[:line], Name.full(call))]}} + new_issues = issues ++ [issue_for(issue_meta, meta[:line], Name.full(call))] + {ast, {min_pipeline_length, new_issues}} _ -> - {ast, {min_pipeline_length, issues}} + {nil, acc} end end end - # Another expression - defp traverse(ast, {_, issues}, _issue_meta, min_pipeline_length) do + # Another expression, we must no longer be in a pipeline + defp traverse(ast, {min_pipeline_length, issues}, _issue_meta) do {ast, {min_pipeline_length, issues}} end - # Call with no arguments - defp length_as_pipeline([{{:., _loc, _call}, _meta, []} | _]) do - 0 - end - # Call with function call for first argument - defp length_as_pipeline([{{:., _loc, _call}, _meta, args} = call_ast | _]) do + defp length_as_pipeline([{_name, _meta, args} = call_ast | _]) do if valid_chain_start?(call_ast) do 0 else @@ -111,15 +111,7 @@ defmodule Credo.Check.Readability.NestedFunctionCalls do ) end - # Taken from the Credo.Check.Refactor.PipeChainStart module, with modifications - # map[:access] - defp valid_chain_start?({{:., _, [Access, :get]}, _, _}), do: true - - # Module.function_call() - defp valid_chain_start?({{:., _, _}, _, []}), do: true - - # Kernel.to_string is invoked for string interpolation e.g. "string #{variable}" - defp valid_chain_start?({{:., _, [Kernel, :to_string]}, _, _}), do: true - - defp valid_chain_start?(_), do: false + defp valid_chain_start?(ast) do + PipeHelpers.valid_chain_start?(ast, [], []) + end end From b8583dca1f2c3d7174a8a05d66733a80b9ddda7b Mon Sep 17 00:00:00 2001 From: Trevor Brown Date: Mon, 13 Mar 2023 09:44:48 -0400 Subject: [PATCH 4/5] Revert "Extract valid_chain_start?/3 pipe helper function into helper module" This reverts commit 772ede8b6010933fad4a3af70b87f814e337339d. --- lib/credo/check/pipe_helpers.ex | 232 ------------------ lib/credo/check/refactor/pipe_chain_start.ex | 233 ++++++++++++++++++- 2 files changed, 231 insertions(+), 234 deletions(-) delete mode 100644 lib/credo/check/pipe_helpers.ex diff --git a/lib/credo/check/pipe_helpers.ex b/lib/credo/check/pipe_helpers.ex deleted file mode 100644 index 2dd835359..000000000 --- a/lib/credo/check/pipe_helpers.ex +++ /dev/null @@ -1,232 +0,0 @@ -defmodule Credo.Check.PipeHelpers do - @elixir_custom_operators [ - :<-, - :|||, - :&&&, - :<<<, - :>>>, - :<<~, - :~>>, - :<~, - :~>, - :<~>, - :"<|>", - :"^^^", - :"~~~", - :"..//" - ] - - def valid_chain_start?( - {:__block__, _, [single_ast_node]}, - excluded_functions, - excluded_argument_types - ) do - valid_chain_start?( - single_ast_node, - excluded_functions, - excluded_argument_types - ) - end - - for atom <- [ - :%, - :%{}, - :.., - :<<>>, - :@, - :__aliases__, - :unquote, - :{}, - :&, - :<>, - :++, - :--, - :&&, - :||, - :+, - :-, - :*, - :/, - :>, - :>=, - :<, - :<=, - :==, - :for, - :with, - :not, - :and, - :or - ] do - def valid_chain_start?( - {unquote(atom), _meta, _arguments}, - _excluded_functions, - _excluded_argument_types - ) do - true - end - end - - for operator <- @elixir_custom_operators do - def valid_chain_start?( - {unquote(operator), _meta, _arguments}, - _excluded_functions, - _excluded_argument_types - ) do - true - end - end - - # anonymous function - def valid_chain_start?( - {:fn, _, [{:->, _, [_args, _body]}]}, - _excluded_functions, - _excluded_argument_types - ) do - true - end - - # function_call() - def valid_chain_start?( - {atom, _, []}, - _excluded_functions, - _excluded_argument_types - ) - when is_atom(atom) do - true - end - - # function_call(with, args) and sigils - def valid_chain_start?( - {atom, _, arguments} = ast, - excluded_functions, - excluded_argument_types - ) - when is_atom(atom) and is_list(arguments) do - sigil?(atom) || - valid_chain_start_function_call?( - ast, - excluded_functions, - excluded_argument_types - ) - end - - # map[:access] - def valid_chain_start?( - {{:., _, [Access, :get]}, _, _}, - _excluded_functions, - _excluded_argument_types - ) do - true - end - - # Module.function_call() - def valid_chain_start?( - {{:., _, _}, _, []}, - _excluded_functions, - _excluded_argument_types - ), - do: true - - # Elixir <= 1.8.0 - # '__#{val}__' are compiled to String.to_charlist("__#{val}__") - # we want to consider these charlists a valid pipe chain start - def valid_chain_start?( - {{:., _, [String, :to_charlist]}, _, [{:<<>>, _, _}]}, - _excluded_functions, - _excluded_argument_types - ), - do: true - - # Elixir >= 1.8.0 - # '__#{val}__' are compiled to String.to_charlist("__#{val}__") - # we want to consider these charlists a valid pipe chain start - def valid_chain_start?( - {{:., _, [List, :to_charlist]}, _, [[_ | _]]}, - _excluded_functions, - _excluded_argument_types - ), - do: true - - # Module.function_call(with, parameters) - def valid_chain_start?( - {{:., _, _}, _, _} = ast, - excluded_functions, - excluded_argument_types - ) do - valid_chain_start_function_call?( - ast, - excluded_functions, - excluded_argument_types - ) - end - - def valid_chain_start?(_, _excluded_functions, _excluded_argument_types), do: true - - def valid_chain_start_function_call?( - {_atom, _, arguments} = ast, - excluded_functions, - excluded_argument_types - ) do - function_name = to_function_call_name(ast) - - found_argument_types = - case arguments do - [nil | _] -> [:atom] - x -> x |> List.first() |> argument_type() - end - - Enum.member?(excluded_functions, function_name) || - Enum.any?( - found_argument_types, - &Enum.member?(excluded_argument_types, &1) - ) - end - - defp sigil?(atom) do - atom - |> to_string - |> String.match?(~r/^sigil_[a-zA-Z]$/) - end - - defp to_function_call_name({_, _, _} = ast) do - {ast, [], []} - |> Macro.to_string() - |> String.replace(~r/\.?\(.*\)$/s, "") - end - - @alphabet_wo_r ~w(a b c d e f g h i j k l m n o p q s t u v w x y z) - @all_sigil_chars Enum.flat_map(@alphabet_wo_r, &[&1, String.upcase(&1)]) - @matchable_sigils Enum.map(@all_sigil_chars, &:"sigil_#{&1}") - - for sigil_atom <- @matchable_sigils do - defp argument_type({unquote(sigil_atom), _, _}) do - [unquote(sigil_atom)] - end - end - - defp argument_type({:sigil_r, _, _}), do: [:sigil_r, :regex] - defp argument_type({:sigil_R, _, _}), do: [:sigil_R, :regex] - - defp argument_type({:fn, _, _}), do: [:fn] - defp argument_type({:%{}, _, _}), do: [:map] - defp argument_type({:{}, _, _}), do: [:tuple] - defp argument_type(nil), do: [] - - defp argument_type(v) when is_atom(v), do: [:atom] - defp argument_type(v) when is_binary(v), do: [:binary] - defp argument_type(v) when is_bitstring(v), do: [:bitstring] - defp argument_type(v) when is_boolean(v), do: [:boolean] - - defp argument_type(v) when is_list(v) do - if Keyword.keyword?(v) do - [:keyword, :list] - else - [:list] - end - end - - defp argument_type(v) when is_number(v), do: [:number] - - defp argument_type(v), do: [:credo_type_error, v] -end diff --git a/lib/credo/check/refactor/pipe_chain_start.ex b/lib/credo/check/refactor/pipe_chain_start.ex index c8bc739c1..6a0e65039 100644 --- a/lib/credo/check/refactor/pipe_chain_start.ex +++ b/lib/credo/check/refactor/pipe_chain_start.ex @@ -32,7 +32,22 @@ defmodule Credo.Check.Refactor.PipeChainStart do ] ] - alias Credo.Check.PipeHelpers + @elixir_custom_operators [ + :<-, + :|||, + :&&&, + :<<<, + :>>>, + :<<~, + :~>>, + :<~, + :~>, + :<~>, + :"<|>", + :"^^^", + :"~~~", + :"..//" + ] @doc false @impl true @@ -67,7 +82,7 @@ defmodule Credo.Check.Refactor.PipeChainStart do excluded_functions, excluded_argument_types ) do - if PipeHelpers.valid_chain_start?(lhs, excluded_functions, excluded_argument_types) do + if valid_chain_start?(lhs, excluded_functions, excluded_argument_types) do {ast, issues} else {ast, issues ++ [issue_for(issue_meta, meta[:line], "TODO")]} @@ -84,6 +99,220 @@ defmodule Credo.Check.Refactor.PipeChainStart do {ast, issues} end + defp valid_chain_start?( + {:__block__, _, [single_ast_node]}, + excluded_functions, + excluded_argument_types + ) do + valid_chain_start?( + single_ast_node, + excluded_functions, + excluded_argument_types + ) + end + + for atom <- [ + :%, + :%{}, + :.., + :<<>>, + :@, + :__aliases__, + :unquote, + :{}, + :&, + :<>, + :++, + :--, + :&&, + :||, + :+, + :-, + :*, + :/, + :>, + :>=, + :<, + :<=, + :==, + :for, + :with, + :not, + :and, + :or + ] do + defp valid_chain_start?( + {unquote(atom), _meta, _arguments}, + _excluded_functions, + _excluded_argument_types + ) do + true + end + end + + for operator <- @elixir_custom_operators do + defp valid_chain_start?( + {unquote(operator), _meta, _arguments}, + _excluded_functions, + _excluded_argument_types + ) do + true + end + end + + # anonymous function + defp valid_chain_start?( + {:fn, _, [{:->, _, [_args, _body]}]}, + _excluded_functions, + _excluded_argument_types + ) do + true + end + + # function_call() + defp valid_chain_start?( + {atom, _, []}, + _excluded_functions, + _excluded_argument_types + ) + when is_atom(atom) do + true + end + + # function_call(with, args) and sigils + defp valid_chain_start?( + {atom, _, arguments} = ast, + excluded_functions, + excluded_argument_types + ) + when is_atom(atom) and is_list(arguments) do + sigil?(atom) || + valid_chain_start_function_call?( + ast, + excluded_functions, + excluded_argument_types + ) + end + + # map[:access] + defp valid_chain_start?( + {{:., _, [Access, :get]}, _, _}, + _excluded_functions, + _excluded_argument_types + ) do + true + end + + # Module.function_call() + defp valid_chain_start?( + {{:., _, _}, _, []}, + _excluded_functions, + _excluded_argument_types + ), + do: true + + # Elixir <= 1.8.0 + # '__#{val}__' are compiled to String.to_charlist("__#{val}__") + # we want to consider these charlists a valid pipe chain start + defp valid_chain_start?( + {{:., _, [String, :to_charlist]}, _, [{:<<>>, _, _}]}, + _excluded_functions, + _excluded_argument_types + ), + do: true + + # Elixir >= 1.8.0 + # '__#{val}__' are compiled to String.to_charlist("__#{val}__") + # we want to consider these charlists a valid pipe chain start + defp valid_chain_start?( + {{:., _, [List, :to_charlist]}, _, [[_ | _]]}, + _excluded_functions, + _excluded_argument_types + ), + do: true + + # Module.function_call(with, parameters) + defp valid_chain_start?( + {{:., _, _}, _, _} = ast, + excluded_functions, + excluded_argument_types + ) do + valid_chain_start_function_call?( + ast, + excluded_functions, + excluded_argument_types + ) + end + + defp valid_chain_start?(_, _excluded_functions, _excluded_argument_types), do: true + + defp valid_chain_start_function_call?( + {_atom, _, arguments} = ast, + excluded_functions, + excluded_argument_types + ) do + function_name = to_function_call_name(ast) + + found_argument_types = + case arguments do + [nil | _] -> [:atom] + x -> x |> List.first() |> argument_type() + end + + Enum.member?(excluded_functions, function_name) || + Enum.any?( + found_argument_types, + &Enum.member?(excluded_argument_types, &1) + ) + end + + defp sigil?(atom) do + atom + |> to_string + |> String.match?(~r/^sigil_[a-zA-Z]$/) + end + + defp to_function_call_name({_, _, _} = ast) do + {ast, [], []} + |> Macro.to_string() + |> String.replace(~r/\.?\(.*\)$/s, "") + end + + @alphabet_wo_r ~w(a b c d e f g h i j k l m n o p q s t u v w x y z) + @all_sigil_chars Enum.flat_map(@alphabet_wo_r, &[&1, String.upcase(&1)]) + @matchable_sigils Enum.map(@all_sigil_chars, &:"sigil_#{&1}") + + for sigil_atom <- @matchable_sigils do + defp argument_type({unquote(sigil_atom), _, _}) do + [unquote(sigil_atom)] + end + end + + defp argument_type({:sigil_r, _, _}), do: [:sigil_r, :regex] + defp argument_type({:sigil_R, _, _}), do: [:sigil_R, :regex] + + defp argument_type({:fn, _, _}), do: [:fn] + defp argument_type({:%{}, _, _}), do: [:map] + defp argument_type({:{}, _, _}), do: [:tuple] + defp argument_type(nil), do: [] + + defp argument_type(v) when is_atom(v), do: [:atom] + defp argument_type(v) when is_binary(v), do: [:binary] + defp argument_type(v) when is_bitstring(v), do: [:bitstring] + defp argument_type(v) when is_boolean(v), do: [:boolean] + + defp argument_type(v) when is_list(v) do + if Keyword.keyword?(v) do + [:keyword, :list] + else + [:list] + end + end + + defp argument_type(v) when is_number(v), do: [:number] + + defp argument_type(v), do: [:credo_type_error, v] + defp issue_for(issue_meta, line_no, trigger) do format_issue( issue_meta, From 5cfea9eee52861b10850e4041be692fa19c16e41 Mon Sep 17 00:00:00 2001 From: Trevor Brown Date: Mon, 13 Mar 2023 09:57:58 -0400 Subject: [PATCH 5/5] Duplicate code into NestedFunctionCalls check module --- .../readability/nested_function_calls.ex | 256 +++++++++++++++++- 1 file changed, 248 insertions(+), 8 deletions(-) diff --git a/lib/credo/check/readability/nested_function_calls.ex b/lib/credo/check/readability/nested_function_calls.ex index 025ca0f3b..b60dff1c6 100644 --- a/lib/credo/check/readability/nested_function_calls.ex +++ b/lib/credo/check/readability/nested_function_calls.ex @@ -33,7 +33,7 @@ defmodule Credo.Check.Readability.NestedFunctionCalls do ] ] - alias Credo.Check.PipeHelpers + alias Credo.Check.Readability.NestedFunctionCalls.PipeHelper alias Credo.Code.Name @doc false @@ -54,12 +54,12 @@ defmodule Credo.Check.Readability.NestedFunctionCalls do end # A call in a pipeline - defp traverse({:|>, _meta, [pipe_input, {{:., _meta2, _fun}, _meta3, args}]}, accumulator, _issue_meta) do - {[pipe_input, args], accumulator} + defp traverse({:|>, _meta, [pipe_input, {{:., _meta2, _fun}, _meta3, args}]}, acc, _issue) do + {[pipe_input, args], acc} end # A fully qualified call with no arguments - defp traverse({{:., _meta, _call}, _meta2, []} = ast, accumulator, _issue_meta) do + defp traverse({{:., _meta, _call}, _meta2, []} = ast, accumulator, _issue) do {ast, accumulator} end @@ -69,7 +69,7 @@ defmodule Credo.Check.Readability.NestedFunctionCalls do {min_pipeline_length, issues} = acc, issue_meta ) do - if valid_chain_start?(ast) do + if cannot_be_in_pipeline?(ast) do {ast, acc} else case length_as_pipeline(args) + 1 do @@ -90,7 +90,7 @@ defmodule Credo.Check.Readability.NestedFunctionCalls do # Call with function call for first argument defp length_as_pipeline([{_name, _meta, args} = call_ast | _]) do - if valid_chain_start?(call_ast) do + if cannot_be_in_pipeline?(call_ast) do 0 else 1 + length_as_pipeline(args) @@ -111,7 +111,247 @@ defmodule Credo.Check.Readability.NestedFunctionCalls do ) end - defp valid_chain_start?(ast) do - PipeHelpers.valid_chain_start?(ast, [], []) + defp cannot_be_in_pipeline?(ast) do + PipeHelper.cannot_be_in_pipeline?(ast, [], []) + end + + defmodule PipeHelper do + @moduledoc """ + This module exists to contain logic for the cannot_be_in_pipline?/3 helper + function. This function was originally copied from the + Credo.Check.Refactor.PipeChainStart module's valid_chain_start?/3 function. + Both functions are identical. + """ + + @elixir_custom_operators [ + :<-, + :|||, + :&&&, + :<<<, + :>>>, + :<<~, + :~>>, + :<~, + :~>, + :<~>, + :"<|>", + :"^^^", + :"~~~", + :"..//" + ] + + def cannot_be_in_pipeline?( + {:__block__, _, [single_ast_node]}, + excluded_functions, + excluded_argument_types + ) do + cannot_be_in_pipeline?( + single_ast_node, + excluded_functions, + excluded_argument_types + ) + end + + for atom <- [ + :%, + :%{}, + :.., + :<<>>, + :@, + :__aliases__, + :unquote, + :{}, + :&, + :<>, + :++, + :--, + :&&, + :||, + :+, + :-, + :*, + :/, + :>, + :>=, + :<, + :<=, + :==, + :for, + :with, + :not, + :and, + :or + ] do + def cannot_be_in_pipeline?( + {unquote(atom), _meta, _arguments}, + _excluded_functions, + _excluded_argument_types + ) do + true + end + end + + for operator <- @elixir_custom_operators do + def cannot_be_in_pipeline?( + {unquote(operator), _meta, _arguments}, + _excluded_functions, + _excluded_argument_types + ) do + true + end + end + + # anonymous function + def cannot_be_in_pipeline?( + {:fn, _, [{:->, _, [_args, _body]}]}, + _excluded_functions, + _excluded_argument_types + ) do + true + end + + # function_call() + def cannot_be_in_pipeline?( + {atom, _, []}, + _excluded_functions, + _excluded_argument_types + ) + when is_atom(atom) do + true + end + + # function_call(with, args) and sigils + def cannot_be_in_pipeline?( + {atom, _, arguments} = ast, + excluded_functions, + excluded_argument_types + ) + when is_atom(atom) and is_list(arguments) do + sigil?(atom) || + valid_chain_start_function_call?( + ast, + excluded_functions, + excluded_argument_types + ) + end + + # map[:access] + def cannot_be_in_pipeline?( + {{:., _, [Access, :get]}, _, _}, + _excluded_functions, + _excluded_argument_types + ) do + true + end + + # Module.function_call() + def cannot_be_in_pipeline?( + {{:., _, _}, _, []}, + _excluded_functions, + _excluded_argument_types + ), + do: true + + # Elixir <= 1.8.0 + # '__#{val}__' are compiled to String.to_charlist("__#{val}__") + # we want to consider these charlists a valid pipe chain start + def cannot_be_in_pipeline?( + {{:., _, [String, :to_charlist]}, _, [{:<<>>, _, _}]}, + _excluded_functions, + _excluded_argument_types + ), + do: true + + # Elixir >= 1.8.0 + # '__#{val}__' are compiled to String.to_charlist("__#{val}__") + # we want to consider these charlists a valid pipe chain start + def cannot_be_in_pipeline?( + {{:., _, [List, :to_charlist]}, _, [[_ | _]]}, + _excluded_functions, + _excluded_argument_types + ), + do: true + + # Module.function_call(with, parameters) + def cannot_be_in_pipeline?( + {{:., _, _}, _, _} = ast, + excluded_functions, + excluded_argument_types + ) do + valid_chain_start_function_call?( + ast, + excluded_functions, + excluded_argument_types + ) + end + + def cannot_be_in_pipeline?(_, _excluded_functions, _excluded_argument_types), do: true + + def valid_chain_start_function_call?( + {_atom, _, arguments} = ast, + excluded_functions, + excluded_argument_types + ) do + function_name = to_function_call_name(ast) + + found_argument_types = + case arguments do + [nil | _] -> [:atom] + x -> x |> List.first() |> argument_type() + end + + Enum.member?(excluded_functions, function_name) || + Enum.any?( + found_argument_types, + &Enum.member?(excluded_argument_types, &1) + ) + end + + defp sigil?(atom) do + atom + |> to_string + |> String.match?(~r/^sigil_[a-zA-Z]$/) + end + + defp to_function_call_name({_, _, _} = ast) do + {ast, [], []} + |> Macro.to_string() + |> String.replace(~r/\.?\(.*\)$/s, "") + end + + @alphabet_wo_r ~w(a b c d e f g h i j k l m n o p q s t u v w x y z) + @all_sigil_chars Enum.flat_map(@alphabet_wo_r, &[&1, String.upcase(&1)]) + @matchable_sigils Enum.map(@all_sigil_chars, &:"sigil_#{&1}") + + for sigil_atom <- @matchable_sigils do + defp argument_type({unquote(sigil_atom), _, _}) do + [unquote(sigil_atom)] + end + end + + defp argument_type({:sigil_r, _, _}), do: [:sigil_r, :regex] + defp argument_type({:sigil_R, _, _}), do: [:sigil_R, :regex] + + defp argument_type({:fn, _, _}), do: [:fn] + defp argument_type({:%{}, _, _}), do: [:map] + defp argument_type({:{}, _, _}), do: [:tuple] + defp argument_type(nil), do: [] + + defp argument_type(v) when is_atom(v), do: [:atom] + defp argument_type(v) when is_binary(v), do: [:binary] + defp argument_type(v) when is_bitstring(v), do: [:bitstring] + defp argument_type(v) when is_boolean(v), do: [:boolean] + + defp argument_type(v) when is_list(v) do + if Keyword.keyword?(v) do + [:keyword, :list] + else + [:list] + end + end + + defp argument_type(v) when is_number(v), do: [:number] + + defp argument_type(v), do: [:credo_type_error, v] end end