ipld / go-ipld-prime

Golang interfaces for the IPLD Data Model, with core Codecs included, IPLD Schemas support, and some handy functional transforms tools.
MIT License
132 stars 50 forks source link

node/bindnode: consider adding generic helpers once we can assume Go 1.18 or later #340

Open mvdan opened 2 years ago

mvdan commented 2 years ago

For instance, we implement ordered maps in the form of:

struct {
    Keys   []K
    Values map[K]V
}

These containers are precisely what generics is good at. For instance, here's how a type OrderedMap[K comparable, V any] could work: https://gotipplay.golang.org/p/bJ1kRHFNWyR

Another two good candidates are optional and nullable types; they are currently represented as a pointer on the Go side, but pointers can cause allocations and memory locality issues, and inferring the schema from a pointer can't know whether it means optional or nullable. So we could for instance use:

type Optional[T any] struct {
    Present bool
    Value   T
}
type Nullable[T any] struct {
    NonNull bool
    Value   T
}

All in all, one can imagine a future where declaring bindnode types is fairly pleasant:

type Person struct {
    Name       string
    Job        bindnode.Optional[JobInfo]
    Attributes bindnode.OrderedMap[string, string]
}
mvdan commented 2 years ago

We could also consider hiding the fields in those generic types, and only exposing the data via methods. This would allow us to prevent misuse (e.g. using Optional.Value without checking Optional.Present) and it would also allow us to change the internal representation of the type without breaking our users (e.g. a more memory-efficient version of OrderedMap).

warpfork commented 2 years ago

This would be really exciting. The current level of exposure that the user of bindnode has to the details of our hacks to persist map ordering has been a sizable wart, and was previously more or less unavoidable. These new possibilities are awesome!

warpfork commented 2 years ago

I wonder if we might want to hoist those Optional and Nullable wrappers somewhere broader.

For example, the gogen package also emits an awful lot of boilerplate "MaybeT" types right now.

Then again, I don't immediately know if replicating types of those purpose would make any difference to users; maybe it's fine to do so if it reduces coupling of implementations to a useful degree.

mvdan commented 2 years ago

Worth pointing out that it's also possible upstream Go will define an "optional" or "maybe" type of some sort. Some standard library packages like encoding/json and databasel/sql already have their own separate solutions to the same problem. So my intuition is to not try to generalize between bindnode and codegen initially, because a year or two down the line we might have a standard generic type or interface we could adhere to.

The current level of exposure that the user of bindnode has to the details of our hacks to persist map ordering has been a sizable wart

Indeed. In my defense, when I designed how we deal with maps, it was always the plan to turn that quasi-generic type to a proper generic type. If we were sure we'd never get generics, I might have gone for a different approach.

Another candidate for generics would be typed links, as per https://github.com/ipld/go-ipld-prime/issues/272:

type Target struct{...}

type TypedLink[TargetType any, LinkType interface{datamodel.Link | cid.Cid}] struct {
    LinkType
    _type *TargetType
}

type Root struct {
    Target bindnode.TypedLink[Target, datamodel.Link]
}

Right now, you can only do Target datamodel.Link, but there's no way to tell bindnode that the link is typed with type Target.

mvdan commented 2 years ago

Bonus round: if we retrofit generics, by allowing users on 1.17 or older to manually construct these types like we do with map structs right now, we could add these helpers in the bindnode package behind a //go:build go1.18 build tag. They are container helpers, and the reflect code would be entirely unaffected, so the bindnode package would work the same on 1.17 and 1.18 - the only difference being that 1.17 users might need to write extra boilerplate.

mvdan commented 2 years ago

Follow-up thought: we should probably not allow chaining optional and nullable on the Go side, like Optional[Nullable[Foo]]. Beyond that perhaps not working, it's also pretty confusing in terms of which should go first. It would be a lot better to instead expose:

type OptionalNullable[T any] struct {
    Present bool
    NonNull bool
    Value   T
}
rvagg commented 2 years ago

OK, so those Optional and Nullable types look pretty useful even without generics, and I take it from your second last comment here @mvdan that you also think these are a legitimate retrofit case? i.e. if a user shows up with an optional field which itself is a struct with just Present and Value fields of the right types then we can just use those and skip the whole pointer dance if they want to swap one type of complexity for another.

mvdan commented 2 years ago

@rvagg that's correct. The generic helpers for Go 1.18+ would just be for UX; once instantiated, the bindnode APIs would still see the same "flat" types that one can type manually today in Go 1.17, just like we already do for the Keys+Values struct dance for maps.