Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
mvdan opened this issue Jan 20, 2022 · 8 comments
Labels
P3 Low: Not priority right now

Comments

@mvdan
Copy link
Contributor

mvdan commented Jan 20, 2022

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
Copy link
Contributor Author

mvdan commented Jan 20, 2022

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Contributor Author

mvdan commented Jan 20, 2022

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 #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
Copy link
Contributor Author

mvdan commented Jan 20, 2022

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
Copy link
Contributor Author

mvdan commented Mar 22, 2022

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
Copy link
Member

rvagg commented Apr 4, 2022

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
Copy link
Contributor Author

mvdan commented Apr 4, 2022

@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.

@BigLep BigLep added the P3 Low: Not priority right now label May 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 Low: Not priority right now
Projects
None yet
Development

No branches or pull requests

4 participants