From 34423dfe396355561fa28cc662a1fdeeaec90e49 Mon Sep 17 00:00:00 2001 From: Tomas Abrahamsson Date: Fri, 23 Jan 2015 23:50:34 +0100 Subject: [PATCH] Handle the enum option allow_alias = true This option allows several enumerators to have the same numeric value. For langages where the enum symbol is really an integer value, this causes no problems, but for gpb, which maps numeric values to atoms, it has been solved this way: Given this example: enum e { option allow_alias = true; x = 1; y = 1; }; message m { required e field1 = 1; }; - encoding accepts all atoms, ie: #m{field1 = x} and #m{field1 = y} would encode to the same. - on decoding, it decodes to the first defined enum symbol: seeing an 1 on the wire would decode to #{field1 = x}. In the introspection functions, such an option field is a three-tuple in list of enumerator values returned: find_enum_def(e) will return the following: [{option, allow_alias, true}, {x, 1}, {y, 1}] The enum_symbol_by_value/2 and enum_symbol_by_value_e/1 will return the atom x for the value 1. This option is true in gpb, regardless of actual option setting. It appeared in google protobuf 2.5.0, and the plan to make it default to true in some future has been advertized. Having it always true in gpb means we won't be generating a compile time error for the case that the option is set to false, and more than enum have the same numeric value. --- src/gpb_compile.erl | 13 +++++++++++-- src/gpb_parse.yrl | 13 ++++++++++++- test/gpb_parse_tests.erl | 9 +++++++++ test/gpb_tests.erl | 21 +++++++++++++++++++++ 4 files changed, 53 insertions(+), 3 deletions(-) diff --git a/src/gpb_compile.erl b/src/gpb_compile.erl index 7cefe3ab..9217a21a 100644 --- a/src/gpb_compile.erl +++ b/src/gpb_compile.erl @@ -1965,7 +1965,7 @@ format_enum_decoders(Defs, #anres{used_types=UsedTypes}) -> [repeat_clauses('', [[replace_term('', EnumValue), replace_term('', EnumSym)] - || {EnumSym, EnumValue} <- EnumDef])]) + || {EnumSym, EnumValue} <- unalias_enum(EnumDef)])]) || {{enum, EnumName}, EnumDef} <- Defs, smember({enum,EnumName}, UsedTypes)]. @@ -3311,7 +3311,7 @@ format_enum_value_symbol_converters(EnumDefs) when EnumDefs /= [] -> [repeat_clauses('', [[replace_term('', EnumValue), replace_term('', EnumSym)] - || {EnumSym, EnumValue} <- EnumDef])]), + || {EnumSym, EnumValue} <- unalias_enum(EnumDef)])]), "\n", gpb_codegen:format_fn( mk_fn(enum_value_by_symbol_, EnumName), @@ -4960,6 +4960,15 @@ map_kvalues(KVars) -> end || {Key, Expr} <- KVars]. +%% The "option allow_alias = true;" inside an enum X { ... } +%% says it is ok to have multiple symbols that map to the same numeric value. +%% Appeared in protobuf 2.5.0. +unalias_enum([{_Sym,Value}=Enum | Rest]) -> + [Enum | unalias_enum([E || {_,V}=E <- Rest, V /= Value])]; +unalias_enum([{option,_Name,_Value} | Rest]) -> + unalias_enum(Rest); +unalias_enum([]) -> + []. var_f_n(N) -> var_n("F", N). var_b_n(N) -> var_n("B", N). diff --git a/src/gpb_parse.yrl b/src/gpb_parse.yrl index 3e6b7499..09c92d91 100644 --- a/src/gpb_parse.yrl +++ b/src/gpb_parse.yrl @@ -101,6 +101,7 @@ enum_def -> enum identifier '{' enum_fields '}': {{enum,identifier_name('$2')},'$4'}. enum_fields -> enum_field enum_fields: ['$1' | '$2']. +enum_fields -> option_def enum_fields: ['$1' | '$2']. enum_fields -> ';' enum_fields: '$2'. enum_fields -> '$empty': []. @@ -713,7 +714,7 @@ reformat_names(Defs) -> lists:map(fun({{msg,Name}, Fields}) -> {{msg,reformat_name(Name)}, reformat_fields(Fields)}; ({{enum,Name}, ENs}) -> - {{enum,reformat_name(Name)}, ENs}; + {{enum,reformat_name(Name)}, reformat_enum_opt_names(ENs)}; ({{extensions,Name}, Exts}) -> {{extensions,reformat_name(Name)}, Exts}; ({{extend,Name}, Fields}) -> @@ -738,6 +739,16 @@ reformat_fields(Fields) -> end, Fields). +%% `Defs' is expected to be parsed. +reformat_enum_opt_names(Def) -> + [case Item of + {option, Name, Value} -> + {option, reformat_name(Name), Value}; + Other -> + Other + end + || Item <- Def]. + reformat_name(Name) -> list_to_atom(string:join([atom_to_list(P) || P <- Name, P /= '.'], diff --git a/test/gpb_parse_tests.erl b/test/gpb_parse_tests.erl index c56edf59..b8f91f08 100644 --- a/test/gpb_parse_tests.erl +++ b/test/gpb_parse_tests.erl @@ -181,6 +181,15 @@ parses_import_test() -> parse_lines(["package p1.p2;", "import \"a/b/c.proto\";"]). +parses_enum_option_test() -> + {ok, Elems} = parse_lines(["enum e1 {", + " option allow_alias = true;", + " ee1 = 1;", + " ee2 = 1;", + "}"]), + [{{enum,e1}, [{option, allow_alias, true}, {ee1,1},{ee2,1}]}] = + do_process_sort_defs(Elems). + generates_correct_absolute_names_test() -> {ok, Elems} = parse_lines(["message m1 {" " message m2 { required uint32 x = 1; }", diff --git a/test/gpb_tests.erl b/test/gpb_tests.erl index 588515cb..255caf2e 100644 --- a/test/gpb_tests.erl +++ b/test/gpb_tests.erl @@ -173,6 +173,17 @@ decode_msg_with_negative_enum_value_test() -> {{enum,e}, [{v1, 100}, {v2, -2}]}]). +decode_msg_with_enum_aliases_test() -> + #m1{a = v1} = + decode_msg(<<8,100>>, + m1, + [{{msg,m1}, [#?gpb_field{name=a, fnum=1, rnum=#m1.a, + type={enum,e}, occurrence=required, + opts=[]}]}, + {{enum,e}, [{option, allow_alias, true}, + {v1, 100}, + {v2, 100}]}]). + decode_msg_with_bool_field_test() -> #m1{a = true} = decode_msg(<<8,1>>, @@ -462,6 +473,16 @@ encode_msg_with_negative_enum_value_test() -> <<254,255,255,255,255,255,255,255,255,1>> -> ok %% encoded as 64 bits end. +encode_msg_with_enum_aliases_test() -> + Defs = [{{msg,m1}, [#?gpb_field{name=a, fnum=1, rnum=#m1.a, + type={enum,e}, occurrence=required, + opts=[]}]}, + {{enum,e}, [{option, allow_alias, true}, + {v1, 100}, + {v2, 100}]}], + <<8,100>> = encode_msg(#m1{a = v1}, Defs), + <<8,100>> = encode_msg(#m1{a = v2}, Defs). + encode_msg_with_bool_field_test() -> <<8,1>> = encode_msg(#m1{a = true},