From fa44e2704a868bc0e5deb09ebc99f456f61e0725 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eric=20Meadows-J=C3=B6nsson?= Date: Thu, 10 Sep 2020 18:55:44 +0200 Subject: [PATCH 1/5] Check map updates --- lib/elixir/lib/map.ex | 6 +- lib/elixir/lib/module/types/expr.ex | 45 ++++++++--- lib/elixir/lib/module/types/infer.ex | 3 + lib/elixir/test/elixir/map_test.exs | 6 +- .../test/elixir/module/types/expr_test.exs | 81 +++++++++++++++++++ 5 files changed, 124 insertions(+), 17 deletions(-) diff --git a/lib/elixir/lib/map.ex b/lib/elixir/lib/map.ex index 9fe069d14f6..9883f3c466d 100644 --- a/lib/elixir/lib/map.ex +++ b/lib/elixir/lib/map.ex @@ -88,8 +88,10 @@ defmodule Map do iex> map = %{one: 1, two: 2} iex> %{map | one: "one"} %{one: "one", two: 2} - iex> %{map | three: 3} - ** (KeyError) key :three not found + + When a key is updated that does not exist on the map a key error will be raised: + + %{map | three: 3} The functions in this module that need to find a specific key work in logarithmic time. This means that the time it takes to find keys grows as the map grows, but it's not diff --git a/lib/elixir/lib/module/types/expr.ex b/lib/elixir/lib/module/types/expr.ex index 530a6e20aec..70c6ee8fa14 100644 --- a/lib/elixir/lib/module/types/expr.ex +++ b/lib/elixir/lib/module/types/expr.ex @@ -139,27 +139,48 @@ defmodule Module.Types.Expr do end # %{map | ...} - def of_expr({:%{}, _, [{:|, _, [_map, args]}]} = expr, stack, context) do + def of_expr({:%{}, _, [{:|, _, [map, args]}]} = expr, stack, context) do stack = push_expr_stack(expr, stack) - case of_pairs(args, stack, context) do - {:ok, _pairs, context} -> {:ok, {:map, [{:optional, :dynamic, :dynamic}]}, context} - {:error, reason} -> {:error, reason} + with {:ok, map_type, context} <- of_expr(map, stack, context), + {:ok, arg_pairs, context} <- of_pairs(args, stack, context), + arg_pairs = pairs_to_unions(arg_pairs, context), + dynamic_value_pairs = + Enum.map(arg_pairs, fn {:required, key, _value} -> {:required, key, :dynamic} end), + args_type = {:map, dynamic_value_pairs ++ [{:optional, :dynamic, :dynamic}]}, + {:ok, type, context} <- unify(args_type, map_type, stack, context) do + {:map, pairs} = resolve_var(type, context) + + updated_pairs = + Enum.reduce(arg_pairs, pairs, fn {:required, key, value}, pairs -> + List.keyreplace(pairs, key, 1, {:required, key, value}) + end) + + {:ok, {:map, updated_pairs}, context} end end # %Struct{map | ...} - def of_expr({:%, meta, [module, {:%{}, _, [{:|, _, [_map, args]}]}]} = expr, stack, context) do + def of_expr({:%, meta, [module, {:%{}, _, [{:|, _, [map, args]}]}]} = expr, stack, context) do context = Remote.check(module, :__struct__, 0, meta, context) stack = push_expr_stack(expr, stack) - case of_pairs(args, stack, context) do - {:ok, _pairs, context} -> - pairs = [{:required, {:atom, :__struct__}, {:atom, module}}] - {:ok, {:map, pairs}, context} - - {:error, reason} -> - {:error, reason} + with {:ok, map_type, context} <- of_expr(map, stack, context), + {:ok, arg_pairs, context} <- of_pairs(args, stack, context), + arg_pairs = pairs_to_unions(arg_pairs, context), + dynamic_value_pairs = + Enum.map(arg_pairs, fn {:required, key, _value} -> {:required, key, :dynamic} end), + struct_pair = {:required, {:atom, :__struct__}, {:atom, module}}, + args_type = {:map, [struct_pair] ++ dynamic_value_pairs}, + {:ok, type, context} <- unify(args_type, map_type, stack, context) do + {:map, pairs} = resolve_var(type, context) + + updated_pairs = + Enum.reduce(arg_pairs, pairs, fn {:required, key, value}, pairs -> + List.keyreplace(pairs, key, 1, {:required, key, value}) + end) + + {:ok, {:map, updated_pairs}, context} end end diff --git a/lib/elixir/lib/module/types/infer.ex b/lib/elixir/lib/module/types/infer.ex index 28e673d7130..593e7574f10 100644 --- a/lib/elixir/lib/module/types/infer.ex +++ b/lib/elixir/lib/module/types/infer.ex @@ -335,6 +335,9 @@ defmodule Module.Types.Infer do {type, context} end + def resolve_var({:var, var}, context), do: resolve_var(Map.fetch!(context.types, var), context) + def resolve_var(other, _context), do: other + # Check unify stack to see if variable was already expanded defp variable_expanded?(var, stack, context) do Enum.any?(stack.unify_stack, &variable_same?(var, &1, context)) diff --git a/lib/elixir/test/elixir/map_test.exs b/lib/elixir/test/elixir/map_test.exs index cee215997c8..4295ad69be7 100644 --- a/lib/elixir/test/elixir/map_test.exs +++ b/lib/elixir/test/elixir/map_test.exs @@ -225,6 +225,8 @@ defmodule MapTest do end end + defp empty_map(), do: %{} + test "structs" do assert %ExternalUser{} == %{__struct__: ExternalUser, name: "john", age: 27} @@ -236,10 +238,8 @@ defmodule MapTest do %ExternalUser{name: name} = %ExternalUser{} assert name == "john" - map = %{} - assert_raise BadStructError, "expected a struct named MapTest.ExternalUser, got: %{}", fn -> - %ExternalUser{map | name: "meg"} + %ExternalUser{empty_map() | name: "meg"} end end diff --git a/lib/elixir/test/elixir/module/types/expr_test.exs b/lib/elixir/test/elixir/module/types/expr_test.exs index c64f894c8b1..c11fd1b6eb7 100644 --- a/lib/elixir/test/elixir/module/types/expr_test.exs +++ b/lib/elixir/test/elixir/module/types/expr_test.exs @@ -263,6 +263,87 @@ defmodule Module.Types.ExprTest do ) end + test "map update" do + assert quoted_expr( + ( + map = %{foo: :a} + %{map | foo: :b} + ) + ) == + {:ok, {:map, [{:required, {:atom, :foo}, {:atom, :b}}]}} + + assert quoted_expr([map], %{map | foo: :b}) == + {:ok, + {:map, [{:required, {:atom, :foo}, {:atom, :b}}, {:optional, :dynamic, :dynamic}]}} + + assert {:error, + {:unable_unify, + {{:map, [{:required, {:atom, :bar}, :dynamic}, {:optional, :dynamic, :dynamic}]}, + {:map, [{:required, {:atom, :foo}, {:atom, :a}}]}, + _}}} = + quoted_expr( + ( + map = %{foo: :a} + %{map | bar: :b} + ) + ) + end + + test "struct update" do + assert quoted_expr( + ( + map = %Module.Types.ExprTest.Struct2{field: :a} + %Module.Types.ExprTest.Struct2{map | field: :b} + ) + ) == + {:ok, + {:map, + [ + {:required, {:atom, :__struct__}, {:atom, Module.Types.ExprTest.Struct2}}, + {:required, {:atom, :field}, {:atom, :b}} + ]}} + + assert {:error, + {:unable_unify, + {{:map, + [ + {:required, {:atom, :__struct__}, {:atom, Module.Types.ExprTest.Struct2}}, + {:required, {:atom, :field}, :dynamic} + ]}, {:map, [{:required, {:atom, :field}, {:atom, :a}}]}, + _}}} = + quoted_expr( + ( + map = %{field: :a} + %Module.Types.ExprTest.Struct2{map | field: :b} + ) + ) + + assert quoted_expr([map], %Module.Types.ExprTest.Struct2{map | field: :b}) == + {:ok, + {:map, + [ + {:required, {:atom, :__struct__}, {:atom, Module.Types.ExprTest.Struct2}}, + {:required, {:atom, :field}, {:atom, :b}} + ]}} + + assert {:error, + {:unable_unify, + {{:map, + [{:required, {:atom, :not_field}, :dynamic}, {:optional, :dynamic, :dynamic}]}, + {:map, + [ + {:required, {:atom, :__struct__}, {:atom, Module.Types.ExprTest.Struct2}}, + {:required, {:atom, :field}, {:atom, nil}} + ]}, + _}}} = + quoted_expr( + ( + map = %Module.Types.ExprTest.Struct2{} + %{map | not_field: :b} + ) + ) + end + # Use module attribute to avoid formatter adding parentheses @mix_module Mix From 2a8c22d31a4441b6c731c9de156d70f7e38697a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eric=20Meadows-J=C3=B6nsson?= Date: Thu, 10 Sep 2020 23:19:08 +0200 Subject: [PATCH 2/5] Update lib/elixir/lib/map.ex Co-authored-by: Eksperimental --- lib/elixir/lib/map.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/elixir/lib/map.ex b/lib/elixir/lib/map.ex index 9883f3c466d..7dd6fc6e0fe 100644 --- a/lib/elixir/lib/map.ex +++ b/lib/elixir/lib/map.ex @@ -89,7 +89,7 @@ defmodule Map do iex> %{map | one: "one"} %{one: "one", two: 2} - When a key is updated that does not exist on the map a key error will be raised: + When a key that does not exist in the map is updated a key error will be raised: %{map | three: 3} From 2dbca340a6ee5753b77cf53ef1edb6790a4a045a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eric=20Meadows-J=C3=B6nsson?= Date: Fri, 11 Sep 2020 10:44:29 +0200 Subject: [PATCH 3/5] Update lib/elixir/lib/map.ex Co-authored-by: Fernando Tapia Rico --- lib/elixir/lib/map.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/elixir/lib/map.ex b/lib/elixir/lib/map.ex index 7dd6fc6e0fe..17ac566b261 100644 --- a/lib/elixir/lib/map.ex +++ b/lib/elixir/lib/map.ex @@ -89,7 +89,7 @@ defmodule Map do iex> %{map | one: "one"} %{one: "one", two: 2} - When a key that does not exist in the map is updated a key error will be raised: + When a key that does not exist in the map is updated a `KeyError` exception will be raised: %{map | three: 3} From 7176baa4cf7bf39c2d6999a18a137ab846d3e825 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eric=20Meadows-J=C3=B6nsson?= Date: Fri, 11 Sep 2020 11:27:30 +0200 Subject: [PATCH 4/5] Reduce duplication --- lib/elixir/lib/map.ex | 2 +- lib/elixir/lib/module/types/expr.ex | 56 ++++++++----------- .../test/elixir/module/types/expr_test.exs | 6 +- 3 files changed, 27 insertions(+), 37 deletions(-) diff --git a/lib/elixir/lib/map.ex b/lib/elixir/lib/map.ex index 17ac566b261..1e51d770930 100644 --- a/lib/elixir/lib/map.ex +++ b/lib/elixir/lib/map.ex @@ -68,7 +68,7 @@ defmodule Map do iex> a 1 - But this will raise a match error: + But this will raise a `MatchError` exception: %{:c => 3} = %{:a => 1, 2 => :b} diff --git a/lib/elixir/lib/module/types/expr.ex b/lib/elixir/lib/module/types/expr.ex index 70c6ee8fa14..b21b74a3c9a 100644 --- a/lib/elixir/lib/module/types/expr.ex +++ b/lib/elixir/lib/module/types/expr.ex @@ -142,22 +142,8 @@ defmodule Module.Types.Expr do def of_expr({:%{}, _, [{:|, _, [map, args]}]} = expr, stack, context) do stack = push_expr_stack(expr, stack) - with {:ok, map_type, context} <- of_expr(map, stack, context), - {:ok, arg_pairs, context} <- of_pairs(args, stack, context), - arg_pairs = pairs_to_unions(arg_pairs, context), - dynamic_value_pairs = - Enum.map(arg_pairs, fn {:required, key, _value} -> {:required, key, :dynamic} end), - args_type = {:map, dynamic_value_pairs ++ [{:optional, :dynamic, :dynamic}]}, - {:ok, type, context} <- unify(args_type, map_type, stack, context) do - {:map, pairs} = resolve_var(type, context) - - updated_pairs = - Enum.reduce(arg_pairs, pairs, fn {:required, key, value}, pairs -> - List.keyreplace(pairs, key, 1, {:required, key, value}) - end) - - {:ok, {:map, updated_pairs}, context} - end + additional_pairs = [{:optional, :dynamic, :dynamic}] + map_update(map, args, additional_pairs, stack, context) end # %Struct{map | ...} @@ -165,23 +151,8 @@ defmodule Module.Types.Expr do context = Remote.check(module, :__struct__, 0, meta, context) stack = push_expr_stack(expr, stack) - with {:ok, map_type, context} <- of_expr(map, stack, context), - {:ok, arg_pairs, context} <- of_pairs(args, stack, context), - arg_pairs = pairs_to_unions(arg_pairs, context), - dynamic_value_pairs = - Enum.map(arg_pairs, fn {:required, key, _value} -> {:required, key, :dynamic} end), - struct_pair = {:required, {:atom, :__struct__}, {:atom, module}}, - args_type = {:map, [struct_pair] ++ dynamic_value_pairs}, - {:ok, type, context} <- unify(args_type, map_type, stack, context) do - {:map, pairs} = resolve_var(type, context) - - updated_pairs = - Enum.reduce(arg_pairs, pairs, fn {:required, key, value}, pairs -> - List.keyreplace(pairs, key, 1, {:required, key, value}) - end) - - {:ok, {:map, updated_pairs}, context} - end + additional_pairs = [{:required, {:atom, :__struct__}, {:atom, module}}] + map_update(map, args, additional_pairs, stack, context) end # %{...} @@ -416,6 +387,25 @@ defmodule Module.Types.Expr do end) end + defp map_update(map, args, additional_pairs, stack, context) do + with {:ok, map_type, context} <- of_expr(map, stack, context), + {:ok, arg_pairs, context} <- of_pairs(args, stack, context), + arg_pairs = pairs_to_unions(arg_pairs, context), + dynamic_value_pairs = + Enum.map(arg_pairs, fn {:required, key, _value} -> {:required, key, :dynamic} end), + args_type = {:map, additional_pairs ++ dynamic_value_pairs}, + {:ok, type, context} <- unify(args_type, map_type, stack, context) do + {:map, pairs} = resolve_var(type, context) + + updated_pairs = + Enum.reduce(arg_pairs, pairs, fn {:required, key, value}, pairs -> + List.keyreplace(pairs, key, 1, {:required, key, value}) + end) + + {:ok, {:map, updated_pairs}, context} + end + end + defp for_clause({:<-, _, [left, expr]}, stack, context) do {pattern, guards} = extract_head([left]) diff --git a/lib/elixir/test/elixir/module/types/expr_test.exs b/lib/elixir/test/elixir/module/types/expr_test.exs index c11fd1b6eb7..b1714d535e9 100644 --- a/lib/elixir/test/elixir/module/types/expr_test.exs +++ b/lib/elixir/test/elixir/module/types/expr_test.exs @@ -274,11 +274,11 @@ defmodule Module.Types.ExprTest do assert quoted_expr([map], %{map | foo: :b}) == {:ok, - {:map, [{:required, {:atom, :foo}, {:atom, :b}}, {:optional, :dynamic, :dynamic}]}} + {:map, [{:optional, :dynamic, :dynamic}, {:required, {:atom, :foo}, {:atom, :b}}]}} assert {:error, {:unable_unify, - {{:map, [{:required, {:atom, :bar}, :dynamic}, {:optional, :dynamic, :dynamic}]}, + {{:map, [{:optional, :dynamic, :dynamic}, {:required, {:atom, :bar}, :dynamic}]}, {:map, [{:required, {:atom, :foo}, {:atom, :a}}]}, _}}} = quoted_expr( @@ -329,7 +329,7 @@ defmodule Module.Types.ExprTest do assert {:error, {:unable_unify, {{:map, - [{:required, {:atom, :not_field}, :dynamic}, {:optional, :dynamic, :dynamic}]}, + [{:optional, :dynamic, :dynamic}, {:required, {:atom, :not_field}, :dynamic}]}, {:map, [ {:required, {:atom, :__struct__}, {:atom, Module.Types.ExprTest.Struct2}}, From 39977392d24fbc60e935041400240c54f6c95f10 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eric=20Meadows-J=C3=B6nsson?= Date: Fri, 11 Sep 2020 11:31:03 +0200 Subject: [PATCH 5/5] Add clarifying comments --- lib/elixir/lib/module/types/expr.ex | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/elixir/lib/module/types/expr.ex b/lib/elixir/lib/module/types/expr.ex index b21b74a3c9a..354d28dd348 100644 --- a/lib/elixir/lib/module/types/expr.ex +++ b/lib/elixir/lib/module/types/expr.ex @@ -391,10 +391,12 @@ defmodule Module.Types.Expr do with {:ok, map_type, context} <- of_expr(map, stack, context), {:ok, arg_pairs, context} <- of_pairs(args, stack, context), arg_pairs = pairs_to_unions(arg_pairs, context), + # Change value types to dynamic to reuse map unification for map updates dynamic_value_pairs = Enum.map(arg_pairs, fn {:required, key, _value} -> {:required, key, :dynamic} end), args_type = {:map, additional_pairs ++ dynamic_value_pairs}, {:ok, type, context} <- unify(args_type, map_type, stack, context) do + # Retrieve map type and overwrite with the new value types from the map update {:map, pairs} = resolve_var(type, context) updated_pairs =