Move struct validation in patterns and updates to type checker

This means changing a struct definition only cases
recompilation when structs are created, no longer on
matches and updates.

Closes #15098.

Author: josevalim

lib/elixir/lib/kernel/parallel_compiler.ex

@@ -460,7 +460,7 @@ defmodule Kernel.ParallelCompiler do
     modules = write_module_binaries(result, state.output, state)
     profile(state, "after compile callback", state.after_compile)
 
-    runtime_warnings =
+    {runtime_warnings, errors} =
       if state.verification? do
         profile(
           state,
@@ -471,11 +471,19 @@ defmodule Kernel.ParallelCompiler do
           fn -> Module.ParallelChecker.verify(state.checker, dependent_modules) end
         )
       else
-        []
+        {[], []}
       end
 
     info = %{compile_warnings: Enum.reverse(compile_warnings), runtime_warnings: runtime_warnings}
-    {{:ok, modules, info}, state}
+
+    case errors do
+      [] ->
+        {{:ok, modules, info}, state}
+
+      _ ->
+        IO.puts(:stderr, "== Type checking failed with errors ==")
+        {{:error, errors, info}, state}
+    end
   end
 
   defp profile_init(:time), do: {:time, System.monotonic_time(), 0}

lib/elixir/lib/macro.ex

@@ -971,9 +971,15 @@ defmodule Macro do
             }
           ]
   def struct_info!(module, env) when is_atom(module) do
-    case :elixir_map.maybe_load_struct_info([line: env.line], module, [], true, env) do
-      {:ok, info} -> info
-      {:error, desc} -> raise ArgumentError, List.to_string(:elixir_map.format_error(desc))
+    meta = [line: env.line]
+
+    case :elixir_map.maybe_load_struct_info(meta, module, env) do
+      {:ok, info} ->
+        :elixir_env.trace({:struct_expansion, meta, module, []}, env)
+        info
+
+      {:error, desc} ->
+        raise ArgumentError, List.to_string(:elixir_map.format_error(desc))
     end
   end
 

lib/elixir/lib/module/parallel_checker.ex

@@ -120,14 +120,14 @@ defmodule Module.ParallelChecker do
                 # Set the compiler info so we can collect warnings
                 :erlang.put(:elixir_compiler_info, {pid, self()})
 
-                warnings =
+                {warnings, errors} =
                   if module_tuple do
                     check_module(module_tuple, {checker, table}, log?)
                   else
-                    []
+                    {[], []}
                   end
 
-                send(pid, {__MODULE__, module, warnings})
+                send(pid, {__MODULE__, module, warnings, errors})
                 send(checker, {__MODULE__, :done, module})
             end
 
@@ -194,29 +194,34 @@ defmodule Module.ParallelChecker do
     end
 
     count = :gen_server.call(checker, :start, :infinity)
-    diagnostics = collect_results(count, [])
+    {warnings, errors} = collect_results(count, [], [])
 
     case :erlang.get(:elixir_code_diagnostics) do
       :undefined -> :ok
-      {tail, log?} -> :erlang.put(:elixir_code_diagnostics, {diagnostics ++ tail, log?})
+      {tail, log?} -> :erlang.put(:elixir_code_diagnostics, {errors ++ warnings ++ tail, log?})
     end
 
-    diagnostics
+    {warnings, errors}
   end
 
-  defp collect_results(0, diagnostics) do
-    diagnostics
+  defp collect_results(0, warnings, errors) do
+    {warnings, errors}
   end
 
-  defp collect_results(count, diagnostics) do
+  defp collect_results(count, warnings, errors) do
     receive do
       {:diagnostic, %{file: file} = diagnostic, read_snippet} ->
         :elixir_errors.print_diagnostic(diagnostic, read_snippet)
         diagnostic = %{diagnostic | file: file && Path.absname(file)}
-        collect_results(count, [diagnostic | diagnostics])
 
-      {__MODULE__, _module, new_diagnostics} ->
-        collect_results(count - 1, new_diagnostics ++ diagnostics)
+        if Map.get(diagnostic, :severity, :warning) == :error do
+          collect_results(count, warnings, [diagnostic | errors])
+        else
+          collect_results(count, [diagnostic | warnings], errors)
+        end
+
+      {__MODULE__, _module, new_warnings, new_errors} ->
+        collect_results(count - 1, new_warnings ++ warnings, new_errors ++ errors)
     end
   end
 
@@ -270,18 +275,19 @@ defmodule Module.ParallelChecker do
         definitions
       )
 
-    diagnostics =
+    {warnings, errors} =
       module
       |> Module.Types.warnings(file, attrs, definitions, no_warn_undefined, cache)
       |> Kernel.++(behaviour_warnings)
-      |> group_warnings()
-      |> emit_warnings(file, log?)
+      |> group_diagnostics()
+      |> emit_diagnostics(file, log?)
+      |> Enum.split_with(&(&1.severity == :warning))
 
     Enum.each(after_verify, fn {verify_mod, verify_fun} ->
       apply(verify_mod, verify_fun, [module])
     end)
 
-    diagnostics
+    {warnings, errors}
   end
 
   defp module_map_to_module_tuple(module_map) do
@@ -332,16 +338,16 @@ defmodule Module.ParallelChecker do
 
   ## Warning helpers
 
-  defp group_warnings(warnings) do
+  defp group_diagnostics(triplets) do
     {ungrouped, grouped} =
-      Enum.reduce(warnings, {[], %{}}, fn {module, warning, location}, {ungrouped, grouped} ->
-        %{message: _} = diagnostic = module.format_diagnostic(warning)
+      Enum.reduce(triplets, {[], %{}}, fn {module, term, location}, {ungrouped, grouped} ->
+        %{message: _} = diagnostic = module.format_diagnostic(term)
 
         if Map.get(diagnostic, :group, false) do
           locations = MapSet.new([location])
 
           grouped =
-            Map.update(grouped, warning, {locations, diagnostic}, fn
+            Map.update(grouped, term, {locations, diagnostic}, fn
               {locations, diagnostic} -> {MapSet.put(locations, location), diagnostic}
             end)
 
@@ -359,7 +365,7 @@ defmodule Module.ParallelChecker do
     Enum.sort(ungrouped ++ grouped)
   end
 
-  defp emit_warnings(warnings, file, log?) do
+  defp emit_diagnostics(warnings, file, log?) do
     Enum.flat_map(warnings, fn {locations, diagnostic} ->
       diagnostics = Enum.map(locations, &to_diagnostic(diagnostic, file, &1))
       log? and print_diagnostics(diagnostics)

lib/elixir/lib/module/types/apply.ex

@@ -1953,17 +1953,10 @@ defmodule Module.Types.Apply do
   end
 
   def format_diagnostic({:undefined, :badmodule, module, fun, arity}) do
-    top =
-      if fun == :__struct__ and arity == 0 do
-        "struct #{inspect(module)}"
-      else
-        Exception.format_mfa(module, fun, arity)
-      end
-
     %{
       message:
         IO.iodata_to_binary([
-          top,
+          Exception.format_mfa(module, fun, arity),
           " is undefined (module ",
           inspect(module),
           " is not available or is yet to be defined)",
@@ -1973,14 +1966,6 @@ defmodule Module.Types.Apply do
     }
   end
 
-  def format_diagnostic({:undefined, {:badfunction, _}, module, :__struct__, 0}) do
-    %{
-      message:
-        "struct #{inspect(module)} is undefined (there is such module but it does not define a struct)",
-      group: true
-    }
-  end
-
   def format_diagnostic({:undefined, {:badfunction, _}, module, fun, arity}) do
     _ = Code.ensure_loaded(module)
 

lib/elixir/lib/module/types/expr.ex

@@ -210,28 +210,41 @@ defmodule Module.Types.Expr do
         stack,
         context
       ) do
-    # We pass the expected type as `term()` because the struct update
-    # operator already expects it to be a map at this point.
-    {map_type, context} = of_expr(map, term(), struct, stack, context)
+    {info, context} = Of.struct_info(module, :expr, meta, stack, context)
 
-    context =
-      with {false, struct_key_type} <- map_fetch_key(map_type, :__struct__),
-           {:finite, [^module]} <- atom_fetch(struct_key_type) do
-        context
-      else
-        _ ->
-          error(__MODULE__, {:badupdate, map_type, struct, context}, meta, stack, context)
-      end
+    if info do
+      # We pass the expected type as `term()` because the struct update
+      # operator already expects it to be a map at this point.
+      {map_type, context} = of_expr(map, term(), struct, stack, context)
 
-    Enum.reduce(pairs, {map_type, context}, fn {key, value}, {acc, context} ->
-      # TODO: Once we support typed structs, we need to type check them here
-      {type, context} = of_expr(value, term(), expr, stack, context)
+      context =
+        with {false, struct_key_type} <- map_fetch_key(map_type, :__struct__),
+             {:finite, [^module]} <- atom_fetch(struct_key_type) do
+          context
+        else
+          _ ->
+            error(__MODULE__, {:badupdate, map_type, struct, context}, meta, stack, context)
+        end
 
-      case map_put_key(acc, key, type) do
-        {:ok, acc} -> {acc, context}
-        _ -> {acc, context}
-      end
-    end)
+      Enum.reduce(pairs, {map_type, context}, fn {key, value}, {acc, context} ->
+        context =
+          if Enum.any?(info, &(&1.field == key)) do
+            context
+          else
+            Of.unknown_struct_field(module, key, :expr, meta, stack, context)
+          end
+
+        # TODO: Once we support typed structs, we need to type check them here
+        {type, context} = of_expr(value, term(), expr, stack, context)
+
+        case map_put_key(acc, key, type) do
+          {:ok, acc} -> {acc, context}
+          _ -> {acc, context}
+        end
+      end)
+    else
+      {error_type(), context}
+    end
   end
 
   # %{...}
@@ -543,12 +556,11 @@ defmodule Module.Types.Expr do
       Enum.map_reduce(exceptions, original, fn exception, context ->
         # Exceptions are not validated in the compiler,
         # to avoid export dependencies. So we do it here.
-        if Code.ensure_loaded?(exception) and function_exported?(exception, :__struct__, 0) do
-          {info, context} = Of.struct_info(exception, meta, stack, context)
+        {info, context} = Of.struct_info(exception, :expr, meta, stack, context)
+
+        if info do
           {Of.struct_type(exception, info, args), context}
         else
-          # If the exception cannot be found or is invalid, fetch the signature to emit warnings.
-          {_, context} = Apply.signature(exception, :__struct__, 0, meta, stack, context)
           {error_type(), context}
         end
       end)

lib/elixir/lib/module/types/of.ex

@@ -434,11 +434,17 @@ defmodule Module.Types.Of do
 
   @doc """
   Handles instantiation of a new struct.
+
+  This is expanded and validated by the compiler, so don't need to check the fields.
   """
   # TODO: Type check the fields match the struct
   def struct_instance(struct, args, expected, meta, stack, context, of_fun)
       when is_atom(struct) do
-    {_info, context} = struct_info(struct, meta, stack, context)
+    {info, context} = struct_info(struct, :expr, meta, stack, context)
+
+    if is_nil(info) do
+      raise "expected #{inspect(struct)} to return struct metadata, but got none"
+    end
 
     # The compiler has already checked the keys are atoms and which ones are required.
     {args_types, context} =
@@ -459,23 +465,28 @@ defmodule Module.Types.Of do
   @doc """
   Returns `__info__(:struct)` information about a struct.
   """
-  def struct_info(struct, meta, stack, context) do
+  def struct_info(struct, kind, meta, stack, context) do
     case stack.no_warn_undefined do
       %Macro.Env{} = env ->
-        case :elixir_map.maybe_load_struct_info(meta, struct, [], false, env) do
+        case :elixir_map.maybe_load_struct_info(meta, struct, env) do
           {:ok, info} -> {info, context}
-          {:error, desc} -> raise ArgumentError, List.to_string(:elixir_map.format_error(desc))
+          {:error, _desc} -> {nil, context}
         end
 
       _ ->
-        # Fetch the signature to validate for warnings.
-        {_, context} = Module.Types.Apply.signature(struct, :__struct__, 0, meta, stack, context)
-
         info =
-          struct.__info__(:struct) ||
-            raise "expected #{inspect(struct)} to return struct metadata, but got none"
+          Code.ensure_loaded?(struct) and function_exported?(struct, :__info__, 1) and
+            struct.__info__(:struct)
+
+        if info do
+          {_, context} =
+            Module.Types.Apply.signature(struct, :__struct__, 0, meta, stack, context)
 
-        {info, context}
+          {info, context}
+        else
+          error = {:unknown_struct, kind, struct}
+          {nil, error(error, meta, stack, context)}
+        end
     end
   end
 
@@ -492,6 +503,14 @@ defmodule Module.Types.Of do
     closed_map(pairs)
   end
 
+  @doc """
+  Returns shared error for unknown struct field.
+  """
+  def unknown_struct_field(struct, field, kind, meta, stack, context) do
+    error = {:unknown_struct_field, kind, struct, field}
+    error(error, meta, stack, context)
+  end
+
   ## Bitstrings
 
   @doc """
@@ -802,6 +821,30 @@ defmodule Module.Types.Of do
     }
   end
 
+  def format_diagnostic({:unknown_struct, kind, module}) do
+    message =
+      if Code.ensure_loaded?(module) do
+        "struct #{inspect(module)} is undefined (there is such module but it does not define a struct)"
+      else
+        "struct #{inspect(module)} is undefined " <>
+          "(module #{inspect(module)} is not available or is yet to be defined)"
+      end
+
+    %{
+      message: message,
+      group: true,
+      severity: if(kind == :pattern, do: :error, else: :warning)
+    }
+  end
+
+  def format_diagnostic({:unknown_struct_field, kind, module, field}) do
+    %{
+      message: "unknown key #{inspect(field)} for struct #{inspect(module)}",
+      group: true,
+      severity: if(kind == :pattern, do: :error, else: :warning)
+    }
+  end
+
   defp dot_var?(expr) do
     match?({{:., _, [var, _fun]}, _, _args} when is_var(var), expr)
   end