add: custom types

[fdymylja]

closes #2

[aaronc]

Not sure we need to make it this complex. On one hand this allows caching of the protoreflect.Value + custom type, but feels like it mixes too many concerns. Can you say a little bit more about the use case for messages and Mutable?

My thinking was that we only handle string or bytes types for now because those are the only cases we have with a very simple two method Unmarshal/Marshal interface, and that a scalar can only work with one proto type (string or bytes)

[fdymylja]

We can see it in this way:
Do we want a custom type to be capable of overriding a message (ex we want to override anypb.Any with pulsar.Any) ?
If this is the intention then this is the only API which does not require multiple implementations of custom type logic that can serve all purposes.
If not then we can simplify by removing unmarshal/marshal/size inputs and Mutable

[aaronc]

So as we discussed I don't think we want to extend this for full Message type overriding (for Any's we can just replace the type name).

For simple scalars, we could use something like I described in #2 for CustomStringType or CustomBytesType or even something as simple as:

type CustomType interface {
  Set(value protoreflect.Value) error
  Get() protoreflect.Value
}

[fdymylja]

doing one custom type for each primitive is going to be a big chunk of work codegen wise (basically we need one extra check for each kind and for each method), this is why I was going for a more generalized interface. Plus custom types are going to be rare and I think they're an advanced level, so I suppose someone creating one is fully aware of protobuf semantics (like if i am customizing a string then i know bytes that are being passed are those of a string).

What if we simplified the current custom type interface to this:

type CustomType interface {
	UnmarshalBytes(b []byte) error
	MarshalBytes() ([]byte, error)
	Size() int
	Set(value protoreflect.Value)
	Get() protoreflect.Value
	Clear()
	IsSet() bool
}

[aaronc]

which approach from #2 are you thinking of going with? why do we need both Unmarshal/Marshal + Set/Get? I'm not clear as to why we couldn't deal everything with Set/Get? With Unmarshal/Marshal the type doesn't know if it was passed a string, bytes or int64 say so it's less type safe

[fdymylja]

Those methods are needed for Protoreflect.Clear/Has/Range, I need to signal to custom types that they need to be unset/cleared and I need to have a way to know if they're set or not.

Size is needed for Marshal to compute the actual size.

I wouldn't need those if custom types were simply aliases of primitives, but since we plan to substitute string with a struct (case of sdk.Int for example), we need those methods otherwise the code wouldn't know how to unset them.

Using nil is not viable (in case of fields substituted by structs) because they would break proto presence (if a type is not: optional, repeated, map or message then we cannot determine presence) - and also if we wanted to use nil then we'd need custom logic for struct custom types and for example non struct ones (Such as address being a string converted into []byte)

[aaronc]

right, but if we go with the second approach in #2 we don't need those.

I guess I'm thinking with whichever approach we take we do want the underlying proto value to get cached either by the proto message itself or the custom type

[fdymylja]

using the 1st approach is simpler to implement (from codegen perspective).

second approach would need more time because generating that logic is a lot more complex, and needs to be handled with a case by case basis in every method.

Also I think both obtain the same result, with the custom type implementation being a little harder to implement (in case 1) - but I think that's fine, I don't expect every dev to want to implement its own custom type.

[fdymylja]

Also another reason I'd go with 1) is because in the sdk that's how we deal with custom types currently. I think the API is more dev friendly and less breaking.

[aaronc]

do you think it's reasonable to expect custom types to cache the protoreflect.Value?

second approach would need more time because generating that logic is a lot more complex, and needs to be handled with a case by case basis in every method.

why is this the case? it just adds two fields plus a getter/setter pair without impacting any of the other marshaling/protoreflect code