Add type checking for Map.fetch/2 and improve docs

Author: josevalim

lib/elixir/lib/map.ex

@@ -287,8 +287,13 @@ defmodule Map do
   @doc """
   Fetches the value for a specific `key` in the given `map`.
 
-  If `map` contains the given `key` then its value is returned in the shape of `{:ok, value}`.
-  If `map` doesn't contain `key`, `:error` is returned.
+  If `map` contains the given `key` then its value is returned
+  in the shape of `{:ok, value}`. If `map` doesn't contain `key`,
+  `:error` is returned.
+
+  If the type system can verify `:error` is always returned
+  (which means key is never available in the map), it will emit
+  an error.
 
   Inlined by the compiler.
 
@@ -307,8 +312,11 @@ defmodule Map do
   Fetches the value for a specific `key` in the given `map`, erroring out if
   `map` doesn't contain `key`.
 
-  If `map` contains `key`, the corresponding value is returned. If
-  `map` doesn't contain `key`, a `KeyError` exception is raised.
+  The exclamation mark (`!`) implies this function can raise a `KeyError`
+  exception at runtime if `map` doesn't contain `key`. If the type system
+  can verify this function will always raise (which means the key is never
+  available), then it will emit a warning at compile-time. See the "Type
+  checking" section below.
 
   Inlined by the compiler.
 
@@ -318,10 +326,50 @@ defmodule Map do
       1
 
   When the key is missing, an exception is raised:
-      
+
       Map.fetch!(%{a: 1}, :b)
       ** (KeyError) key :b not found in: %{a: 1}
 
+  ## Type checking
+
+  The compiler will emit a warning if it can verify that
+  none of the keys given are available in the map.
+
+  When the key is an atom, because only single key is given,
+  a warning will be emitted in case the type system proves
+  the key is not present.
+
+  However, this behaviour matters when the type of the key
+  represents multiple values. For example:
+
+      key = returns_foo_or_bar() #=> :foo or :bar
+      Map.fetch!(%{foo: 123}, key)
+
+  Although the key can be `:foo` or `:bar`, there is no
+  warning emitted, as `:foo` will succeed. This is by design:
+  the exclamation mark in Elixir denotes precisely that a
+  runtime exception may be raised.
+
+  In case you are looking up multiple keys and you don't know
+  if they may be present, you can use `Map.fetch/2` instead
+  and deal with the error case accordingly:
+
+      case Map.fetch(%{foo: 123}, key) do
+        {:ok, value} -> ...
+        :error -> ...
+      end
+
+  Both `Map.fetch!/2` and `Map.fetch/2` will emit a warning if
+  it proves that both `:foo` or `:bar` are absent in the map.
+
+  Alternatively, if you want to statically prove that all of keys
+  are in the map, you can match on the possible values and access
+  them directly:
+
+      case returns_foo_or_bar() do
+        :foo -> map.foo
+        :bar -> map.bar
+      end
   """
   @spec fetch!(map, key) :: value
   def fetch!(map, key) do

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

@@ -245,6 +245,8 @@ defmodule Module.Types.Apply do
 
         ## Map
         {:maps, :from_keys, [{[list(term()), term()], open_map()}]},
+        {:maps, :find,
+         [{[term(), open_map()], tuple([atom([:ok]), term()]) |> union(atom([:error]))}]},
         {:maps, :get, [{[term(), open_map()], term()}]},
         {:maps, :is_key, [{[term(), open_map()], boolean()}]},
         {:maps, :keys, [{[open_map()], dynamic(list(term()))}]},
@@ -335,6 +337,11 @@ defmodule Module.Types.Apply do
     {{:compare, name, check?}, [term(), term()], context}
   end
 
+  def remote_domain(:maps, :get, [key, _], expected, _meta, _stack, context) when is_atom(key) do
+    domain = [term(), open_map([{key, expected}])]
+    {{:strong, nil, [{domain, term()}]}, domain, context}
+  end
+
   def remote_domain(mod, fun, args, expected, meta, stack, context) do
     arity = length(args)
 
@@ -425,6 +432,20 @@ defmodule Module.Types.Apply do
     end
   end
 
+  defp remote_apply(:maps, :find, _info, [key, map] = args_types, stack) do
+    case map_get(map, key) do
+      {_, value} ->
+        result = tuple([atom([:ok]), value]) |> union(atom([:error]))
+        {:ok, return(result, args_types, stack)}
+
+      :badmap ->
+        {:error, badremote(:maps, :find, 2)}
+
+      :error ->
+        {:error, {:badkeydomain, map, key, atom([:error])}}
+    end
+  end
+
   defp remote_apply(:maps, :get, _info, [key, map] = args_types, stack) do
     case map_get(map, key) do
       {_, value} ->
@@ -434,7 +455,7 @@ defmodule Module.Types.Apply do
         {:error, badremote(:maps, :get, 2)}
 
       :error ->
-        {:error, {:badkeydomain, map, key, []}}
+        {:error, {:badkeydomain, map, key, nil}}
     end
   end
 
@@ -458,7 +479,7 @@ defmodule Module.Types.Apply do
         {:error, badremote(:maps, :take, 2)}
 
       {:error, _errors} ->
-        {:ok, return(atom([:error]), args_types, stack)}
+        {:error, {:badkeydomain, map, key, atom([:error])}}
     end
   end
 
@@ -1073,17 +1094,11 @@ defmodule Module.Types.Apply do
     }
   end
 
-  def format_diagnostic({{:badkeydomain, map, key, errors}, _args_types, mfac, expr, context}) do
+  def format_diagnostic({{:badkeydomain, map, key, error}, _args_types, mfac, expr, context}) do
     {mod, fun, arity, _converter} = mfac
     traces = collect_traces(expr, context)
     mfa_or_fa = if mod, do: Exception.format_mfa(mod, fun, arity), else: "#{fun}/#{arity}"
 
-    error_key =
-      Enum.reduce(errors, none(), fn
-        {:badkey, key}, acc -> union(atom([key]), acc)
-        {:baddomain, domain}, acc -> union(domain, acc)
-      end)
-
     %{
       details: %{typing_traces: traces},
       message:
@@ -1093,33 +1108,20 @@ defmodule Module.Types.Apply do
 
               #{expr_to_string(expr) |> indent(4)}
 
-          """,
-          if errors == [] do
-            """
-            the map:
+          the map:
 
-                #{to_quoted_string(map) |> indent(4)}
+              #{to_quoted_string(map) |> indent(4)}
 
-            does not have the given keys:
+          does not have all required keys:
 
-                #{to_quoted_string(key) |> indent(4)}
+              #{to_quoted_string(key) |> indent(4)}
 
-            """
+          therefore this function will always #{if error do
+            "return #{to_quoted_string(error)}"
           else
-            """
-            expected the map:
-
-                #{to_quoted_string(map) |> indent(4)}
-
-            to have the keys:
-
-                #{to_quoted_string(key) |> indent(4)}
-
-            but the following keys are missing:
-
-                #{to_quoted_string(error_key) |> indent(4)}
-            """
-          end,
+            "raise"
+          end}
+          """,
           format_traces(traces)
         ])
     }

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

@@ -13,8 +13,6 @@ defmodule Module.Types.MapTest do
 
   describe ":maps.take/2" do
     test "checking" do
-      assert typecheck!(:maps.take(:key, %{})) == atom([:error])
-
       assert typecheck!(:maps.take(:key, %{key: 123})) |> equal?(tuple([integer(), empty_map()]))
 
       assert typecheck!([x], :maps.take(:key, x))
@@ -41,7 +39,7 @@ defmodule Module.Types.MapTest do
                )
              )
 
-      assert typecheck!([x], :maps.take(integer(), x))
+      assert typecheck!([x], :maps.take(123, x))
              |> equal?(
                union(
                  dynamic(tuple([term(), open_map()])),
@@ -63,10 +61,98 @@ defmodule Module.Types.MapTest do
     test "errors" do
       assert typeerror!([x = []], :maps.take(:foo, x)) =~
                "incompatible types given to :maps.take/2"
+
+      assert typeerror!(:maps.take(:key, %{})) =~ """
+             incompatible types given to :maps.take/2:
+
+                 :maps.take(:key, %{})
+
+             the map:
+
+                 empty_map()
+
+             does not have all required keys:
+
+                 :key
+
+             therefore this function will always return :error
+             """
+    end
+  end
+
+  describe "Map.fetch/2" do
+    test "checking" do
+      assert typecheck!(Map.fetch(%{key: 123}, :key)) ==
+               tuple([atom([:ok]), integer()]) |> union(atom([:error]))
+
+      assert typecheck!([x], Map.fetch(x, :key))
+             |> equal?(dynamic(tuple([atom([:ok]), term()])) |> union(atom([:error])))
+
+      # If one of them succeeds, we are still fine!
+      assert typecheck!(
+               [condition?],
+               Map.fetch(%{foo: 123}, if(condition?, do: :foo, else: :bar))
+             ) == tuple([atom([:ok]), integer()]) |> union(atom([:error]))
+
+      assert typecheck!([x], Map.fetch(x, 123))
+             |> equal?(dynamic(tuple([atom([:ok]), term()])) |> union(atom([:error])))
+    end
+
+    test "inference" do
+      assert typecheck!(
+               [x],
+               (
+                 _ = Map.fetch(x, :key)
+                 x
+               )
+             ) == dynamic(open_map())
+    end
+
+    test "errors" do
+      assert typeerror!(Map.fetch(%{}, :foo)) =~
+               """
+               incompatible types given to Map.fetch/2:
+
+                   Map.fetch(%{}, :foo)
+
+               the map:
+
+                   empty_map()
+
+               does not have all required keys:
+
+                   :foo
+
+               therefore this function will always return :error
+               """
     end
   end
 
   describe "Map.fetch!/2" do
+    test "checking" do
+      assert typecheck!(Map.fetch!(%{key: 123}, :key)) == integer()
+
+      assert typecheck!([x], Map.fetch!(x, :key)) == dynamic()
+
+      # If one of them succeeds, we are still fine!
+      assert typecheck!(
+               [condition?],
+               Map.fetch!(%{foo: 123}, if(condition?, do: :foo, else: :bar))
+             ) == integer()
+
+      assert typecheck!([x], Map.fetch!(x, 123)) |> equal?(dynamic())
+    end
+
+    test "inference" do
+      assert typecheck!(
+               [x],
+               (
+                 y = Integer.to_string(Map.fetch!(x, :key))
+                 {x, y}
+               )
+             ) == dynamic(tuple([open_map(key: integer()), binary()]))
+    end
+
     test "errors" do
       assert typeerror!(Map.fetch!(%{}, :foo)) =~
                """
@@ -78,10 +164,11 @@ defmodule Module.Types.MapTest do
 
                    empty_map()
 
-               does not have the given keys:
+               does not have all required keys:
 
                    :foo
 
+               therefore this function will always raise
                """
 
       assert typeerror!(Map.fetch!(%{}, 123)) =~
@@ -94,10 +181,11 @@ defmodule Module.Types.MapTest do
 
                    empty_map()
 
-               does not have the given keys:
+               does not have all required keys:
 
                    integer()
 
+               therefore this function will always raise
                """
     end
   end