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.

Sign up for GitHub

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

Jump to bottom

Fix handling of standalone .cbor and tagged types #112

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix handling of standalone .cbor and tagged types #112

wants to merge 3 commits into from

Conversation

SebastienGllmt
Copy link
Collaborator

Fix #91

Currently this fails with the following error that I don't really understand

error[E0308]: mismatched types
   --> src/lib.rs:654:37
    |
654 |         deser_test(&CborInCbor::new(Foo::new(0, String::new(), vec![]), 9))
    |                     --------------- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `FooBytes`, found struct `Foo`
    |                     |
    |                     arguments to this function are incorrect
    |
note: associated function defined here
   --> src/lib.rs:63:12
    |
63  |     pub fn new(foo_bytes: FooBytes, uint_bytes: u64) -> Self {
    |            ^^^ -------------------  ---------------
help: try wrapping the expression in `FooBytes`
    |
654 |         deser_test(&CborInCbor::new(FooBytes(Foo::new(0, String::new(), vec![])), 9))
    |                                     +++++++++                                  +

@SebastienGllmt SebastienGllmt self-assigned this Nov 2, 2022
rooooooooob
rooooooooob reviewed Nov 2, 2022
View reviewed changes
src/parsing.rs
fn has_embed<'a>(parent_visitor: &'a ParentVisitor, rule: &Rule<'a>) -> bool {
_has_embed(parent_visitor, &CDDLType::from(rule))
}
fn _has_embed<'a, 'b>(parent_visitor: &'a ParentVisitor, cddl_type: &CDDLType<'a, 'b>) -> bool {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't use this name as _ prefixes in rust are to signify they're unused.

src/parsing.rs
@@ -366,7 +375,11 @@ fn parse_type(types: &mut IntermediateTypes, parent_visitor: &ParentVisitor, typ
let base_type = types
.apply_type_aliases(&AliasIdent::new(CDDLIdent::new(ident.to_string())))
.expect(&format!("Please move definition for {} above {}", type_name, ident));
types.register_type_alias(type_name.clone(), RustType::Tagged(tag_unwrap, Box::new(base_type)), true, true);
if has_embed(parent_visitor, get_rule(parent_visitor, &CDDLType::from(&inner_type.type1.type2))) {
types.register_type_alias(type_name.clone(), RustType::Tagged(tag_unwrap, Box::new(base_type)), false, true);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need a minor change for conflict with #111

SebastienGllmt
SebastienGllmt commented Nov 2, 2022
View reviewed changes
src/intermediate.rs
|| self.rust_structs.contains_key(ident)
|| self.generic_defs.contains_key(ident)
|| self.generic_instances.contains_key(ident)
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI the code removed in this file is all unused. It comes from a previous PR that I forgot to delete and it didn't work because has_ident isn't stable across passes of the parsing.rs

@rooooooooob
Copy link
Collaborator

rooooooooob commented Nov 2, 2022 •
edited
Loading

The error it fails with is in one of the unit tests. It's treating the test that's:

foo_bytes = bytes .cbor foo

; since we don't generate code for definitions like the above (should we if no one refers to it?)
cbor_in_cbor = [foo_bytes, uint_bytes: bytes .cbor uint]

in the core unit test group (tests/core/input.cddl / tests/core/tests.rs. there should be one in preserve-encodings too, but this might not be an issue since it's not creating from rust).

Before we considered that anywhere taking in foo_bytes should only care about that it's conceptually a Foo and not how it's encoded, we only took in a Foo in all parameters. I think this is a very worthwhile property to preserve. If that unit test is failing to compile that means we are now forcing it everywhere to be its own FooBytes wrapper type which is less usable. IMO we should only generate the wrapper struct if no one is referring to foo_bytes in the CDDL elsewhere, or have the behavior be a toggle with some comment flag to choose.

@SebastienGllmt SebastienGllmt mentioned this pull request Nov 3, 2022
Merged
@rooooooooob rooooooooob mentioned this pull request Nov 18, 2022
Closed
@SebastienGllmt
Copy link
Collaborator Author

TODO: somebody needs to go through and check if this PR is still necessary (other than the commit that deletes unused code) given #117 was merged

@SebastienGllmt SebastienGllmt mentioned this pull request Jan 27, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rooooooooob rooooooooob rooooooooob left review comments

Assignees

@SebastienGllmt SebastienGllmt

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

.cbor handling incorrect for standalone types
2 participants
@SebastienGllmt @rooooooooob