Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
mvdan opened this issue Feb 23, 2022 · 4 comments
Assignees

Comments

@mvdan
Copy link
Contributor

mvdan commented Feb 23, 2022

The implementation at

// Optional fields for tuple representation are only allowed at the end, and contiguously.
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 #368.

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

@mvdan
Copy link
Contributor Author

mvdan commented Feb 23, 2022

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
Copy link
Member

rvagg commented Feb 23, 2022

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 mvdan changed the title properly document how structs with tuple reps allow optional fields have the spec and implementations agree on how tuple reprs support optional fields Feb 23, 2022
@mvdan
Copy link
Contributor Author

mvdan commented Feb 23, 2022

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

@warpfork
Copy link
Collaborator

  • Trailing optionals are pretty important -- a schema using tuple representation becomes brittle and unevolvable without them
  • Greedy matching (i.e. if there's one more field than the minimum, it's the first optional, etc) is clear and easy to parse
  • I think we should basically do those two (and probably nothing else). (aka, what the code says already.)

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:

  • The above is already sufficient for evolvability
  • non-greedy matching wouldn't really help increase evolvability much...? afaict?
  • the complexity of more options would increase very rapidly, and probably isn't worth it unless there's correspondingly large payoffs (which aren't evident, at least to my current imagination).

@rvagg rvagg self-assigned this May 3, 2022
@rvagg rvagg moved this to 🥞 Todo in IPLD team's weekly tracker May 3, 2022
@rvagg rvagg moved this from 🥞 Todo to 🗄️ Backlog in IPLD team's weekly tracker Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🗄️ Backlog
Development

No branches or pull requests

3 participants