cbor-gen icon indicating copy to clipboard operation
cbor-gen copied to clipboard

feat: basic generics handling

Open rvagg opened this issue 1 year ago • 6 comments

See https://github.com/filecoin-project/go-hamt-ipld/pull/122 for this in use and some output of the code here.

This is pretty basic, currently just targeting fields in structs. There's probably other variations that this is interesting for but this is all I need right now.

rvagg avatar Aug 05 '24 12:08 rvagg

My main concern here is multiple generic values. IMO, we should support them (although we can also just "fail cleanly" for now and do that later).

Stebalien avatar Aug 05 '24 15:08 Stebalien

Multiple generic parameters implemented, added tests for it with a nested struct and multiples. Also did some (more) minor whitespace adjusting that was annoying me in the generated output.

rvagg avatar Aug 06 '24 10:08 rvagg

See the bottom of testing/cbor_gen.go for how it comes out.

rvagg avatar Aug 06 '24 10:08 rvagg

Used this to re-generate cbor_gen.go in https://github.com/filecoin-project/go-hamt-ipld/pull/122

Also updated HamtValue there to be:

type HamtValue[V any] interface {
	cbg.CBORSerializer[V]
	Equals(V) bool
}

rvagg avatar Aug 06 '24 10:08 rvagg

I've been back and forth on various interfaces and options here but it's all terrible. So the best I have is exposing these interfaces:

// CBORGeneric is a base interface for CBOR serializers that can be used as a generic type T such
// that a serialization and deserialization methods will be called on any field of type T.
//
// Nullable fields are not handled by the cbor-gen generated code for generic types but should be
// handled by the ToCBOR and FromCBOR methods of the type.
type CBORGeneric[V any] interface {
	// ToCBOR serializes the value to the given writer. ToCBOR must properly handle nil cases: where
	// the value is nullable, ToCBOR should write a CBOR null value; where the value is not nullable,
	// it should return an error if the receiver is nil.
	ToCBOR(w io.Writer) error
	// FromCBOR deserializes the value from the given reader and returns an instance of the value type
	// or a copy of the called value which is modified by the deserialization process.
	// FromCBOR must properly handle CBOR null values if this value is nullable.
	FromCBOR(r io.Reader) (V, error)
}

// CBORSerializer is an interface for types that can be serialized and deserialized using CBOR.
// Types generated by cbor-gen will conform to this interface.
type CBORSerializer interface {
	MarshalCBOR(w io.Writer) error
	UnmarshalCBOR(r io.Reader) error
}

Then I leave it up to the caller to deal with nullable and pointers. We just always assume we can do ToCBOR and FromCBOR.

We also export some utilities to make it slightly easier to deal with nullable values so you don't have to get dirty with CBOR null details.

// NullableMarshalCBOR can be used as a wrapper around a CBORSerializer that doesn't support
// handling of null values itself. If the value is nil, it will write a CBOR null value, otherwise
// it will call the MarshalCBOR method on the serializer.
func NullableMarshalCBOR(w io.Writer, s CBORSerializer) error

// NullableUnmarshalCBOR can be used as a wrapper around a CBORSerializer that doesn't support
// handling of null values itself. If the reader contains a CBOR null value, it will not call the
// UnmarshalCBOR method on the serializer, otherwise it will call it.
func NullableUnmarshalCBOR(r io.Reader, s CBORSerializer) error

Unfortunately they're still not pretty to use, but the pattern is straightforward. NullableCborByteArray in the tests does this to make the bytes nullable while using a standard cbor-gen typed []byte serializer.

In the tests we also have a non-pointer CborInt that has standard pointer receiver UnmarshalCBOR / MarshalCBOR methods but we have non-pointer receiver FromCBOR and ToCBOR methods so it can never be a pointer in the struct field and won't allow serialization or deserialization involving nulls at all.

rvagg avatar Aug 07 '24 07:08 rvagg

Happily, this works quite nicely for non-pointer values here: https://github.com/filecoin-project/go-hamt-ipld/pull/122

We test with *CborByteArray and a non-pointer CborInt, and in that case we don't have to pass or receive anything by reference so it gets a little bit nicer.

rvagg avatar Aug 07 '24 07:08 rvagg

Going to bail on this for now. Although it works fine, and delivers a performance gain (~10% in the main high-volume situation I was testing), the API is a bit too clunky to extend what's here already. Maybe it'd be nicer in a new library that was designed with generics from the start? Maybe it'd be nicer if Go had a nice way of restricting generics to pointers?

rvagg avatar Oct 08 '24 05:10 rvagg