JuliaIO / ProtoBuf.jl

Julia protobuf implementation
Other
205 stars 55 forks source link

Allow storing unknown enum values #209

Closed Drvi closed 2 years ago

Drvi commented 2 years ago

Until now, when we encountered an unknown enum member, we pretended that we got the default value. This is not very useful and could lead to a lot of confusion. This is what the spec has to say about this:

During deserialization, unrecognized enum values will be preserved in the message, though how this is represented when the message is deserialized is language-dependent. In languages that support open enum types with values outside the range of specified symbols, such as C++ and Go, the unknown enum value is simply stored as its underlying integer representation. In languages with closed enum types such as Java, a case in the enum is used to represent an unrecognized value, and the underlying integer can be accessed with special accessors. In either case, if the message is serialized the unrecognized value will still be serialized with the message.

So I feel we should try harder to preserve unknown enum members.

Inside structs, these unknown enums are pretty distinguishable:

TestStruct{OneOf{TestEnum.T}}(OneOf{TestEnum.T}(:enum, TestEnum.B)) # we got 2, a known value

TestStruct{OneOf{TestEnum.T}}(OneOf{TestEnum.T}(:enum, TestEnum.<invalid #7>)) # we got 7, an unknown value
codecov[bot] commented 2 years ago

Codecov Report

Merging #209 (b770b99) into master (491d4c8) will not change coverage. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #209   +/-   ##
=======================================
  Coverage   88.82%   88.82%           
=======================================
  Files          25       25           
  Lines        2782     2782           
=======================================
  Hits         2471     2471           
  Misses        311      311           
Impacted Files Coverage Δ
src/codec/decode.jl 86.44% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

quinnj commented 2 years ago

This LGTM. One question though: what if I want to access the unknown enum value or convert it to an int or something. Is there syntax/getproperty support for that?

Drvi commented 2 years ago

Afaict, the value should be easily accessible and converting to Int should just work (same as with other enum values). Here is a little session:

julia> using EnumX

julia> @enumx A a b c

julia> struct Test
          t::A.T
       end

julia> t = Test(Core.bitcast(A.T, Int32(42)))
Test(Main.A.<invalid #42>)

julia> t.t # this show method is not the greatest, but we don't do anything special for Int32/Int64 either...
42

julia> typeof(t.t)
Enum type A.T <: Enum{Int32} with 3 instances:
 A.a = 0
 A.b = 1
 A.c = 2

julia> Int(t.t)
42

julia> typeof(Int(t.t))
Int64

Does this answer your question?

quinnj commented 2 years ago

Ah yes, that answers it. I guess I was thinking about the case where you want to do A.invalid or something, but I guess that doesn't really make sense. When it's a field, we're good as you've shown.

Drvi commented 2 years ago

We could provide a helper function isvalid(x::Enum) to make things a little easier for the users.