From b063690f99703468299f9989fd47a39b20f95db8 Mon Sep 17 00:00:00 2001 From: Jesse Herrick Date: Wed, 6 May 2026 00:55:04 -0400 Subject: [PATCH 1/2] Don't lift require aliases due to our ordering rules --- lib/style/module_directives.ex | 9 ++-- .../module_directives/alias_lifting_test.exs | 41 +++++++++++++++++-- test/style/module_directives_test.exs | 2 +- 3 files changed, 44 insertions(+), 8 deletions(-) diff --git a/lib/style/module_directives.ex b/lib/style/module_directives.ex index 5e4b4d2..4ee9f4f 100644 --- a/lib/style/module_directives.ex +++ b/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 diff --git a/test/style/module_directives/alias_lifting_test.exs b/test/style/module_directives/alias_lifting_test.exs index e8c277c..ec8c358 100644 --- a/test/style/module_directives/alias_lifting_test.exs +++ b/test/style/module_directives/alias_lifting_test.exs @@ -176,7 +176,7 @@ defmodule Styler.Style.ModuleDirectives.AliasLiftingTest do import A.B.C - require C + require A.B.C alias A.B.C @@ -216,7 +216,9 @@ defmodule Styler.Style.ModuleDirectives.AliasLiftingTest do ) end - test "re-sorts requires after lifting" do + test "lifting does not rewrite require directives" do + # Requires sort above aliases, so a `require A.B.C` cannot be shortened to `require C` - at that point in the file, + # `C` isn't aliased yet. assert_style( """ defmodule A do @@ -224,16 +226,49 @@ defmodule Styler.Style.ModuleDirectives.AliasLiftingTest do require B A.B.C.foo() + A.B.C.foo() end """, """ defmodule A do + require A.B.C require B - require C alias A.B.C C.foo() + C.foo() + end + """ + ) + end + + test "lifting via a require sighting does not rewrite the require itself" do + # Regression test: one require + one nondirective use of the same FQDN used to + # produce a `require Baz` ordered above its `alias Foo.Bar.Baz` (broken Elixir). + assert_style( + """ + defmodule Foo do + @moduledoc false + + require Foo.Bar.Baz + + def go do + Foo.Bar.Baz.go() + end + end + """, + """ + defmodule Foo do + @moduledoc false + + require Foo.Bar.Baz + + alias Foo.Bar.Baz + + def go do + Baz.go() + end end """ ) diff --git a/test/style/module_directives_test.exs b/test/style/module_directives_test.exs index 7255abb..50c9b61 100644 --- a/test/style/module_directives_test.exs +++ b/test/style/module_directives_test.exs @@ -294,7 +294,7 @@ defmodule Styler.Style.ModuleDirectivesTest do alias A.A """, """ - require A + require A.A require A.C alias A.A From 0462740281469eff5fd9de3464ce3d29e2b5cd26 Mon Sep 17 00:00:00 2001 From: Jesse Herrick Date: Wed, 6 May 2026 11:50:19 -0400 Subject: [PATCH 2/2] Add four more Credo rule rewrites - Refactor.FilterFilter: Enum.filter |> Enum.filter => single filter combined with && - Refactor.RejectReject: Enum.reject |> Enum.reject => single reject combined with || - Warning.ExpensiveEmptyEnumCheck: length(x) == 0 => x == []; Enum.count(x) == 0 => Enum.empty?(x) - Warning.RaiseInsideRescue: raise foo inside a rescue clause => reraise foo, __STACKTRACE__ --- docs/credo.md | 4 + lib/style/blocks.ex | 56 ++++++++++++++ lib/style/pipes.ex | 40 ++++++++++ lib/style/single_node.ex | 61 +++++++++++++++ test/style/blocks_test.exs | 130 ++++++++++++++++++++++++++++++++ test/style/pipes_test.exs | 53 +++++++++++++ test/style/single_node_test.exs | 38 ++++++++++ 7 files changed, 382 insertions(+) diff --git a/docs/credo.md b/docs/credo.md index 6a89b10..41ea257 100644 --- a/docs/credo.md +++ b/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}, ``` diff --git a/lib/style/blocks.ex b/lib/style/blocks.ex index 6630471..c5baac8 100644 --- a/lib/style/blocks.ex +++ b/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 diff --git a/lib/style/pipes.ex b/lib/style/pipes.ex index 88c51c0..68a6219 100644 --- a/lib/style/pipes.ex +++ b/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) 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 diff --git a/lib/style/single_node.ex b/lib/style/single_node.ex index 43ce25c..8e0f464 100644 --- a/lib/style/single_node.ex +++ b/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) 0|1` => `x == []` or `x != []`. Avoids walking the whole list to check emptiness. + # `Enum.count(x) 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 diff --git a/test/style/blocks_test.exs b/test/style/blocks_test.exs index 6c09555..96aa055 100644 --- a/test/style/blocks_test.exs +++ b/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 diff --git a/test/style/pipes_test.exs b/test/style/pipes_test.exs index 570f5b9..1055d7b 100644 --- a/test/style/pipes_test.exs +++ b/test/style/pipes_test.exs @@ -643,6 +643,59 @@ defmodule Styler.Style.PipesTest do ) end + test "FilterFilter: combines two Enum.filter calls with &&" do + assert_style( + """ + list + |> Enum.filter(f1) + |> Enum.filter(f2) + """, + """ + Enum.filter(list, fn item -> f1.(item) && f2.(item) end) + """ + ) + end + + test "FilterFilter: combines captures and inline predicates" do + assert_style( + """ + list + |> Enum.filter(&String.contains?(&1, "x")) + |> Enum.filter(fn s -> String.contains?(s, "a") end) + """, + """ + Enum.filter(list, fn item -> (&String.contains?(&1, "x")).(item) && (fn s -> String.contains?(s, "a") end).(item) end) + """ + ) + end + + test "FilterFilter: leaves non-Enum.filter chains alone" do + assert_style(""" + list + |> Stream.filter(f1) + |> Stream.filter(f2) + """) + + assert_style(""" + list + |> Enum.map(f1) + |> Enum.filter(f2) + """) + end + + test "RejectReject: combines two Enum.reject calls with ||" do + assert_style( + """ + list + |> Enum.reject(f1) + |> Enum.reject(f2) + """, + """ + Enum.reject(list, fn item -> f1.(item) || f2.(item) end) + """ + ) + end + test "filter/count" do for enum <- ~w(Enum Stream) do assert_style( diff --git a/test/style/single_node_test.exs b/test/style/single_node_test.exs index 8717fd1..cf46442 100644 --- a/test/style/single_node_test.exs +++ b/test/style/single_node_test.exs @@ -377,6 +377,44 @@ defmodule Styler.Style.SingleNodeTest do end end + describe "ExpensiveEmptyEnumCheck" do + test "rewrites `length(x) 0|1` to comparisons against `[]`" do + assert_style("length(x) == 0", "x == []") + assert_style("length(x) === 0", "x == []") + assert_style("length(x) != 0", "x != []") + assert_style("length(x) !== 0", "x != []") + assert_style("length(x) > 0", "x != []") + assert_style("length(x) <= 0", "x == []") + assert_style("length(x) >= 1", "x != []") + assert_style("length(x) < 1", "x == []") + end + + test "handles the reversed forms (literal on the left)" do + assert_style("0 == length(x)", "x == []") + assert_style("0 != length(x)", "x != []") + assert_style("0 < length(x)", "x != []") + assert_style("0 >= length(x)", "x == []") + assert_style("1 <= length(x)", "x != []") + assert_style("1 > length(x)", "x == []") + end + + test "rewrites `Enum.count(x) 0|1` to `Enum.empty?/1` (or `not Enum.empty?/1`)" do + assert_style("Enum.count(x) == 0", "Enum.empty?(x)") + assert_style("Enum.count(x) != 0", "not Enum.empty?(x)") + assert_style("Enum.count(x) > 0", "not Enum.empty?(x)") + assert_style("Enum.count(x) < 1", "Enum.empty?(x)") + assert_style("Enum.count(x) >= 1", "not Enum.empty?(x)") + assert_style("0 < Enum.count(x)", "not Enum.empty?(x)") + end + + test "leaves unrelated comparisons alone" do + assert_style("length(x) == 5") + assert_style("length(x) > 1") + assert_style("foo == 0") + assert_style("Enum.count(x, fun) == 0") + end + end + describe "to_timeout" do test "to next unit" do facts = [