Avoid applying map update callbacks to absent branches

Author: gldubc

lib/elixir/lib/module/types/descr.ex

@@ -3650,6 +3650,7 @@ defmodule Module.Types.Descr do
   However, the third argument is an anonymous function that receives the current
   value and returns `type_fun`. Note the value returned by `type_fun` cannot hold
   dynamic. Any dynamic conversion must happen before invoking this function.
+
   """
   def map_update_fun(descr, key_descr, type_fun, return_type? \\ true, force? \\ false) do
     gradual? = gradual?(descr)
@@ -3766,7 +3767,7 @@ defmodule Module.Types.Descr do
         # If any of required or optional domains are satisfied, then we compute the
         # initial return type. `map_update_keys_static` will then union into the
         # computed type below, using the original bdd/dnf, not the one with updated domains.
-        descr = map_update_put_domains(bdd, domains, type_fun)
+        descr = map_update_put_domains(bdd, domains, type_fun, force?)
         {remove_optional(value), descr, errors, true}
       else
         {remove_optional(value), none(), errors, false}
@@ -3919,31 +3920,42 @@ defmodule Module.Types.Descr do
     :found_key -> true
   end
 
+  # For each domain key, check if it exists in the map DNF and classify it
+  # as valid (matched) or invalid (missing). Accumulates the value type if require_type? is set.
+  # Returns {found?, valid_domains, invalid_domains, accumulated_value_type},
+  # where found? tracks whether at least one domain key was matched in the map.
   defp map_update_get_domains(dnf, domain_keys, acc, require_type?, any_atom_key) do
     Enum.reduce(domain_keys, {false, [], [], acc}, fn domain_key, {found?, valid, invalid, acc} ->
+      # Get the value type for this domain key, excluding optional entries
       value = map_get_domain_no_optional(dnf, domain_key, none())
 
       cond do
+        # Atom domains are special: we also check for individually named atom keys
         domain_key == :atom ->
           atom_acc = any_atom_key.()
 
           cond do
+            # Domain has a direct match: valid, union both atom keys and domain value
             not empty?(value) ->
               acc = if require_type?, do: union(union(atom_acc, acc), value), else: acc
               {true, [:atom | valid], invalid, acc}
 
+            # No direct match, but individual atom keys exist: found but domain is invalid
             not empty?(atom_acc) ->
               acc = if require_type?, do: union(atom_acc, acc), else: acc
               {true, valid, [:atom | invalid], acc}
 
+            # No match at all
             true ->
               {found?, valid, [:atom | invalid], acc}
           end
 
+        # Non-atom domain key has a match: mark as valid
         not empty?(value) ->
           acc = if require_type?, do: union(acc, value), else: acc
           {true, [domain_key | valid], invalid, acc}
 
+        # Non-atom domain key not found: mark as invalid
         true ->
           {found?, valid, [domain_key | invalid], acc}
       end
@@ -3973,24 +3985,27 @@ defmodule Module.Types.Descr do
   # But that would not be helpful, as we can't distinguish between these two
   # in Elixir code. It only makes sense to build the union for domain keys
   # that do not exist.
-  defp map_update_put_domains(bdd, [], _type_fun), do: %{map: bdd}
+  defp map_update_put_domains(bdd, [], _type_fun, _force?), do: %{map: bdd}
 
-  defp map_update_put_domains(bdd, domain_keys, type_fun) do
+  defp map_update_put_domains(bdd, domain_keys, type_fun, force?) do
     bdd =
       bdd_map(bdd, fn {tag, fields} ->
-        {map_update_put_domain(tag, domain_keys, type_fun), fields}
+        {map_update_put_domain(tag, domain_keys, type_fun, force?), fields}
       end)
 
     %{map: bdd}
   end
 
-  defp map_update_put_domain(tag_or_domains, domain_keys, type_fun) do
+  defp map_update_put_domain(tag_or_domains, domain_keys, type_fun, force?) do
     case tag_or_domains do
       :open ->
         :open
 
       :closed ->
-        fields_from_keys(domain_keys, if_set(type_fun.(true, none())))
+        # Non-forced updates must not invoke the callback on absent branches:
+        # the callback may itself typecheck a function application, and
+        # applying it to `none()` will raise undue warnings.
+        if force?, do: fields_from_keys(domain_keys, if_set(type_fun.(true, none()))), else: :closed
 
       # Note: domain_keys may contain duplicates, so we cannot
       # do a side-by-side traversal here.
@@ -4001,7 +4016,8 @@ defmodule Module.Types.Descr do
               fields_store(domain_key, union(value, type_fun.(true, remove_optional(value))), acc)
 
             :error ->
-              fields_store(domain_key, if_set(type_fun.(true, none())), acc)
+              # Likewise, only forced updates may synthesize missing domain keys.
+              if force?, do: fields_store(domain_key, if_set(type_fun.(true, none())), acc), else: acc
           end
         end)
     end
@@ -4078,7 +4094,7 @@ defmodule Module.Types.Descr do
     descr =
       case required_domains ++ optional_domains do
         [] -> none()
-        domains -> map_update_put_domains(bdd, domains, type_fun)
+        domains -> map_update_put_domains(bdd, domains, type_fun, true)
       end
 
     dnf = map_bdd_to_dnf_with_empty(bdd)

lib/elixir/test/elixir/module/types/descr_test.exs

@@ -2275,6 +2275,30 @@ defmodule Module.Types.DescrTest do
              |> map_update(atom([:b]), integer(), true, true) == {none(), none(), []}
     end
 
+    test "with non-empty open maps does not call the callback with none from absent branches" do
+      # This is a test of the map_update_fun/5 with forced?: false parameter.
+      # We check that it does not call its typed_fun argument with `none()`
+      # due to the key being absent in the map.
+
+      type = dynamic(difference(open_map(), empty_map()))
+      ref = make_ref()
+
+      fun = fn _optional?, value ->
+        send(self(), {ref, value})
+        value
+      end
+
+      _ = map_update_fun(type, binary(), fun, false, false)
+
+      messages = Process.info(self(), :messages) |> elem(1)
+
+      # Check that the callback was not invoked with `none()`
+      refute Enum.any?(messages, fn
+               {seen_ref, value} when seen_ref == ref -> empty?(value)
+               _ -> false
+             end)
+    end
+
     test "with dynamic atom keys" do
       assert map_update(closed_map(key: atom([:value])), dynamic(), atom([:new_value])) ==
                {atom([:value]), closed_map(key: atom([:value, :new_value])), []}