Do not raise on Macro.Env.expand_import, closes #13573

Author: josevalim

lib/elixir/lib/macro.ex

@@ -1871,8 +1871,11 @@ defmodule Macro do
       {:function, _receiver, _name} ->
         {original, false}
 
-      :error ->
+      {:error, :not_found} ->
         {original, false}
+
+      {:error, other} ->
+        :elixir_errors.file_error(meta, env, :elixir_dispatch, {:import, other, name, arity})
     end
   end
 

lib/elixir/lib/macro/env.ex

@@ -490,7 +490,7 @@ defmodule Macro.Env do
   If any import is found, the appropriate compiler tracing
   event will be emitted.
 
-  Otherwise returns `:error`.
+  Otherwise returns `{:error, reason}`.
 
   ## Options
 
@@ -507,12 +507,12 @@ defmodule Macro.Env do
   @spec expand_import(t, keyword, atom(), arity(), keyword) ::
           {:macro, module(), (Macro.metadata(), args :: [Macro.t()] -> Macro.t())}
           | {:function, module(), atom()}
-          | :error
+          | {:error, :not_found | {:conflict, module()} | {:ambiguous, [module()]}}
   def expand_import(env, meta, name, arity, opts \\ [])
       when is_list(meta) and is_atom(name) and is_integer(arity) and is_list(opts) do
     case :elixir_import.special_form(name, arity) do
       true ->
-        :error
+        {:error, :not_found}
 
       false ->
         allow_locals = Keyword.get(opts, :allow_locals, true)
@@ -525,10 +525,16 @@ defmodule Macro.Env do
             false -> []
           end
 
-        result =
-          :elixir_dispatch.expand_import(meta, name, arity, env, extra, allow_locals, trace)
+        case :elixir_dispatch.expand_import(meta, name, arity, env, extra, allow_locals, trace) do
+          {:macro, receiver, expander} ->
+            {:macro, receiver, wrap_expansion(receiver, expander, meta, name, arity, env, opts)}
+
+          {:function, receiver, name} ->
+            {:function, receiver, name}
 
-        wrap_expansion(result, meta, name, arity, env, opts)
+          error ->
+            {:error, error}
+        end
     end
   end
 
@@ -561,24 +567,10 @@ defmodule Macro.Env do
       when is_list(meta) and is_atom(module) and is_atom(name) and is_integer(arity) and
              is_list(opts) do
     trace = Keyword.get(opts, :trace, true)
-    result = :elixir_dispatch.expand_require(meta, module, name, arity, env, trace)
-    wrap_expansion(result, meta, name, arity, env, opts)
-  end
 
-  defp wrap_expansion(result, meta, name, arity, env, opts) do
-    case result do
+    case :elixir_dispatch.expand_require(meta, module, name, arity, env, trace) do
       {:macro, receiver, expander} ->
-        fun = fn expansion_meta, args ->
-          if Keyword.get(opts, :check_deprecations, true) do
-            :elixir_dispatch.check_deprecated(:macro, meta, receiver, name, arity, env)
-          end
-
-          quoted = expander.(args, :elixir_env.env_to_ex(env))
-          next = :elixir_module.next_counter(env.module)
-          :elixir_quote.linify_with_context_counter(expansion_meta, {receiver, next}, quoted)
-        end
-
-        {:macro, receiver, fun}
+        {:macro, receiver, wrap_expansion(receiver, expander, meta, name, arity, env, opts)}
 
       {:function, receiver, name} ->
         {:function, receiver, name}
@@ -588,6 +580,18 @@ defmodule Macro.Env do
     end
   end
 
+  defp wrap_expansion(receiver, expander, meta, name, arity, env, opts) do
+    fn expansion_meta, args ->
+      if Keyword.get(opts, :check_deprecations, true) do
+        :elixir_dispatch.check_deprecated(:macro, meta, receiver, name, arity, env)
+      end
+
+      quoted = expander.(args, :elixir_env.env_to_ex(env))
+      next = :elixir_module.next_counter(env.module)
+      :elixir_quote.linify_with_context_counter(expansion_meta, {receiver, next}, quoted)
+    end
+  end
+
   @doc """
   Returns an environment in the guard context.
   """

lib/elixir/src/elixir_dispatch.erl

@@ -32,6 +32,8 @@ find_import(Meta, Name, Arity, E) ->
     {macro, Receiver} ->
       elixir_env:trace({imported_macro, Meta, Receiver, Name, Arity}, E),
       Receiver;
+    {ambiguous, _} = Ambiguous ->
+      elixir_errors:file_error(Meta, E, ?MODULE, {import, Ambiguous, Name, Arity});
     _ ->
       false
   end.
@@ -75,6 +77,8 @@ import_function(Meta, Name, Arity, E) ->
       false;
     {import, Receiver} ->
       require_function(Meta, Receiver, Name, Arity, E);
+    {ambiguous, Ambiguous} ->
+      elixir_errors:file_error(Meta, E, ?MODULE, {import, Ambiguous, Name, Arity});
     false ->
       case elixir_import:special_form(Name, Arity) of
         true ->
@@ -130,8 +134,10 @@ dispatch_import(Meta, Name, Args, S, E, Callback) ->
       expand_quoted(Meta, Receiver, Name, Arity, Expander(Args, S), S, E);
     {function, Receiver, NewName} ->
       elixir_expand:expand({{'.', Meta, [Receiver, NewName]}, Meta, Args}, S, E);
-    error ->
-      Callback()
+    not_found ->
+      Callback();
+    Error ->
+      elixir_errors:file_error(Meta, E, ?MODULE, {import, Error, Name, Arity})
   end.
 
 dispatch_require(Meta, Receiver, Name, Args, S, E, Callback) when is_atom(Receiver) ->
@@ -164,17 +170,20 @@ expand_import(Meta, Name, Arity, E, Extra, AllowLocals, Trace) ->
   Dispatch = find_import_by_name_arity(Meta, Tuple, Extra, E),
 
   case Dispatch of
+    {ambiguous, Ambiguous} ->
+      {ambiguous, Ambiguous};
+
     {import, _} ->
       do_expand_import(Dispatch, Meta, Name, Arity, Module, E, Trace);
+
     _ ->
       Local = AllowLocals andalso elixir_def:local_for(Meta, Name, Arity, [defmacro, defmacrop], E),
 
       case Dispatch of
         %% There is a local and an import. This is a conflict unless
         %% the receiver is the same as module (happens on bootstrap).
         {_, Receiver} when Local /= false, Receiver /= Module ->
-          Error = {macro_conflict, {Receiver, Name, Arity}},
-          elixir_errors:file_error(Meta, E, ?MODULE, Error);
+          {conflict, Receiver};
 
         %% There is no local. Dispatch the import.
         _ when Local == false ->
@@ -212,10 +221,10 @@ do_expand_import(Result, Meta, Name, Arity, Module, E, Trace) ->
     false when Module == ?kernel ->
       case elixir_rewrite:inline(Module, Name, Arity) of
         {AR, AN} -> {function, AR, AN};
-        false -> error
+        false -> not_found
       end;
     false ->
-      error
+      not_found
   end.
 
 expand_require(Meta, Receiver, Name, Arity, E, Trace) ->
@@ -286,7 +295,7 @@ find_imports_by_name([], Acc, _Name, _Meta, _E) ->
 find_imports_by_name(Name, [{Name, Arity} | Imports], Acc, Mod, Meta, E) ->
   case Acc of
     #{Arity := OtherMod} ->
-      Error = {ambiguous_call, {Mod, OtherMod, Name, Arity}},
+      Error = {import, {ambiguous, [Mod, OtherMod]}, Name, Arity},
       elixir_errors:file_error(Meta, E, ?MODULE, Error);
 
     #{} ->
@@ -311,11 +320,7 @@ find_import_by_name_arity(Meta, {_Name, Arity} = Tuple, Extra, E) ->
         {[], [Receiver]} -> {macro, Receiver};
         {[Receiver], []} -> {function, Receiver};
         {[], []} -> false;
-        _ ->
-          {Name, Arity} = Tuple,
-          [First, Second | _] = FunMatch ++ MacMatch,
-          Error = {ambiguous_call, {First, Second, Name, Arity}},
-          elixir_errors:file_error(Meta, E, ?MODULE, Error)
+        _ -> {ambiguous, FunMatch ++ MacMatch}
       end
   end.
 
@@ -352,11 +357,11 @@ prune_stacktrace([], _MFA, Info, _E) ->
 
 %% ERROR HANDLING
 
-format_error({macro_conflict, {Receiver, Name, Arity}}) ->
+format_error({import, {conflict, Receiver}, Name, Arity}) ->
   io_lib:format("call to local macro ~ts/~B conflicts with imported ~ts.~ts/~B, "
     "please rename the local macro or remove the conflicting import",
     [Name, Arity, elixir_aliases:inspect(Receiver), Name, Arity]);
-format_error({ambiguous_call, {Mod1, Mod2, Name, Arity}}) ->
+format_error({import, {ambiguous, [Mod1, Mod2 | _]}, Name, Arity}) ->
   io_lib:format("function ~ts/~B imported from both ~ts and ~ts, call is ambiguous",
     [Name, Arity, elixir_aliases:inspect(Mod1), elixir_aliases:inspect(Mod2)]);
 format_error({compile_env, Name, Arity}) ->

lib/elixir/test/elixir/macro/env_test.exs

@@ -177,8 +177,19 @@ defmodule Macro.EnvTest do
   end
 
   describe "expand_import/5" do
-    test "returns :error for unknown imports" do
-      assert :error = expand_import(env(), meta(), :flatten, 1)
+    test "returns tagged :error for unknown imports" do
+      assert {:error, :not_found} = expand_import(env(), meta(), :flatten, 1)
+    end
+
+    test "returns tagged :error for special forms" do
+      assert {:error, :not_found} = expand_import(env(), meta(), :case, 1)
+    end
+
+    test "returns tagged :error for ambiguous" do
+      import Date, warn: false
+      import Time, warn: false
+      assert {:error, {:ambiguous, mods}} = expand_import(__ENV__, meta(), :new, 3)
+      assert Enum.sort(mods) == [Date, Time]
     end
 
     test "returns :function tuple" do