From 7e1eb2ed1f724945884d839bd8a99154e9382849 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Gustavsson?= Date: Fri, 10 Aug 2018 07:43:31 +0200 Subject: Correct error behavior of is_map_key/2 in guards Consider the following functions: foo() -> bar(not_a_map). bar(M) when not is_map_key(a, M) -> ok; bar(_) -> error. What will `foo/0` return? It depends. If the module is compiled with the default compiler options, the return value will be `ok`. If the module is compiled with the `inline` option, the return value will be `error`. The correct value is `error`, because the call to `is_map_key/2` when the second argument is not a map should fail the entire guard. That is the way other failing guards BIFs are handled. For example: foo() -> bar(not_a_tuple). bar(T) when not element(1, T) -> ok; bar(_) -> error. `foo/0` always returns `error` (whether the code is inlined or not). This bug can be fixed by changing the classification of `is_map_key/2` in the `erl_internal` module. It is now classified as a type test, which is incorrect because type tests should not fail. Reclassifying it as a plain guard BIF corrects the bug. This correction also fixes the internal consistency check failure which was reported in: https://bugs.erlang.org/browse/ERL-699 --- lib/compiler/src/v3_codegen.erl | 5 ----- lib/compiler/test/map_SUITE.erl | 46 ++++++++++++++++++++++++++++++++++++++++- lib/stdlib/src/erl_internal.erl | 2 +- 3 files changed, 46 insertions(+), 7 deletions(-) diff --git a/lib/compiler/src/v3_codegen.erl b/lib/compiler/src/v3_codegen.erl index 6cd114abf7..e9152ba88f 100644 --- a/lib/compiler/src/v3_codegen.erl +++ b/lib/compiler/src/v3_codegen.erl @@ -1621,11 +1621,6 @@ test_cg(is_boolean, [#k_atom{val=Val}], Fail, I, Vdb, Bef, St) -> false -> [{jump,{f,Fail}}] end, {Is,Aft,St}; -test_cg(is_map_key, As, Fail, I, Vdb, Bef, St) -> - [Key,Map] = cg_reg_args(As, Bef), - Aft = clear_dead(Bef, I, Vdb), - F = {f,Fail}, - {[{test,is_map,F,[Map]},{test,has_map_fields,F,Map,{list,[Key]}}],Aft,St}; test_cg(Test, As, Fail, I, Vdb, Bef, St) -> Args = cg_reg_args(As, Bef), Aft = clear_dead(Bef, I, Vdb), diff --git a/lib/compiler/test/map_SUITE.erl b/lib/compiler/test/map_SUITE.erl index 9182c1b5ed..c004dca834 100644 --- a/lib/compiler/test/map_SUITE.erl +++ b/lib/compiler/test/map_SUITE.erl @@ -1212,10 +1212,25 @@ t_guard_bifs(Config) when is_list(Config) -> true = map_guard_empty_2(), true = map_guard_head(#{a=>1}), false = map_guard_head([]), + true = map_get_head(#{a=>1}), + false = map_get_head(#{}), + false = map_get_head([]), + + true = map_get_head_not(#{a=>false}), + false = map_get_head_not(#{a=>true}), + false = map_get_head(#{}), false = map_get_head([]), + true = map_is_key_head(#{a=>1}), false = map_is_key_head(#{}), + false = map_is_key_head(not_a_map), + + false = map_is_key_head_not(#{a=>1}), + true = map_is_key_head_not(#{b=>1}), + true = map_is_key_head_not(#{}), + false = map_is_key_head_not(not_a_map), + true = map_guard_body(#{a=>1}), false = map_guard_body({}), true = map_guard_pattern(#{a=>1, <<"hi">> => "hi" }), @@ -1224,6 +1239,25 @@ t_guard_bifs(Config) when is_list(Config) -> true = map_guard_ill_map_size(), true = map_field_check_sequence(#{a=>1}), false = map_field_check_sequence(#{}), + + %% The guard BIFs used in a body. + + v = map_get(a, id(#{a=>v})), + {'EXIT',{{badkey,a},_}} = + (catch map_get(a, id(#{}))), + {'EXIT',{{badmap,not_a_map},_}} = + (catch map_get(a, id(not_a_map))), + + true = is_map_key(a, id(#{a=>1})), + false = is_map_key(b, id(#{a=>1})), + false = is_map_key(b, id(#{})), + {'EXIT',{{badmap,not_a_map},_}} = + (catch is_map_key(b, id(not_a_map))), + + {true,v} = erl_699(#{k=>v}), + {'EXIT',{{badkey,k},_}} = (catch erl_699(#{})), + {'EXIT',{{badmap,not_a_map},_}} = (catch erl_699(not_a_map)), + ok. map_guard_empty() when is_map(#{}); false -> true. @@ -1236,8 +1270,14 @@ map_guard_head(_) -> false. map_get_head(M) when map_get(a, M) =:= 1 -> true; map_get_head(_) -> false. +map_get_head_not(M) when not map_get(a, M) -> true; +map_get_head_not(_) -> false. + map_is_key_head(M) when is_map_key(a, M) -> true; -map_is_key_head(M) -> false. +map_is_key_head(_) -> false. + +map_is_key_head_not(M) when not is_map_key(a, M) -> true; +map_is_key_head_not(_) -> false. map_guard_body(M) -> is_map(M). @@ -1254,6 +1294,10 @@ map_field_check_sequence(M) map_field_check_sequence(_) -> false. +erl_699(M) -> + %% Used to cause an internal consistency failure. + {is_map_key(k, M),maps:get(k, M)}. + t_guard_sequence(Config) when is_list(Config) -> {1, "a"} = map_guard_sequence_1(#{seq=>1,val=>id("a")}), {2, "b"} = map_guard_sequence_1(#{seq=>2,val=>id("b")}), diff --git a/lib/stdlib/src/erl_internal.erl b/lib/stdlib/src/erl_internal.erl index b311a843c2..939abaff00 100644 --- a/lib/stdlib/src/erl_internal.erl +++ b/lib/stdlib/src/erl_internal.erl @@ -74,6 +74,7 @@ guard_bif(element, 2) -> true; guard_bif(float, 1) -> true; guard_bif(floor, 1) -> true; guard_bif(hd, 1) -> true; +guard_bif(is_map_key, 2) -> true; guard_bif(length, 1) -> true; guard_bif(map_size, 1) -> true; guard_bif(map_get, 2) -> true; @@ -109,7 +110,6 @@ new_type_test(is_function, 2) -> true; new_type_test(is_integer, 1) -> true; new_type_test(is_list, 1) -> true; new_type_test(is_map, 1) -> true; -new_type_test(is_map_key, 2) -> true; new_type_test(is_number, 1) -> true; new_type_test(is_pid, 1) -> true; new_type_test(is_port, 1) -> true; -- cgit v1.2.3