node/bindnode: allow nilable types instead of a pointer for optional/nullable schemas #378
Comments
|
Thinking aloud: I wonder if |
The implementation expected map value types in Go to be a pointer when the IPLD schema has the value as nullable. Unfortunately, we forgot to enforce that in verifyCompatibility, so the user could run into weird panics: panic: interface conversion: basicnode.plainList is not datamodel.Node: missing method Length The added test case in TestPrototypePointerCombinations reproduces the panic, but is commented out, because we're fixing the check logic to instead have bindnode panic upfront with a helpful message. Note that I understand why the user tried to avoid the pointer to Node. Since Node is an interface, it is already nilable, so bindnode can store a nil value in it directly without the need for a pointer. We do not support that right now, but soon will; #378 now tracks that. Fixes #377.
|
@rvagg that seems like a question for the schema language more than for bindnode :) My interpretation of That said, bindnode still deals with |
|
@mvdan : are you going to be able to finish landing this before leaving? |
|
@BigLep I have a work-in-progress patch that I intend to send by the end of the week; it might not land this week, but since this is open source, I'm not too worried about it leaking into next week. |
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.
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.
Earlier versions of bindnode required pointers in order to represent optional/nullable fields even if the go type was nillable. Now that bindnode allows nillable types to represent optional/nulalble fields, remove pointer to interfaces and simply use the interface type straight up. Relates to: - ipld/go-ipld-prime#378
Earlier versions of bindnode required pointers in order to represent optional/nullable fields even if the go type was nillable. Now that bindnode allows nillable types to represent optional/nulalble fields, remove pointer to interfaces and simply use the interface type straight up. Relates to: - ipld/go-ipld-prime#378
For example, right now, to bind an IPLD
struct { Foo optional Any }, one must use a pointer for the field type, likestruct { Foo *datamodel.Node }.However, in this case and some others, the Go type
datamodel.Nodealready has anilstate to signal "absent", so we could reuse that instead of adding our own via a pointer. Not only is that type easier for the user to read and write, but it's also less prone to bugs, because a pointer to an interface has two nil states: nil pointer, and non-nil pointer to a nil interface.This would also apply to IPLD schema's
nullable, which similarly requires a pointer now.This would have helped with #371, and also with the new #377.
What to do with
optional nullablefields remains a bit of a TODO, because right now those use two chained pointers, which is a bit weird. We might want to rethink that with generics at some point, to end up with a wrapper likeOptionalNullablethat keeps two boolean fields, akin to #340.The text was updated successfully, but these errors were encountered: