Skip to content

Add enum support#43

Draft
lchenut wants to merge 5 commits intomasterfrom
add-enum-support
Draft

Add enum support#43
lchenut wants to merge 5 commits intomasterfrom
add-enum-support

Conversation

@lchenut
Copy link
Copy Markdown
Contributor

@lchenut lchenut commented Jan 24, 2023

Things left to do:

For the last point, it might be hard with the way nim works with enum:

type
  Test = enum
    A = 0
    B = 10

echo Test(parseInt("10")) # print "B"
echo Test(parseInt("5"))  # print "5 (invalid data!)"
echo Test(parseInt("15")) # crash with 15 notin 0 .. 10 [RangeDefect]

So for now, I set the enum at the default value if it isn't recognized.

@Menduist
Copy link
Copy Markdown
Collaborator

save unrecognized values during deserialization

IMO, the best way to do this would be to save every unknown value (of enums & unknown fields) in a Table[int, seq[byte]] (wrapped in a specific object)
So for instance

type
  Heya {.proto3.} = object
    f {.fieldNumber: 1.}: int
    otherFields: UnknownProtoFields

If the user cares about unknown enum values & field, he'll have to add it (and .proto parsing will always add it)
The decoder would put every unknown stuff there, and the encoder would append every fields
And then we can create some helpers in the UnknownProtoFields object to check & retrieve unknown fields

This would kill two birds in one stone, wdyt @arnetheduck @lchenut ?

@arnetheduck
Copy link
Copy Markdown
Member

So, the c++ proto3 behavior is to allow the fields to take on non-enum values - I would likely start with field: PbEnum[T] which holds values in an internal int32 and has a convert-to-Opt[T] and a "raw" value accessor.

https://developers.google.com/protocol-buffers/docs/reference/cpp-generated#enum:

Unrecognized enum values will be kept when parsing a proto3 message and returned by the enum field accessors.

the proto2 behavior is indeed to treat the enum as an unknown field - one way to understand the difference vs proto3 is that this approach isn't great.

That said, unknown field preservation is indeed a desireable feature in general so in proto2 we could potentially use this solution - I imagine we'd add some marker that says {.unknownfields.} there - alternatively, we turn {.proto3.} into a macro that rewrites the type and adds these features, something like {.proto3(unknownFields = true).}.

Because protobuf enums 99% work like const+int32 in nim, I kind of see this feature as one of the lower-priority things - a PbEnum solves this with a slightly better API but I wonder if it's too cumbersome in practice - perhaps {.pbenum.} should simply enforce that the field is an int32 so that people don't use the wrong type (like uint64).

@arnetheduck
Copy link
Copy Markdown
Member

Another thing for the todo list: conformance suite support - ie this feature was removed because it wasn't conforming to upstream, so we should do it right this time

@lchenut lchenut mentioned this pull request Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants