ipld / go-ipld-prime

Golang interfaces for the IPLD Data Model, with core Codecs included, IPLD Schemas support, and some handy functional transforms tools.
MIT License
132 stars 50 forks source link

have the spec and implementations agree on how tuple reprs support optional fields #369

Open mvdan opened 2 years ago

mvdan commented 2 years ago

The implementation at https://github.com/ipld/go-ipld-prime/blob/e39d20bf18dcc21a50ef2005bae6de11e5c311e0/schema/gen/go/genStructReprTuple.go#L13 says they are allowed as trailing fields:

// Optional fields for tuple representation are only allowed at the end, and contiguously.

However, the current spec at https://ipld.io/docs/schemas/features/representation-strategies/#struct-tuple-representation says:

Optional or implicit fields are not possible with the tuple Struct representation strategy, all elements must be present.

For now, bindnode follows codegen; see https://github.com/ipld/go-ipld-prime/pull/368.

We should probably adjust the docs to agree with the codegen. cc @warpfork @rvagg

mvdan commented 2 years ago

The alternative is to keep the current spec and remove support for trailing optionals in both codegen and bindnode. I don't have a strong opinion there, but if they are already implemented, it's not hard to keep them. And one can argue that users may already be relying on them.

Also: the schema package ought to enforce whatever rule we end up with, be it "optionals only as trailing fields" or "no optionals".

rvagg commented 2 years ago

Repeating what I added to #368:

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?)

Its this ambiguity problem I'm most keenly interested in as I think about it more, that kind of ambiguity doesn't exist with maps when matching data forms to schemas. The rules are clear, but it's kind of mushy when eyeballing data. 🤷 . I'd be willing to bet nobody is actively using this anywhere yet and the pain of removing it would be minimal. However, it might buy us use within the Filecoin chain as it goes modifying all its tuples over time.

mvdan commented 2 years ago

Retitled to clarify that this issue is about making all the bits agree, whatever the solution we agree to.

warpfork commented 2 years ago

The brunt of the reasoning I'd use here is evolvability vs unevolvability. Tuple reprs shouldn't induce that degree of unevolvability.

One could argue for more features than that -- say, non-greedy matching, even if only back-to-front order or such, or etc -- but I'll note immediately that the arguments for that would become significantly weaker: