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.

Sign up for GitHub

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

Jump to bottom

Bindnode fixes continued #233

Merged
merged 8 commits into from
Aug 24, 2021
Merged

Bindnode fixes continued #233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ac67dd3
bindnode: finish adding details to many ErrWrongKind sites.
warpfork Aug 20, 2021
9f6c999
bindnode: iterating representation level of maps must continue to yie…
warpfork Aug 20, 2021
a5c9d6e
bindnode: critical that union member setting happens before finisher …
warpfork Aug 20, 2021
9d26d28
bindnode: mark todo about fragility around unions.
warpfork Aug 20, 2021
575b958
bindnode: iterating representation level of lists must continue to yi…
warpfork Aug 20, 2021
60ca8d9
facades: UnmarshalStreaming should stop massaging data after an error.
warpfork Aug 20, 2021
5dcbc68
bindnode: asking a kinded union representation what its kind is shoul…
warpfork Aug 20, 2021
eba8e56
bindnode: don't go questing for a representation node just to switch …
warpfork Aug 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions codecHelpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,9 @@ func UnmarshalStreaming(r io.Reader, decFn Decoder, bind interface{}, typ schema
// Decode is fairly straightforward.
np := bindnode.Prototype(bind, typ)
n, err := DecodeStreamingUsingPrototype(r, decFn, np.Representation())
if err != nil {
return nil, err
}
// ... but our approach above allocated new memory, and we have to copy it back out.
// In the future, the bindnode API could be improved to make this easier.
if !reflect.ValueOf(bind).IsNil() {
Expand Down
3 changes: 3 additions & 0 deletions datamodel/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ type ErrWrongKind struct {
// In the case of typed nodes, this will typically refer to the 'natural'
// data-model kind for such a type (e.g., structs will say 'map' here).
ActualKind Kind

// TODO: it may be desirable for this error to be able to describe the schema typekind, too, if applicable.
// Of course this presents certain package import graph problems. Solution to this that maximizes user usability is unclear.
}

func (e ErrWrongKind) Error() string {
Expand Down
91 changes: 52 additions & 39 deletions node/bindnode/node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,9 +174,10 @@ func (w *_node) LookupByString(key string) (datamodel.Node, error) {
return node, nil
}
return nil, datamodel.ErrWrongKind{
TypeName: w.schemaType.Name().String(),
MethodName: "LookupByString",
// TODO
TypeName: w.schemaType.Name().String(),
MethodName: "LookupByString",
AppropriateKind: datamodel.KindSet_JustMap,
ActualKind: w.Kind(),
}
}

Expand Down Expand Up @@ -218,9 +219,10 @@ func (w *_node) LookupByIndex(idx int64) (datamodel.Node, error) {
return &_node{schemaType: typ.ValueType(), val: val}, nil
}
return nil, datamodel.ErrWrongKind{
TypeName: w.schemaType.Name().String(),
MethodName: "LookupByIndex",
// TODO
TypeName: w.schemaType.Name().String(),
MethodName: "LookupByIndex",
AppropriateKind: datamodel.KindSet_JustList,
ActualKind: w.Kind(),
}
}

Expand All @@ -236,9 +238,10 @@ func (w *_node) LookupBySegment(seg datamodel.PathSegment) (datamodel.Node, erro
return w.LookupByIndex(idx)
}
return nil, datamodel.ErrWrongKind{
TypeName: w.schemaType.Name().String(),
MethodName: "LookupBySegment",
// TODO
TypeName: w.schemaType.Name().String(),
MethodName: "LookupBySegment",
AppropriateKind: datamodel.KindSet_Recursive,
ActualKind: w.Kind(),
}
}

Expand All @@ -258,9 +261,10 @@ func (w *_node) LookupByNode(key datamodel.Node) (datamodel.Node, error) {
return w.LookupByIndex(i)
}
return nil, datamodel.ErrWrongKind{
TypeName: w.schemaType.Name().String(),
MethodName: "LookupByNode",
// TODO
TypeName: w.schemaType.Name().String(),
MethodName: "LookupByNode",
AppropriateKind: datamodel.KindSet_Recursive,
ActualKind: w.Kind(),
}
}

Expand Down Expand Up @@ -331,9 +335,10 @@ func (w *_node) IsNull() bool {
func (w *_node) AsBool() (bool, error) {
if w.Kind() != datamodel.Kind_Bool {
return false, datamodel.ErrWrongKind{
TypeName: w.schemaType.Name().String(),
MethodName: "AsBool",
// TODO
TypeName: w.schemaType.Name().String(),
MethodName: "AsBool",
AppropriateKind: datamodel.KindSet_JustBool,
ActualKind: w.Kind(),
}
}
return w.val.Bool(), nil
Expand All @@ -342,9 +347,10 @@ func (w *_node) AsBool() (bool, error) {
func (w *_node) AsInt() (int64, error) {
if w.Kind() != datamodel.Kind_Int {
return 0, datamodel.ErrWrongKind{
TypeName: w.schemaType.Name().String(),
MethodName: "AsInt",
// TODO
TypeName: w.schemaType.Name().String(),
MethodName: "AsInt",
AppropriateKind: datamodel.KindSet_JustInt,
ActualKind: w.Kind(),
}
}
return w.val.Int(), nil
Expand All @@ -353,9 +359,10 @@ func (w *_node) AsInt() (int64, error) {
func (w *_node) AsFloat() (float64, error) {
if w.Kind() != datamodel.Kind_Float {
return 0, datamodel.ErrWrongKind{
TypeName: w.schemaType.Name().String(),
MethodName: "AsFloat",
// TODO
TypeName: w.schemaType.Name().String(),
MethodName: "AsFloat",
AppropriateKind: datamodel.KindSet_JustFloat,
ActualKind: w.Kind(),
}
}
return w.val.Float(), nil
Expand All @@ -364,9 +371,10 @@ func (w *_node) AsFloat() (float64, error) {
func (w *_node) AsString() (string, error) {
if w.Kind() != datamodel.Kind_String {
return "", datamodel.ErrWrongKind{
TypeName: w.schemaType.Name().String(),
MethodName: "AsString",
// TODO
TypeName: w.schemaType.Name().String(),
MethodName: "AsString",
AppropriateKind: datamodel.KindSet_JustString,
ActualKind: w.Kind(),
}
}
return w.val.String(), nil
Expand All @@ -375,9 +383,10 @@ func (w *_node) AsString() (string, error) {
func (w *_node) AsBytes() ([]byte, error) {
if w.Kind() != datamodel.Kind_Bytes {
return nil, datamodel.ErrWrongKind{
TypeName: w.schemaType.Name().String(),
MethodName: "AsBytes",
// TODO
TypeName: w.schemaType.Name().String(),
MethodName: "AsBytes",
AppropriateKind: datamodel.KindSet_JustBytes,
ActualKind: w.Kind(),
}
}
return w.val.Bytes(), nil
Expand All @@ -386,9 +395,10 @@ func (w *_node) AsBytes() ([]byte, error) {
func (w *_node) AsLink() (datamodel.Link, error) {
if w.Kind() != datamodel.Kind_Link {
return nil, datamodel.ErrWrongKind{
TypeName: w.schemaType.Name().String(),
MethodName: "AsLink",
// TODO
TypeName: w.schemaType.Name().String(),
MethodName: "AsLink",
AppropriateKind: datamodel.KindSet_JustLink,
ActualKind: w.Kind(),
}
}
link, _ := w.val.Interface().(datamodel.Link)
Expand Down Expand Up @@ -471,9 +481,10 @@ func (w *_assembler) BeginMap(sizeHint int64) (datamodel.MapAssembler, error) {
}, nil
}
return nil, datamodel.ErrWrongKind{
TypeName: w.schemaType.Name().String(),
MethodName: "BeginMap",
// TODO
TypeName: w.schemaType.Name().String(),
MethodName: "BeginMap",
AppropriateKind: datamodel.KindSet_JustMap,
ActualKind: w.kind(),
}
}

Expand All @@ -488,9 +499,10 @@ func (w *_assembler) BeginList(sizeHint int64) (datamodel.ListAssembler, error)
}, nil
}
return nil, datamodel.ErrWrongKind{
TypeName: w.schemaType.Name().String(),
MethodName: "BeginList",
// TODO
TypeName: w.schemaType.Name().String(),
MethodName: "BeginList",
AppropriateKind: datamodel.KindSet_JustList,
ActualKind: w.kind(),
}
}

Expand Down Expand Up @@ -600,9 +612,10 @@ func (w *_assembler) AssignLink(link datamodel.Link) error {
newVal := reflect.ValueOf(link)
if !newVal.Type().AssignableTo(w.val.Type()) {
return datamodel.ErrWrongKind{
TypeName: w.schemaType.Name().String(),
MethodName: "AssignLink",
// TODO
TypeName: w.schemaType.Name().String(),
MethodName: "AssignLink",
AppropriateKind: datamodel.KindSet_JustLink,
ActualKind: w.kind(),
}
}
w.nonPtrVal().Set(newVal)
Expand Down
80 changes: 61 additions & 19 deletions node/bindnode/repr.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,8 @@ func (w *_nodeRepr) Kind() datamodel.Kind {
return datamodel.Kind_Map
case schema.UnionRepresentation_Kinded:
haveIdx, _ := unionMember(w.val)
mtyp := w.schemaType.(*schema.TypeUnion).Members()[haveIdx]
return mtyp.TypeKind().ActsLike()
mtyp := w.schemaType.(*schema.TypeUnion).Members()[haveIdx] // TODO this is fragile: panics with index-out-of-range if the user has nil'd all fields.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

put a , ok and an error for out-of-range rather than panic?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nontrivial. This method can't return errors. It's a core interface method, and a break-the-universe-badly one if we changed its return type.

return mtyp.RepresentationBehavior()
case schema.UnionRepresentation_Stringprefix:
return datamodel.Kind_String
case nil:
Expand Down Expand Up @@ -183,9 +183,10 @@ func (w *_nodeRepr) LookupBySegment(seg datamodel.PathSegment) (datamodel.Node,
return w.LookupByIndex(idx)
}
return nil, datamodel.ErrWrongKind{
TypeName: w.schemaType.Name().String(),
MethodName: "LookupBySegment",
// TODO
TypeName: w.schemaType.Name().String(),
MethodName: "LookupBySegment",
AppropriateKind: datamodel.KindSet_Recursive,
ActualKind: w.Kind(),
}
}

Expand All @@ -205,9 +206,10 @@ func (w *_nodeRepr) LookupByNode(key datamodel.Node) (datamodel.Node, error) {
return w.LookupByIndex(i)
}
return nil, datamodel.ErrWrongKind{
TypeName: w.schemaType.Name().String(),
MethodName: "LookupByNode",
// TODO
TypeName: w.schemaType.Name().String(),
MethodName: "LookupByNode",
AppropriateKind: datamodel.KindSet_Recursive,
ActualKind: w.Kind(),
}
}

Expand All @@ -224,19 +226,57 @@ func (w *_nodeRepr) MapIterator() datamodel.MapIterator {
itr := (*_node)(w).MapIterator().(*_unionIterator)
return (*_unionIteratorRepr)(itr)
case nil:
return (*_node)(w).MapIterator()
switch st := (w.schemaType).(type) {
case *schema.TypeMap:
return &_mapIteratorRepr{
schemaType: st,
keysVal: w.val.FieldByName("Keys"),
valuesVal: w.val.FieldByName("Values"),
}
default:
panic(fmt.Sprintf("TODO: mapitr.repr for typekind %s", w.schemaType.TypeKind()))
}
default:
panic(fmt.Sprintf("TODO: %T", stg))
}
}

type _mapIteratorRepr _mapIterator
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment to explain why this wrapping and not just calling w's MapIterator is needed would be helpful


func (w *_mapIteratorRepr) Next() (key, value datamodel.Node, _ error) {
k, v, err := (*_mapIterator)(w).Next()
if err != nil {
return nil, nil, err
}
return k.(schema.TypedNode).Representation(), v.(schema.TypedNode).Representation(), nil
}

func (w *_mapIteratorRepr) Done() bool {
return w.nextIndex >= w.keysVal.Len()
}

func (w *_nodeRepr) ListIterator() datamodel.ListIterator {
if reprStrategy(w.schemaType) == nil {
return (*_node)(w).ListIterator()
return (*_listIteratorRepr)((*_node)(w).ListIterator().(*_listIterator))
}
// TODO this is probably failing silently for structs with tuple representation.
return nil
}

type _listIteratorRepr _listIterator

func (w *_listIteratorRepr) Next() (index int64, value datamodel.Node, _ error) {
idx, v, err := (*_listIterator)(w).Next()
if err != nil {
return idx, nil, err
}
return idx, v.(schema.TypedNode).Representation(), nil
}

func (w *_listIteratorRepr) Done() bool {
return w.nextIndex >= w.val.Len()
}

func (w *_nodeRepr) lengthMinusTrailingAbsents() int64 {
fields := w.schemaType.(*schema.TypeStruct).Fields()
for i := len(fields) - 1; i >= 0; i-- {
Expand Down Expand Up @@ -291,7 +331,7 @@ func (w *_nodeRepr) AsBool() (bool, error) {
TypeName: w.schemaType.Name().String(),
MethodName: "AsBool",
AppropriateKind: datamodel.KindSet_JustBool,
// TODO
ActualKind: w.Kind(),
}
}

Expand All @@ -303,7 +343,7 @@ func (w *_nodeRepr) AsInt() (int64, error) {
TypeName: w.schemaType.Name().String(),
MethodName: "AsInt",
AppropriateKind: datamodel.KindSet_JustInt,
// TODO
ActualKind: w.Kind(),
}
}

Expand All @@ -315,7 +355,7 @@ func (w *_nodeRepr) AsFloat() (float64, error) {
TypeName: w.schemaType.Name().String(),
MethodName: "AsFloat",
AppropriateKind: datamodel.KindSet_JustFloat,
// TODO
ActualKind: w.Kind(),
}
}

Expand Down Expand Up @@ -354,13 +394,15 @@ func (w *_nodeRepr) AsString() (string, error) {
name := stg.GetDiscriminant(mtyp)
return name + stg.GetDelim() + s, nil
case schema.UnionRepresentation_Kinded:
w = w.asKinded(stg, datamodel.Kind_String)
// continues below
return w.asKinded(stg, datamodel.Kind_String).AsString()
case nil:
// continues below
default:
panic(fmt.Sprintf("TODO: %T", stg))
}
// Things that have reached here, should be only those things that have string kind naturally,
// and so using the same method as the type level is correct.
// TODO is this applicable for anything other than... strings?
return (*_node)(w).AsString()
}

Expand All @@ -372,7 +414,7 @@ func (w *_nodeRepr) AsBytes() ([]byte, error) {
TypeName: w.schemaType.Name().String(),
MethodName: "AsBytes",
AppropriateKind: datamodel.KindSet_JustBytes,
// TODO
ActualKind: w.Kind(),
}
}

Expand All @@ -384,7 +426,7 @@ func (w *_nodeRepr) AsLink() (datamodel.Link, error) {
TypeName: w.schemaType.Name().String(),
MethodName: "AsLink",
AppropriateKind: datamodel.KindSet_JustLink,
// TODO
ActualKind: w.Kind(),
}
}

Expand Down Expand Up @@ -439,12 +481,12 @@ func (w *_assemblerRepr) asKinded(stg schema.UnionRepresentation_Kinded, kind da

// Layer a new finish func on top, to set Index/Value.
w2.finish = func() error {
unionSetMember(w.val, idx, valPtr)
if w.finish != nil {
if err := w.finish(); err != nil {
return err
}
}
unionSetMember(w.val, idx, valPtr)
return nil
}
return &w2
Expand Down Expand Up @@ -589,12 +631,12 @@ func (w *_assemblerRepr) AssignString(s string) error {
w2.val = valPtr.Elem()
w2.schemaType = member
w2.finish = func() error {
unionSetMember(w.val, idx, valPtr)
if w.finish != nil {
if err := w.finish(); err != nil {
return err
}
}
unionSetMember(w.val, idx, valPtr)
return nil
}
return w2.AssignString(remainder)
Expand Down
Loading