GitLab

add int, bytes, and link types to code-gen

fill in generated type for list

Fill in generated types for int, link, and bytes nodes

feat(selectors): support union limit type

Refactors the limit data on recursive selectors to be a union type

switch maxDepth on ExploreRecursive to union limit type per spec

Child-builder recursion; and typed unmarshal over codegen native structs.

(At long last!  These child-builder methods are a step that was known
to be needed for some time now, but I've intentionally been pushing off
the implementation of it until we finally reached some *other* piece of
functionality that would depend on it and force the issue.  Finally,
that's happened: that issue is unmarshalling into natively-typed
codegenerated nodes, and that's now.  Woo!)

(Though the current focus of this is for natively-typed codegen'd nodes,
if we put further effort into reflective-bind node implementations,
they'll also lean heavily on these child-builder-getters for internally
tracking the correct `reflect.Type` to create new values of -- very
similar to what our schema-typed things are doing, just with the Golang
type system instead of our schema types.)

The child-builder getter methods more or less explain themselves.
They yield a new NodeBuilder that's correctly specialized for child
nodes in recursive structures.  This is necessary for both keys and
values in maps, and for values in lists.  For values in lists and maps,
getting the child-builder requires takes a parameter -- this is so
typed nodes (namely, structs; but some unions will also lean on this)
may have discriminated contents that differ per key or index.

The BuilderForValue functions are able to take primitive strings and
ints as parameters rather than Node, because all uses that have
discriminated results are places where complex keys are not acceptable.
Therefore primitives are fine, and preferred since it avoids boxing.

The 'free' node implementation is updated to include this feature,
as are codegenerated nodes.
(The runtime wrapper typed node implementations have partial support,
but stubbed methods where the path forward is clear but the
implementation effort nontrivial and not currently on-priority-path.)

Unmarshalling now uses these child-builder-getters to correctly handle
the passing down of constraints and typeinfo.  This addresses several
TODO's that have been in the unmarshalling code for aeons -- hooray!
(Somewhat remarkably, our solution here is much better than the one
proposed in those ancient TODO's -- we actually got this done *without*
direct abstraction-breaking examination for typed nodes.  Nice!)

Unmarshalling tests are also in this same diff.
There's even a wee little benchmark using a snippet of JSON as fixture.
This is the first time we see unmarshalling and marshalling end-to-end
connected with codegen'd nodes.  This is very exciting!

There's still some questions to be answered about the most correct
(and most performance-friendly) place to put error handling against
asks for a child NodeBuilder with an invalid key or index, which is
a very real invalid state that's reachable for typed nodes that are
structs.  Comments about that are inline in the diff.  We'll probably
have to come back to that soon, but I'm gonna let it stew on the
back burner for a bit.  Currently, panics are used, but this may not
be acceptable ergonomics.

---

It's somewhat effortful to even detail how many thoughts about
performance and the future optimization potentials go into this diff.
Some comments I had in the area of the child-builder specialization
functions included concerns on:

- where common-subexpression-elimination will and won't apply
- where inlining is possible vs facing function call stack shuffle overhead
- ... as well as how that affects common-subexpression-elimination
- ... as well as how that affects escape analysis and shifts to heap
- whether an isDiscriminated property will make any of these things better
- can BuilderForValue hang onto a builder, boxed into interface already, when appropriate?
- ... for the maps case, should amortize in a way comparable to isDiscriminated, yes?  verify.
- ... can we do that without the builder keeping another two words of memory just... around?  i don't think so.  question is if it pays for itself.
- question: do zero value no-fields structs get a bonus?  reading runtime/iface.go -- doesn't look like it.  but needs confirmation.
- ... revised statement: they do, it's just nonobvious.  iface.go doesn't have a specialization, but the mallocgc call hits a special path internally for zero, and then the typedmemmove call 'coincidentally' hits its "src == dst" short-circuit, thus also being fast.

... all in all, as many of these as possible of these concerns have
been considered and this design should be optimization-friendly;
some of them are bucketed into "we'll see", because we need to build
nontrivial programs to see how significant the needs are, as well as
to see how clever the compiler can get with what we've already done.

There are some possible alternative ways we might re-engineer the
builders in the future to make them more performance-friendly.
In particular, the fact that many builders carry a previous value
with them in anticipation of one of the 'Amend' calls -- which is
not necessarily going to be used! -- is a potential obstruction to
optimization, since it makes many structures go from zero to
more-than-zero size, and that hits significantly different paths
in the runtime with very different performance characteristics.
However, that is a bit much to change today; so the thought lives
here as a note for potential future work.

Adding a `BuilderForValueIsDiscriminated() bool` method later seems
still seems on the table, but since it wouldn't require substantial
restructurings or headaches, can be deferred until benchmarks show
us exactly how much it matters.
(A single `type ChildBuilders interface { Style() Complexity; ... }`
with some enum for `Compleixty` resembling
`SimpleList | SimpleMap | DiscriminatedMap | DiscriminatedList` might
also be a tempting way to go about this.)

---

An alternative design considered that deserves a quick note: we
could've added a `ChildBuilders()` method to the MapBuilder and
ListBuilder, and attached the further getter methods there.
This might still be viable, if we found that there are more methods
involved in the future and we want to group them: it *should* always
land on the zero-fields struct path, which means it can be done
without hitting an expensive malloc.  However, at present this doesn't
seem warranted: we don't have enough methods to make it feel important
(especially after the recognition that Node variants of methods aren't
needed in addition to the primitive args variants, described above).

---

Whew.

Lines of code in diff does not correlate to hours of thought required.

Glad to have this landing.  Even performance minutae aside,
there was a significant period of time where I was terrified that all
these abstractions were going to shatter magnificently when it came
time to do the final end-to-end unification of all this massive arc
of work.  But now it's done.  And it works.  Whew.

Next: more fleshing out of more types; perhaps more benchmarks; and
also it's time to do some more drafts of the native accessors and
builders.  Some of that already has been done in parallel in gists
while this diff was incubating, but it's time to commit it too.

Noticed due to the previous commit adding this need to the listbuilder;
but the maps have been quietly wrong for a while now.  Eep.
Signed-off-by: Eric Myhre <hash@exultant.us>

Lacking these has been an oversight.

At the data model level, appending basically can't go wrong.
As a result, we got away this this oversight so far.

The grace period is over, now.

At the schema level, this is not true -- type constraints can certainly
cause some modification to end in errors!  For lists, inconsistent
types can cause errors; for struct-with-tuple-representation, we can
encounter length limits and even distinct constraints per index!
Signed-off-by: Eric Myhre <hash@exultant.us>

Codegen continues

All flies right, yey

We also get to shorten up the asserts drastically in this set,
because we can just flat out assert that the result equals exactly
the value that we got from the type-level builder earlier.
(And go-cmp output from go-wish makes this very, very easy.)
Signed-off-by: Eric Myhre <hash@exultant.us>

I'm not sure if I like these symbol names.  Or rather, I don't,
but haven't decided on what would be preferable yet.

There's a couple of options that have come to mind:

- option 1
  - `func {{ .Type.Name }}__NodeBuilder() ipld.NodeBuilder`
  - `func {{ .Type.Name }}__ReprBuilder() ipld.NodeBuilder`

- option 2
  - `func NewBuilderFor{{ .Type.Name }}() ipld.NodeBuilder`
  - `func NewReprBuilderFor{{ .Type.Name }}() ipld.NodeBuilder`

- option 3
  - `func (Builders) {{ .Type.Name }}() ipld.NodeBuilder`
  - `func (ReprBuilders) {{ .Type.Name }}() ipld.NodeBuilder`

Option 3 would make 'Builders' and 'ReprBuilders' effectively reserved
as type names if you're using codegen.  Schemas using them could use
adjunct config specific to golang to rename things out of conflict in
the generated code, but it's still a potential friction.

Option 2 would also have some naming collision hijinx to worry about,
on further though.  Only Option 1 is immune, by virtue of using "__"
in combination with the schema rule that type names can't contain "__".

This diff is implementing Option 1.  I think I'm partial to Option 3,
but not quite confident enough in it to lunge for it yet.

Putting more methods on the *concrete* types would also be another
interesting fourth option!  These methods would ignore the actual
value, and typically be used on the zero value: e.g., usage would
resemble `Foo{}.ReprBuilder()`.
The upside of this would be we'd then have no package scoped exported
symbols except exactly the set matching type names in the schema.
However, the opportunities for confusion with this would be numerous:
we couldn't use the 'NodeBuilder' method name (because that's the
potentially-stateful/COW one), but would still be returning a
NodeBuilder type?  Etc.  Might not be good.

More to think about here in the future.
Signed-off-by: Eric Myhre <hash@exultant.us>

Fixed at least one bug along the way (in iterators, which don't have
test coverage yet, so no test fix.  Still planning to cover those
via serialization, when we get that feature, "soon").

'go doc .' on the generated code now only lists one type per type in
the schema which seems like a good sanity heuristic; and
'go doc -u .' on the package now looks much more consistent.
(There's *8* types for every struct in the schema!  Uffdah.
But if that's what it takes to make a focused,
correctness-emphasizing library surface area, so be it.)
Signed-off-by: Eric Myhre <hash@exultant.us>

I'm still aiming to keep this as simple and un-clever as possible,
because putting lipstick on a pig -- this is all about to become
strings which get shoveled back to a compiler parser anyway -- is
not useful, and becomes antiuseful if it obstructs readability...

But I'm starting to find these elements are repeated enough that
it will help rather than hurt readability to extract some things.

Also, since the munges have recently started to appear in both go code
source as well as in the templates, that starts to put more weight in
favor of extracting a function for it, which keeps the two syntactic
universes from drifting on this subject.

At the same time, moved all NodeBuilders to being unexported (by using
idents prefixed with a "_").  I looked at the godoc for the generated
code and felt this is looking like a wiser choice than exporting.

We'll need to export more methods for getting initial instances of the
now-unexported stuff... but we should be adding those anyway, so this
is not an argument against unexporting.

Some additional type idents around iterators and map builders have not
yet been hoisted to DRYed munge methods.  I'm debating if that's useful
(as you can see in the comments in that file), but leaning towards
it being more parsimoneous to just go for it.  So that'll probably be
the next commit.
Signed-off-by: Eric Myhre <hash@exultant.us>

And fixed tests for same.

The value returned is still ipld.Undef, and I'm not sure I'm going to
particularly defend that choice vs returning null, but it seems
six of one and half a dozen of the other.  Might be worth review later,
once we have some field reports on whether that vs nils would be
recieved as the more surprising/annoying choice in non-toy usage.

ipldfree.Node is now a much better implementation of Node.

In particular, this means it will work correctly when combined with the
typed.Node wrapper implementations which expect to be able to use
ErrNotExists for logic purposes.
Signed-off-by: Eric Myhre <hash@exultant.us>

Add LookupSegment to Node interface.

Fix traversal internals to use it (!) rather than converting segments
to strings, which was both wasteful, and in some cases, *wrong* (!)
(although by coincidence happened to mostly work at present because of
another thing from early-days code that was also technically wrong).

Fix ipldfree.Node to reject LookupString if used on a list node!
(This is the other "wrong" thing that made the traversal coincidentally
work.)

LookupSegment method generation also added to codegen.
Signed-off-by: Eric Myhre <hash@exultant.us>

Move selector.PathSegment up to ipld.PathSegment.

ipld.Path is now a slice of ipld.PathSegment instead of strings.

This should be an improvement in sanity: there are now several fewer
places importing "strconv", and that's just always a good thing.

We will also be free in the future to add PathSegment-based accessor
methods to ipld.Node, as has already been commented as a desire;
and, to use PathSegment in building better typed errors
(which is the specific thing that provokes this diff today and now).

The implementation of PathSegment is now also based on a struct-style
union rather than an interface style union.  There are comments about
this in the diff.  I do not wish to comment on how much time I've spent
looking at golang assembler and runtime internals while trying to find
a path to a more perfect compromise between ergonomics and performance.
tl;dr Selectors will probably get faster and trigger fewer allocations;
ipld.Path will probably take slightly more memory (another word per
path segment), but not enough to care about for any practical purposes.

I did not attempt to hoist the SegmentIterator features from the
selector package to anywhere more central.
It may be a fine idea to do so someday; I just don't presently have
a formed opinion and am not budgeting time to consider it today.
Signed-off-by: Eric Myhre <hash@exultant.us>

It does vary from the baselines somewhat.

There's a fixme in the codegen for struct-with-repr-map for where to
address the bug found in the test commit preceeding this one, but
solving it well depends on another change I've been planning in core,
so I'm probably going to park this branch soon and go do that work
on master; then come back to fix this here after that lands.
Signed-off-by: Eric Myhre <hash@exultant.us>

Mostly passes, but found one semantic bug.
Signed-off-by: Eric Myhre <hash@exultant.us>

Just getting all the indentation changes out of the way.
New tests coming in the next commit.

This resolves a *lot* of questions that were previously open.
(Progress will probably be faster after this.)

- It's now clear how GetRepresentationNodeGen works at all.
  Turns out it really does just return a nodeGenerator, and that
  works... really well.

- We've got the first example of a 'EmitTypedNodeMethodRepresentation'
  method which generates a switch statement, so that's under the belt.

- Let's not bury the lede: the entire suite of generation code for
  emitting an ipld.Node for the representation of a struct as a map,
  and emitting the entire corresponding ipld.NodeBuilder for building
  a struct out of map entries!  Includes validation for all required
  fields being set, the usual type checks, support for rename mappings,
  and also validation against repeated entries (this lattermost bit is
  a bit controversial, given that there may be other more efficient
  places to do this check, but it's in for now; and see next bullets).

- The solution to the "what if there are multiple possible
  representation implementations?" question is frankly to ignore it.
  I had to think about this a long (long, long) time; time to move on.
  Seealso the comments in the 'EmitNodebuilderMethodCreateMap' method
  on 'generateStructReprMapNb' -- in short, this problem is too big
  to tackle right now.  We also, mostly, *don't need to* -- the
  solution of "push it to the codec layer" can address the correctness
  concerns in all cases I can think of, and the rest is hedging on
  efficiency (for which we really need more complete implementations
  and thereafter *benchmarks* in order to be conclusive anyway).
  Endgame: the current course of action is to build things the way
  that will operate correctly for the widest range of inputs.

- (Note to the future, regarding that last bullet point: some of
  trickiest bits in this choice matrix around efficiency are where
  concerns would be mostly in the codec layer, but would get efficiency
  boosts from knowledge that's only available from the schema layer.
  But the future-planned feature of generating ultra-fastpath direct
  marshal and unmarshal functions with codec specialization will have
  enough information at hand to actually cut straight through all of
  those concerns!)

- Not appearing in this commit, but: expect a fairly huge writeup about
  all these map ordering choices to be coming up in an exploration
  report document in the ipld/specs repo soon.

The two commits coming before this one -- especially the "generality
of codegen helper mixins" one -- also were direct leadups for all this.

Several additional things remain todo:

- This all needs test coverage, and I haven't mustered that far yet.
  Coming in the next commit or so.  I won't be surprised if there's at
  least one bug in this area until those are done.  (I don't like
  committing without tests included, but the current tests probably
  need a small refactor in order to grow smoothly, and I'm not gonna
  try to heap that onto the current diff.  On the plus side: everything
  in the generated output typechecks so far, and that's quite a bit.)

- Support for "implicit" values is missing.  TODOs inline.  They'll
  interact with roughly the same parts of the code as optionals do.

- The representation gen for strings is, as you can see, a todo.
  (It's probably an "easy" one -- but also, it would be nice to get
  it to reuse as much code as possible, because afaict the
  representation node and the type-semantics node are almost identical,
  so that might turn out to be interesting.)

- Note that before we can rig unmarshall up to this and have it work
  recursively and completely, we'll need to address the known todo of
  nodebuilders-need-methods-to-propose-child-nodebuilders.  I've been
  putting that one off for a while, but I think we're coming up on
  when it's correct to get that one done -- just before adding any more
  generators or representations would be good timing.

- Several error paths are still using very stringy errors, and yearn to
  be refactored into typed error structures.  These are mostly the same
  ones as have already appeared in other recent commits; we have
  learned a few more things about which parts of the error message need
  to be flexible, though... so the time to tackle these will also be
  "soon".  (Probably right after we do some more testing work, so we
  can then immediately add tests for the unhappy paths right as we
  upgrade the errors to typed constructions.)

Some other organizational open questions:

- Note that for the type-level node and nodebuilders, we're using two
  separate files; and for the representation and its builder, I haven't
  done so (yet).  Would be good to move to one way or the other.
  Undecided which one is more readable vs shocking yet.

- The names of the types we're using inside the generation isn't very
  consistent right now either.  It's evolving towards consistency as we
  get more cases explored, and I think it's nearly at the mark now, but
  I haven't been proactively refactoring the older stuff yet.  Should;
  but since it'll be roughly sed levels of complexity, not a blocker.

Things that look like tempting todos, but probably aren't:

- It *looks* at first glance like there's a lot of duplicated code
  between the map representation of the struct and the struct itself.
  I'm fairly sure this is a red herring and should not be pursued:
  the places which are the same are many, it's true; but the places
  that are different are wormed in all over the place, and trying to
  extract the common features will likely result in templates which
  are completely unreadable.  This degree of almost-commonality is
  also probably going to be unique in the entire set of kinds and
  representation strategies that we'll deal with, making it further
  unworthy of special attempts at "simplification".  (The strings
  case mentioned above as a todo is different from this, because there,
  things are actually *identical*, not merely close (... I think!).)
  I could be wrong about this, but if so, it'll be better to revisit
  the question after several more kinds and representations get at
  least their first draft.

Whew.  Not sure what the hours-vs-sloc ratio is on this diff, but
it's *high*.  Also worth it: a lot of the future course of development
is set out in the implications of the choices touched on here, and as
much as I'd like to develop iteratively, past experience (on refmt
in particular) tells me some of these will not be easy to revisit.
Signed-off-by: Eric Myhre <hash@exultant.us>

Will need this in a moment for the body of the representation codegen
for structs-represented-as-maps.

This is not a particularly strongly consistency-checked method; if you
give it a struct field from another struct, it'll silently do a wrong
thing.  This is because checking for that would require back-pointers,
and I... don't wanna.  Can revisit this if it becomes problematic.
Signed-off-by: Eric Myhre <hash@exultant.us>

So far everything we've used these mixins for was generating based on
a *schema.Type*.  This isn't true anymore: representation nodes are
still nodes, so most of the same helpers are useful for the same
reasons and we want to reuse them... but then now they're not always
directly related to a particular reified schema.Type anymore.

Correspondingly, these helper templates were using the .Type.Kind
property as a shortcut to avoid more args, but that is wrong now:
for example, if we're going to generate the representation node for
a struct with a stringjoin representation, the closest thing we would
have to a schema.Type is the struct; but that kind clearly isn't right.
So, now that is another property passed internally to the embeddable
helpers (the helper-helpers, I guess?), and affixed by the helper
rather than provided by a schema.Type param (which, again, we no
longer have).

Note for more future work: this is the first time that a "TypeIdent"
property has shown up explicitly, but it's also *all over* in the
templates in a defacto way.  So far, my policy has been that extracting
consts from the templates isn't a priority until it proves it has a
reason to also be handled *outside* the immediate locality of the
templates (because if we pulled *everything* out the templates will
become a thin unreadable soup; and doing bulk sed-like edits to the
templates is fairly easy as long as they're consistent).  Now, that
criteria is probably satisfied, so more refactors on this are probably
due very soon.

And one more further sub-note on that note: I'm not actually sure if
some of these types should have a "_" prefix on them or not.
"$T_NodeBuilder" currently doesn't -- meaning it's exported -- and that
might not be desirable.  I started them that way, so for now I'm just
sticking with it, but it's a thing that deserves consideration.
(It might be nice for godoc readability if there's a clear hint that
the builders exist; but also, at present, there's no guarantee that
their zero values are at all usable, and that should be cause for
concern.  Other considerations may also exist; I haven't attepmted to
weigh them all yet.)
Signed-off-by: Eric Myhre <hash@exultant.us>

It always bamboozles me that ranging modifies the "dot" in go
templates.  Even if you're assigning the range yield into other values
explicitly: it still frobs dot.  Why.

In this case, it was also a silent error because both the template
input object and a type field both have ".Type.Name" properties.  Ow.
Signed-off-by: Eric Myhre <hash@exultant.us>

This is pretty much a lost cause, though.  If we're pretending these
methods can be called in any order, then getting an outcome that's
guaranteed to match what go-fmt will do is not possible without also
passing more context around than we are.

So, I'm just running on a loose heuristic of clustering similar things
together, and trying roughly to get a linebreak between sections that
have a bigger jump between them.  It's... eh.  Not gonna spend much
time on it.
Signed-off-by: Eric Myhre <hash@exultant.us>

The gen helpers for node uses the map stuff for structs.  Do the same
in the gen helpers for nodebuilder.

Seems we'll be deploying nodeGenerating several times (more than once
per typed node).
Signed-off-by: Eric Myhre <hash@exultant.us>

One bug fixed.
Signed-off-by: Eric Myhre <hash@exultant.us>

Before this diff, when getting a field back out, if it was nullable or
optional, you'd get one more level of pointer than you started with.
Oddly, this still compiled.  Tests sensibly rejected it, though.
Signed-off-by: Eric Myhre <hash@exultant.us>

And new tests.
Signed-off-by: Eric Myhre <hash@exultant.us>