diff --git a/lib/recode/ast.ex b/lib/recode/ast.ex index da93d899..efdc801b 100644 --- a/lib/recode/ast.ex +++ b/lib/recode/ast.ex @@ -452,6 +452,9 @@ defmodule Recode.AST do @doc """ Returns a `mfa`-tuple for the given `.`-call. + A `mfa`-tuple is a three-element tuple containing the module, function name, + and arity. + ## Examples iex> ast = quote do @@ -460,12 +463,38 @@ defmodule Recode.AST do ...> mfa(ast) {Foo.Bar, :baz, 1} """ - @spec mfa({{:., keyword(), list()}, metadata(), t()}) :: + @spec mfa({{:., metadata(), list()}, metadata(), t()}) :: {module(), atom(), non_neg_integer()} def mfa({{:., _meta1, [{:__aliases__, _meta2, aliases}, fun]}, _meta3, args}) do {Module.concat(aliases), fun, length(args)} end + @doc """ + Returns a `mf`-tuple for the given `.`-call or `.`-expression. + + A `mf`-tuple is a two-element tuple containing the module and function name. + + ## Examples + + iex> ast = quote do + ...> Foo.Bar.baz(x) + ...> end + ...> mf(ast) + {Foo.Bar, :baz} + iex> {ast, _, _} = ast + ...> mf(ast) + {Foo.Bar, :baz} + """ + @spec mf({{:., metadata(), list()}, metadata(), t()} | {:., metadata(), list()}) :: + {module(), atom()} + def mf({{:., _meta1, [{:__aliases__, _meta2, aliases}, fun]}, _meta3, _args3}) do + {Module.concat(aliases), fun} + end + + def mf({:., _meta1, [{:__aliases__, _meta2, aliases}, fun]}) do + {Module.concat(aliases), fun} + end + @doc """ Puts the given value `newlines` under the key `nevlines` in `meta[:end_of_expression]`. @@ -658,7 +687,7 @@ defmodule Recode.AST do ...> def baz, do: baz ...> end ...> """) - ...> block = block(args) + ...> block = block(args) ...> Sourceror.to_string({:__block__,[], block}) """ def bar, do: bar diff --git a/lib/recode/config.ex b/lib/recode/config.ex index 22ea7705..ca3d4140 100644 --- a/lib/recode/config.ex +++ b/lib/recode/config.ex @@ -48,6 +48,7 @@ defmodule Recode.Config do {Recode.Task.LocalsWithoutParens, []}, {Recode.Task.Moduledoc, [exclude: ["test/**/*.{ex,exs}", "mix.exs"]]}, {Recode.Task.Nesting, []}, + {Recode.Task.PipeChainStart, [active: false]}, {Recode.Task.PipeFunOne, []}, {Recode.Task.SinglePipe, []}, {Recode.Task.Specs, [exclude: ["test/**/*.{ex,exs}", "mix.exs"], config: [only: :visible]]}, diff --git a/lib/recode/task/pipe_chain_start.ex b/lib/recode/task/pipe_chain_start.ex new file mode 100644 index 00000000..99b27c26 --- /dev/null +++ b/lib/recode/task/pipe_chain_start.ex @@ -0,0 +1,261 @@ +defmodule Recode.Task.PipeChainStart do + @shortdoc "Checks if a pipe chain starts with a raw value." + + @moduledoc """ + Pipes should start with a raw value to improve readability. + + # preferred + user + |> User.changeset(params) + |> Repo.insert() + + # not preferred + User.changeset(user, params) + |> Repo.insert() + + Starting a pipe chain with a function call instead of a raw value can make + the code harder to follow, as the data flow is less clear. + + This task rewrites the code when `mix recode` runs with `autocorrect: true`. + """ + + use Recode.Task, corrector: true, category: :readability + + alias Recode.AST + alias Rewrite.Source + alias Sourceror.Zipper + + @sigils [ + :sigil_C, + :sigil_c, + :sigil_D, + :sigil_N, + :sigil_r, + :sigil_S, + :sigil_s, + :sigil_T, + :sigil_U, + :sigil_W, + :sigil_w + ] + + @unary_ops [ + :!, + :"~~~", + :&, + :+, + :-, + :@, + :^, + :not + ] + + @binary_ops [ + :!=, + :!==, + :"//", + :"::", + :"<|>", + :"^^^", + :&&&, + :&&, + :**, + :*, + :+++, + :++, + :+, + :-, + :--, + :---, + :., + :.., + :..//, + :/, + :<, + :<-, + :<<<, + :<<~, + :<=, + :<>, + :<~, + :<~>, + :=, + :==, + :===, + :=~, + :>, + :>=, + :>>>, + :\\, + :and, + :in, + :or, + :when, + :|, + :|>, + :||, + :|||, + :~>, + :~>> + ] + + @special_forms [ + :<<>>, + :__block__, + :case, + :cond, + :fn, + :for, + :if, + :unquote, + :unquote_splicing, + :with + ] + + @exclude_functions Enum.concat([@special_forms, @sigils, @unary_ops, @binary_ops]) + + @impl Recode.Task + def run(source, opts) do + source + |> Source.get(:quoted) + |> Zipper.zip() + |> Zipper.traverse([], fn zipper, issues -> + pipe_chain_start(zipper, issues, opts[:autocorrect], opts) + end) + |> update(source, opts) + end + + defp update({zipper, issues}, source, opts) do + update_source(source, opts, quoted: zipper, issues: issues) + end + + @impl Recode.Task + def init(config) do + config = + config + |> Keyword.put_new(:exclude_functions, []) + |> Keyword.put_new(:exclude_arguments, []) + + {:ok, config} + end + + # inside pipe + defp pipe_chain_start( + %Zipper{node: {:|>, _, [{:|>, _, _} | _]}} = zipper, + issues, + _autocorrect, + _opts + ) do + {zipper, issues} + end + + # pipe start + defp pipe_chain_start( + %Zipper{node: {:|>, meta, [lhs, rhs]}} = zipper, + issues, + autocorrect, + opts + ) do + case check(lhs, opts) do + :ok -> + {zipper, issues} + + {:error, correction} when autocorrect -> + {correct(zipper, correction, rhs), issues} + + {:error, _correction} when not autocorrect -> + {zipper, add_issue(issues, meta)} + end + end + + defp pipe_chain_start(zipper, issues, _autocorrect, _opts) do + {zipper, issues} + end + + defp check( + {{:., _meta1, [{:__aliases__, _meta2, _aliases2}, _fun]} = form, meta, [arg | rest]}, + opts + ) do + mf = AST.mf(form) + + with :error <- check(mf, meta, arg, opts) do + correction(arg, form, rest) + end + end + + defp check({{:., _meta1, [Access, :get]}, _meta, _args}, _opts) do + :ok + end + + defp check({{:., _meta1, [_arg1 | _rest1]} = form, meta, [arg | rest]}, opts) do + with :error <- check(:., meta, arg, opts) do + correction(arg, form, rest) + end + end + + defp check({form, meta, [arg | rest]}, opts) + when is_atom(form) and form not in @exclude_functions do + with :error <- check(form, meta, arg, opts) do + correction(arg, form, rest) + end + end + + defp check(_ast, _opts), do: :ok + + defp check(form, meta, arg, opts) do + with :error <- exclude_function(form, opts[:exclude_functions]), + :error <- exclude_argument(arg, opts[:exclude_arguments]) do + custom_sigil(form, meta[:delimiter]) + end + end + + defp correction(arg, form, rest) do + {:error, {:|>, [], [arg, {form, [], rest}]}} + end + + defp custom_sigil(_atom, nil), do: :error + + defp custom_sigil(atom, _delimiter) do + sigil? = atom |> to_string() |> String.starts_with?("sigil_") + + if sigil?, do: :ok, else: :error + end + + defp correct(zipper, lhs, rhs) do + Zipper.replace(zipper, {:|>, [], [lhs, rhs]}) + end + + defp add_issue(issues, meta) do + message = "Pipe chain should start with a raw value." + [new_issue(message, meta) | issues] + end + + defp exclude_function(_fun, []), do: :error + + defp exclude_function({module, fun}, exclude) do + if {module, fun} in exclude or {module, :*} in exclude, do: :ok, else: :error + end + + defp exclude_function(fun, exclude) do + if fun in exclude, do: :ok, else: :error + end + + defp exclude_argument({:__block__, _meta, [literal]}, _include) + when is_atom(literal) or is_number(literal) or is_binary(literal) or is_list(literal) do + :error + end + + defp exclude_argument({form, _meta, nil}, _include) when is_atom(form) do + :error + end + + defp exclude_argument({{:., _meta1, _args1}, _meta2, _args2}, _include) do + :error + end + + defp exclude_argument({form, _meta, _}, exclude) do + if form in exclude, do: :ok, else: :error + end + + defp exclude_argument(_arg, _include), do: :ok +end diff --git a/test/recode/task/pipe_chain_start_test.exs b/test/recode/task/pipe_chain_start_test.exs new file mode 100644 index 00000000..3bcfac6e --- /dev/null +++ b/test/recode/task/pipe_chain_start_test.exs @@ -0,0 +1,406 @@ +defmodule Recode.Task.PipeChainStartTest do + use RecodeCase + + alias Recode.Task.PipeChainStart + + describe "corrects" do + test "pipes with vars" do + code = """ + foo(x) |> bar() + foo(x, y) |> bar() + foo(x) |> bar() |> baz() + foo(x.y) |> bar() + foo(x.y, z) |> bar() + foo(u.v.w.x, y.z) |> bar() + foo(x[:y]) |> bar() + foo(x[:x][:y], z) |> bar() + foo(x["y"]) |> bar() + """ + + expected = """ + x |> foo() |> bar() + x |> foo(y) |> bar() + x |> foo() |> bar() |> baz() + x.y |> foo() |> bar() + x.y |> foo(z) |> bar() + u.v.w.x |> foo(y.z) |> bar() + x[:y] |> foo() |> bar() + x[:x][:y] |> foo(z) |> bar() + x["y"] |> foo() |> bar() + """ + + code + |> run_task(PipeChainStart, autocorrect: true) + |> assert_code(expected) + end + + test "pipes with vars in dot calls" do + code = """ + Enum.reverse(x) |> bar() + Foo.foo(x, y) |> bar() + Foo.foo(x) |> bar() |> baz() + Foo.foo(x.y) |> bar() + Foo.foo(x.y, z) |> bar() + Foo.foo(u.v.w.x, y.z) |> bar() + Foo.foo(x[:y]) |> bar() + Foo.foo(x[:x][:y], z) |> bar() + Foo.foo(x["y"]) |> bar() + """ + + expected = """ + x |> Enum.reverse() |> bar() + x |> Foo.foo(y) |> bar() + x |> Foo.foo() |> bar() |> baz() + x.y |> Foo.foo() |> bar() + x.y |> Foo.foo(z) |> bar() + u.v.w.x |> Foo.foo(y.z) |> bar() + x[:y] |> Foo.foo() |> bar() + x[:x][:y] |> Foo.foo(z) |> bar() + x["y"] |> Foo.foo() |> bar() + """ + + code + |> run_task(PipeChainStart, autocorrect: true) + |> assert_code(expected) + end + + test "pipes with vars in var function calls" do + code = """ + foo.(x) |> bar() + foo.bar.(x) |> bar() + foo.(x, y) |> bar() + foo.(x) |> bar() |> baz() + foo.(x) |> bar.() |> baz() + foo.(x.y) |> bar() + foo.(x.y, z) |> bar() + foo.(u.v.w.x, y.z) |> bar() + foo.(x[:y]) |> bar() + foo.(x[:x][:y], z) |> bar() + foo.(x["y"]) |> bar() + """ + + expected = """ + x |> foo.() |> bar() + x |> foo.bar.() |> bar() + x |> foo.(y) |> bar() + x |> foo.() |> bar() |> baz() + x |> foo.() |> bar.() |> baz() + x.y |> foo.() |> bar() + x.y |> foo.(z) |> bar() + u.v.w.x |> foo.(y.z) |> bar() + x[:y] |> foo.() |> bar() + x[:x][:y] |> foo.(z) |> bar() + x["y"] |> foo.() |> bar() + """ + + code + |> run_task(PipeChainStart, autocorrect: true) + |> assert_code(expected) + end + + test "pipes with literals" do + code = """ + foo("x") |> bar() + foo("x", y) |> bar() + foo("x") |> bar() |> baz() + foo(:x) |> bar() + foo(42) |> bar() + foo(4.2) |> bar() + foo([2]) |> bar() + """ + + expected = """ + "x" |> foo() |> bar() + "x" |> foo(y) |> bar() + "x" |> foo() |> bar() |> baz() + :x |> foo() |> bar() + 42 |> foo() |> bar() + 4.2 |> foo() |> bar() + [2] |> foo() |> bar() + """ + + code + |> run_task(PipeChainStart, autocorrect: true) + |> assert_code(expected) + end + + test "pipes with ranges" do + code = """ + foo(1..11) |> bar() + foo(1..11//3) |> bar() + """ + + expected = """ + 1..11 |> foo() |> bar() + 1..11//3 |> foo() |> bar() + """ + + code + |> run_task(PipeChainStart, autocorrect: true) + |> assert_code(expected) + end + + test "pipes with functions and refs" do + code = """ + foo(fn x -> x * 2 end) |> bar() + foo(fn x -> x * 2 end, y, z) |> bar() + foo(&baz/5) |> bar() + foo(&baz/5,y ,z) |> bar() + """ + + expected = """ + fn x -> x * 2 end |> foo() |> bar() + fn x -> x * 2 end |> foo(y, z) |> bar() + (&baz/5) |> foo() |> bar() + (&baz/5) |> foo(y, z) |> bar() + """ + + code + |> run_task(PipeChainStart, autocorrect: true) + |> assert_code(expected) + end + + test "in depth" do + code = """ + bar(foo(x)) |> baz() + bar(foo(a,b),c) |> baz(d) + foo(Enum.reverse(bar(x))) |> baz() + """ + + expected = """ + x |> foo() |> bar() |> baz() + a |> foo(b) |> bar(c) |> baz(d) + x |> bar() |> Enum.reverse() |> foo() |> baz() + """ + + code + |> run_task(PipeChainStart, autocorrect: true) + |> assert_code(expected) + end + + test "crazy constructs" do + code = """ + bar(case x do + :u -> :x + :x -> :u + end) |> baz() + """ + + expected = """ + case x do + :u -> :x + :x -> :u + end + |> bar() + |> baz() + """ + + code + |> run_task(PipeChainStart, autocorrect: true) + |> assert_code(expected) + end + end + + describe "does not correct" do + test "pipes starting with a var" do + """ + def test(x) do + foo = x |> bar() + x |> baz(foo) |> bang() + end + """ + |> run_task(PipeChainStart, autocorrect: true) + |> refute_update() + end + + test "pipes starting with accessing a var" do + """ + x.y |> foo() + x[:y] |> foo() + x["y"] |> foo() + """ + |> run_task(PipeChainStart, autocorrect: true) + |> refute_update() + end + + test "pipes starting with a literal" do + ~s''' + :foo |> foo() + 1 |> add(1) + 1 |> add(1) |> add(2) + "1" |> foo(1) + """ + foo + """ + |> foo() + ''' + |> run_task(PipeChainStart, autocorrect: true) + |> refute_update() + end + + test "pipes starting with a sigil" do + """ + ~s''' + foo + ''' |> foo() + ~r/.*/ |> foo() + ~q"asdf" |> foo() + """ + |> run_task(PipeChainStart, autocorrect: true) + |> refute_update() + end + + test "pipes starting with an unary op" do + """ + +1 |> add(1) + -1 |> add(1) |> add(2) + not x |> foo() + """ + |> run_task(PipeChainStart, autocorrect: true) + |> refute_update() + end + + test "pipes starting with an binary op" do + """ + (1 + 1) |> foo(x) + x or y |> foo() + (x or y) |> foo() + x ~> foo() |> bar() + """ + |> run_task(PipeChainStart, autocorrect: true) + |> refute_update() + end + + test "pipes starting with a range" do + """ + 1..10 |> foo() + 1..10//2 |> foo() + """ + |> run_task(PipeChainStart, autocorrect: true) + |> refute_update() + end + + test "pipes starting with module attribute" do + """ + @foo |> bar() + """ + |> run_task(PipeChainStart, autocorrect: true) + |> refute_update() + end + + test "pipes starting with special forms" do + """ + quote do + unquote(x) |> foo() + end + + if x do + y + end + |> foo() + + case x do + :a -> "a" + _ -> "b" + end + |> foo() + + <> |> foo() + + cond do + x -> y + end + |> foo() + + with x <- y do + foo(x) + end |> bar() + """ + |> run_task(PipeChainStart, autocorrect: true) + |> refute_update() + end + + test "pipes starting with functions and refs" do + """ + fn x -> x + 1 end |> foo() + (&Enum.reverse/1) |> foo() + foo() |> bar() + """ + |> run_task(PipeChainStart, autocorrect: true) + |> refute_update() + end + + test "pipes starting with expected code" do + """ + defmodule RecodeTest do + defmacro a ~> b do + quote do + unquote(a) |> unquote(b) + end + end + + def test do + 1 + |> Kernel.*(2) + ~> Kernel.*(2) + |> Kernel.*(2) + end + end + """ + |> run_task(PipeChainStart, autocorrect: true) + |> refute_update() + end + + test "pipes with functions and refs if excluded" do + """ + foo(fn x -> x * 2 end) |> bar() + foo(&baz/5) |> bar() + """ + |> run_task(PipeChainStart, autocorrect: true, exclude_arguments: [:fn, :&]) + |> refute_update() + end + + test "crazy constructs if excluded" do + """ + bar(case x do + :u -> :x + :x -> :u + end) |> baz() + """ + |> run_task(PipeChainStart, autocorrect: true, exclude_arguments: [:case]) + |> refute_update() + end + + test "pipes if start function is excluded" do + """ + foo(x) |> bar() + Foo.foo(x) |> bar() + Bar.foo(x) |> foo() + Bar.bar(x) |> foo() + """ + |> run_task(PipeChainStart, + autocorrect: true, + exclude_functions: [:foo, {Foo, :foo}, {Bar, :*}] + ) + |> refute_update() + end + end + + describe "reports an issue for" do + test "a single pipe" do + """ + def test(x) do + foo(x) |> bar() + end + """ + |> run_task(PipeChainStart, autocorrect: false) + |> assert_issue_with( + message: "Pipe chain should start with a raw value.", + reporter: Recode.Task.PipeChainStart, + line: 2, + column: 10, + meta: nil + ) + end + end +end diff --git a/test/support/recode_case.ex b/test/support/recode_case.ex index 99191278..46e5f31c 100644 --- a/test/support/recode_case.ex +++ b/test/support/recode_case.ex @@ -76,13 +76,30 @@ defmodule RecodeCase do defmacro refute_update(source) do quote bind_quoted: [source: source] do - refute Source.updated?(source) + message = + Escape.format([ + :red, + "Expected no updates, got #{Source.version(source) - 1} update(s):\n\n", + :reset, + Source.diff(source) + ]) + + refute Source.updated?(source), IO.iodata_to_binary(message) end end defmacro assert_code(source, expected) do quote bind_quoted: [source: source, expected: expected] do - assert Source.get(source, :content) == expected + content = Source.get(source, :content) + + diff = + Escape.format([ + :red, + "Assertion code == expected\n\n", + TextDiff.format(content, expected) + ]) + + assert content == expected, IO.iodata_to_binary(diff) end end