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

Avoid simple try-all deserialization for type choices when possible #194

Closed
rooooooooob opened this issue Jul 3, 2023 · 1 comment · Fixed by #199
Closed

Avoid simple try-all deserialization for type choices when possible #194

rooooooooob opened this issue Jul 3, 2023 · 1 comment · Fixed by #199
Labels
Quality-of-life feature Feature that helps, but is outside of the CDDL specification

Comments

@rooooooooob
Copy link
Collaborator

e.g.

transaction_metadatum =
    { * transaction_metadatum => transaction_metadatum } ; @name map
  / [ * transaction_metadatum ] ; @name list
  / int
  / bytes .size (0..64)
  / text .size (0..64)

For type choice we just start trying variants and backtrack if we encounter an error. This allows us to simply support all type choices with one codegen method, but has several downsides when the type choice consists of completely different types like the above. One downside is we waste time trying the other variants, but more critically is that errors are essentially eaten completely making debugging more of a pain. Instead of getting specific errors telling us exactly what went wrong and where, it will be eaten up by the topmost type/group choice as simply "No variant matched".

This would complicate codegen slightly but I think it's worth it just for the better errors. I just wasted some time debugging CML block parsing that would have been made immediately obvious if we did this instead. The faster deserialization time for choices would be an added bonus.

It can be solved by matching on raw.cbor_type() and having branches for each covered type in the choice. If the user isn't using extern types we have the ability to check all possible cbor starting tags for types, although the support isn't 100% for structs (or at least plain group ones?) it is better than nothing and would still help us for most cardano CDDL specs.

@rooooooooob rooooooooob added the Quality-of-life feature Feature that helps, but is outside of the CDDL specification label Jul 3, 2023
@rooooooooob
Copy link
Collaborator Author

Another option could be to just give DeserializeFailure::NoVariantMatched a Vec<DeserializeError> value which would require less changes. Or we could do the above match for when they're disjoint, and the old approach with the vec of errs when they're not.

rooooooooob added a commit that referenced this issue Jul 18, 2023
Addresses #194

This helps avoid any type/group choice from eating any errors on
variants as instead of receiving a NoVariantMatched you will receive one
with errors as to why each variant failed.

This drastically helps debugging and also works for nested choices as
well.
rooooooooob added a commit that referenced this issue Jul 19, 2023
Together these fix #194 and #145

* NoVariantMatchedWithCauses deser error variant

Addresses #194

This helps avoid any type/group choice from eating any errors on
variants as instead of receiving a NoVariantMatched you will receive one
with errors as to why each variant failed.

This drastically helps debugging and also works for nested choices as
well.

* Avoid try-all on enums with non-overlapping CBOR

Fixes #145

When all variants have non-overlapping first CBOR type we can avoid
brute-force trying all possible variants for type/group choices and
instead branch on raw.cbor_type() to only try the variant that makes
sense.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Quality-of-life feature Feature that helps, but is outside of the CDDL specification
Projects
Development

Successfully merging a pull request may close this issue.

1 participant