protobuf icon indicating copy to clipboard operation
protobuf copied to clipboard

generated typespec doesn't seem to take `proto3_optional: true` into account

Open JerzyDziala opened this issue 1 year ago • 5 comments

JerzyDziala avatar Aug 25 '22 06:08 JerzyDziala

Just to add more context to the issue, here is a snippet generating typespecs for a message having both proto3_optional?: true and proto3_optional?: false:

iex> message_props = %MessageProps{
  field_props: %{
    1 => %FieldProps{name_atom: :foo, type: :int32, proto3_optional?: true},
    2 => %FieldProps{name_atom: :bar, type: :int32, proto3_optional?: false}
  },
  oneof: []
}

iex> quoted = Typespecs.quoted_message_typespec(message_props)
iex> Macro.to_string(quoted) 
%__MODULE__{
  foo: integer(),
  bar: integer()
}

antedeguemon avatar Aug 30 '22 17:08 antedeguemon

@JerzyDziala Since we can't have a optional(:field) typespec for a structure, I am not sure what the typespec should be for the optional fields.

According to Elixir docs, having a key in a Struct type means the key is required:

| %SomeStruct{key: value_type} # struct with required key :key of value_type`

A naive idea is to use multiple definitions, but it may rendered too many permutations:

iex> quoted = Typespecs.quoted_message_typespec_v2(message_props)
iex> Macro.to_string(quoted)
%__MODULE__{foo: integer(), bar: integer()} | %__MODULE__{foo: integer()}

antedeguemon avatar Aug 30 '22 17:08 antedeguemon

Since we can't have a optional(:field) typespec for a structure, I am not sure what the typespec should be for the optional fields.

When an optional field is not set it will be set to the value nil, so the field will always be present on the struct. The fix to the typespec would be to add | nil to the field type:

%__MODULE__{
  foo: integer() | nil,
  bar: integer()
}

ericmj avatar Aug 31 '22 16:08 ericmj

@antedeguemon any chance you want to work on a PR for this? 🙃

whatyouhide avatar Sep 03 '22 07:09 whatyouhide

@whatyouhide sure thing! I will give it a try this week. Thank you for the support!

antedeguemon avatar Sep 06 '22 14:09 antedeguemon