GitLab

codegen: assembler for struct with map representation now validates all non-optional fields are present.

This continues what https://github.com/ipld/go-ipld-prime/pull/111/ did
and adds the same logic to the map representation.  The actual state
tracking works the same way (and was mostly already there).

Rearranged the tests slightly.

Made error messages include both field name and serial key when they
differ due to a rename directive.  (It's possible this error would get
nicer if it used a list of StructField instead of just strings, but it
would also get more complicated.  Maybe revisit later.)

They do quite a lot of work, including setting up a type system,
generating Go code, and building it.

This package is by far the slowest to test. On a warm cache, 'go test
-count=1 ./...' shows all packages under 0.2s, while this one sits at
~1.5s.

1.5s is not unreasonable with a warm cache, but we can bring that down
to 0.6s on my quad-core laptop by just making all tests parallel. Note
that sub-tests aren't parallel for now, since there are multiple layers
of them and I'm not sure I even follow what's going on.

Mac is also disabled for the time being, since it seems to time out too
often.

Though in general, letting golang's test system replace spaces and
other unusual characters with underscores has very little downside,
here, the resulting "_--_" is quite unpleasant on the eyes.

Change things to use TitleCase.

This new system focuses on table-driven tests, and leans heavily upon
json as a shorthand for expressing fixtures.

It also makes a great deal more effort to exercise the different
features of nodes (and their paired representation nodes) from all
directions at once for each test datum, rather than requring that all
be written out manually.

The result is that the struct tests we've renovated have a lovely
diffstat shrinkage: 111 insertions, 299 deletions...

And yet the smaller line count results in *more* coverage.

(Okay, the linecount increase for the testcase structure and helper
methods is much bigger than the savings in fixture size... but,
only *so far*.  I assume this will continue to pay off in the future.)

Relatedly: a bug in struct map representations has been fixed.
(It was the sibling of 5f589653, embarassingly.)  Thank goodness we now
get proper coverage of this area.

There's a few TODOs left to further expand the exercises, but those
can slot in easily in subsequent commits.  Same goes for further
expansion of usage of this new system.

This is full of mundane plonking away at adding stars and ampersands,
of which approximately none are interesting.

(I'm particularly frustrated by this because these are all placeholder
types, and we're getting *very* close to replacing them, as we get
closer and closer to self-hosting... at which point all of this bonking
about will be made totally irrelevant.  And yet to close the last mile,
these "small" fixes are surprisingly irritating.  Ah well.)

The bits that *are* interesting:

- the Spawn functions for type info now take strings rather than types
(so that they don't provoke a cycle problem for the user when
constructing the information to describe cyclic type info);

- all of the Type info structure hold a pointer to the TypeSystem, and
use that to look up reified Type info for related types, so that their
methods don't force the caller to do that themselves.

(The TypeSystem pointer was already there, amusingly; just never before
initialized, because it hadn't turned out to be load-bearing yet.)

It also would've been possible to just change all the methods on the
Type types to return TypeName rather than full Type info.  That would
avoid the need to use a TypeSystem pointer.  I didn't because:

Overall, this was done in such a way as to minimize the diff that
impacts within the templates.  This was a goal because updating
templates is a fair bit more work than other code due to the weak
compiler support.  And we'll end up reviewing and changing these
methods when we get to our goal of self-hosting generation of the
schema types from the schema-schema, so, it's not worth pushing around
diffs in that same area when they'd be sure to be churned under soon.

Hopefully this increases clarity and eases comprehension.

Notes and discussion can be found at
https://github.com/ipld/go-ipld-prime/issues/54 (and also
I suppose in some of our weekly video chats, but I'd have
to go on quite a dig to find the relevant links and time).

Many many refernces to 'ns' are also updated to 'np',
making the line count in this diff pretty wild.

See the changelog for discussion; this had already been on the docket
for a while now.

Output is fairly fabulous.  Really happy with this.

Diff size is nice.  Took the opportunity to do some cleanups and the
net result is 193 insertions and 351 deletions counted by line.

Most importantly, though: running 'go test .' in the gengo package
actually generates, compiles, and tests **everything**.

The test file inside the "_test" dir that you previously had to jankily
run manually is gone.  Hooray!

And already, we've got three distinct packages being generated and
tested.  (Try 'diff _test/stroct*/tStroct.go'.  It's neat.)

The time overhead is acceptable so far.  The total time is up to 0.9sec
on my current machine.  Not as bad as feared.

Overall: nice.  I think this reduces the test friction to a point that
will significantly enable further progress.

Some touches to ImplicitValue came along for the ride, but aren't
exercised yet.

Tests are getting unwieldy.  I think something must be done about
this before proceding much further or it's going to start resulting
in increasingly significant velocity loss.

Still have a lot of uncertainty to resolve around nullable and optional
in general.  But writing that up elsewhere -- trying to treat it from
the spec angle without getting the implementation too mixed up in it.

I was particularly worried about trailing optionals.  Happily,
they appear to work fine.

Representation iterators and smooth stopping included.

Starting to wonder how best to organize these tests to continue.
333 lines already?  Questionable scalability.

I started doing double-duty in tests to keep the volume down:
the block for nullables tests both 'nullable' and 'nullable optional';
and the optionals similarly.  Will make failures require more careful
reading to discern, but probably a fine trade.

I'm almost a little staggered.  We've got structures with maybes
both using pointers and not, and everything seems fine.

A few fixes were necessary, but they were mostly "d'oh" grade.
(The finishCallback logic is a bit hairy, but appears correct now.)

More cases for more variations in presence coming up next.

In research: I found that there are more low-cost ways to switch which
methods are available to call on a value than I thought.  Also, these
techniques work even for methods of the same name.  This is going to
improve some code in NodeAssemblers significantly -- there are several
situations where this will let us reuse existing pieces of memory
instead of needing to allocate new ones; even the basicnode package
now already needs updates to improve this.  It's also going to make
returning representation nodes from our typed nodes *significantly*
easier and and lower in performance costs.  (Before finding methodsets
are in fact so feasible, I was afraid this was going to require our
typed nodes to embed yet another small struct with a pointer back to
themselves so we can have amortized availability of value that contains
the representation's logic for the Node interface... which while it
certainly would've worked, would've definitely made me sigh deeply.)
Quite exciting for several reasons; only wish I'd noticed this earlier.

Also in research: I found a novel way to make it (I believe) impossible
to create zero values of a type, whilst also making a symbol available
for it in other packages, so that we can do type assertions, etc,
with that symbol.  This is neat.  We're gonna use this to make sure
that types in your schema package can never be created without passing
through any validation logic that the user applies.

In codegen: lots of files disappear.  I'm doing a tabula rasa workflow.
(A bunch of the old files stick around in my working directory, and
are being... "inspirational"... but everything is getting whitelisted
before any of it ports over to the new commits.  This is an effective
way to force myself to do things like naming consistency re-checks
across the board.  And there's *very* little that's getting zero change
since the changes to pointer strategy and assembler interface are so
sweeping, so... there's very little reason *not* to tabula rasa.)

Strings are reimplemented already.  *With* representations.

Most of the codegen interfaces stay roughly the same so far.
I've exported more things this time around.

Lots of "mixins" based on lessons learned in the prior joust.
(Also a bunch of those kind-based rejections look *much* nicer now,
since we also made those standard across the other node packages.)

Some parts of the symbol munging still up in the air a bit.
I think I'm going to go for getting all the infrastructure in place
for allowing symbol-rename adjunct configuration this time.
(I doubt I'll wire it all the way up to real usable configuration yet,
but it'll be nice to get as many of the interventions as possible into
topologically the right places to minimize future effort required.)

There's a HACKME_wip.md file which contains some other notes on
priorities/goals/lessoned-learned-now-being-applied in this rewrite
which may contain some information about what's changing at a higher
level than trying to track the diffs.  (But, caveat: I'm not really
writing it for an audience; more my own tracking.  So, it comes with
no guarantee it will make sense or be useful.)

Previously it was in the 'impl/typed' package, next to the
runtime-wrapper implementation of the interface.  This was strange.

Not only should those two things be separated just on principle,
this was also causing more import cycle problems down the road:
for example, the traversal package needs to consider the *interface*
for a schema-typed node in order to gracefully handle some features...
and if this also brings in a *concrete* dependency on the
runtime-wrapper implementation of typed nodes, not only is that
incorrect bloat, it becomes a show stopper because (currently, at
least) that implementation also in turn transitively imports the
ipldfree package for some of its scalars.  Ouchouch.

So.  Now the interface lives over in the 'schema' package, with all
the other interfaces for that feature set.  Where it probably always
should have been.

('typed.Maybe' also became known as 'schema.Maybe', which... does not
roll off the tongue as nicely.  But this is a minor concern and we
might reconsider the naming and appearance of that thing later anyway.)

(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.

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>

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.

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.

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>

A bit fun; wish there was a way to compress that block of ifdefs about
optional vs nullable stuff, but so far nothing clearer has emerged.

Added ErrIteratorOverread while at it.  Should refactoring other node
types to use it too, I suppose.  But perhaps we'll just accumlate those
todos for a while, since there's already one other error refactor with
a chain of blocks in front of it.

Exercise unmarshal against our generated "Strang" type.  Works!

(Note: this will fail if you *haven't* run the codegen yet; but it's in
an underscore-prefixed package, so it won't run unless you dive in here
intentionally.  Overall, this continues to be early-days hijinx, and
does not represent a good long-term testing strategy.)