From 7b7a6aed4126a95f85a99c593872a7ac4e723568 Mon Sep 17 00:00:00 2001
From: Quang Nguyen <chox.negi@gmail.com>
Date: Sun, 24 Mar 2024 16:27:50 +0700
Subject: [PATCH 1/6] code and test

---
 .../readability/predicate_function_names.ex   | 60 ++++++++++++++-----
 .../predicate_function_names_test.exs         | 28 +++++++++
 2 files changed, 74 insertions(+), 14 deletions(-)

diff --git a/lib/credo/check/readability/predicate_function_names.ex b/lib/credo/check/readability/predicate_function_names.ex
index 4a51b50b7..7ad0c1f0f 100644
--- a/lib/credo/check/readability/predicate_function_names.ex
+++ b/lib/credo/check/readability/predicate_function_names.ex
@@ -52,41 +52,73 @@ 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))
+    impl_list = Credo.Code.prewalk(source_file, &find_impls(&1, &2))
+
+    Credo.Code.prewalk(source_file, &traverse(&1, &2, issue_meta, impl_list))
+  end
+
+  defp find_impls({:impl, _, [impl]} = ast, impls) when impl != false do
+    {ast, [:impl | impls]}
+  end
+
+  # def when
+  defp find_impls({keyword, meta, [{:when, _, def_ast} | _]}, [:impl | impls])
+       when keyword in @def_ops do
+    find_impls({keyword, meta, def_ast}, [:impl | impls])
+  end
+
+  # def 0 arity
+  defp find_impls({keyword, _meta, [{name, _, nil} | _]} = ast, [:impl | impls])
+       when keyword in @def_ops do
+    {ast, [{name, 0} | impls]}
+  end
+
+  # def n arity
+  defp find_impls({keyword, _meta, [{name, _, args} | _]} = ast, [:impl | impls])
+       when keyword in @def_ops do
+    {ast, [{name, length(args)} | impls]}
+  end
+
+  defp find_impls(ast, impls) do
+    {ast, impls}
   end
 
   for op <- @def_ops do
     # catch variables named e.g. `defp`
-    defp traverse({unquote(op), _meta, nil} = ast, issues, _issue_meta) do
+    defp traverse({unquote(op), _meta, nil} = ast, issues, _issue_meta, _impl_list) do
       {ast, issues}
     end
 
     defp traverse(
            {unquote(op) = op, _meta, arguments} = ast,
            issues,
-           issue_meta
+           issue_meta,
+           impl_list
          ) do
-      {ast, issues_for_definition(op, arguments, issues, issue_meta)}
+      {ast, issues_for_definition(op, arguments, issues, issue_meta, impl_list)}
     end
   end
 
-  defp traverse(ast, issues, _issue_meta) do
+  defp traverse(ast, issues, _issue_meta, _impl_list) 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)
-
-      {name, meta, [_ | _]} ->
-        issues_for_name(op, name, meta, issues, issue_meta)
+  defp issues_for_definition(op, [{name, meta, nil} | _], issues, issue_meta, impl_list) do
+    issues_for_definition(op, [{name, meta, []}], issues, issue_meta, impl_list)
+  end
 
-      _ ->
-        issues
+  defp issues_for_definition(op, [{name, meta, args} | _], issues, issue_meta, impl_list) do
+    if {name, length(args)} not in impl_list do
+      issues_for_name(op, name, meta, issues, issue_meta)
+    else
+      issues
     end
   end
 
+  defp issues_for_definition(_op, _, issues, _issue_meta, _impl_list) do
+    issues
+  end
+
   defp issues_for_name(_op, {:unquote, _, [_ | _]} = _name, _meta, issues, _issue_meta) do
     issues
   end
diff --git a/test/credo/check/readability/predicate_function_names_test.exs b/test/credo/check/readability/predicate_function_names_test.exs
index 2ff3301e0..187113d7b 100644
--- a/test/credo/check/readability/predicate_function_names_test.exs
+++ b/test/credo/check/readability/predicate_function_names_test.exs
@@ -81,6 +81,34 @@ 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
+
   #
   # cases raising issues
   #

From 79f50f00ec6d8bfa807fd9bf4f9bf486edc1ffec Mon Sep 17 00:00:00 2001
From: Quang Nguyen <chox.negi@gmail.com>
Date: Sun, 24 Mar 2024 21:37:46 +0700
Subject: [PATCH 2/6] minor

---
 lib/credo/check/readability/predicate_function_names.ex | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/credo/check/readability/predicate_function_names.ex b/lib/credo/check/readability/predicate_function_names.ex
index 7ad0c1f0f..88d0f1216 100644
--- a/lib/credo/check/readability/predicate_function_names.ex
+++ b/lib/credo/check/readability/predicate_function_names.ex
@@ -108,10 +108,10 @@ defmodule Credo.Check.Readability.PredicateFunctionNames do
   end
 
   defp issues_for_definition(op, [{name, meta, args} | _], issues, issue_meta, impl_list) do
-    if {name, length(args)} not in impl_list do
-      issues_for_name(op, name, meta, issues, issue_meta)
-    else
+    if {name, length(args)} in impl_list do
       issues
+    else
+      issues_for_name(op, name, meta, issues, issue_meta)
     end
   end
 

From 2aa05b1b1e226c0d5d67dec718479495a700a419 Mon Sep 17 00:00:00 2001
From: Quang Nguyen <chox.negi@gmail.com>
Date: Wed, 27 Mar 2024 10:16:27 +0700
Subject: [PATCH 3/6] add suggested tests

---
 .../predicate_function_names_test.exs         | 57 +++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/test/credo/check/readability/predicate_function_names_test.exs b/test/credo/check/readability/predicate_function_names_test.exs
index 187113d7b..69ef8ee0c 100644
--- a/test/credo/check/readability/predicate_function_names_test.exs
+++ b/test/credo/check/readability/predicate_function_names_test.exs
@@ -109,6 +109,63 @@ defmodule Credo.Check.Readability.PredicateFunctionNamesTest do
     |> 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
   #

From f7a7a51808cfd3b9926e2bea6de8a944295b75d7 Mon Sep 17 00:00:00 2001
From: Quang Nguyen <chox.negi@gmail.com>
Date: Wed, 27 Mar 2024 10:17:23 +0700
Subject: [PATCH 4/6] match impls and defs within only a block

---
 .../readability/predicate_function_names.ex   | 34 +++++++++++++------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/lib/credo/check/readability/predicate_function_names.ex b/lib/credo/check/readability/predicate_function_names.ex
index 88d0f1216..b001c641e 100644
--- a/lib/credo/check/readability/predicate_function_names.ex
+++ b/lib/credo/check/readability/predicate_function_names.ex
@@ -57,30 +57,44 @@ defmodule Credo.Check.Readability.PredicateFunctionNames do
     Credo.Code.prewalk(source_file, &traverse(&1, &2, issue_meta, impl_list))
   end
 
-  defp find_impls({:impl, _, [impl]} = ast, impls) when impl != false do
-    {ast, [:impl | impls]}
+  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
+    [:impl | acc]
   end
 
   # def when
-  defp find_impls({keyword, meta, [{:when, _, def_ast} | _]}, [:impl | impls])
+  defp do_find_impls_in_block({keyword, meta, [{:when, _, def_ast} | _]}, [:impl | impls])
        when keyword in @def_ops do
-    find_impls({keyword, meta, def_ast}, [:impl | impls])
+    do_find_impls_in_block({keyword, meta, def_ast}, [:impl | impls])
   end
 
   # def 0 arity
-  defp find_impls({keyword, _meta, [{name, _, nil} | _]} = ast, [:impl | impls])
+  defp do_find_impls_in_block({keyword, _meta, [{name, _, nil} | _]}, [:impl | impls])
        when keyword in @def_ops do
-    {ast, [{name, 0} | impls]}
+    [{name, 0} | impls]
   end
 
   # def n arity
-  defp find_impls({keyword, _meta, [{name, _, args} | _]} = ast, [:impl | impls])
+  defp do_find_impls_in_block({keyword, _meta, [{name, _, args} | _]}, [:impl | impls])
        when keyword in @def_ops do
-    {ast, [{name, length(args)} | impls]}
+    [{name, length(args)} | impls]
   end
 
-  defp find_impls(ast, impls) do
-    {ast, impls}
+  defp do_find_impls_in_block(_, acc) do
+    acc
   end
 
   for op <- @def_ops do

From 908bc45f3e72e22c3d045f75d70e56848ef03ec7 Mon Sep 17 00:00:00 2001
From: Quang Nguyen <chox.negi@gmail.com>
Date: Sat, 30 Mar 2024 21:39:04 +0700
Subject: [PATCH 5/6] rename atom

---
 .../check/readability/predicate_function_names.ex  | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/lib/credo/check/readability/predicate_function_names.ex b/lib/credo/check/readability/predicate_function_names.ex
index b001c641e..98a01e8c9 100644
--- a/lib/credo/check/readability/predicate_function_names.ex
+++ b/lib/credo/check/readability/predicate_function_names.ex
@@ -72,23 +72,29 @@ defmodule Credo.Check.Readability.PredicateFunctionNames do
   end
 
   defp do_find_impls_in_block({:@, _, [{:impl, _, [impl]}]}, acc) when impl != false do
-    [:impl | acc]
+    [:record_next_definition | acc]
   end
 
   # def when
-  defp do_find_impls_in_block({keyword, meta, [{:when, _, def_ast} | _]}, [:impl | impls])
+  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}, [:impl | impls])
   end
 
   # def 0 arity
-  defp do_find_impls_in_block({keyword, _meta, [{name, _, nil} | _]}, [:impl | impls])
+  defp do_find_impls_in_block({keyword, _meta, [{name, _, nil} | _]}, [
+         :record_next_definition | impls
+       ])
        when keyword in @def_ops do
     [{name, 0} | impls]
   end
 
   # def n arity
-  defp do_find_impls_in_block({keyword, _meta, [{name, _, args} | _]}, [:impl | impls])
+  defp do_find_impls_in_block({keyword, _meta, [{name, _, args} | _]}, [
+         :record_next_definition | impls
+       ])
        when keyword in @def_ops do
     [{name, length(args)} | impls]
   end

From 1b905d5b2fd5e80dc8a1b60b4444f030cc863148 Mon Sep 17 00:00:00 2001
From: Quang Nguyen <chox.negi@gmail.com>
Date: Sat, 30 Mar 2024 22:02:35 +0700
Subject: [PATCH 6/6] reverse the logic

---
 .../readability/predicate_function_names.ex   | 72 +++++++++++--------
 1 file changed, 41 insertions(+), 31 deletions(-)

diff --git a/lib/credo/check/readability/predicate_function_names.ex b/lib/credo/check/readability/predicate_function_names.ex
index 98a01e8c9..fe2ec3a3a 100644
--- a/lib/credo/check/readability/predicate_function_names.ex
+++ b/lib/credo/check/readability/predicate_function_names.ex
@@ -52,9 +52,17 @@ defmodule Credo.Check.Readability.PredicateFunctionNames do
   def run(%SourceFile{} = source_file, params) do
     issue_meta = IssueMeta.for(source_file, params)
 
-    impl_list = Credo.Code.prewalk(source_file, &find_impls(&1, &2))
+    issue_candidates = Credo.Code.prewalk(source_file, &traverse(&1, &2, issue_meta))
 
-    Credo.Code.prewalk(source_file, &traverse(&1, &2, issue_meta, impl_list))
+    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
@@ -80,7 +88,7 @@ defmodule Credo.Check.Readability.PredicateFunctionNames do
          :record_next_definition | impls
        ])
        when keyword in @def_ops do
-    do_find_impls_in_block({keyword, meta, def_ast}, [:impl | impls])
+    do_find_impls_in_block({keyword, meta, def_ast}, [:record_next_definition | impls])
   end
 
   # def 0 arity
@@ -88,7 +96,7 @@ defmodule Credo.Check.Readability.PredicateFunctionNames do
          :record_next_definition | impls
        ])
        when keyword in @def_ops do
-    [{name, 0} | impls]
+    [{to_string(name), 0} | impls]
   end
 
   # def n arity
@@ -96,7 +104,7 @@ defmodule Credo.Check.Readability.PredicateFunctionNames do
          :record_next_definition | impls
        ])
        when keyword in @def_ops do
-    [{name, length(args)} | impls]
+    [{to_string(name), length(args)} | impls]
   end
 
   defp do_find_impls_in_block(_, acc) do
@@ -105,69 +113,71 @@ defmodule Credo.Check.Readability.PredicateFunctionNames do
 
   for op <- @def_ops do
     # catch variables named e.g. `defp`
-    defp traverse({unquote(op), _meta, nil} = ast, issues, _issue_meta, _impl_list) do
+    defp traverse({unquote(op), _meta, nil} = ast, issues, _issue_meta) do
       {ast, issues}
     end
 
     defp traverse(
            {unquote(op) = op, _meta, arguments} = ast,
            issues,
-           issue_meta,
-           impl_list
+           issue_meta
          ) do
-      {ast, issues_for_definition(op, arguments, issues, issue_meta, impl_list)}
+      {ast, issues_candidate_for_definition(op, arguments, issues, issue_meta)}
     end
   end
 
-  defp traverse(ast, issues, _issue_meta, _impl_list) do
+  defp traverse(ast, issues, _issue_meta) do
     {ast, issues}
   end
 
-  defp issues_for_definition(op, [{name, meta, nil} | _], issues, issue_meta, impl_list) do
-    issues_for_definition(op, [{name, meta, []}], issues, issue_meta, impl_list)
+  defp issues_candidate_for_definition(op, [{name, meta, nil} | _], issues, issue_meta) do
+    issues_candidate_for_definition(op, [{name, meta, []}], issues, issue_meta)
   end
 
-  defp issues_for_definition(op, [{name, meta, args} | _], issues, issue_meta, impl_list) do
-    if {name, length(args)} in impl_list do
-      issues
-    else
-      issues_for_name(op, name, meta, issues, issue_meta)
-    end
+  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
 
-  defp issues_for_definition(_op, _, issues, _issue_meta, _impl_list) do
+  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