diff --git a/lib/credo/check.ex b/lib/credo/check.ex index d85ac16d4..a6ad226bd 100644 --- a/lib/credo/check.ex +++ b/lib/credo/check.ex @@ -153,6 +153,9 @@ defmodule Credo.Check do @callback format_issue(issue_meta :: Credo.IssueMeta.t(), opts :: Keyword.t()) :: Credo.Issue.t() + @doc false + @callback autofix(source_file :: String.t(), issue :: Issue.t()) :: String.t() + @base_category_exit_status_map %{ consistency: 1, design: 2, @@ -395,6 +398,12 @@ defmodule Credo.Check do throw("Implement me") end + @doc false + @impl true + def autofix(source_file, _issue) do + source_file + end + defoverridable Credo.Check defp append_issues_and_timings([] = _issues, exec) do diff --git a/lib/credo/check/consistency/line_endings.ex b/lib/credo/check/consistency/line_endings.ex index becb10e39..8fc327ab2 100644 --- a/lib/credo/check/consistency/line_endings.ex +++ b/lib/credo/check/consistency/line_endings.ex @@ -44,4 +44,15 @@ defmodule Credo.Check.Consistency.LineEndings do defp message_for(:windows = _expected) do "File is using unix line endings while most of the files use windows line endings." end + + def autofix(file, issue) do + if issue.message == message_for(:windows) do + do_autofix(file, :unix_to_windows) + else + do_autofix(file, :windows_to_unix) + end + end + + defp do_autofix(file, :windows_to_unix), do: String.replace(file, "\r\n", "\n") + defp do_autofix(file, :unix_to_windows), do: String.replace(file, "\n", "\r\n") end diff --git a/lib/credo/check/readability/alias_order.ex b/lib/credo/check/readability/alias_order.ex index eeee15cd4..2a53e0e49 100644 --- a/lib/credo/check/readability/alias_order.ex +++ b/lib/credo/check/readability/alias_order.ex @@ -223,4 +223,79 @@ defmodule Credo.Check.Readability.AliasOrder do line_no: line_no ) end + + def autofix(file, _issue) do + {:ok, quoted} = :"Elixir.Code".string_to_quoted(file) + + modified = + quoted + |> Macro.prewalk(&do_autofix/1) + |> Macro.to_string() + |> :"Elixir.Code".format_string!() + |> to_string() + + "#{modified}\n" + end + + defp do_autofix({:__block__ = op, meta, [{:alias, _, _} | _] = aliases}) do + modified = + aliases + |> group_aliases() + |> Enum.map(fn {line, group} -> + group + |> Macro.prewalk(&remove_line_numbers/1) + |> Enum.map(&sort_multi_aliases/1) + |> sort_aliases() + |> put_line_number(line) + end) + |> List.flatten() + + {op, Keyword.delete(meta, :line), modified} + end + + defp do_autofix(ast), do: ast + + defp put_line_number([{op, meta, args} | tail], line) do + modified = {op, Keyword.put(meta, :line, line), args} + [modified | tail] + end + + defp group_aliases(aliases) do + chunk_fun = fn + {_, meta, _} = node, {_, []} -> + {:cont, {meta[:line], [node]}} + + {_, meta, _} = node, {line, [{_, meta2, _} | _] = chunk} -> + if meta[:line] - 1 > meta2[:line] do + {:cont, {line, chunk}, {meta[:line], [node]}} + else + {:cont, {line, [node | chunk]}} + end + end + + after_fun = fn acc -> {:cont, acc, []} end + + Enum.chunk_while(aliases, {nil, []}, chunk_fun, after_fun) + end + + defp sort_multi_aliases({op, meta, [{op2, meta2, [{_, _, _} | _] = aliases}]}) do + {op, meta, [{op2, meta2, sort_aliases(aliases)}]} + end + + defp sort_multi_aliases(node), do: node + + defp sort_aliases(aliases) do + Enum.sort_by(aliases, &name_for_sorting/1, fn left, right -> + Enum.sort([left, right]) == [left, right] + end) + end + + defp remove_line_numbers({op, meta, args}) when is_list(meta), + do: {op, Keyword.delete(meta, :line), args} + + defp remove_line_numbers(ast), do: ast + + defp name_for_sorting({_, _, [{_, _, node}, [as: _]]}), do: compare_name(node) + defp name_for_sorting({_, _, [{_, _, _} = node]}), do: compare_name(node) + defp name_for_sorting({_, _, [node]}), do: compare_name(node) end diff --git a/lib/credo/check/readability/trailing_blank_line.ex b/lib/credo/check/readability/trailing_blank_line.ex index 7215e90e8..0a7226b74 100644 --- a/lib/credo/check/readability/trailing_blank_line.ex +++ b/lib/credo/check/readability/trailing_blank_line.ex @@ -39,4 +39,8 @@ defmodule Credo.Check.Readability.TrailingBlankLine do line_no: line_no ) end + + def autofix(file, _issue) do + "#{file}\n" + end end diff --git a/lib/credo/check/refactor/unless_with_else.ex b/lib/credo/check/refactor/unless_with_else.ex index fbd282387..7f8f7ea39 100644 --- a/lib/credo/check/refactor/unless_with_else.ex +++ b/lib/credo/check/refactor/unless_with_else.ex @@ -71,4 +71,23 @@ defmodule Credo.Check.Refactor.UnlessWithElse do line_no: line_no ) end + + def autofix(file, _issue) do + {:ok, quoted} = :"Elixir.Code".string_to_quoted(file) + + modified = + quoted + |> Macro.prewalk(&do_autofix/1) + |> Macro.to_string() + |> :"Elixir.Code".format_string!() + |> to_string() + + "#{modified}\n" + end + + defp do_autofix({:unless, meta, [clause, [do: falsy, else: truthy]]}) do + {:if, meta, [clause, [do: truthy, else: falsy]]} + end + + defp do_autofix(ast), do: ast end diff --git a/lib/credo/check/warning/unused_string_operation.ex b/lib/credo/check/warning/unused_string_operation.ex index 93142953f..ad66c525c 100644 --- a/lib/credo/check/warning/unused_string_operation.ex +++ b/lib/credo/check/warning/unused_string_operation.ex @@ -28,6 +28,7 @@ defmodule Credo.Check.Warning.UnusedStringOperation do """ ] + alias Credo.Check.Warning.UnusedFunctionReturnHelper alias Credo.Check.Warning.UnusedOperation @checked_module :String @@ -44,4 +45,47 @@ defmodule Credo.Check.Warning.UnusedStringOperation do &format_issue/2 ) end + + def autofix(file, _issue) do + {_, quoted} = Credo.Code.ast(file) + source_file = SourceFile.parse(file, "nofile") + + unused_calls = + UnusedFunctionReturnHelper.find_unused_calls( + source_file, + [], + [@checked_module], + @funs_with_return_value + ) + + modified = + quoted + |> Macro.prewalk(&do_autofix(&1, unused_calls)) + |> Macro.to_string() + |> :"Elixir.Code".format_string!() + |> to_string() + + "#{modified}\n" + end + + defp do_autofix({:__block__, meta, [{:|>, _pipe_meta, pipe_args} | tail] = args}, unused_calls) do + args = + if List.last(pipe_args) in unused_calls do + [hd(pipe_args) | tail] + else + args + end + + {:__block__, meta, args} + end + + defp do_autofix({:__block__, meta, args}, unused_calls) do + {:__block__, meta, Enum.reject(args, & &1 in unused_calls)} + end + + defp do_autofix({:do, node}, unused_calls) do + {:do, do_autofix(node, unused_calls)} + end + + defp do_autofix(ast, _), do: ast end diff --git a/lib/credo/cli/command/suggest/suggest_command.ex b/lib/credo/cli/command/suggest/suggest_command.ex index c8a284999..b0d65ce9d 100644 --- a/lib/credo/cli/command/suggest/suggest_command.ex +++ b/lib/credo/cli/command/suggest/suggest_command.ex @@ -30,6 +30,7 @@ defmodule Credo.CLI.Command.Suggest.SuggestCommand do Switch.boolean("read_from_stdin"), Switch.boolean("strict"), Switch.boolean("verbose"), + Switch.boolean("autofix"), Switch.boolean("watch") ] @@ -41,6 +42,7 @@ defmodule Credo.CLI.Command.Suggest.SuggestCommand do print_before_analysis: [__MODULE__.PrintBeforeInfo], run_analysis: [Task.RunChecks], filter_issues: [Task.SetRelevantIssues], + run_autofix: [Task.RunAutofix], print_after_analysis: [__MODULE__.PrintResultsAndSummary] ) end diff --git a/lib/credo/cli/task/run_autocorrect.ex b/lib/credo/cli/task/run_autocorrect.ex new file mode 100644 index 000000000..ace9839d7 --- /dev/null +++ b/lib/credo/cli/task/run_autocorrect.ex @@ -0,0 +1,40 @@ +defmodule Credo.CLI.Task.RunAutofix do + @moduledoc false + + use Credo.Execution.Task + + def call(exec, opts, read_fun \\ &File.read!/1, write_fun \\ &File.write!/2) do + if exec.autofix do + issues = Keyword.get_lazy(opts, :issues, fn -> Execution.get_issues(exec) end) + + issues + |> group_by_file + |> Enum.each(fn {file_path, issues} -> + file = read_fun.(file_path) + + corrected = Enum.reduce(issues, file, &run_autofix(&1, &2, exec)) + + write_fun.(file_path, corrected) + end) + end + + exec + end + + defp group_by_file(issues) do + Enum.reduce(issues, %{}, fn issue, acc -> + Map.update(acc, issue.filename, [issue], &[issue | &1]) + end) + end + + defp run_autofix(issue, file, exec) do + case issue.check.autofix(file, issue) do + ^file -> + file + + corrected_file -> + Execution.remove_issue(exec, issue) + corrected_file + end + end +end diff --git a/lib/credo/config_builder.ex b/lib/credo/config_builder.ex index 689002a42..e8832b3a5 100644 --- a/lib/credo/config_builder.ex +++ b/lib/credo/config_builder.ex @@ -81,6 +81,7 @@ defmodule Credo.ConfigBuilder do |> add_switch_ignore(switches) |> add_switch_mute_exit_status(switches) |> add_switch_only(switches) + |> add_switch_autofix(switches) |> add_switch_read_from_stdin(switches) |> add_switch_strict(switches) |> add_switch_min_priority(switches) @@ -262,6 +263,14 @@ defmodule Credo.ConfigBuilder do defp add_switch_only(exec, _), do: exec + # add_switch_autofix + + defp add_switch_autofix(exec, %{autofix: true}) do + %Execution{exec | autofix: true} + end + + defp add_switch_autofix(exec, _), do: exec + # add_switch_ignore # exclude/ignore certain checks diff --git a/lib/credo/execution.ex b/lib/credo/execution.ex index 414eb5987..6339d4276 100644 --- a/lib/credo/execution.ex +++ b/lib/credo/execution.ex @@ -29,6 +29,7 @@ defmodule Credo.Execution do plugins: [], parse_timeout: 5000, strict: false, + autofix: false, # options, set by the command line format: nil, @@ -493,6 +494,14 @@ defmodule Credo.Execution do |> Map.get(filename, []) end + @doc """ + This removes an issue for the given `exec` struct if the issue has been automatically resolved + before the run has completed. + """ + def remove_issue(exec, issue) do + ExecutionIssues.remove_issue(exec, issue) + end + @doc """ Sets the issues for the given `exec` struct, overwriting any existing issues. """ diff --git a/lib/credo/execution/execution_issues.ex b/lib/credo/execution/execution_issues.ex index 0b9ab32fe..e85b578c2 100644 --- a/lib/credo/execution/execution_issues.ex +++ b/lib/credo/execution/execution_issues.ex @@ -46,6 +46,10 @@ defmodule Credo.Execution.ExecutionIssues do GenServer.call(pid, :to_map) end + def remove_issue(%Execution{issues_pid: pid}, issue) do + GenServer.call(pid, {:remove_issue, issue}) + end + # callbacks def init(_) do @@ -73,4 +77,12 @@ defmodule Credo.Execution.ExecutionIssues do def handle_call(:to_map, _from, current_state) do {:reply, current_state, current_state} end + + def handle_call({:remove_issue, issue}, _from, current_state) do + existing_issues = List.wrap(current_state[issue.filename]) + new_issue_list = List.delete(existing_issues, issue) + new_current_state = Map.put(current_state, issue.filename, new_issue_list) + + {:reply, new_current_state, new_current_state} + end end diff --git a/test/credo/check/consistency/line_endings_test.exs b/test/credo/check/consistency/line_endings_test.exs index d4aed95fa..18dd8ccc7 100644 --- a/test/credo/check/consistency/line_endings_test.exs +++ b/test/credo/check/consistency/line_endings_test.exs @@ -55,4 +55,29 @@ defmodule Credo.Check.Readability.LineEndingsTest do |> run_check(@described_check) |> assert_issue end + + describe "autofix/1" do + test "switches unix to windows line endings" do + starting = "defmodule Credo.Sample do\n@test_attribute :foo\nend\n" + expected = "defmodule Credo.Sample do\r\n@test_attribute :foo\r\nend\r\n" + + issue = %Credo.Issue{message: "File is using unix line endings while most of the files use windows line endings."} + + assert @described_check.autofix(starting, issue) == expected + end + + test "switches windows to unix line endings" do + starting = """ + defmodule Credo.Sample do\r\n@test_attribute :foo\r\nend + """ + + expected = """ + defmodule Credo.Sample do\n@test_attribute :foo\nend + """ + + issue = %Credo.Issue{message: "File is using windows line endings while most of the files use unix line endings."} + + assert @described_check.autofix(starting, issue) == expected + end + end end diff --git a/test/credo/check/readability/alias_order_test.exs b/test/credo/check/readability/alias_order_test.exs index 62793ae41..b15aee85a 100644 --- a/test/credo/check/readability/alias_order_test.exs +++ b/test/credo/check/readability/alias_order_test.exs @@ -228,4 +228,89 @@ defmodule Credo.Check.Readability.AliasOrderTest do assert issue.trigger == "Sorter" end) end + + describe "autofix/1" do + test "puts all the aliases in the right order" do + starting = """ + defmodule CredoSampleModule do + alias Credo.CLI.Sorter + alias Credo.CLI.Command + alias Credo.CLI.Filename + end + """ + + expected = """ + defmodule CredoSampleModule do + alias Credo.CLI.Command + alias Credo.CLI.Filename + alias Credo.CLI.Sorter + end + """ + + assert @described_check.autofix(starting, nil) == expected + end + + test "works with multi-aliases" do + starting = """ + defmodule CredoSampleModule do + alias Credo.CLI.{Sorter, Command} + alias Credo.CLI.Filename + end + """ + + expected = """ + defmodule CredoSampleModule do + alias Credo.CLI.{Command, Sorter} + alias Credo.CLI.Filename + end + """ + + assert @described_check.autofix(starting, nil) == expected + end + + test "works with as: option" do + starting = """ + defmodule CredoSampleModule do + alias App.Module2 + alias App.Module1, as: Module3 + end + """ + + expected = """ + defmodule CredoSampleModule do + alias App.Module1, as: Module3 + alias App.Module2 + end + """ + + assert @described_check.autofix(starting, nil) == expected + end + + test "works with multiple blocks of aliases including multi-aliases" do + starting = """ + defmodule CredoSampleModule do + alias Credo.CLI.Filename + alias Credo.CLI.{Sorter, Command} + + alias App.Module2 + alias App.Module1 + end + """ + + # NOTE: For some reason I couldn't get the formatter to keep the newline between these + # groups. I'm likely missing some of the metadata that influences how the formatter is told + # to keep user-inserted newlines, but I'll need to dig deeper into the formatter to make + # that stuff work. For now, this has almost all the behavior that we want! + expected = """ + defmodule CredoSampleModule do + alias Credo.CLI.{Command, Sorter} + alias Credo.CLI.Filename + alias App.Module1 + alias App.Module2 + end + """ + + assert @described_check.autofix(starting, nil) == expected + end + end end diff --git a/test/credo/check/refactor/unless_with_else_test.exs b/test/credo/check/refactor/unless_with_else_test.exs index fac35cb36..340f947e0 100644 --- a/test/credo/check/refactor/unless_with_else_test.exs +++ b/test/credo/check/refactor/unless_with_else_test.exs @@ -47,4 +47,38 @@ defmodule Credo.Check.Refactor.UnlessWithElseTest do |> run_check(@described_check) |> assert_issue() end + + describe "autofix/2" do + test "changes the unless with else to an if clause" do + starting = """ + defmodule CredoSampleModule do + def some_function(parameter1, parameter2) do + unless allowed? do + unless other_condition? do + something + end + else + something_else + end + end + end + """ + + expected = """ + defmodule CredoSampleModule do + def some_function(parameter1, parameter2) do + if allowed? do + something_else + else + unless other_condition? do + something + end + end + end + end + """ + + assert @described_check.autofix(starting, nil) == expected + end + end end diff --git a/test/credo/check/warning/unused_string_operation_test.exs b/test/credo/check/warning/unused_string_operation_test.exs index 3d44d80c3..533cda557 100644 --- a/test/credo/check/warning/unused_string_operation_test.exs +++ b/test/credo/check/warning/unused_string_operation_test.exs @@ -720,4 +720,55 @@ defmodule Credo.Check.Warning.UnusedStringOperationTest do assert "String.to_float" == issue.trigger end) end + + describe "autofix/2" do + test "removes the unused String function" do + starting = """ + defmodule CredoSampleModule do + def some_function(parameter1, parameter2) do + x = parameter1 + parameter2 + + String.split(parameter1) + + parameter1 + end + end + """ + + expected = """ + defmodule CredoSampleModule do + def some_function(parameter1, parameter2) do + x = parameter1 + parameter2 + parameter1 + end + end + """ + + assert @described_check.autofix(starting, nil) == expected + end + + test "it should report a violation when end of pipe" do + starting = """ + defmodule CredoSampleModule do + def some_function(parameter1, parameter2) do + parameter1 + parameter2 + |> String.split(parameter1) + + parameter1 + end + end + """ + + expected = """ + defmodule CredoSampleModule do + def some_function(parameter1, parameter2) do + parameter1 + parameter2 + parameter1 + end + end + """ + + assert @described_check.autofix(starting, nil) == expected + end + end end diff --git a/test/credo/cli/task/run_autocorrect_test.exs b/test/credo/cli/task/run_autocorrect_test.exs new file mode 100644 index 000000000..1400b1ef0 --- /dev/null +++ b/test/credo/cli/task/run_autocorrect_test.exs @@ -0,0 +1,60 @@ +defmodule Credo.CLI.Task.RunAutofixTest do + use Credo.Test.Case + + alias Credo.CLI.Task.RunAutofix + + defmodule CheckOne do + def autofix(file, issue) do + send(self(), {:check_one, file, issue}) + "check_one" + end + end + + defmodule CheckTwo do + def autofix(file, issue) do + send(self(), {:check_two, file, issue}) + "check_two" + end + end + + defmodule CheckThree do + def autofix(file, issue) do + send(self(), {:check_three, file, issue}) + "check_three" + end + end + + describe "run/2" do + test "calls `autofix/1` for each issue with the correct arguments if autofix is true" do + issue1 = %Credo.Issue{filename: "a", check: CheckOne} + issue2 = %Credo.Issue{filename: "a", check: CheckTwo} + issue3 = %Credo.Issue{filename: "b", check: CheckThree} + + issues = [issue2, issue1, issue3] + + exec = %Credo.Execution{autofix: true} |> Credo.Execution.ExecutionIssues.start_server() + read_fun = fn _ -> "start" end + write_fun = fn file_path, contents -> send(self(), {:write, file_path, contents}) end + RunAutofix.call(exec, [issues: issues], read_fun, write_fun) + assert_receive({:check_one, "start", ^issue1}) + assert_receive({:check_two, "check_one", ^issue2}) + assert_receive({:check_three, "start", ^issue3}) + assert_receive({:write, "a", "check_two"}) + assert_receive({:write, "b", "check_three"}) + end + + test "does nothing if autofix is false" do + issues = [ + %Credo.Issue{filename: "a", check: CheckTwo}, + %Credo.Issue{filename: "a", check: CheckOne}, + %Credo.Issue{filename: "b", check: CheckThree} + ] + + exec = %Credo.Execution{autofix: false} + read_fun = fn _ -> "start" end + write_fun = fn file_path, contents -> send(self(), {:write, file_path, contents}) end + RunAutofix.call(exec, [issues: issues], read_fun, write_fun) + refute_receive(_) + end + end +end