Merge pull request #1121 from quangngd/ignore_predicate_name_when_impl

Author: rrrene

lib/credo/check/readability/predicate_function_names.ex

@@ -52,7 +52,63 @@ defmodule Credo.Check.Readability.PredicateFunctionNames do
   def run(%SourceFile{} = source_file, params) do
     issue_meta = IssueMeta.for(source_file, params)
 
-    Credo.Code.prewalk(source_file, &traverse(&1, &2, issue_meta))
+    issue_candidates = Credo.Code.prewalk(source_file, &traverse(&1, &2, issue_meta))
+
+    if issue_candidates == [] do
+      []
+    else
+      impl_list = Credo.Code.prewalk(source_file, &find_impls(&1, &2))
+
+      issue_candidates
+      |> Enum.reject(fn {_, signature} -> signature in impl_list end)
+      |> Enum.map(fn {issue, _} -> issue end)
+    end
+  end
+
+  defp find_impls({:__block__, _meta, args} = ast, impls) do
+    block_impls = find_impls_in_block(args)
+    {ast, block_impls ++ impls}
+  end
+
+  defp find_impls(ast, impls) do
+    {ast, impls}
+  end
+
+  defp find_impls_in_block(block_args) when is_list(block_args) do
+    block_args
+    |> Enum.reduce([], &do_find_impls_in_block/2)
+  end
+
+  defp do_find_impls_in_block({:@, _, [{:impl, _, [impl]}]}, acc) when impl != false do
+    [:record_next_definition | acc]
+  end
+
+  # def when
+  defp do_find_impls_in_block({keyword, meta, [{:when, _, def_ast} | _]}, [
+         :record_next_definition | impls
+       ])
+       when keyword in @def_ops do
+    do_find_impls_in_block({keyword, meta, def_ast}, [:record_next_definition | impls])
+  end
+
+  # def 0 arity
+  defp do_find_impls_in_block({keyword, _meta, [{name, _, nil} | _]}, [
+         :record_next_definition | impls
+       ])
+       when keyword in @def_ops do
+    [{to_string(name), 0} | impls]
+  end
+
+  # def n arity
+  defp do_find_impls_in_block({keyword, _meta, [{name, _, args} | _]}, [
+         :record_next_definition | impls
+       ])
+       when keyword in @def_ops do
+    [{to_string(name), length(args)} | impls]
+  end
+
+  defp do_find_impls_in_block(_, acc) do
+    acc
   end
 
   for op <- @def_ops do
@@ -66,56 +122,62 @@ defmodule Credo.Check.Readability.PredicateFunctionNames do
            issues,
            issue_meta
          ) do
-      {ast, issues_for_definition(op, arguments, issues, issue_meta)}
+      {ast, issues_candidate_for_definition(op, arguments, issues, issue_meta)}
     end
   end
 
   defp traverse(ast, issues, _issue_meta) do
     {ast, issues}
   end
 
-  defp issues_for_definition(op, body, issues, issue_meta) do
-    case Enum.at(body, 0) do
-      {name, meta, nil} ->
-        issues_for_name(op, name, meta, issues, issue_meta)
+  defp issues_candidate_for_definition(op, [{name, meta, nil} | _], issues, issue_meta) do
+    issues_candidate_for_definition(op, [{name, meta, []}], issues, issue_meta)
+  end
 
-      {name, meta, [_ | _]} ->
-        issues_for_name(op, name, meta, issues, issue_meta)
+  defp issues_candidate_for_definition(op, [{name, meta, args} | _], issues, issue_meta) do
+    issues_candidate_for_name(op, name, meta, issues, issue_meta, args)
+  end
 
-      _ ->
-        issues
-    end
+  defp issues_candidate_for_definition(_op, _, issues, _issue_meta) do
+    issues
   end
 
-  defp issues_for_name(_op, {:unquote, _, [_ | _]} = _name, _meta, issues, _issue_meta) do
+  defp issues_candidate_for_name(
+         _op,
+         {:unquote, _, [_ | _]} = _name,
+         _meta,
+         issues,
+         _issue_meta,
+         _args
+       ) do
     issues
   end
 
-  defp issues_for_name(op, name, meta, issues, issue_meta) do
+  defp issues_candidate_for_name(op, name, meta, issues, issue_meta, args) do
     name = to_string(name)
 
     cond do
       String.starts_with?(name, "is_") && String.ends_with?(name, "?") ->
         [
-          issue_for(issue_meta, meta[:line], name, :predicate_and_question_mark)
+          issue_candidate_for(issue_meta, meta[:line], name, args, :predicate_and_question_mark)
           | issues
         ]
 
       String.starts_with?(name, "is_") && op != :defmacro ->
-        [issue_for(issue_meta, meta[:line], name, :only_predicate) | issues]
+        [issue_candidate_for(issue_meta, meta[:line], name, args, :only_predicate) | issues]
 
       true ->
         issues
     end
   end
 
-  defp issue_for(issue_meta, line_no, trigger, _) do
-    format_issue(
-      issue_meta,
-      message:
-        "Predicate function names should not start with 'is', and should end in a question mark.",
-      trigger: trigger,
-      line_no: line_no
-    )
+  defp issue_candidate_for(issue_meta, line_no, trigger, args, _) do
+    {format_issue(
+       issue_meta,
+       message:
+         "Predicate function names should not start with 'is', and should end in a question mark.",
+       trigger: trigger,
+       line_no: line_no
+     ), {trigger, length(args)}}
   end
 end

test/credo/check/readability/predicate_function_names_test.exs

@@ -81,6 +81,91 @@ defmodule Credo.Check.Readability.PredicateFunctionNamesTest do
     |> refute_issues()
   end
 
+  test "it should NOT report a violation with callback" do
+    """
+    defmodule Foo do
+      @callback is_bar
+      @callback is_bar(a)
+    end
+
+    defmodule FooImpl do
+      @behaviour Foo
+
+      @impl Foo
+      def is_bar do
+      end
+
+      @impl Foo
+      def is_bar(a) when is_binary(a) do
+      end
+
+      @impl Foo
+      def is_bar(a) do
+      end
+    end
+    """
+    |> to_source_file
+    |> run_check(@described_check)
+    |> refute_issues()
+  end
+
+  test "it should report a violation with false negatives" do
+    """
+    defmodule FooImpl do
+      def impl(false), do: false
+      def impl(true), do: true
+      def is_bar do
+      end
+    end
+    """
+    |> to_source_file
+    |> run_check(@described_check)
+    |> assert_issue()
+  end
+
+  test "it should report a violation with false negatives /2" do
+    """
+    defmodule FooImpl do
+      impl(true)
+      def is_bar do
+      end
+    end
+    """
+    |> to_source_file
+    |> run_check(@described_check)
+    |> assert_issue()
+  end
+
+  test "it should report a violation with false negatives /3" do
+    """
+    defmodule Foo do
+      @impl is_bar(a)
+    end
+    defmodule FooImpl do
+      def is_bar do
+      end
+    end
+    """
+    |> to_source_file
+    |> run_check(@described_check)
+    |> assert_issue()
+  end
+
+  test "it should report a violation with false negatives /4" do
+    """
+    defmodule Foo do
+      @impl true
+    end
+    defmodule FooImpl do
+      def is_bar do
+      end
+    end
+    """
+    |> to_source_file
+    |> run_check(@described_check)
+    |> assert_issue()
+  end
+
   #
   # cases raising issues
   #