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

Rust tests for new compiler #53

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

Rust tests for new compiler #53

wants to merge 6 commits into from

Conversation

bspeice
Copy link

@bspeice bspeice commented May 13, 2019

Companion to kaitai-io/kaitai_struct_compiler#164.

Sets up the the translator and test infrastructure to work better with Rust and the new runtime library. Currently the CI script fails with a parse error in cargo junit (@CWood1 may need to look at that), but cargo test runs just fine.

Right now we're not actually asserting anything (because the compiler isn't actually generating anything), but this at least proves that everything compiles. Getting it to work can be done incrementally, this is a foundation.

bspeice added 4 commits May 10, 2019 12:51
We're working with a reduced spec set; some of the Rust codegen
is failing, so those specs have been removed here.
All tests generate and pass, but aren't actually testing anything at the
moment.
@bspeice
Copy link
Author

bspeice commented May 14, 2019

@CWood1 and @GreyCat - Design question for you guys. Right now, Rust uses lifetimes so it can avoid structs having to copy all the bytes off the stream into owned storage. This causes issues with opaque types (meta: ks-opaque-types=true) that may or may not have references (this for example).

I'm going to skip the test for now, but there are a couple potential solutions I can think of:

  1. Have structs own their data by copying from the stream into Vec<u8> storage. This allows us to remove the 'stream lifetime, but imposes a runtime cost for allocation and memcpy.
  2. A meta attribute for the types involved so that Rust can choose correctly. Requires users to set the attribute in response to compile errors, which may be a pain.
  3. Add a PhantomData<&'r ()> marker so that all structs have an associated 'stream lifetime. Because we derive Default, the implementation doesn't change at all, but the API doesn't look as nice.

My preference is towards number 3 (since the point of generating code is to deal with APIs that don't look nice, and Rust should be able to figure things out without needing to be explicit about lifetimes when using generated structs), but figured it was worth getting some input.

@bspeice
Copy link
Author

bspeice commented May 15, 2019

Alright, next issue: even using the PhantomData<&'r ()> to fix the lifetimes, I can't detect opaque classes recursing/owning each other, which is a compile error because Rust can't calculate the struct size. The solution is to insert either an owned reference (Box<>, equivalent to std::unique_ptr), or a plain reference (and then lifetimes become an issue again).

While I'd normally prefer using a plain reference so that I don't need heap allocation, we'd run into an issue because Rust would be unable to establish ownership; the "child" wouldn't be owned by anyone. Instead, I think what can be done is non-opaque user types that don't recurse (the vast majority of cases) are owned by the parent (thus avoiding allocation), while opaque and recursive classes use the heap-allocated boxes because they're unable to establish that it's safe without.

@bspeice
Copy link
Author

bspeice commented May 17, 2019

Next version is available, and tests basic asserts. There's plenty of functionality left to test, but some of it will need support from the compiler before it can be enabled.

The following test cases aren't currently part of the full suite, but should be added later:

  • process_coerce_switch: Kaitai resolves this as AnyType, which is illegal in Rust. The actual type is UserType(Foo), but it seems the compiler isn't quite able to figure that out. C++ has issues with this as well, so I don't think it's critical to solve at the moment.
  • str_encodings: Rust refuses to compile because of the presence of non-UTF8 symbols in the source code.

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.

1 participant