Styler-ify Credo rules: FilterFilter, RejectReject, ExpensiveEmptyEnumCheck, and RaiseInsideRescue

docs/credo.md

@@ -36,12 +36,16 @@ Disabling the rules means updating your `.credo.exs` depending on your configura
 {Credo.Check.Refactor.CaseTrivialMatches, false},
 {Credo.Check.Refactor.CondStatements, false},
 {Credo.Check.Refactor.FilterCount, false},
+{Credo.Check.Refactor.FilterFilter, false},
 {Credo.Check.Refactor.MapInto, false},
 {Credo.Check.Refactor.MapJoin, false},
 {Credo.Check.Refactor.NegatedConditionsInUnless, false},
 {Credo.Check.Refactor.NegatedConditionsWithElse, false},
 {Credo.Check.Refactor.PipeChainStart, false},
 {Credo.Check.Refactor.RedundantWithClauseResult, false},
+{Credo.Check.Refactor.RejectReject, false},
 {Credo.Check.Refactor.UnlessWithElse, false},
 {Credo.Check.Refactor.WithClauses, false},
+{Credo.Check.Warning.ExpensiveEmptyEnumCheck, false},
+{Credo.Check.Warning.RaiseInsideRescue, false},
  ```

lib/style/blocks.ex

@@ -23,6 +23,7 @@ defmodule Styler.Style.Blocks do
   * Credo.Check.Refactor.CondStatements
   * Credo.Check.Refactor.RedundantWithClauseResult
   * Credo.Check.Refactor.WithClauses
+  * Credo.Check.Warning.RaiseInsideRescue
   """
 
   alias Styler.Style
@@ -216,6 +217,19 @@ defmodule Styler.Style.Blocks do
     end
   end
 
+  # `try ... rescue clauses ... end` — within each rescue clause body, rewrite `raise foo` to
+  # `reraise foo, __STACKTRACE__` so the original stacktrace is preserved.
+  # (Credo.Check.Warning.RaiseInsideRescue)
+  def run({{:try, _, [_]} = node, meta}, ctx) do
+    {:cont, {rewrite_rescue_raises(node), meta}, ctx}
+  end
+
+  # `def fname, do: ..., rescue: clauses` — same rewrite within the rescue clauses.
+  def run({{op, _, [_head, kwlist]} = node, meta}, ctx)
+      when op in [:def, :defp, :defmacro, :defmacrop] and is_list(kwlist) do
+    {:cont, {rewrite_rescue_raises(node), meta}, ctx}
+  end
+
   def run(zipper, ctx), do: {:cont, zipper, ctx}
 
   # with statements can do _a lot_, so this beast of a function likewise does a lot.
@@ -423,4 +437,46 @@ defmodule Styler.Style.Blocks do
   defp invert({:not, _, [condition]}), do: condition
   defp invert({:in, m, [_, _]} = ast), do: {:not, m, [ast]}
   defp invert({_, m, _} = ast), do: {:!, [line: m[:line]], [ast]}
+
+  # RaiseInsideRescue helpers — rewrites raises within the rescue clauses of a try or def.
+  defp rewrite_rescue_raises({:try, m, [kwlist]}) do
+    {:try, m, [Enum.map(kwlist, &rewrite_kw_pair/1)]}
+  end
+
+  defp rewrite_rescue_raises({op, m, [head, kwlist]}) when op in [:def, :defp, :defmacro, :defmacrop] do
+    {op, m, [head, Enum.map(kwlist, &rewrite_kw_pair/1)]}
+  end
+
+  defp rewrite_rescue_raises(node), do: node
+
+  # `:rescue` keys appear under literal_encoder as {:__block__, _, [:rescue]}; bare atom is the safety net.
+  defp rewrite_kw_pair({{:__block__, _, [:rescue]} = key, clauses}), do: {key, rewrite_rescue_clauses(clauses)}
+  defp rewrite_kw_pair({:rescue, clauses}), do: {:rescue, rewrite_rescue_clauses(clauses)}
+  defp rewrite_kw_pair(other), do: other
+
+  defp rewrite_rescue_clauses(clauses) when is_list(clauses) do
+    Enum.map(clauses, fn
+      {:->, m, [match, body]} -> {:->, m, [match, rewrite_raises_in_rescue_body(body)]}
+      other -> other
+    end)
+  end
+
+  defp rewrite_rescue_clauses(other), do: other
+
+  # Walks the rescue body rewriting `raise foo` to `reraise foo, __STACKTRACE__`. Doesn't descend into
+  # nested `try`, `fn`, or nested defs — those are different rescue contexts (or none at all).
+  defp rewrite_raises_in_rescue_body(ast), do: walk_raise(ast)
+
+  defp walk_raise({:raise, m, args}) when is_list(args) and args != [] do
+    {:reraise, m, args ++ [{:__STACKTRACE__, [line: m[:line]], nil}]}
+  end
+
+  defp walk_raise({:try, _, _} = node), do: node
+  defp walk_raise({:fn, _, _} = node), do: node
+  defp walk_raise({op, _, _} = node) when op in [:def, :defp, :defmacro, :defmacrop], do: node
+
+  defp walk_raise({a, m, b}) when is_list(b), do: {walk_raise(a), m, Enum.map(b, &walk_raise/1)}
+  defp walk_raise({a, b}), do: {walk_raise(a), walk_raise(b)}
+  defp walk_raise(list) when is_list(list), do: Enum.map(list, &walk_raise/1)
+  defp walk_raise(other), do: other
 end

lib/style/module_directives.ex

@@ -433,11 +433,12 @@ defmodule Styler.Style.ModuleDirectives do
 
   defp apply_aliases(acc, inverted_env) when map_size(inverted_env) == 0, do: acc
 
-  defp apply_aliases(%{require: requires, nondirectives: nondirectives, alias_env: alias_env} = acc, inverted_env) do
-    # applying aliases to requires can change their ordering again
-    requires = requires |> apply_aliases(inverted_env, alias_env) |> sort()
+  defp apply_aliases(%{nondirectives: nondirectives, alias_env: alias_env} = acc, inverted_env) do
+    # Requires are intentionally left alone: they sort above aliases in strict layout,
+    # so shortening `require Foo.Bar` to `require Bar` would reference an alias declared
+    # below it (broken Elixir).
     nondirectives = apply_aliases(nondirectives, inverted_env, alias_env)
-    %{acc | require: requires, nondirectives: nondirectives}
+    %{acc | nondirectives: nondirectives}
   end
 
   # applies the aliases withi `to_as` across the given ast

lib/style/pipes.ex

@@ -19,9 +19,11 @@ defmodule Styler.Style.Pipes do
     * Credo.Check.Readability.PipeIntoAnonymousFunctions
     * Credo.Check.Readability.SinglePipe
     * Credo.Check.Refactor.FilterCount
+    * Credo.Check.Refactor.FilterFilter
     * Credo.Check.Refactor.MapInto
     * Credo.Check.Refactor.MapJoin
     * Credo.Check.Refactor.PipeChainStart, excluded_functions: ["from"]
+    * Credo.Check.Refactor.RejectReject
   """
 
   alias Styler.Style
@@ -367,6 +369,30 @@ defmodule Styler.Style.Pipes do
        when mod in @enum,
        do: {:|>, pm, [lhs, {count, meta, [filterer]}]}
 
+  # `lhs |> Enum.filter(f1) |> Enum.filter(f2)` => `lhs |> Enum.filter(fn item -> f1.(item) && f2.(item) end)`
+  # (Credo.Check.Refactor.FilterFilter)
+  defp fix_pipe(
+         pipe_chain(
+           pm,
+           lhs,
+           {{:., _, [{_, _, [:Enum]}, :filter]} = filter, fm, [f1]},
+           {{:., _, [{_, _, [:Enum]}, :filter]}, _, [f2]}
+         )
+       ),
+       do: {:|>, pm, [lhs, {filter, fm, [combined_predicate(f1, f2, :&&, fm)]}]}
+
+  # `lhs |> Enum.reject(f1) |> Enum.reject(f2)` => `lhs |> Enum.reject(fn item -> f1.(item) || f2.(item) end)`
+  # (Credo.Check.Refactor.RejectReject)
+  defp fix_pipe(
+         pipe_chain(
+           pm,
+           lhs,
+           {{:., _, [{_, _, [:Enum]}, :reject]} = reject, fm, [f1]},
+           {{:., _, [{_, _, [:Enum]}, :reject]}, _, [f2]}
+         )
+       ),
+       do: {:|>, pm, [lhs, {reject, fm, [combined_predicate(f1, f2, :||, fm)]}]}
+
   # `lhs |> Stream.map(fun) |> Stream.run()` => `lhs |> Enum.each(fun)`
   # `lhs |> Stream.each(fun) |> Stream.run()` => `lhs |> Enum.each(fun)`
   defp fix_pipe(
@@ -475,4 +501,18 @@ defmodule Styler.Style.Pipes do
   # function_call(with, args) or sigils. sigils are allowed, function w/ args is not
   defp valid_pipe_start?({fun, _, _args}) when is_atom(fun), do: String.match?("#{fun}", ~r/^sigil_[a-zA-Z]$/)
   defp valid_pipe_start?(_), do: true
+
+  # Combines two 1-arity predicates into a single anonymous function: `fn item -> f1.(item) <op> f2.(item) end`.
+  # Universal form that's correct regardless of whether each predicate is a capture, an `&(...)` shortform,
+  # or an explicit `fn x -> ... end`. Used by FilterFilter (op: `&&`) and RejectReject (op: `||`).
+  defp combined_predicate(f1, f2, op, m) do
+    line = m[:line]
+    item = {:item, [line: line], nil}
+    body = {op, [line: line], [predicate_call(f1, item, line), predicate_call(f2, item, line)]}
+    {:fn, [closing: [line: line], line: line], [{:->, [line: line], [[item], body]}]}
+  end
+
+  defp predicate_call(fun, arg, line) do
+    {{:., [line: line], [fun]}, [closing: [line: line], line: line], [arg]}
+  end
 end

lib/style/single_node.ex

@@ -23,6 +23,7 @@ defmodule Styler.Style.SingleNode do
   * Credo.Check.Refactor.CondStatements
   * Credo.Check.Refactor.RedundantWithClauseResult
   * Credo.Check.Refactor.WithClauses
+  * Credo.Check.Warning.ExpensiveEmptyEnumCheck
   """
 
   @behaviour Styler.Style
@@ -204,6 +205,13 @@ defmodule Styler.Style.SingleNode do
     {{:., dm, [{:__aliases__, am, [mod]}, fun]}, funm, args}
   end
 
+  # `length(x) <op> 0|1` => `x == []` or `x != []`. Avoids walking the whole list to check emptiness.
+  # `Enum.count(x) <op> 0|1` => `Enum.empty?(x)` or `not Enum.empty?(x)` (same reason).
+  # (Credo.Check.Warning.ExpensiveEmptyEnumCheck)
+  defp style({op, m, [lhs, rhs]} = ast) when op in [:==, :!=, :===, :!==, :>, :<, :>=, :<=] do
+    rewrite_empty_check(op, lhs, rhs, m) || ast
+  end
+
   # Remove parens from 0 arity funs (Credo.Check.Readability.ParenthesesOnZeroArityDefs)
   defp style({def, dm, [{fun, funm, []} | rest]}) when def in ~w(def defp)a and is_atom(fun),
     do: style({def, dm, [{fun, Keyword.delete(funm, :closing), nil} | rest]})
@@ -337,4 +345,57 @@ defmodule Styler.Style.SingleNode do
 
   defp add_underscores([a, b, c, d | rest], acc), do: add_underscores([d | rest], [?_, c, b, a | acc])
   defp add_underscores(reversed_list, acc), do: Enum.reverse(reversed_list, acc)
+
+  # ExpensiveEmptyEnumCheck helpers
+  # Picks out a `length(x)` or `Enum.count(x)` call paired with a literal `0` or `1` and rewrites
+  # the entire comparison. Returns nil for any shape that isn't a recognized empty-check pattern.
+  defp rewrite_empty_check(op, lhs, rhs, m) do
+    case {size_call(lhs), int_literal(rhs), size_call(rhs), int_literal(lhs)} do
+      {kind, n, _, _} when not is_nil(kind) and not is_nil(n) -> emit_empty_check(op, kind, n, m)
+      {_, _, kind, n} when not is_nil(kind) and not is_nil(n) -> emit_empty_check(swap_op(op), kind, n, m)
+      _ -> nil
+    end
+  end
+
+  defp size_call({:length, _, [x]}), do: {:length, x}
+  defp size_call({{:., _, [{:__aliases__, _, [:Enum]}, :count]}, _, [x]}), do: {:enum_count, x}
+  defp size_call(_), do: nil
+
+  defp int_literal({:__block__, _, [n]}) when n in [0, 1], do: n
+  defp int_literal(_), do: nil
+
+  # `length(x) <= 0` is also "empty" because length is non-negative; same for `length(x) >= 0` (tautology, skip).
+  defp empty_class(:==, 0), do: :empty
+  defp empty_class(:===, 0), do: :empty
+  defp empty_class(:!=, 0), do: :not_empty
+  defp empty_class(:!==, 0), do: :not_empty
+  defp empty_class(:>, 0), do: :not_empty
+  defp empty_class(:<=, 0), do: :empty
+  defp empty_class(:>=, 1), do: :not_empty
+  defp empty_class(:<, 1), do: :empty
+  defp empty_class(_, _), do: nil
+
+  defp swap_op(:>), do: :<
+  defp swap_op(:<), do: :>
+  defp swap_op(:>=), do: :<=
+  defp swap_op(:<=), do: :>=
+  defp swap_op(op), do: op
+
+  defp emit_empty_check(op, {:length, x}, n, m) do
+    case empty_class(op, n) do
+      :empty -> {:==, m, [x, {:__block__, [line: m[:line]], [[]]}]}
+      :not_empty -> {:!=, m, [x, {:__block__, [line: m[:line]], [[]]}]}
+      nil -> nil
+    end
+  end
+
+  defp emit_empty_check(op, {:enum_count, x}, n, m) do
+    empty_call = {{:., m, [{:__aliases__, m, [:Enum]}, :empty?]}, m, [x]}
+
+    case empty_class(op, n) do
+      :empty -> empty_call
+      :not_empty -> {:not, m, [empty_call]}
+      nil -> nil
+    end
+  end
 end

test/style/blocks_test.exs

@@ -1058,4 +1058,134 @@ defmodule Styler.Style.BlocksTest do
       )
     end
   end
+
+  describe "RaiseInsideRescue" do
+    test "rewrites raise to reraise inside try/rescue" do
+      assert_style(
+        """
+        try do
+          foo()
+        rescue
+          e in RuntimeError -> raise e
+        end
+        """,
+        """
+        try do
+          foo()
+        rescue
+          e in RuntimeError -> reraise e, __STACKTRACE__
+        end
+        """
+      )
+    end
+
+    test "rewrites raise inside a multi-line rescue body" do
+      assert_style(
+        """
+        try do
+          foo()
+        rescue
+          e ->
+            Logger.error("oops")
+            raise e
+        end
+        """,
+        """
+        try do
+          foo()
+        rescue
+          e ->
+            Logger.error("oops")
+            reraise e, __STACKTRACE__
+        end
+        """
+      )
+    end
+
+    test "rewrites raise with multiple args" do
+      assert_style(
+        """
+        try do
+          foo()
+        rescue
+          _ -> raise ArgumentError, "bad"
+        end
+        """,
+        """
+        try do
+          foo()
+        rescue
+          _ -> reraise ArgumentError, "bad", __STACKTRACE__
+        end
+        """
+      )
+    end
+
+    test "rewrites raises inside def's rescue clause" do
+      assert_style(
+        """
+        def foo do
+          bar()
+        rescue
+          e -> raise e
+        end
+        """,
+        """
+        def foo do
+          bar()
+        rescue
+          e -> reraise e, __STACKTRACE__
+        end
+        """
+      )
+    end
+
+    test "leaves raise in the do block alone" do
+      assert_style("""
+      try do
+        raise "oops"
+      rescue
+        e -> Logger.error(e)
+      end
+      """)
+    end
+
+    test "leaves an existing reraise alone" do
+      assert_style("""
+      try do
+        foo()
+      rescue
+        e -> reraise e, __STACKTRACE__
+      end
+      """)
+    end
+
+    test "doesn't descend into a nested try" do
+      # The nested `raise` is inside the inner try's `do` block — it should be left alone
+      # (Styler will visit the inner try on its own and apply its own rewrites).
+      assert_style("""
+      try do
+        foo()
+      rescue
+        _ ->
+          try do
+            raise "inner do"
+          rescue
+            inner -> reraise inner, __STACKTRACE__
+          end
+      end
+      """)
+    end
+
+    test "doesn't descend into anonymous functions inside the rescue body" do
+      assert_style("""
+      try do
+        foo()
+      rescue
+        _ ->
+          fn -> raise "later" end
+      end
+      """)
+    end
+  end
 end