Do not bake nil assumption into map_get/2

Author: josevalim

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

@@ -962,6 +962,11 @@ defmodule Module.Types.Descr do
   defp atom_only?(descr), do: empty?(Map.delete(descr, :atom))
   defp atom_new(as) when is_list(as), do: {:union, :sets.from_list(as, version: 2)}
 
+  defp infinite_atoms?(:term), do: true
+  defp infinite_atoms?(%{dynamic: dynamic}), do: infinite_atoms?(dynamic)
+  defp infinite_atoms?(%{atom: {:negation, _}}), do: true
+  defp infinite_atoms?(%{}), do: false
+
   defp atom_intersection({tag1, s1}, {tag2, s2}) do
     {tag, s} =
       case {tag1, tag2} do
@@ -2705,7 +2710,7 @@ defmodule Module.Types.Descr do
   end
 
   defp map_fetch_key_static(%{map: bdd}, key) do
-    map_bdd_to_dnf(bdd) |> map_dnf_fetch_static(key)
+    map_bdd_to_dnf(bdd) |> map_dnf_fetch_static(key) |> pop_optional_static()
   end
 
   defp map_fetch_key_static(%{}, _key), do: {false, none()}
@@ -2737,7 +2742,6 @@ defmodule Module.Types.Descr do
             |> union(acc)
         end
     end)
-    |> pop_optional_static()
   end
 
   @doc """
@@ -2975,7 +2979,7 @@ defmodule Module.Types.Descr do
           if Map.has_key?(seen, key) do
             {seen, acc}
           else
-            {_, value} = map_dnf_fetch_static(dnf, key)
+            value = dnf |> map_dnf_fetch_static(key) |> remove_optional()
             {Map.put(seen, key, []), union(acc, value)}
           end
         end)
@@ -2990,8 +2994,8 @@ defmodule Module.Types.Descr do
         if Map.has_key?(acc, key) do
           acc
         else
-          {_, value} = map_dnf_fetch_static(dnf, key)
-          not empty?(value) and throw(:found_key)
+          value = map_dnf_fetch_static(dnf, key)
+          not empty_or_optional?(value) and throw(:found_key)
           Map.put(acc, key, [])
         end
       end)
@@ -3183,59 +3187,39 @@ defmodule Module.Types.Descr do
   @doc """
   Computes the union of types for keys matching `key_type` within the `map_type`.
 
-  This generalizes `map_fetch_key/2` (which operates on a single literal key) to
-  work with a key type (e.g., `atom()`, `integer()`, `:a or :b`). It's based
-  on the map-selection operator t.[t'] described in Section 4.2 of "Typing Records,
-  Maps, and Structs" (Castagna et al., ICFP 2023).
-
-  ## Return Values
-
-  The function returns a tuple indicating the outcome and the resulting type union:
-
-  * `{:ok, type}`: Standard success. `type` is the resulting union of types
-    found for the matching keys. This covers two sub-cases:
-      * **Keys definitely exist:** If `disjoint?(type, not_set())` is true,
-        all keys matching `key_type` are guaranteed to exist.
-      * **Keys may exist:** If `type` includes `not_set()`, some keys
-        matching `key_type` might exist (contributing their types) while
-        others might be absent (contributing `not_set()`).
-
-  * `{:ok_absent, type}`: Success, but the resulting `type` is `none()` or a
-    subtype of `not_set()`. This indicates that no key matching `key_type`
-    can exist with a value other than `not_set()`. The caller may wish to
-    issue a warning, as this often implies selecting a field that is
-    effectively undefined.
-
-  # TODO: implement/decide if worth it (it's from the paper)
-  * `{:ok_spillover, type}`: Success, and `type` is the resulting union.
-    However, this indicates that the `key_type` included keys not explicitly
-    covered by the `map_type`'s fields or domain specifications. The
-    projection relied on the map's default behavior (e.g., the `term()`
-    value type for unspecified keys in an open map). The caller may wish to
-    issue a warning, as this could conceal issues like selecting keys
-    not intended by the map's definition.
-
-  * `:badmap`: The input `map_type` was invalid (e.g., not a map type or
-    a dynamic type wrapping a map type).
-
-  * `:badkeytype`: The input `key_type` was invalid (e.g., not a subtype
-    of the allowed key types like `atom()`, `integer()`, etc.).
+  Returns `{optional?, descr}`, `:error` (if no value across the whole domain is found),
+  or `:badmap`.
+
+  This is called `map_get/2` but it can be used to power `Map.fetch`, `Map.fetch!`,
+  `Map.get`, etc. except `map.key`.
   """
-  # TODO: Figure out how to use this operation from Elixir
   def map_get(:term, _key_descr), do: :badmap
 
   def map_get(%{} = descr, key_descr) do
     split_keys = map_split_keys_and_domains(key_descr)
 
+    # If we are looking for infinite atoms, then either there are required/optional
+    # keys which may be selected, so we use not_set().
+    #
+    # In case, there are no keys, which will fail unless there are domain keys,
+    # so `not_set()` is still correct.
+    acc =
+      if infinite_atoms?(key_descr) do
+        not_set()
+      else
+        none()
+      end
+
     case :maps.take(:dynamic, descr) do
       :error ->
         if descr_key?(descr, :map) and map_only?(descr) do
-          {optional?, type_selected} = map_get_static(descr, split_keys) |> pop_optional_static()
+          {optional?, type_selected} =
+            map_get_static(descr, split_keys, acc) |> pop_optional_static()
 
-          cond do
-            empty?(type_selected) -> {:ok_absent, atom([nil])}
-            optional? -> {:ok, nil_or_type(type_selected)}
-            true -> {:ok_present, type_selected}
+          if empty?(type_selected) do
+            :error
+          else
+            {optional?, type_selected}
           end
         else
           :badmap
@@ -3244,33 +3228,28 @@ defmodule Module.Types.Descr do
       {dynamic, static} ->
         if descr_key?(dynamic, :map) and map_only?(static) do
           {optional_dynamic?, dynamic_type} =
-            map_get_static(dynamic, split_keys) |> pop_optional_static()
+            map_get_static(dynamic, split_keys, acc) |> pop_optional_static()
 
           {optional_static?, static_type} =
-            map_get_static(static, split_keys) |> pop_optional_static()
+            map_get_static(static, split_keys, acc) |> pop_optional_static()
 
-          type_selected = union(dynamic(dynamic_type), static_type)
-
-          cond do
-            empty?(type_selected) -> {:ok_absent, atom([nil])}
-            optional_dynamic? or optional_static? -> {:ok, nil_or_type(type_selected)}
-            true -> {:ok_present, type_selected}
+          if empty?(dynamic_type) do
+            :error
+          else
+            {optional_dynamic? or optional_static?, union(dynamic(dynamic_type), static_type)}
           end
         else
           :badmap
         end
     end
   end
 
-  defp nil_or_type(type), do: union(type, atom([nil]))
-
-  defp map_get_static(%{map: bdd}, split_keys) do
+  defp map_get_static(%{map: bdd}, split_keys, acc) do
     {required_keys, optional_keys, maybe_negated_set, required_domains, optional_domains} =
       split_keys
 
     dnf = map_bdd_to_dnf(bdd)
 
-    acc = none()
     acc = map_get_keys(dnf, required_keys, acc)
     acc = map_get_keys(dnf, optional_keys, acc)
     acc = map_get_keys(dnf, map_materialize_negated_set(maybe_negated_set, bdd), acc)
@@ -3279,18 +3258,12 @@ defmodule Module.Types.Descr do
     acc
   end
 
-  defp map_get_static(%{}, _key), do: not_set()
-  defp map_get_static(:term, _key), do: term_or_optional()
+  defp map_get_static(%{}, _split_keys, acc), do: acc
+  defp map_get_static(:term, _split_keys, _acc), do: term_or_optional()
 
   defp map_get_keys(dnf, keys, acc) do
     Enum.reduce(keys, acc, fn atom, acc ->
-      {static_optional?, type} = map_dnf_fetch_static(dnf, atom)
-
-      if static_optional? do
-        union(type, acc) |> nil_or_type() |> if_set()
-      else
-        union(type, acc)
-      end
+      union(map_dnf_fetch_static(dnf, atom), acc)
     end)
   end
 

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

@@ -23,7 +23,6 @@ defmodule Module.Types.DescrTest do
   defp tuple_of_size(n) when is_integer(n) and n >= 0, do: tuple(List.duplicate(term(), n))
   defp list(elem_type, tail_type), do: union(empty_list(), non_empty_list(elem_type, tail_type))
   defp map_with_default(descr), do: open_map([], if_set(descr))
-  defp nil_or_type(type), do: union(type, atom([nil]))
 
   describe "union" do
     test "bitmap" do
@@ -1655,12 +1654,14 @@ defmodule Module.Types.DescrTest do
              |> difference(closed_map(b: if_set(integer())))
              |> map_fetch_key(:a) == :badkey
     end
+  end
 
-    test "map_get with domain keys" do
+  describe "map_get" do
+    test "with domain keys" do
       assert map_get(term(), term()) == :badmap
 
       map_type = closed_map([{domain_key(:tuple), binary()}])
-      assert map_get(map_type, tuple()) == {:ok, nil_or_type(binary())}
+      assert map_get(map_type, tuple()) == {true, binary()}
 
       # Type with all domain types
       # %{:bar => :ok, integer() => :int, float() => :float, atom() => binary(), binary() => integer(), tuple() => float(), map() => pid(), reference() => port(), pid() => boolean()}
@@ -1678,76 +1679,88 @@ defmodule Module.Types.DescrTest do
           {domain_key(:port), boolean()}
         ])
 
-      assert map_get(all_domains, atom([:bar])) == {:ok_present, atom([:ok])}
+      assert map_get(all_domains, atom([:bar])) == {false, atom([:ok])}
 
-      assert map_get(all_domains, integer()) == {:ok, atom([:int]) |> nil_or_type()}
-      assert map_get(all_domains, number()) == {:ok, atom([:int, :float]) |> nil_or_type()}
+      assert map_get(all_domains, integer()) == {true, atom([:int])}
+      assert map_get(all_domains, number()) == {true, atom([:int, :float])}
 
-      assert map_get(all_domains, empty_list()) == {:ok_absent, atom([nil])}
-      assert map_get(all_domains, atom([:foo])) == {:ok, binary() |> nil_or_type()}
-      assert map_get(all_domains, binary()) == {:ok, integer() |> nil_or_type()}
-      assert map_get(all_domains, tuple([integer(), atom()])) == {:ok, nil_or_type(float())}
-      assert map_get(all_domains, empty_map()) == {:ok, pid() |> nil_or_type()}
+      assert map_get(all_domains, empty_list()) == :error
+      assert map_get(all_domains, atom([:foo])) == {true, binary()}
+      assert map_get(all_domains, binary()) == {true, integer()}
+      assert map_get(all_domains, tuple([integer(), atom()])) == {true, float()}
+      assert map_get(all_domains, empty_map()) == {true, pid()}
 
       # Union
       assert map_get(all_domains, union(tuple(), empty_map())) ==
-               {:ok, union(float(), pid() |> nil_or_type())}
+               {true, union(float(), pid())}
 
       # Removing all maps with tuple keys
       t_no_tuple = difference(all_domains, closed_map([{domain_key(:tuple), float()}]))
       t_really_no_tuple = difference(all_domains, open_map([{domain_key(:tuple), float()}]))
       assert subtype?(all_domains, open_map())
       # It's only closed maps, so it should not change
-      assert map_get(t_no_tuple, tuple()) == {:ok, float() |> nil_or_type()}
+      assert map_get(t_no_tuple, tuple()) == {true, float()}
       # This time we actually removed all tuple to float keys
-      assert map_get(t_really_no_tuple, tuple()) == {:ok_absent, atom([nil])}
+      assert map_get(t_really_no_tuple, tuple()) == :error
 
       t1 = closed_map([{domain_key(:tuple), integer()}])
       t2 = closed_map([{domain_key(:tuple), float()}])
       t3 = union(t1, t2)
-      assert map_get(t3, tuple()) == {:ok, number() |> nil_or_type()}
+      assert map_get(t3, tuple()) == {true, number()}
     end
 
-    test "map_get with dynamic" do
+    test "with dynamic" do
       {_answer, type_selected} = map_get(dynamic(), term())
-      assert equal?(type_selected, dynamic() |> nil_or_type())
+      assert equal?(type_selected, dynamic())
     end
 
-    test "map_get with atom fall back" do
+    test "with atom fall back" do
       map = closed_map([{:a, atom([:a])}, {:b, atom([:b])}, {domain_key(:atom), pid()}])
-      assert map_get(map, atom([:a, :b])) == {:ok_present, atom([:a, :b])}
-      assert map_get(map, atom([:a, :c])) == {:ok, union(atom([:a]), pid() |> nil_or_type())}
-      assert map_get(map, atom() |> difference(atom([:a, :b]))) == {:ok, pid() |> nil_or_type()}
+
+      assert map_get(map, atom([:a, :b])) ==
+               {false, atom([:a, :b])}
+
+      assert map_get(map, atom([:a, :c])) ==
+               {true, union(atom([:a]), pid())}
+
+      assert map_get(map, atom() |> difference(atom([:a, :b]))) ==
+               {true, pid()}
 
       assert map_get(map, atom() |> difference(atom([:a]))) ==
-               {:ok, union(atom([:b]), pid() |> nil_or_type())}
+               {true, union(atom([:b]), pid())}
+
+      assert map_get(closed_map(a: atom([:a]), b: atom([:b])), atom()) ==
+               {true, atom([:a, :b])}
+
+      assert map_get(closed_map([{domain_key(:atom), integer()}]), atom([:a, :b])) ==
+               {true, integer()}
     end
 
-    test "map_get with lists" do
+    test "with lists" do
       # Verify that empty_list() bitmap type maps to :list domain (not :empty_list domain)
       map_with_list_domain = closed_map([{domain_key(:list), atom([:empty])}])
 
       # empty_list() should access the :list domain
-      assert map_get(map_with_list_domain, empty_list()) == {:ok, atom([:empty]) |> nil_or_type()}
+      assert map_get(map_with_list_domain, empty_list()) == {true, atom([:empty])}
 
       # non_empty_list() should also access the :list domain
       assert map_get(map_with_list_domain, non_empty_list(integer())) ==
-               {:ok, atom([:empty]) |> nil_or_type()}
+               {true, atom([:empty])}
 
       # list() should also access the :list domain
       assert map_get(map_with_list_domain, list(integer())) ==
-               {:ok, atom([:empty]) |> nil_or_type()}
+               {true, atom([:empty])}
 
       # If I create a map and instantiate both empty_list() and non_empty_list(integer()), it should return the union of the two types
       map =
         closed_map([{domain_key(:list), atom([:empty])}, {domain_key(:list), atom([:non_empty])}])
 
-      assert map_get(map, empty_list()) == {:ok, atom([:empty, :non_empty]) |> nil_or_type()}
+      assert map_get(map, empty_list()) == {true, atom([:empty, :non_empty])}
 
       assert map_get(map, non_empty_list(integer())) ==
-               {:ok, atom([:empty, :non_empty]) |> nil_or_type()}
+               {true, atom([:empty, :non_empty])}
 
-      assert map_get(map, list(integer())) == {:ok, atom([:empty, :non_empty]) |> nil_or_type()}
+      assert map_get(map, list(integer())) == {true, atom([:empty, :non_empty])}
     end
   end