node/bindnode: allow nilable types for IPLD optional/nullable #401
Conversation
|
@masih @hannahhoward (and Rod) please test this in projects like graphsync and storetheindex, since I believe those use optional/nullable with bindnode and had run into the limitation with pointers. This should work, but it's best to double check there are no known regressions before it's merged. |
For simplicity, bindnode used to always require a pointer to represent optional or nullable IPLD types. This is because we need an extra bit to store whether the value is absent or null. However, some Go types can already store a "nil" value to represent that bit without needing that extra pointer. Most notably: []T versus *[]T map[K]V versus *map[K]V datamodel.Node versus *datamodel.Node Avoiding the extra pointer makes the types easier for humans to deal with, and it also avoids a potential footgun due to the extra "nil" state that bindnode doesn't actually use. Note that we still require pointers for "optional nullable" struct fields, as those need two extra bits. A TODO is left for that edge case. Fixes #378.
| qt.Assert(t, err, qt.Not(qt.IsNil)) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func nilable(kind reflect.Kind) bool { |
There was a problem hiding this comment.
do you need this, or can you look at the second return of ptrOrNilable?
There was a problem hiding this comment.
The funcs are unexported so they cannot be shared between the test and non-test packages - that's how I arrived at the smaller copy. Any other alternative is more lines of code AFAICT.
| if goType.Kind() != reflect.Ptr { | ||
| doPanic("union members must be pointers") | ||
| if ptr, nilable := ptrOrNilable(goType.Kind()); !nilable { | ||
| doPanic("union members must be nilable") |
There was a problem hiding this comment.
I don't think this is the ideal error here - it's talking schema language about a Go struct, maybe that's the best we have? but istm that this would be better stated as "must be pointers or interfaces".
Not blocking, just noting that it's weird encountering this error in the test when it's refering to the Go type.
(see commit message)
Fixes #378.