1. 01 Sep, 2019 3 commits
    • Eric Myhre's avatar
      Finish migrating to ident munge helper funcs. · ec1434e7
      Eric Myhre authored
      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: default avatarEric Myhre <hash@exultant.us>
      ec1434e7
    • Eric Myhre's avatar
      DRYing some type ident munges. · a84c7ec1
      Eric Myhre authored
      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: default avatarEric Myhre <hash@exultant.us>
      a84c7ec1
    • Eric Myhre's avatar
      Gen'd map reprs of struct dtrt for absent fields. · 116a4db7
      Eric Myhre authored
      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.
      116a4db7
  2. 30 Aug, 2019 7 commits
    • Eric Myhre's avatar
      Merge branch 'errnotexists-updates' · 53e4ebbe
      Eric Myhre authored
      53e4ebbe
    • Eric Myhre's avatar
      ErrNotExists uses PathSegment; use in more places. · 3a229598
      Eric Myhre authored
      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: default avatarEric Myhre <hash@exultant.us>
      3a229598
    • Eric Myhre's avatar
      Merge pull request #30 from ipld/lookup-segment-node-method · c157126c
      Eric Myhre authored
      Add LookupSegment to Node interface.
      c157126c
    • Eric Myhre's avatar
      Add LookupSegment to Node interface. · c96c25d3
      Eric Myhre authored
      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: default avatarEric Myhre <hash@exultant.us>
      c96c25d3
    • Eric Myhre's avatar
      Merge pull request #29 from ipld/pathsegment-is-core · 0b91e330
      Eric Myhre authored
      Move selector.PathSegment up to ipld.PathSegment.
      0b91e330
    • Eric Myhre's avatar
      Move selector.PathSegment up to ipld.PathSegment. · fc1f83d7
      Eric Myhre authored
      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: default avatarEric Myhre <hash@exultant.us>
      fc1f83d7
    • Eric Myhre's avatar
      Merge branch 'codegen-progress' · 4a9130c4
      Eric Myhre authored
      4a9130c4
  3. 26 Aug, 2019 7 commits
    • Eric Myhre's avatar
      Doc for expected error behavior on typed nodes. · eb70084a
      Eric Myhre authored
      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: default avatarEric Myhre <hash@exultant.us>
      eb70084a
    • Eric Myhre's avatar
      Tests for representational read of struct as map. · 9c4c9977
      Eric Myhre authored
      Mostly passes, but found one semantic bug.
      Signed-off-by: default avatarEric Myhre <hash@exultant.us>
      9c4c9977
    • Eric Myhre's avatar
      First phase of gen'd code test refactor. · 1facf46b
      Eric Myhre authored
      Just getting all the indentation changes out of the way.
      New tests coming in the next commit.
      1facf46b
    • Eric Myhre's avatar
      GetRepresentationNodeGen, and impl of struct->map. · 8569cb3e
      Eric Myhre authored
      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: default avatarEric Myhre <hash@exultant.us>
      8569cb3e
    • Eric Myhre's avatar
      Add accessors to struct representation details. · 0343dba1
      Eric Myhre authored
      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: default avatarEric Myhre <hash@exultant.us>
      0343dba1
    • Eric Myhre's avatar
      Fix generality of codegen helper mixins. · 5042c7e4
      Eric Myhre authored
      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: default avatarEric Myhre <hash@exultant.us>
      5042c7e4
    • Eric Myhre's avatar
      Merge branch 'codegen-progress' · a753b902
      Eric Myhre authored
      a753b902
  4. 15 Aug, 2019 6 commits
  5. 13 Aug, 2019 9 commits
  6. 12 Aug, 2019 8 commits