fix(bindnode): tuple struct iterator should handle absent fields properly #368
Conversation
| @@ -320,7 +322,7 @@ _skipAbsent: | |||
| if err != nil { | |||
| return 0, nil, err | |||
| } | |||
| if value.IsAbsent() { | |||
| if w.nextIndex > w.reprEnd { | |||
There was a problem hiding this comment.
Is it possible to be in a situation where value.IsAbsent() and we're not at the end amongst trailing absents? e.g. [absent,present,present,absent,absent]? Is there a use of this iterator where that can happen, perhaps by constructing this from the typed form and then list iterating on the repr form?
There was a problem hiding this comment.
Absents can only be at the end, as per the undocumented spec :) See
.Arguably this needs to be properly documented in the specs, which is why I'll raise an issue elsewhere. But in short: we should support trailingn optionals in bindnode, to at least match codegen in terms of features.
|
btw, I could go either way on the tuple optionals, I've always thought that it was reasonable to allow trailing optionals but it does complicate matters somewhat for marginal gain and increased ambiguity. Maybe we should rip that support out (for now, or for good?) |
| @@ -932,6 +930,7 @@ func (w *_listStructAssemblerRepr) AssembleValue() datamodel.NodeAssembler { | |||
| }} | |||
| } | |||
| field := fields[w.nextIndex] | |||
| w.doneFields[w.nextIndex] = true | |||
| @@ -320,7 +322,7 @@ _skipAbsent: | |||
| if err != nil { | |||
| return 0, nil, err | |||
| } | |||
| if value.IsAbsent() { | |||
| if w.nextIndex > w.reprEnd { | |||
There was a problem hiding this comment.
Absents can only be at the end, as per the undocumented spec :) See
.Arguably this needs to be properly documented in the specs, which is why I'll raise an issue elsewhere. But in short: we should support trailingn optionals in bindnode, to at least match codegen in terms of features.
|
TL;DR fine to merge as is, looks great! |
Fixes: #367