Odd generation of optional fields about protoc-gen-elm HOT 8 CLOSED

I think this broke generation for optional enum fields. Looks like a simple fix, but I don't know how easy it is to actually implement.

Consider the following protobuf code:

enum Enum {
  A = 0;
  B = 1;
}

message Foo {
  optional Enum e = 1;

  // Working optional field, for reference
  optional int32 i = 2;
}

It will generate the following decoder and encoder:

decodeProto__Foo__V1__Foo : Protobuf.Decode.Decoder Proto__Foo__V1__Foo
decodeProto__Foo__V1__Foo =
    Protobuf.Decode.message
        defaultProto__Foo__V1__Foo
        [ Protobuf.Decode.optional 1 decodeProto__Foo__V1__Enum (\a r -> { r | e = a })
        , Protobuf.Decode.optional 2 (Protobuf.Decode.map Just Protobuf.Decode.int32) (\a r -> { r | i = a })
        ]


encodeProto__Foo__V1__Foo : Proto__Foo__V1__Foo -> Protobuf.Encode.Encoder
encodeProto__Foo__V1__Foo value =
    Protobuf.Encode.message
        [ ( 1, encodeProto__Foo__V1__Enum value.e )
        , ( 2, (Maybe.map Protobuf.Encode.int32 >> Maybe.withDefault Protobuf.Encode.none) value.i )
        ]

This fails to compile, as Proto__Foo__V1__Foo expects a Maybe Proto__Foo__V1__Enum, but the decoder and encoder assume it is not a Maybe. It should probably generate code for the enum field similar to the code generated for the int field, like the following:

decodeProto__Foo__V1__Foo : Protobuf.Decode.Decoder Proto__Foo__V1__Foo
decodeProto__Foo__V1__Foo =
    Protobuf.Decode.message
        defaultProto__Foo__V1__Foo
        [ Protobuf.Decode.optional 1 (Protobuf.Decode.map Just decodeProto__Foo__V1__Enum) (\a r -> { r | e = a })
        , Protobuf.Decode.optional 2 (Protobuf.Decode.map Just Protobuf.Decode.int32) (\a r -> { r | i = a })
        ]


encodeProto__Foo__V1__Foo : Proto__Foo__V1__Foo -> Protobuf.Encode.Encoder
encodeProto__Foo__V1__Foo value =
    Protobuf.Encode.message
        [ ( 1, (Maybe.map encodeProto__Foo__V1__Enum >> Maybe.withDefault Protobuf.Encode.none) value.e )
        , ( 2, (Maybe.map Protobuf.Encode.int32 >> Maybe.withDefault Protobuf.Encode.none) value.i )
        ]

I tested this by manually editing the generated file, and it seems to work.

from protoc-gen-elm.

Thanks for finding this! I did not handle the Proto3Optional case correctly for the enum de/encoders.
Should work with version 3.0.1 now.

from protoc-gen-elm.

That is most likely a bug. I'll add a regression test and figure out why the unnecessary wrapper occurs. Wrappers should only be necessary in recursive messages.

from protoc-gen-elm.

Very interesting. This is not just me having some weird case distinction but rather protoc generating an interesting request:

This file:

syntax = "proto3";

package proto3_optional;

message WithOptional {
  optional string field = 1;
  optional int32 field2 = 2;
}

generates a request with the following messageType

{ enumType = []
    , extension = []
    , extensionRange = []
    , field =
        [ { defaultValue = "", extendee = "", jsonName = "field", label = FieldDescriptorProto_Label_LABELOPTIONAL, name = "field", number = 1, oneofIndex = 0, options = Nothing, proto3Optional = True, typeName = "", type_ = FieldDescriptorProto_Type_TYPESTRING }
        , { defaultValue = "", extendee = "", jsonName = "field2", label = FieldDescriptorProto_Label_LABELOPTIONAL, name = "field2", number = 2, oneofIndex = 1, options = Nothing, proto3Optional = True, typeName = "", type_ = FieldDescriptorProto_Type_TYPEINT32 }
        ]
    , name = "WithOptional"
    , nestedType = []
    , oneofDecl = [ { name = "_field", options = Nothing }, { name = "_field2", options = Nothing } ]
    , options = Nothing
    , reservedName = []
    , reservedRange = []
    }

Note that aside from the expected "field" and "field2" occurences in the field attribute, a oneOf declaration is added.
I'll see what I can do 🤔

from protoc-gen-elm.

I just published [email protected] which should fix this weird behaviour.

from protoc-gen-elm.

I'll close this for now, since I released 3.0.0. Feel free to reopen if you encounter the issue again.

from protoc-gen-elm.

Hi. Thank you for looking into that. I finally have some time to give this a try today.

from protoc-gen-elm.

Yeah, works great. Thank you!

from protoc-gen-elm.