Skip to content

Fix handling of standalone .cbor and tagged types - #112

Closed
SebastienGllmt wants to merge 157 commits into
masterfrom
standalone-types
Closed

Fix handling of standalone .cbor and tagged types#112
SebastienGllmt wants to merge 157 commits into
masterfrom
standalone-types

Conversation

@SebastienGllmt

Copy link
Copy Markdown
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))
    |                                     +++++++++                                  +

This is just a proof of concept to see how much of the CDDL specs we can
use codegen to generate rust code. This would have the benefit of not
having to write code any time we use a future CDDL spec.
* Homogenous arrays (all same type, no groups) work now. This includes
  some nested types like arrays of arrays, but not arrays of groups.

* Before struct fields simply used the `Display` trait in the CDDL
  library for computing the field string. Now it is implemented
  with just our own code. This includes converts ie tstr -> String
  or int -> i32.
This is useful to have as it lets us have one representation for both
map and array represented groups, which can differ by just
serializaiton in the future.

This also allows for easier creation of named nested inline groups
which will greatly simplify the implementation of codegen for nested
groups.

The future wrappers over this group representation will be defined
at the root level and have wasm exposure, rather than being hidden in
the group module.
For groups we generate basic cbor_event serialize trait impls which we
can later use to expose the to_bytes/from_bytes wasm API in wrapper
types that will be defined for concrete map or array representations
of the groups.
This allows us to serialize properly these types, as we can't specialize
Vec to cbor_event's Serialize trait as they're in separate external
crates.
This way we can do address.new_address0([], []) instead of
address.new_address0(bytes.new([]), bytes.new([])), etc.
Can't pass generic types (ie no Vec<T>, Array<T>) through wasm boundary
so we need to generate an array-like type for each type that is exposed
as a wasm type.

This is similar to the approach in js-chain-libs, ie PublicKey ->
PublicKeys.
All static structs now live in `static/prelude.rs`.

Tags are now detected and a wrapper type is implemented, however
right now we can't construct it since it will need a refactor of the
type system first.
In order to better support tagged types, arrays, and conversion between
the wasm boundary instead of a simple String type there is now a
composable RustType enum to allow for more information at every stage
and better code generation as a result.

This allows us to not require the tag for paremters while stilly
serialization it for tagged types, as well as not requiring separate
types for vecs of primitive types.
Behavior is the same in this place, but this way we have more control
instead of relying on the to_string of the literal value.
It was this way before, but was accidentally changed in the prior
refactor commit after updating the dependency.
Use case: `address` in `shelley.cddl`

For groups are not defined in terms of array or map representation yet
we can still use them inside of other structures. If they are array then
their elements are inserted into the wrapping array ie:
x = (1, 2) y = [3, x, x, 4] should be [3, 1, 2, 1, 2, 4]
not [3, [1, 2], [1, 2], 4]
Also made group members always point to external types and never to the
group module types.
This is for 2 reasons:
1) All the binary types we have should probably have their own structs
   so we can have a better type system + add in specific code later
   on to them like verifying structure + helpers.

2) We can't expose Vec<Vec<u8>> (or any nested vec) to wasm using
   wasm_bindgen but we can generate for example Scripts from
   `script = bytes` and then using `[script]` somewhere.
Updating the cddl lib allows correct parsing of:
`withdrawals = { * [credential] => coin }`

and now that it parses correctly, we had to support using
generated types as map keys, so they all now have comparison
derives generated automatically.

Also root table constructions were broken so those were fixed too.
1) Properly support optional fields for both arrays/maps
2) Only generate array or map related functions when necessary
3) Store all groups inside `groups.rs` instead of in a module
   in `lib.rs`
SebastienGllmt and others added 14 commits August 28, 2022 03:54
* Automatically generate prelude integer types

* Support explicit value range
* Dynamic Int generation

Previously the `Int` struct was statically defined. This causes issues
because some traits, wrappers, etc are only needed for certain runtime
configurations (e.g. preserve-encodings) or usages (e.g. if it's used as a key).

This also introduces more functionality like a wasm wrapper again and
conversion traits.

Fixes #55

* Fixes for different int precisions + Tests

Fixes #57

Also addresses issues with other primtive types e.g. i8, i64, u8, etc
and adds unit tests to cover all of them.

Fixes a few issues with wasm bindings for `Int`.

* Misc fixes

* Option<T> for non-primitive T compiles properly now (doesn't use a
ref)

* removed TODO that was actually already implemented

* fix for maps in keys (OrderedHashMap now derives comparisons when
needed)

* Full nint support

Now even with `preserve-encodings=false` we use the
`*negative_integer_sz()` functions in `cbor_event` as the non-`_sz` ones
have two issues: 1) they use `i64` as an interface and thus only cover
half of `nint`'s range and 2) there is a panic when serializing
`i64::MIN` so even for `i64` it is not complete.

See primetype/cbor_event#9

This commit also changes the `nint` type to be an unsigned `u64` to
cover the complete range and not allow in the rust code values from both
sides of the sign. This might be a little awkward it is represents
exactly the values possible with `nint`.

Negative constants are also added by this.

* link cbor_event upstream issue

Co-authored-by: Sebastien Guillemot <sebastiengllmt@gmail.com>
When used as a key `Int` requires more traits, which can't be
auto-generated when used as a tuple enum. This switches it to a named
enum any ignores encoding fields for those traits. This also stops
duplicate keys which were possible previously when the encoding
differed.

We now only generate `Int` when it is actually referenced within the
CDDL definitions being generated.
Fix for `Int` when used as key, only gen when needed
* Multi-file support

For generating larger projects it may be desirable to generate into
different files. Now --input can take a directory. If it is given a
directory it will read all .cddl files and generate each in its own .rs
file for export. Serialization/*Encodings are still all in one file.

* Multi-file fix for cddl lib parsing errors

cddl doesn't support parsing incomplete cddl strings so will error
on some inputs. This works around it by merging it into 1 file with
scope markers then using those instead to mark scope.

* Dependency graph ordering of rules

Fixes #93

This is extra important with multi file support now, as it can be
impossible in some cases to order things properly when the circular
dependencies were across multiple files. Now `babbage.cddl` processes
totally fine even when separated across many files.

This commit also includes some fixes for multi file support with
inter-file dependencies (e.g. fixes for visibility/module use/etc).

* removing useless code in intermediate form creation
* checkpoint

* Update upstream cddl lib

* Fix more of the parsing TODOs

* Bump cddl and remove old fallback logic
@SebastienGllmt SebastienGllmt self-assigned this Nov 2, 2022
Comment thread 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
Copy Markdown
Contributor

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.

Comment thread src/parsing.rs
.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
Copy Markdown
Contributor

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

Comment thread src/intermediate.rs Outdated
|| self.rust_structs.contains_key(ident)
|| self.generic_defs.contains_key(ident)
|| self.generic_instances.contains_key(ident)
}

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

rooooooooob commented Nov 2, 2022

Copy link
Copy Markdown
Contributor

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

.cbor handling incorrect for standalone types

2 participants