node/bindnode: redesign the shape of unions in Go #223
Conversation
Before, the shape of IPLD schema Unions in Go was as follows:
struct {
Index int // 0..len(typ.Members)-1
Value interface{}
}
This worked perfectly fine when inferring Go types from a schema.
The inferred Go types would be anonymous,
so it didn't really matter that the value was behind interface{}.
However, this mechanism did not work for providing a custom Go type.
For example, the equivalent of the added example would be:
type CustomIntType int64
type StringOrInt struct {
Index int
Value interface{}
}
proto := bindnode.Prototype((*StringOrInt)(nil), schemaType)
bindnode failed to use CustomIntType at all,
since StringOrInt did not reference CustomIntType in any way.
Moreover, the interface{} layer also felt prone to type assert panics.
Now, the shape of Unions in Go is:
struct {
Type1 *Type1
Type2 *Type2
...
}
The problem described above is no longer present anymore,
as the added runnable example demonstrates.
That alone makes the redesign worthwhile.
One minor downside of the new method is large unions,
since looking up the "index" is now a linear search for the first
non-nil field pointer,
and the size of the Go type increases with each member type.
However, unions with more than a dozen member types should be rare.
A different design direction would have been to keep interface{} values,
and have "Register" APIs to tell bindnode about the member Go types.
However, that feels worse in terms of usability and design complexity.
Fixes #210.
|
This... looks essentially sensible. 👍 The one thing I'm trying to wrap my head around is what the error pathways look like when the golang structure for a union doesn't actually match the union membership in some way. E.g. if it has too few fields, or too many; or if a field's golang type doesn't actually have the appropriate internal structure for whatever we expect next according to the schema. Most of these are going to result in panics fairly late in the game, right? I guess that's probably acceptable for the current level of productionization of |
|
That's right. This is what I alluded to earlier in #209 (comment). When this is finished, providing both a schema and a Go type will mean a "sanity check" to ensure that the shapes match properly. If that's happy, then reflect panics later on should be impossible (assuming we don't make mistakes in the implementation). And indeed, I skipped that for the sake of cutting corners towards a PoC :) It's one of the biggest TODOs in the code in terms of what's required to call this ready for production use. |
(see commit message)