File 5891-maps-raise-a-badmap-error-if-an-iterator-f... of Package erlang

Overview Repositories Revisions Requests Users Attributes Meta

File 5891-maps-raise-a-badmap-error-if-an-iterator-fails-later.patch of Package erlang (Revision 58ddfd9fcf4c281c82656d8c2d2a71df)

Currently displaying revision 58ddfd9fcf4c281c82656d8c2d2a71df , Show latest

From e913d16df30ecb08844c3e63ed5d4132baf38254 Mon Sep 17 00:00:00 2001
From: Maria Scott <maria-12648430@hnc-agency.org>
Date: Thu, 2 Feb 2023 11:59:22 +0100
Subject: [PATCH] maps: raise a badmap error if an iterator fails later

The implementation of filter/2, filtermap/2, fold/3, foreach/2 and map/2
raises a badmap exception if and only if the given iterator is not an
iterator at all, ie when the first call to next/1 on it fails. If a later
subsequent call to next/1 fails (ie, if something like {x, y, z} is given
as an iterator), the result will be a badarg error instead.

The changes in this commit change this behavior so that a badmap error is
always raised in such circumstances, that is, even if the given iterator
fails only on later subsequent calls.

Likewise, the stdlib error reporting did not complain about an iterator
being faulty if the first call to maps:next/1 on it succeeded. This has
also been addressed by the changes in this commit.
---
 lib/stdlib/src/erl_stdlib_errors.erl |   8 +-
 lib/stdlib/src/maps.erl              | 174 +++++++++++++++------------
 lib/stdlib/test/maps_SUITE.erl       |  74 +++++++++++-
 3 files changed, 171 insertions(+), 85 deletions(-)

diff --git a/lib/stdlib/src/erl_stdlib_errors.erl b/lib/stdlib/src/erl_stdlib_errors.erl
index 2d2fe03d29..37e86c9e7b 100644
--- a/lib/stdlib/src/erl_stdlib_errors.erl
+++ b/lib/stdlib/src/erl_stdlib_errors.erl
@@ -982,11 +982,9 @@ must_be_map_iterator_order(_) ->
 must_be_map_or_iter(Map) when is_map(Map) ->
     [];
 must_be_map_or_iter(Iter) ->
-    try maps:next(Iter) of
-        _ -> []
-    catch
-        error:_ ->
-            not_map_or_iterator
+    case maps:is_iterator_valid(Iter) of
+        true -> [];
+        false -> not_map_or_iterator
     end.
 
 must_be_number(N) ->
diff --git a/lib/stdlib/src/maps.erl b/lib/stdlib/src/maps.erl
index 98bff5c12d..12d6b47c8b 100644
--- a/lib/stdlib/src/maps.erl
+++ b/lib/stdlib/src/maps.erl
@@ -30,6 +30,9 @@
          merge_with/3,
          groups_from_list/2, groups_from_list/3]).
 
+%% Internal
+-export([is_iterator_valid/1]).
+
 %% BIFs
 -export([get/2, find/2, from_list/1, from_keys/2,
          is_key/2, keys/1, merge/2,
@@ -313,31 +316,28 @@ get(Key,Map,Default) ->
       MapOrIter :: #{Key => Value} | iterator(Key, Value),
       Map :: #{Key => Value}.
 
-filter(Pred, MapOrIter) when is_function(Pred, 2) ->
-    Iter = if
-               is_map(MapOrIter) ->
-                   iterator(MapOrIter);
-               true ->
-                   MapOrIter
-           end,
-    try next(Iter) of
-        Next ->
-            maps:from_list(filter_1(Pred, Next))
+filter(Pred, Map) when is_map(Map), is_function(Pred, 2) ->
+    maps:from_list(filter_1(Pred, next(iterator(Map)), undefined));
+filter(Pred, Iter) when is_function(Pred, 2) ->
+    ErrorTag = make_ref(),
+    try filter_1(Pred, try_next(Iter, ErrorTag), ErrorTag) of
+        Result ->
+            maps:from_list(Result)
     catch
-        error:_ ->
-            error_with_info({badmap,MapOrIter}, [Pred, MapOrIter])
+        error:ErrorTag ->
+            error_with_info({badmap, Iter}, [Pred, Iter])
     end;
 filter(Pred, Map) ->
     badarg_with_info([Pred, Map]).
 
-filter_1(Pred, {K, V, Iter}) ->
+filter_1(Pred, {K, V, Iter}, ErrorTag) ->
     case Pred(K, V) of
         true ->
-            [{K,V} | filter_1(Pred, next(Iter))];
+            [{K,V} | filter_1(Pred, try_next(Iter, ErrorTag), ErrorTag)];
         false ->
-            filter_1(Pred, next(Iter))
+            filter_1(Pred, try_next(Iter, ErrorTag), ErrorTag)
     end;
-filter_1(_Pred, none) ->
+filter_1(_Pred, none, _ErrorTag) ->
     [].
 
 -spec filtermap(Fun, MapOrIter) -> Map when
@@ -345,57 +345,52 @@ filter_1(_Pred, none) ->
       MapOrIter :: #{Key => Value1} | iterator(Key, Value1),
       Map :: #{Key => Value1 | Value2}.
 
-filtermap(Fun, MapOrIter) when is_function(Fun, 2) ->
-    Iter = if
-               is_map(MapOrIter) ->
-                   iterator(MapOrIter);
-               true ->
-                   MapOrIter
-           end,
-    try next(Iter) of
-        Next ->
-            maps:from_list(filtermap_1(Fun, Next))
+filtermap(Fun, Map) when is_map(Map), is_function(Fun, 2) ->
+    maps:from_list(filtermap_1(Fun, next(iterator(Map)), undefined));
+filtermap(Fun, Iter) when is_function(Fun, 2) ->
+    ErrorTag = make_ref(),
+    try filtermap_1(Fun, try_next(Iter, ErrorTag), ErrorTag) of
+        Result ->
+            maps:from_list(Result)
     catch
-        error:_ ->
-            error_with_info({badmap,MapOrIter}, [Fun, MapOrIter])
+        error:ErrorTag ->
+            error_with_info({badmap, Iter}, [Fun, Iter])
     end;
 filtermap(Fun, Map) ->
     badarg_with_info([Fun, Map]).
 
-filtermap_1(Pred, {K, V, Iter}) ->
-    case Pred(K, V) of
+filtermap_1(Fun, {K, V, Iter}, ErrorTag) ->
+    case Fun(K, V) of
         true ->
-            [{K, V} | filtermap_1(Pred, next(Iter))];
+            [{K, V} | filtermap_1(Fun, try_next(Iter, ErrorTag), ErrorTag)];
         {true, NewV} ->
-            [{K, NewV} | filtermap_1(Pred, next(Iter))];
+            [{K, NewV} | filtermap_1(Fun, try_next(Iter, ErrorTag), ErrorTag)];
         false ->
-            filtermap_1(Pred, next(Iter))
+            filtermap_1(Fun, try_next(Iter, ErrorTag), ErrorTag)
     end;
-filtermap_1(_Pred, none) ->
+filtermap_1(_Fun, none, _ErrorTag) ->
     [].
 
 -spec foreach(Fun,MapOrIter) -> ok when
       Fun :: fun((Key, Value) -> term()),
       MapOrIter :: #{Key => Value} | iterator(Key, Value).
 
-foreach(Fun, MapOrIter) when is_function(Fun, 2) ->
-    Iter = if is_map(MapOrIter) -> iterator(MapOrIter);
-              true -> MapOrIter
-           end,
-    try next(Iter) of
-        Next ->
-            foreach_1(Fun, Next)
+foreach(Fun, Map) when is_map(Map), is_function(Fun, 2) ->
+    foreach_1(Fun, next(iterator(Map)), undefined);
+foreach(Fun, Iter) when is_function(Fun, 2) ->
+    ErrorTag = make_ref(),
+    try foreach_1(Fun, try_next(Iter, ErrorTag), ErrorTag)
     catch
-        error:_ ->
-            error_with_info({badmap, MapOrIter}, [Fun, MapOrIter])
+        error:ErrorTag ->
+            error_with_info({badmap, Iter}, [Fun, Iter])
     end;
-foreach(Pred, Map) ->
-    badarg_with_info([Pred, Map]).
+foreach(Fun, Map) ->
+    badarg_with_info([Fun, Map]).
 
-foreach_1(Fun, {K, V, Iter}) ->
+foreach_1(Fun, {K, V, Iter}, ErrorTag) ->
     Fun(K,V),
-    foreach_1(Fun, next(Iter));
-foreach_1(_Fun, none) ->
+    foreach_1(Fun, try_next(Iter, ErrorTag), ErrorTag);
+foreach_1(_Fun, none, _ErrorTag) ->
     ok.
 
 -spec fold(Fun,Init,MapOrIter) -> Acc when
@@ -405,26 +400,21 @@ foreach_1(_Fun, none) ->
     AccIn :: Init | AccOut,
     MapOrIter :: #{Key => Value} | iterator(Key, Value).
 
-fold(Fun, Init, MapOrIter) when is_function(Fun, 3) ->
-    Iter = if
-               is_map(MapOrIter) ->
-                   iterator(MapOrIter);
-               true ->
-                   MapOrIter
-           end,
-    try next(Iter) of
-        Next ->
-            fold_1(Fun, Init, Next)
+fold(Fun, Init, Map) when is_map(Map), is_function(Fun, 3) ->
+    fold_1(Fun, Init, next(iterator(Map)), undefined);
+fold(Fun, Init, Iter) when is_function(Fun, 3) ->
+    ErrorTag = make_ref(),
+    try fold_1(Fun, Init, try_next(Iter, ErrorTag), ErrorTag)
     catch
-        error:_ ->
-            error_with_info({badmap,MapOrIter}, [Fun, Init, MapOrIter])
+        error:ErrorTag ->
+            error_with_info({badmap, Iter}, [Fun, Init, Iter])
     end;
 fold(Fun, Init, Map) ->
     badarg_with_info([Fun, Init, Map]).
 
-fold_1(Fun, Acc, {K, V, Iter}) ->
-    fold_1(Fun, Fun(K,V,Acc), next(Iter));
-fold_1(_Fun, Acc, none) ->
+fold_1(Fun, Acc, {K, V, Iter}, ErrorTag) ->
+    fold_1(Fun, Fun(K,V,Acc), try_next(Iter, ErrorTag), ErrorTag);
+fold_1(_Fun, Acc, none, _ErrorTag) ->
     Acc.
 
 -spec map(Fun,MapOrIter) -> Map when
@@ -432,27 +422,24 @@ fold_1(_Fun, Acc, none) ->
     MapOrIter :: #{Key => Value1} | iterator(Key, Value1),
     Map :: #{Key => Value2}.
 
-map(Fun, MapOrIter) when is_function(Fun, 2) ->
-    Iter = if
-               is_map(MapOrIter) ->
-                   iterator(MapOrIter);
-               true ->
-                   MapOrIter
-           end,
-    try next(Iter) of
-        Next ->
-            maps:from_list(map_1(Fun, Next))
+map(Fun, Map) when is_map(Map), is_function(Fun, 2) ->
+    maps:from_list(map_1(Fun, next(iterator(Map)), undefined));
+map(Fun, Iter) when is_function(Fun, 2) ->
+    ErrorTag = make_ref(),
+    try map_1(Fun, try_next(Iter, ErrorTag), ErrorTag) of
+        Result ->
+            maps:from_list(Result)
     catch
-        error:_ ->
-            error_with_info({badmap,MapOrIter}, [Fun, MapOrIter])
+        error:ErrorTag ->
+            error_with_info({badmap, Iter}, [Fun, Iter])
     end;
 map(Fun, Map) ->
     badarg_with_info([Fun, Map]).
 
 
-map_1(Fun, {K, V, Iter}) ->
-    [{K, Fun(K, V)} | map_1(Fun, next(Iter))];
-map_1(_Fun, none) ->
+map_1(Fun, {K, V, Iter}, ErrorTag) ->
+    [{K, Fun(K, V)} | map_1(Fun, try_next(Iter, ErrorTag), ErrorTag)];
+map_1(_Fun, none, _ErrorTag) ->
     [].
 
 -spec size(Map) -> non_neg_integer() when
@@ -622,3 +609,34 @@ badarg_with_info(Args) ->
 
 error_with_info(Reason, Args) ->
     erlang:error(Reason, Args, [{error_info, #{module => erl_stdlib_errors}}]).
+
+-spec is_iterator_valid(MaybeIter) -> boolean() when
+    MaybeIter :: iterator() | term().
+
+is_iterator_valid(Iter) ->
+    try is_iterator_valid_1(Iter)
+    catch
+        error:badarg ->
+            false
+    end.
+
+is_iterator_valid_1(none) ->
+    true;
+is_iterator_valid_1({_, _, Next}) ->
+    is_iterator_valid_1(next(Next));
+is_iterator_valid_1(Iter) ->
+    _ = next(Iter),
+    true.
+
+try_next({_, _, _} = KVI, _ErrorTag) ->
+    KVI;
+try_next(none, _ErrorTag) ->
+    none;
+try_next(Iter, undefined) ->
+    next(Iter);
+try_next(Iter, ErrorTag) ->
+    try next(Iter)
+    catch
+        error:badarg ->
+            error(ErrorTag)
+    end.
diff --git a/lib/stdlib/test/maps_SUITE.erl b/lib/stdlib/test/maps_SUITE.erl
index 7ab669c155..0ed0a59192 100644
--- a/lib/stdlib/test/maps_SUITE.erl
+++ b/lib/stdlib/test/maps_SUITE.erl
@@ -31,7 +31,9 @@
 -export([t_update_with_3/1, t_update_with_4/1,
          t_get_3/1, t_filter_2/1, t_filtermap_2/1,
          t_fold_3/1,t_map_2/1,t_size_1/1, t_foreach_2/1,
-         t_iterator_1/1, t_put_opt/1, t_merge_opt/1,
+         t_iterator_1/1,
+         t_iterator_valid/1,
+         t_put_opt/1, t_merge_opt/1,
          t_with_2/1,t_without_2/1,
          t_intersect/1, t_intersect_with/1,
          t_merge_with/1, t_from_keys/1,
@@ -59,7 +60,9 @@ all() ->
     [t_update_with_3,t_update_with_4,
      t_get_3,t_filter_2,t_filtermap_2,
      t_fold_3,t_map_2,t_size_1,t_foreach_2,
-     t_iterator_1,t_put_opt,t_merge_opt,
+     t_iterator_1,
+     t_iterator_valid,
+     t_put_opt,t_merge_opt,
      t_with_2,t_without_2,
      t_intersect, t_intersect_with,
      t_merge_with, t_from_keys,
@@ -396,7 +398,8 @@ t_filter_2(Config) when is_list(Config) ->
     #{a := 2,c := 4} = maps:filter(Pred1,maps:iterator(M)),
     #{"b" := 2,"c" := 4} = maps:filter(Pred2,maps:iterator(M)),
     %% error case
-    ?badmap(a,filter,[_,a]) = (catch maps:filter(fun(_,_) -> ok end,id(a))),
+    ?badmap(a,filter,[_,a]) = (catch maps:filter(fun(_,_) -> true end,id(a))),
+    ?badmap({a,b,c},filter,[_,{a,b,c}]) = (catch maps:filter(fun(_,_) -> true end,id({a,b,c}))),
     ?badarg(filter,[<<>>,#{}]) = (catch maps:filter(id(<<>>),#{})),
     ok.
 
@@ -410,7 +413,8 @@ t_filtermap_2(Config) when is_list(Config) ->
     false = maps:is_key(20, M1),
     true = M1 =:= M2,
     %% error case
-    ?badmap(a,filtermap,[_,a]) = (catch maps:filtermap(fun(_,_) -> ok end,id(a))),
+    ?badmap(a,filtermap,[_,a]) = (catch maps:filtermap(fun(_,_) -> true end,id(a))),
+    ?badmap({a,b,c},filtermap,[_,{a,b,c}]) = (catch maps:filtermap(fun(_,_) -> true end,id({a,b,c}))),
     ?badarg(filtermap,[<<>>,#{}]) = (catch maps:filtermap(id(<<>>),#{})),
     ok.
 
@@ -426,6 +430,7 @@ t_fold_3(Config) when is_list(Config) ->
 
     %% error case
     ?badmap(a,fold,[_,0,a]) = (catch maps:fold(fun(_,_,_) -> ok end,0,id(a))),
+    ?badmap({a,b,c},fold,[_,0,{a,b,c}]) = (catch maps:fold(fun(_,_,_) -> ok end,0,id({a,b,c}))),
     ?badarg(fold,[<<>>,0,#{}]) = (catch maps:fold(id(<<>>),0,#{})),
     ok.
 
@@ -440,6 +445,7 @@ t_map_2(Config) when is_list(Config) ->
 
     %% error case
     ?badmap(a,map,[_,a]) = (catch maps:map(fun(_,_) -> ok end, id(a))),
+    ?badmap({a,b,c},map,[_,{a,b,c}]) = (catch maps:map(fun(_,_) -> ok end, id({a,b,c}))),
     ?badarg(map,[<<>>,#{}]) = (catch maps:map(id(<<>>),#{})),
     ok.
 
@@ -450,6 +456,7 @@ t_foreach_2(Config) when is_list(Config) ->
     ?badmap({},foreach,[_,{}]) = (catch maps:foreach(fun(_,_) -> ok end, id({}))),
     ?badmap(42,foreach,[_,42]) = (catch maps:foreach(fun(_,_) -> ok end, id(42))),
     ?badmap(<<>>,foreach,[_,<<>>]) = (catch maps:foreach(fun(_,_) -> ok end, id(<<>>))),
+    ?badmap({a,b,c},foreach,[_,{a,b,c}]) = (catch maps:foreach(fun(_,_) -> ok end, id({a,b,c}))),
 
     ?badarg(foreach,[<<>>,#{}]) = (catch maps:foreach(id(<<>>),#{})),
     F0 = fun() -> ok end,
@@ -593,6 +600,59 @@ iter_kv(I) ->
             [{K,V} | iter_kv(NI)]
     end.
 
+t_iterator_valid(Config) when is_list(Config) ->
+    %% good iterators created via maps:iterator
+    true = maps:is_iterator_valid(maps:iterator(#{})),
+    true = maps:is_iterator_valid(maps:iterator(#{a => b})),
+    true = maps:is_iterator_valid(maps:iterator(#{a => b, c => d})),
+    true = maps:is_iterator_valid(maps:iterator(#{}, undefined)),
+    true = maps:is_iterator_valid(maps:iterator(#{a => b}, undefined)),
+    true = maps:is_iterator_valid(maps:iterator(#{a => b, c => d}, undefined)),
+    true = maps:is_iterator_valid(maps:iterator(#{}, ordered)),
+    true = maps:is_iterator_valid(maps:iterator(#{a => b}, ordered)),
+    true = maps:is_iterator_valid(maps:iterator(#{a => b, c => d}, ordered)),
+    true = maps:is_iterator_valid(maps:iterator(#{}, reversed)),
+    true = maps:is_iterator_valid(maps:iterator(#{a => b}, reversed)),
+    true = maps:is_iterator_valid(maps:iterator(#{a => b, c => d}, reversed)),
+    true = maps:is_iterator_valid(maps:iterator(#{}, fun erlang:'=<'/2)),
+    true = maps:is_iterator_valid(maps:iterator(#{a => b}, fun erlang:'=<'/2)),
+    true = maps:is_iterator_valid(maps:iterator(#{a => b, c => d}, fun erlang:'=<'/2)),
+
+    %% good makeshift iterators
+    true = maps:is_iterator_valid(none),
+    true = maps:is_iterator_valid({a, b, none}),
+    true = maps:is_iterator_valid({a, b, {c, d, none}}),
+    true = maps:is_iterator_valid({a, b, {c, d, {e, f, none}}}),
+    true = maps:is_iterator_valid({a, b, maps:iterator(#{})}),
+    true = maps:is_iterator_valid({a, b, maps:iterator(#{c => d})}),
+
+    %% not iterators
+    false = maps:is_iterator_valid(no_iter),
+    false = maps:is_iterator_valid(1),
+    false = maps:is_iterator_valid(1.0),
+    false = maps:is_iterator_valid([]),
+    false = maps:is_iterator_valid("foo"),
+    false = maps:is_iterator_valid(<<"foo">>),
+    false = maps:is_iterator_valid(fun() -> ok end),
+    false = maps:is_iterator_valid(self()),
+    false = maps:is_iterator_valid(make_ref()),
+    false = maps:is_iterator_valid(#{}),
+    false = maps:is_iterator_valid({}),
+    false = maps:is_iterator_valid({a}),
+    false = maps:is_iterator_valid({a, b}),
+    false = maps:is_iterator_valid({a, b, c, d}),
+
+    %% bad makeshift iterators that only fail on later (ie, subsequent) calls to maps:next/1
+    %%    maps:next({a, b, c}) -> {a, b, c}
+    %%    maps:next(c) -> badarg
+    false = maps:is_iterator_valid({a, b, c}),
+    %%    maps:next({a, b, {c, d, e}}) -> {a, b, {c, d, e}}
+    %%    maps:next({c, d, e}) -> {c, d, e}
+    %%    maps:next(e) -> badarg
+    false = maps:is_iterator_valid({a, b, {c, d, e}}),
+
+    ok.
+
 t_put_opt(Config) when is_list(Config) ->
     Value = id(#{complex => map}),
     Small = id(#{a => Value}),
@@ -890,11 +950,14 @@ error_info(_Config) ->
 
     L = [{filter, [fun(_, _) -> true end, abc]},
          {filter, [fun(_, _) -> true end, BadIterator]},
+         {filter, [fun(_, _) -> true end, BadIterator2]},
          {filter, [bad_fun, BadIterator],[{1,".*"},{2,".*"}]},
+         {filter, [bad_fun, BadIterator2],[{1,".*"},{2,".*"}]},
          {filter, [bad_fun, GoodIterator]},
 
          {filtermap, [fun(_, _) -> true end, abc]},
          {filtermap, [fun(_, _) -> true end, BadIterator]},
+         {filtermap, [fun(_, _) -> true end, BadIterator2]},
          {filtermap, [fun(_) -> true end, GoodIterator]},
          {filtermap, [fun(_) -> ok end, #{}]},
 
@@ -902,11 +965,13 @@ error_info(_Config) ->
 
          {fold, [fun(_, _, _) -> true end, init, abc]},
          {fold, [fun(_, _, _) -> true end, init, BadIterator]},
+         {fold, [fun(_, _, _) -> true end, init, BadIterator2]},
          {fold, [fun(_) -> true end, init, GoodIterator]},
          {fold, [fun(_) -> ok end, init, #{}]},
 
          {foreach, [fun(_, _) -> ok end, no_map]},
          {foreach, [fun(_, _) -> ok end, BadIterator]},
+         {foreach, [fun(_, _) -> ok end, BadIterator2]},
          {foreach, [fun(_) -> ok end, GoodIterator]},
          {foreach, [fun(_) -> ok end, #{}]},
 
@@ -936,6 +1001,10 @@ error_info(_Config) ->
          {intersect_with, [fun(_, _, _) -> ok end, x, y],[{2,".*"},{3,".*"}]},
          {intersect_with, [fun(_, _) -> ok end, #{}, #{}]},
 
+         {is_iterator_valid, [GoodIterator], [no_fail]},
+         {is_iterator_valid, [BadIterator], [no_fail]},
+         {is_iterator_valid, [BadIterator2], [no_fail]},
+
          {is_key,[key, no_map]},
 
          {iterator,[{no,map}]},
@@ -951,6 +1020,7 @@ error_info(_Config) ->
 
          {map, [fun(_, _) -> true end, abc]},
          {map, [fun(_, _) -> true end, BadIterator]},
+         {map, [fun(_, _) -> true end, BadIterator2]},
          {map, [fun(_) -> true end, GoodIterator]},
          {map, [fun(_) -> ok end, #{}]},
 
-- 
2.35.3

Places

File 5891-maps-raise-a-badmap-error-if-an-iterator-fails-later.patch of Package erlang (Revision 58ddfd9fcf4c281c82656d8c2d2a71df)

Places