From 57d53833cb2580ea5ff69c5ff497972826b7e987 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Dolata?= Date: Wed, 16 Feb 2022 19:21:09 +0100 Subject: [PATCH 1/5] Fix: warnings-as-errors for compiling with all-warnings --- lib/mix/lib/mix/compilers/elixir.ex | 178 +++++++++++------- .../test/mix/tasks/compile.elixir_test.exs | 27 +++ 2 files changed, 133 insertions(+), 72 deletions(-) diff --git a/lib/mix/lib/mix/compilers/elixir.ex b/lib/mix/lib/mix/compilers/elixir.ex index bd9e178231b..8ae7f96ce5f 100644 --- a/lib/mix/lib/mix/compilers/elixir.ex +++ b/lib/mix/lib/mix/compilers/elixir.ex @@ -138,30 +138,96 @@ defmodule Mix.Compilers.Elixir do {sources, removed_modules} = update_stale_sources(sources, stale, removed_modules, sources_stats) - if opts[:all_warnings], do: show_warnings(sources) - - if stale != [] do - Mix.Utils.compiling_n(length(stale), :ex) - Mix.Project.ensure_structure() - true = Code.prepend_path(dest) + previous_warning_causes_error? = + if opts[:all_warnings] do + warnings = get_warnings(sources) + show_warnings(warnings) + warnings != [] and opts[:warnings_as_errors] + else + false + end - {pending_tracer, tracer_opts} = Mix.Compilers.ApplicationTracer.prepare(app_tracer, opts) - previous_opts = set_compiler_opts(tracer_opts) + cond do + stale != [] -> + Mix.Utils.compiling_n(length(stale), :ex) + Mix.Project.ensure_structure() + true = Code.prepend_path(dest) + + {pending_tracer, tracer_opts} = Mix.Compilers.ApplicationTracer.prepare(app_tracer, opts) + previous_opts = set_compiler_opts(tracer_opts) + + # Stores state for keeping track which files were compiled + # and the dependencies between them. + put_compiler_info({[], exports, sources, modules, removed_modules}) + + try do + compile_path(stale, dest, timestamp, opts) + else + {:ok, _, warnings} -> + {modules, _exports, sources, pending_modules, _pending_exports} = get_compiler_info() + + if previous_warning_causes_error? do + display_warnings_as_error_message() + warnings = Enum.map(warnings, &diagnostic(&1, :warning)) + + {:error, warning_diagnostics(sources) ++ warnings} + else + sources = apply_warnings(sources, warnings) + + write_manifest( + manifest, + modules ++ pending_modules, + sources, + all_local_exports, + new_parents, + new_cache_key, + new_lock, + new_config, + timestamp + ) + + put_compile_env(sources) + {:ok, Enum.map(warnings, &diagnostic(&1, :warning))} + end + + {:error, errors, warnings} -> + # In case of errors, we show all previous warnings and all new ones + {_, _, sources, _, _} = get_compiler_info() + errors = Enum.map(errors, &diagnostic(&1, :error)) + warnings = Enum.map(warnings, &diagnostic(&1, :warning)) + {:error, warning_diagnostics(sources) ++ warnings ++ errors} + after + Code.compiler_options(previous_opts) + Mix.Compilers.ApplicationTracer.stop(pending_tracer) + Code.purge_compiler_modules() + delete_compiler_info() + end - # Stores state for keeping track which files were compiled - # and the dependencies between them. - put_compiler_info({[], exports, sources, modules, removed_modules}) - - try do - compile_path(stale, dest, timestamp, opts) - else - {:ok, _, warnings} -> - {modules, _exports, sources, pending_modules, _pending_exports} = get_compiler_info() - sources = apply_warnings(sources, warnings) + previous_warning_causes_error? -> + display_warnings_as_error_message() + {:error, warning_diagnostics(sources)} + true -> + # We need to return ok if deps_changed? or stale_modules changed, + # even if no code was compiled, because we need to propagate the changed + # status to compile.protocols. This will be the case whenever: + # + # * the lock file or a config changes + # * any module in a path dependency changes + # * the mix.exs changes + # * the Erlang manifest updates (Erlang files are compiled) + # + # In the first case, we will consolidate from scratch. In the remaining, we + # will only compute the diff with current protocols. In fact, there is no + # need to reconsolidate if an Erlang file changes and it doesn't trigger + # any other change, but the diff check should be reasonably fast anyway. + status = if removed != [] or deps_changed? or stale_modules != %{}, do: :ok, else: :noop + + # If nothing changed but there is one more recent mtime, bump the manifest + if status != :noop or Enum.any?(Map.values(sources_stats), &(elem(&1, 0) > modified)) do write_manifest( manifest, - modules ++ pending_modules, + modules, sources, all_local_exports, new_parents, @@ -170,54 +236,9 @@ defmodule Mix.Compilers.Elixir do new_config, timestamp ) + end - put_compile_env(sources) - {:ok, Enum.map(warnings, &diagnostic(&1, :warning))} - - {:error, errors, warnings} -> - # In case of errors, we show all previous warnings and all new ones - {_, _, sources, _, _} = get_compiler_info() - errors = Enum.map(errors, &diagnostic(&1, :error)) - warnings = Enum.map(warnings, &diagnostic(&1, :warning)) - {:error, warning_diagnostics(sources) ++ warnings ++ errors} - after - Code.compiler_options(previous_opts) - Mix.Compilers.ApplicationTracer.stop(pending_tracer) - Code.purge_compiler_modules() - delete_compiler_info() - end - else - # We need to return ok if deps_changed? or stale_modules changed, - # even if no code was compiled, because we need to propagate the changed - # status to compile.protocols. This will be the case whenever: - # - # * the lock file or a config changes - # * any module in a path dependency changes - # * the mix.exs changes - # * the Erlang manifest updates (Erlang files are compiled) - # - # In the first case, we will consolidate from scratch. In the remaining, we - # will only compute the diff with current protocols. In fact, there is no - # need to reconsolidate if an Erlang file changes and it doesn't trigger - # any other change, but the diff check should be reasonably fast anyway. - status = if removed != [] or deps_changed? or stale_modules != %{}, do: :ok, else: :noop - - # If nothing changed but there is one more recent mtime, bump the manifest - if status != :noop or Enum.any?(Map.values(sources_stats), &(elem(&1, 0) > modified)) do - write_manifest( - manifest, - modules, - sources, - all_local_exports, - new_parents, - new_cache_key, - new_lock, - new_config, - timestamp - ) - end - - {status, warning_diagnostics(sources)} + {status, warning_diagnostics(sources)} end after Mix.Compilers.ApplicationTracer.stop() @@ -803,16 +824,24 @@ defmodule Mix.Compilers.Elixir do _ = :code.delete(module) end - defp show_warnings(sources) do - for source(source: source, warnings: warnings) <- sources do - file = Path.absname(source) + defp get_warnings(sources) do + for source(source: source, warnings: warnings) <- sources, reduce: [] do + acc -> + file = Path.absname(source) - for {location, message} <- warnings do - Kernel.ParallelCompiler.print_warning({file, location, message}) - end + warnings = + for {location, message} <- warnings do + {file, location, message} + end + + acc ++ warnings end end + defp show_warnings(warnings) do + Enum.each(warnings, &Kernel.ParallelCompiler.print_warning/1) + end + defp apply_warnings(sources, warnings) do warnings = Enum.group_by(warnings, &elem(&1, 0), &{elem(&1, 1), elem(&1, 2)}) @@ -1014,4 +1043,9 @@ defmodule Mix.Compilers.Elixir do defp delete_checkpoint(manifest) do File.rm(manifest <> ".checkpoint") end + + defp display_warnings_as_error_message do + message = "Compilation failed due to warnings while using the --warnings-as-errors option" + IO.puts(:stderr, message) + end end diff --git a/lib/mix/test/mix/tasks/compile.elixir_test.exs b/lib/mix/test/mix/tasks/compile.elixir_test.exs index b2958451991..f58dff89a4b 100644 --- a/lib/mix/test/mix/tasks/compile.elixir_test.exs +++ b/lib/mix/test/mix/tasks/compile.elixir_test.exs @@ -1267,6 +1267,33 @@ defmodule Mix.Tasks.Compile.ElixirTest do end) end + test "warning from --all-warnings are treated as errors with --warnings-as-errors" do + in_fixture("no_mixfile", fn -> + Mix.Project.push(MixTest.Case.Sample) + + File.write!("lib/a.ex", """ + defmodule A do + def my_fn(unused), do: :ok + end + """) + + message = "variable \"unused\" is unused" + + assert capture_io(:stderr, fn -> + Mix.Tasks.Compile.Elixir.run([]) == :ok + end) =~ message + + assert capture_io(:stderr, fn -> + assert catch_exit( + Mix.Task.run("compile", [ + "--all-warnings", + "--warnings-as-errors" + ]) + ) + end) =~ message + end) + end + test "returns warning diagnostics" do in_fixture("no_mixfile", fn -> Mix.Project.push(MixTest.Case.Sample) From 24e81449c7bbdf9e52562c4a55ea1f9b4252c003 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 16 Feb 2022 22:30:25 +0100 Subject: [PATCH 2/5] Refactor to avoid nesting --- lib/mix/lib/mix/compilers/elixir.ex | 197 ++++++++++++---------------- 1 file changed, 87 insertions(+), 110 deletions(-) diff --git a/lib/mix/lib/mix/compilers/elixir.ex b/lib/mix/lib/mix/compilers/elixir.ex index 8ae7f96ce5f..4a6ba45e0c8 100644 --- a/lib/mix/lib/mix/compilers/elixir.ex +++ b/lib/mix/lib/mix/compilers/elixir.ex @@ -138,96 +138,34 @@ defmodule Mix.Compilers.Elixir do {sources, removed_modules} = update_stale_sources(sources, stale, removed_modules, sources_stats) - previous_warning_causes_error? = - if opts[:all_warnings] do - warnings = get_warnings(sources) - show_warnings(warnings) - warnings != [] and opts[:warnings_as_errors] + if stale != [] do + Mix.Utils.compiling_n(length(stale), :ex) + Mix.Project.ensure_structure() + true = Code.prepend_path(dest) + + {pending_tracer, tracer_opts} = Mix.Compilers.ApplicationTracer.prepare(app_tracer, opts) + previous_opts = set_compiler_opts(tracer_opts) + + # Stores state for keeping track which files were compiled + # and the dependencies between them. + put_compiler_info({[], exports, sources, modules, removed_modules}) + + try do + compile_path(stale, dest, timestamp, opts) else - false - end + {:ok, _, warnings} -> + {modules, _exports, sources, pending_modules, _pending_exports} = get_compiler_info() - cond do - stale != [] -> - Mix.Utils.compiling_n(length(stale), :ex) - Mix.Project.ensure_structure() - true = Code.prepend_path(dest) - - {pending_tracer, tracer_opts} = Mix.Compilers.ApplicationTracer.prepare(app_tracer, opts) - previous_opts = set_compiler_opts(tracer_opts) - - # Stores state for keeping track which files were compiled - # and the dependencies between them. - put_compiler_info({[], exports, sources, modules, removed_modules}) - - try do - compile_path(stale, dest, timestamp, opts) - else - {:ok, _, warnings} -> - {modules, _exports, sources, pending_modules, _pending_exports} = get_compiler_info() - - if previous_warning_causes_error? do - display_warnings_as_error_message() - warnings = Enum.map(warnings, &diagnostic(&1, :warning)) - - {:error, warning_diagnostics(sources) ++ warnings} - else - sources = apply_warnings(sources, warnings) - - write_manifest( - manifest, - modules ++ pending_modules, - sources, - all_local_exports, - new_parents, - new_cache_key, - new_lock, - new_config, - timestamp - ) - - put_compile_env(sources) - {:ok, Enum.map(warnings, &diagnostic(&1, :warning))} - end - - {:error, errors, warnings} -> - # In case of errors, we show all previous warnings and all new ones - {_, _, sources, _, _} = get_compiler_info() - errors = Enum.map(errors, &diagnostic(&1, :error)) - warnings = Enum.map(warnings, &diagnostic(&1, :warning)) - {:error, warning_diagnostics(sources) ++ warnings ++ errors} - after - Code.compiler_options(previous_opts) - Mix.Compilers.ApplicationTracer.stop(pending_tracer) - Code.purge_compiler_modules() - delete_compiler_info() - end + # We only collect the warnings if --all-warnings is given. + # In this case, we print them too. Then we apply the new warnings. + previous_warnings = + if opts[:all_warnings], do: previous_warnings(sources, true), else: [] - previous_warning_causes_error? -> - display_warnings_as_error_message() - {:error, warning_diagnostics(sources)} + sources = apply_warnings(sources, warnings) - true -> - # We need to return ok if deps_changed? or stale_modules changed, - # even if no code was compiled, because we need to propagate the changed - # status to compile.protocols. This will be the case whenever: - # - # * the lock file or a config changes - # * any module in a path dependency changes - # * the mix.exs changes - # * the Erlang manifest updates (Erlang files are compiled) - # - # In the first case, we will consolidate from scratch. In the remaining, we - # will only compute the diff with current protocols. In fact, there is no - # need to reconsolidate if an Erlang file changes and it doesn't trigger - # any other change, but the diff check should be reasonably fast anyway. - status = if removed != [] or deps_changed? or stale_modules != %{}, do: :ok, else: :noop - - # If nothing changed but there is one more recent mtime, bump the manifest - if status != :noop or Enum.any?(Map.values(sources_stats), &(elem(&1, 0) > modified)) do write_manifest( manifest, - modules, + modules ++ pending_modules, sources, all_local_exports, new_parents, @@ -236,9 +174,57 @@ defmodule Mix.Compilers.Elixir do new_config, timestamp ) - end - {status, warning_diagnostics(sources)} + put_compile_env(sources) + all_warnings = previous_warnings ++ Enum.map(warnings, &diagnostic(&1, :warning)) + unless_previous_warnings_as_errors(previous_warnings, opts, {:ok, all_warnings}) + + {:error, errors, warnings} -> + # In case of errors, we show all previous warnings and all new ones. + # Print the new ones if --all-warnings was given. + {_, _, sources, _, _} = get_compiler_info() + errors = Enum.map(errors, &diagnostic(&1, :error)) + warnings = Enum.map(warnings, &diagnostic(&1, :warning)) + {:error, previous_warnings(sources, opts[:all_warnings]) ++ warnings ++ errors} + after + Code.compiler_options(previous_opts) + Mix.Compilers.ApplicationTracer.stop(pending_tracer) + Code.purge_compiler_modules() + delete_compiler_info() + end + else + # We need to return ok if deps_changed? or stale_modules changed, + # even if no code was compiled, because we need to propagate the changed + # status to compile.protocols. This will be the case whenever: + # + # * the lock file or a config changes + # * any module in a path dependency changes + # * the mix.exs changes + # * the Erlang manifest updates (Erlang files are compiled) + # + # In the first case, we will consolidate from scratch. In the remaining, we + # will only compute the diff with current protocols. In fact, there is no + # need to reconsolidate if an Erlang file changes and it doesn't trigger + # any other change, but the diff check should be reasonably fast anyway. + status = if removed != [] or deps_changed? or stale_modules != %{}, do: :ok, else: :noop + + # If nothing changed but there is one more recent mtime, bump the manifest + if status != :noop or Enum.any?(Map.values(sources_stats), &(elem(&1, 0) > modified)) do + write_manifest( + manifest, + modules, + sources, + all_local_exports, + new_parents, + new_cache_key, + new_lock, + new_config, + timestamp + ) + end + + previous_warnings = previous_warnings(sources, opts[:all_warnings]) + unless_previous_warnings_as_errors(previous_warnings, opts, {status, previous_warnings}) end after Mix.Compilers.ApplicationTracer.stop() @@ -824,24 +810,16 @@ defmodule Mix.Compilers.Elixir do _ = :code.delete(module) end - defp get_warnings(sources) do - for source(source: source, warnings: warnings) <- sources, reduce: [] do - acc -> - file = Path.absname(source) - - warnings = - for {location, message} <- warnings do - {file, location, message} - end - - acc ++ warnings + defp previous_warnings(sources, print?) do + for source(source: source, warnings: warnings) <- sources, + file = Path.absname(source), + {location, message} <- warnings do + warning = {file, location, message} + print? && Kernel.ParallelCompiler.print_warning(warning) + diagnostic(warning, :warning) end end - defp show_warnings(warnings) do - Enum.each(warnings, &Kernel.ParallelCompiler.print_warning/1) - end - defp apply_warnings(sources, warnings) do warnings = Enum.group_by(warnings, &elem(&1, 0), &{elem(&1, 1), elem(&1, 2)}) @@ -850,12 +828,6 @@ defmodule Mix.Compilers.Elixir do end end - defp warning_diagnostics(sources) do - for source(source: source, warnings: warnings) <- sources, - {location, message} <- warnings, - do: diagnostic({Path.absname(source), location, message}, :warning) - end - defp diagnostic({file, location, message}, severity) do %Mix.Task.Compiler.Diagnostic{ file: file, @@ -1044,8 +1016,13 @@ defmodule Mix.Compilers.Elixir do File.rm(manifest <> ".checkpoint") end - defp display_warnings_as_error_message do - message = "Compilation failed due to warnings while using the --warnings-as-errors option" - IO.puts(:stderr, message) + defp unless_previous_warnings_as_errors(previous_warnings, opts, {status, all_warnings}) do + if previous_warnings != [] and opts[:warnings_as_errors] do + message = "Compilation failed due to warnings while using the --warnings-as-errors option" + IO.puts(:stderr, message) + {:error, all_warnings} + else + {status, all_warnings} + end end end From c19f54464070a652ec99dce01c2e271978ee1566 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 16 Feb 2022 22:33:53 +0100 Subject: [PATCH 3/5] Apply suggestions from code review --- lib/mix/test/mix/tasks/compile.elixir_test.exs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/lib/mix/test/mix/tasks/compile.elixir_test.exs b/lib/mix/test/mix/tasks/compile.elixir_test.exs index f58dff89a4b..f40f195bcce 100644 --- a/lib/mix/test/mix/tasks/compile.elixir_test.exs +++ b/lib/mix/test/mix/tasks/compile.elixir_test.exs @@ -1283,13 +1283,9 @@ defmodule Mix.Tasks.Compile.ElixirTest do Mix.Tasks.Compile.Elixir.run([]) == :ok end) =~ message + # Stale compilation fails due to warnings as errors assert capture_io(:stderr, fn -> - assert catch_exit( - Mix.Task.run("compile", [ - "--all-warnings", - "--warnings-as-errors" - ]) - ) + catch_exit(Mix.Task.run("compile", ["--all-warnings", "--warnings-as-errors"])) end) =~ message end) end From bfa5ed227efa9367678e800f2338faced24eda04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 16 Feb 2022 22:34:12 +0100 Subject: [PATCH 4/5] Apply suggestions from code review --- lib/mix/test/mix/tasks/compile.elixir_test.exs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/lib/mix/test/mix/tasks/compile.elixir_test.exs b/lib/mix/test/mix/tasks/compile.elixir_test.exs index f40f195bcce..51fc550a227 100644 --- a/lib/mix/test/mix/tasks/compile.elixir_test.exs +++ b/lib/mix/test/mix/tasks/compile.elixir_test.exs @@ -1287,6 +1287,17 @@ defmodule Mix.Tasks.Compile.ElixirTest do assert capture_io(:stderr, fn -> catch_exit(Mix.Task.run("compile", ["--all-warnings", "--warnings-as-errors"])) end) =~ message + Mix.Task.clear() + + File.write!("lib/b.ex", """ + defmodule B do + end + """) + + # Recompiling a new file still fails due to warnings as errors + assert capture_io(:stderr, fn -> + catch_exit(Mix.Task.run("compile", ["--all-warnings", "--warnings-as-errors"])) + end) =~ message end) end From ed56f6a118ddd91104594830e0c689498207c793 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 16 Feb 2022 22:34:38 +0100 Subject: [PATCH 5/5] Update lib/mix/test/mix/tasks/compile.elixir_test.exs --- lib/mix/test/mix/tasks/compile.elixir_test.exs | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/mix/test/mix/tasks/compile.elixir_test.exs b/lib/mix/test/mix/tasks/compile.elixir_test.exs index 51fc550a227..0090fc4a75d 100644 --- a/lib/mix/test/mix/tasks/compile.elixir_test.exs +++ b/lib/mix/test/mix/tasks/compile.elixir_test.exs @@ -1287,6 +1287,7 @@ defmodule Mix.Tasks.Compile.ElixirTest do assert capture_io(:stderr, fn -> catch_exit(Mix.Task.run("compile", ["--all-warnings", "--warnings-as-errors"])) end) =~ message + Mix.Task.clear() File.write!("lib/b.ex", """