-
Notifications
You must be signed in to change notification settings - Fork 15
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
.default support / RustType
refactor
#111
Conversation
Before `RustType` represented how things are serialized e.g. `Tagged`, `CBORBytes`, etc as well as the underlying rust type we use, despite in 90% of cases us only caring about the underlying rust type. This is now split up into several structs with `RustType` being the complete type with both components and `ConceptualRustType` being the underlying rust type used outside of serialization contexts. This is work done ahead of supporting `.default` specs as that would have necessitated an additional `RustType::Default(FixedValue, Box<RustType>)` variant otherwise, further adding more serialization-only information, causing potential bugs when not properly dealt with.
Adds .default support to typenames (`foo = bar .default baz`) and to map fields (`foo = { ? 1 => bar .default baz }`). This is not applied into nested contexts e.g.: ``` foo = uint .default 2 bar = { * str => foo } ``` as how this should be handled is extremely ambiguous. Should this result in an extra encoding variable which is a map e.g. `default_included: BTreeSet<K>` and having the behavior be to omit it if it's not included in this encoding var and it's equal to the default ? As per the CDDL RFC we only apply it to optional fields: ``` This control is only meaningful when the control type is used in an optional context; otherwise, there would be no way to make use of the default value. ``` We support encoding preservation for round-tripping i.e. if a default value is explicitly present it will remain explicitly present. With `preserve-encodings=false` it will be omitted if the value present is equal to the default value. No changes were done with canonical encodings as the RFC doesn't seem to mention how that should be handled as canonical encodings are specified in the CBOR RFC not the CDDL one, and `.default` is only present in the CDDL one. It's that possible we could omit fields equal to the default if needed but we need to confirm this first. Optional fields with `.default` are no longer encoded as `Option<T>` but instead just `T` now which should help with usability. The field will default to the default via `new()` and deserialization. Fixes #42
@@ -1356,11 +1356,6 @@ impl GenerationScope { | |||
// new | |||
for variant in variants.iter() { | |||
let variant_arg = variant.name_as_var(); | |||
let enc_fields = if CLI_ARGS.preserve_encodings { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was this just unused before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was not needed. It might have been used before we changed how enums handle storing/using encoding variables as those now have a dedicated struct to abstract all of that away starting in #52.
encs.push(EncodingField { | ||
field_name: format!("{}_default_present", name), | ||
type_name: "bool".to_owned(), | ||
default_expr: "false", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't fully get how it works, why default is false? could you share some context pls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default behavior, and the idea behind the .default
notation in CDDL, is to avoid serializing a value if it is equal to the default. When we're keeping track of encodings we keep track of when there is a default value explicitly present and set this to true when that happens to keep the serialization format consistent for round-tripping. This default_expr
means that unless we happen to encounter the explicitly present default value during deserialization, we should make this encoding value false
to signify there was no explicitly present default value. This default_expr
is part of the EncodingField
and is present for all encoding variables, not just for handling .default
. e.g. for encoding for str
s it defaults to LenEncoding::default()
i.e. canonical encoding (definite length using a minimum of bytes necessary for the length).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Before
RustType
represented how things are serialized e.g.Tagged
,CBORBytes
, etc as well as the underlying rust type we use, despite in 90% of cases us only caring about the underlying rust type. This is now split up into several structs withRustType
being the complete type with both components andConceptualRustType
being the underlying rust type used outside of serialization contexts. This is work done ahead of supporting.default
specs as that would have necessitated an additionalRustType::Default(FixedValue, Box<RustType>)
variant otherwise, further adding more serialization-only information, causing potential bugs when not properly dealt with..default
supportAdds .default support to typenames (
foo = bar .default baz
) and to mapfields (
foo = { ? 1 => bar .default baz }
).This is not applied into nested contexts e.g.:
as how this should be handled is extremely ambiguous.
Should this result in an extra encoding variable which is a map e.g.
default_included: BTreeSet<K>
and having the behavior be to omit it ifit's not included in this encoding var and it's equal to the default ?
As per the CDDL RFC we only apply it to optional fields:
We support encoding preservation for round-tripping i.e. if a default
value is explicitly present it will remain explicitly present. With
preserve-encodings=false
it will be omitted if the value present isequal to the default value.
No changes were done with canonical encodings as the RFC doesn't seem to
mention how that should be handled as canonical encodings are specified
in the CBOR RFC not the CDDL one, and
.default
is only present in theCDDL one. It's that possible we could omit fields equal to the default
if needed but we need to confirm this first.
Optional fields with
.default
are no longer encoded asOption<T>
butinstead just
T
now which should help with usability. The field willdefault to the default via
new()
and deserialization.Fixes #42