Track removed modules and exports across local deps, closes #12707

Author: josevalim

lib/mix/lib/mix/compilers/elixir.ex

@@ -56,7 +56,7 @@ defmodule Mix.Compilers.Elixir do
         []
       end
 
-    local_deps = Enum.reject(Mix.Dep.cached(), & &1.scm.fetchable?)
+    local_deps = Enum.reject(Mix.Dep.cached(), & &1.scm.fetchable?())
 
     # If mix.exs has changed, recompile anything that calls Mix.Project.
     stale =
@@ -569,53 +569,65 @@ defmodule Mix.Compilers.Elixir do
     # as any export from a dependency needs to be recompiled.
     stale_modules = Map.from_keys(stale_modules, true)
 
-    for %{opts: opts} <- local_deps,
-        manifest = Path.join([opts[:build], ".mix", base]),
-        Mix.Utils.last_modified(manifest) > modified,
-        reduce: {stale_modules, stale_modules, old_exports} do
-      {modules, exports, new_exports} ->
-        {manifest_modules, manifest_sources} = read_manifest(manifest)
-
-        dep_modules =
-          for module(module: module, timestamp: timestamp) <- manifest_modules,
-              timestamp > modified,
-              do: module
-
-        # If any module has a compile time dependency on a changed module
-        # within the dependency, they will be recompiled. However, export
-        # and runtime dependencies won't have recompiled so we need to
-        # propagate them to the parent app.
-        {dep_modules, _, _} =
-          fixpoint_runtime_modules(manifest_sources, Map.from_keys(dep_modules, true))
-
-        # Update exports
-        {exports, new_exports} =
-          for {module, _} <- dep_modules, reduce: {exports, new_exports} do
-            {exports, new_exports} ->
-              export = exports_md5(module, false)
-
-              # If the exports are the same, then the API did not change,
-              # so we do not mark the export as stale. Note this has to
-              # be very conservative. If the module is not loaded or if
-              # the exports were not there, we need to consider it a stale
-              # export.
-              exports =
-                if export && old_exports[module] == export,
-                  do: exports,
-                  else: Map.put(exports, module, true)
-
-              # In any case, we always store it as the most update export
-              # that we have, otherwise we delete it.
-              new_exports =
-                if export,
-                  do: Map.put(new_exports, module, export),
-                  else: Map.delete(new_exports, module)
-
-              {exports, new_exports}
-          end
+    {stale_modules, stale_exports, new_exports} =
+      for %{opts: opts} <- local_deps,
+          manifest = Path.join([opts[:build], ".mix", base]),
+          Mix.Utils.last_modified(manifest) > modified,
+          reduce: {stale_modules, stale_modules, []} do
+        {modules, exports, new_exports} ->
+          {manifest_modules, manifest_sources} = read_manifest(manifest)
+
+          dep_modules =
+            for module(module: module, timestamp: timestamp) <- manifest_modules,
+                timestamp > modified,
+                do: module
+
+          # If any module has a compile time dependency on a changed module
+          # within the dependency, they will be recompiled. However, export
+          # and runtime dependencies won't have recompiled so we need to
+          # propagate them to the parent app.
+          {dep_modules, _, _} =
+            fixpoint_runtime_modules(manifest_sources, Map.from_keys(dep_modules, true))
+
+          # Update exports
+          {exports, new_exports} =
+            for {module, _} <- dep_modules, reduce: {exports, new_exports} do
+              {exports, new_exports} ->
+                export = exports_md5(module, false)
+
+                # If the exports are the same, then the API did not change,
+                # so we do not mark the export as stale. Note this has to
+                # be very conservative. If the module is not loaded or if
+                # the exports were not there, we need to consider it a stale
+                # export.
+                exports =
+                  if export && old_exports[module] == export,
+                    do: exports,
+                    else: Map.put(exports, module, true)
+
+                # Then we store the new export if any
+                new_exports =
+                  if export,
+                    do: [{module, export} | new_exports],
+                    else: new_exports
+
+                {exports, new_exports}
+            end
+
+          {Map.merge(modules, dep_modules), exports, new_exports}
+      end
 
-        {Map.merge(modules, dep_modules), exports, new_exports}
-    end
+    # Any dependency in old export but not in new export
+    # was removed so we need to mark them as stale too.
+    new_exports = Map.new(new_exports)
+
+    removed =
+      for {module, _} <- old_exports,
+          not is_map_key(new_exports, module),
+          do: {module, true},
+          into: %{}
+
+    {Map.merge(stale_modules, removed), Map.merge(stale_exports, removed), new_exports}
   end
 
   defp fixpoint_runtime_modules(sources, modules) when modules != %{} do

lib/mix/lib/mix/tasks/compile.ex

@@ -146,9 +146,9 @@ defmodule Mix.Tasks.Compile do
     config = Mix.Project.config()
 
     # If we are in an umbrella project, now load paths from all children.
-    if Mix.Project.umbrella?(config) do
+    if apps_paths = Mix.Project.apps_paths(config) do
       loaded_paths =
-        Mix.Project.apps_paths(config)
+        apps_paths
         |> Map.keys()
         |> Mix.AppLoader.load_apps(Mix.Dep.cached(), config, [], fn
           {_app, path}, acc -> if path, do: [path | acc], else: acc

lib/mix/test/mix/umbrella_test.exs

@@ -252,8 +252,6 @@ defmodule Mix.UmbrellaTest do
     end)
   end
 
-  ## Umbrellas as a dependency
-
   test "list deps for umbrella as dependency" do
     in_fixture("umbrella_dep", fn ->
       Mix.Project.in_project(:umbrella_dep, ".", fn _ ->
@@ -373,7 +371,7 @@ defmodule Mix.UmbrellaTest do
     end)
   end
 
-  test "recompiles after compile time path dependency changes" do
+  test "recompiles when path dependencies change" do
     in_fixture("umbrella_dep/deps/umbrella/apps", fn ->
       Mix.Project.in_project(:bar, "bar", fn _ ->
         Mix.Task.run("compile", [])
@@ -385,30 +383,13 @@ defmodule Mix.UmbrellaTest do
         assert Mix.Task.run("compile", ["--verbose"]) == {:ok, []}
         assert_receive {:mix_shell, :info, ["Compiled lib/bar.ex"]}
 
-        # Emulate local recompilation
+        # Compile-time dependencies are recompiled
         File.write!("../foo/lib/foo.ex", File.read!("../foo/lib/foo.ex") <> "\n")
-        mtime = File.stat!("_build/dev/lib/bar/.mix/compile.elixir").mtime
-        ensure_touched("../foo/lib/foo.ex", mtime)
-
-        Mix.Task.clear()
-        assert Mix.Task.run("compile", ["--verbose"]) == {:ok, []}
-        assert_received {:mix_shell, :info, ["Compiled lib/bar.ex"]}
-
-        # But exports dependencies are not recompiled
-        File.write!("lib/bar.ex", "defmodule Bar, do: (require Foo)")
+        ensure_touched("../foo/lib/foo.ex", "_build/dev/lib/foo/.mix/compile.elixir")
 
         Mix.Task.clear()
         assert Mix.Task.run("compile", ["--verbose"]) == {:ok, []}
         assert_received {:mix_shell, :info, ["Compiled lib/bar.ex"]}
-
-        # Touch to emulate local recompilation
-        File.write!("../foo/lib/foo.ex", File.read!("../foo/lib/foo.ex") <> "\n")
-        mtime = File.stat!("_build/dev/lib/bar/.mix/compile.elixir").mtime
-        ensure_touched("../foo/lib/foo.ex", mtime)
-
-        Mix.Task.clear()
-        assert Mix.Tasks.Compile.Elixir.run(["--verbose"]) == {:noop, []}
-        refute_received {:mix_shell, :info, ["Compiled lib/bar.ex"]}
       end)
 
       # Now let's add a new file to foo
@@ -446,18 +427,39 @@ defmodule Mix.UmbrellaTest do
         File.write!("../foo/lib/foo.ex", "defmodule Foo, do: defstruct [:bar]")
 
         # Add struct dependency
-        File.write!("lib/bar.ex", "defmodule Bar, do: %Foo{bar: true}")
+        File.write!("lib/bar.ex", """
+        defmodule Bar do
+          def is_foo_bar(%Foo{bar: true}), do: true
+        end
+        """)
+
         Mix.Task.run("compile", ["--verbose"])
-        assert_receive {:mix_shell, :info, ["Compiled lib/bar.ex"]}
+        assert_received {:mix_shell, :info, ["Compiled lib/bar.ex"]}
 
-        # Recompiles for struct dependencies
+        # Does not recompiles if export dependency does not change
         File.write!("../foo/lib/foo.ex", File.read!("../foo/lib/foo.ex") <> "\n")
-        mtime = File.stat!("_build/dev/lib/bar/.mix/compile.elixir").mtime
-        ensure_touched("../foo/lib/foo.ex", mtime)
+        ensure_touched("../foo/lib/foo.ex", "_build/dev/lib/bar/.mix/compile.elixir")
 
         Mix.Task.clear()
         assert Mix.Task.run("compile", ["--verbose"]) == {:ok, []}
-        assert_receive {:mix_shell, :info, ["Compiled lib/bar.ex"]}
+        refute_received {:mix_shell, :info, ["Compiled lib/bar.ex"]}
+
+        # Recompiles if export dependency changes
+        File.write!("../foo/lib/foo.ex", "defmodule Foo, do: defstruct [:bar, :baz]")
+        ensure_touched("../foo/lib/foo.ex", "_build/dev/lib/bar/.mix/compile.elixir")
+
+        Mix.Task.clear()
+        assert Mix.Task.run("compile", ["--verbose"]) == {:ok, []}
+        assert_received {:mix_shell, :info, ["Compiled lib/bar.ex"]}
+
+        # Recompiles if export dependency is removed
+        File.rm!("../foo/lib/foo.ex")
+        Mix.Task.clear()
+
+        ExUnit.CaptureIO.capture_io(:stderr, fn ->
+          Process.flag(:trap_exit, true)
+          catch_exit(Mix.Task.run("compile", ["--verbose"]))
+        end)
       end)
     end)
   end
@@ -480,16 +482,15 @@ defmodule Mix.UmbrellaTest do
         # Add compile time to Foo.Bar
         File.write!("lib/bar.ex", "defmodule Bar, do: Foo.Bar.hello()")
         Mix.Task.run("compile", ["--verbose"])
-        assert_receive {:mix_shell, :info, ["Compiled lib/bar.ex"]}
+        assert_received {:mix_shell, :info, ["Compiled lib/bar.ex"]}
 
         # Recompiles for due to compile dependency via runtime dependencies
         File.write!("../foo/lib/foo.baz.ex", File.read!("../foo/lib/foo.baz.ex") <> "\n")
-        mtime = File.stat!("_build/dev/lib/bar/.mix/compile.elixir").mtime
-        ensure_touched("../foo/lib/foo.ex", mtime)
+        ensure_touched("../foo/lib/foo.ex", "_build/dev/lib/bar/.mix/compile.elixir")
 
         Mix.Task.clear()
         assert Mix.Task.run("compile", ["--verbose"]) == {:ok, []}
-        assert_receive {:mix_shell, :info, ["Compiled lib/bar.ex"]}
+        assert_received {:mix_shell, :info, ["Compiled lib/bar.ex"]}
       end)
     end)
   end