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

br(count = ...) conflicts with br(args(...)) #112

Closed
Holzhaus opened this issue Mar 28, 2022 · 4 comments
Closed

br(count = ...) conflicts with br(args(...)) #112

Holzhaus opened this issue Mar 28, 2022 · 4 comments

Comments

@Holzhaus
Copy link
Contributor

Apparently it's not possible to to pass a arguments to a Vec of BinRead objects.

Here's an example:

use binrw::{binrw, BinRead};

#[binrw]
#[derive(Debug)]
#[brw(little)]
struct Data {
    version: u16,
    #[br(temp)]
    #[bw(calc = items.len().try_into().unwrap())]
    num_items: u32,
    #[br(count = num_items, args(version))]
    items: Vec<Item>,
}

#[binrw]
#[derive(Debug)]
#[brw(little)]
#[br(import(version: u16))]
struct Item {
    field1: u32,
    #[br(if(version >= 5, 0))]
    field2: u64,
}

fn main() {
    let path = std::env::args().nth(1).expect("no path given");
    let mut reader = std::fs::File::open(&path).expect("failed to open file");
    let data = Data::read(&mut reader).expect("failed to parse file");
    println!("{:#04x?}", data);
}

This results in the following error:

error[E0308]: mismatched types
 --> src/bin/example.rs:3:1
  |
3 | #[binrw]
  | ^^^^^^^^
  | |
  | expected struct `VecArgs`, found tuple
  | expected due to this
  |
  = note: expected struct `VecArgs<(u16,)>`
              found tuple `(u16,)`
note: return type inferred to be `VecArgs<(u16,)>` here
 --> src/bin/example.rs:3:1
  |
3 | #[binrw]
  | ^^^^^^^^
  = note: this error originates in the attribute macro `binrw` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0308`.
error: could not compile `rekordcrate` due to previous error
@kitlith
Copy link
Collaborator

kitlith commented Mar 30, 2022

The thing that jumps out at me, is that you're using #[br(args(...))]: this means you're using positional (tuple) arguments instead of named arguments (struct), which is what vecs use. I'd be somewhat surprised if that worked without count being present (i.e. the problem isn't with count), but maybe something was done to make that work in the absense of count. (it's been awhile since I touched named arguments)

In general, #[br(count = ...)] is shorthand for #[br(args { count: ... })]. iirc inner arguments are typically passed through #[br(args { inner: ...})]. I believe these are compatible with each other, but I'd just use #[br(args { count: ..., inner: ...})].

Does this help?

@Holzhaus
Copy link
Contributor Author

Thanks, this does indeed work:

use binrw::{binrw, BinRead};

#[binrw]
#[derive(Debug)]
#[brw(little)]
struct Data {
    version: u16,
    #[br(temp)]
    #[bw(calc = items.len().try_into().unwrap())]
    num_items: u32,
    #[br(args{ count: num_items.try_into().unwrap(), inner: (version,) })]
    items: Vec<Item>,
}

#[binrw]
#[derive(Debug)]
#[brw(little)]
#[br(import(version: u16))]
struct Item {
    field1: u32,
    #[br(if(version >= 5, 0))]
    field2: u64,
}

fn main() {
    let path = std::env::args().nth(1).expect("no path given");
    let mut reader = std::fs::File::open(&path).expect("failed to open file");
    let data = Data::read(&mut reader).expect("failed to parse file");
    println!("{:#04x?}", data);
}

But it feels like I'm hacking around some implementation detail of the library, so it would be nice if this would receive a proper fix.

@Holzhaus
Copy link
Contributor Author

Btw, I just saw that this workaround actually documented in 38f731b.

Holzhaus added a commit to Holzhaus/binrw that referenced this issue Mar 30, 2022
Links to the documentation added in
38f731b, which was only referenced by
the `count` documentation, but not `args`.

Related issue: jam1garner#112
jam1garner pushed a commit that referenced this issue Mar 31, 2022
Links to the documentation added in
38f731b, which was only referenced by
the `count` documentation, but not `args`.

Related issue: #112
csnover added a commit to csnover/binrw that referenced this issue Aug 19, 2022
csnover added a commit to csnover/binrw that referenced this issue Aug 22, 2022
csnover added a commit to csnover/binrw that referenced this issue Aug 24, 2022
csnover added a commit to csnover/binrw that referenced this issue Aug 24, 2022
This commit provides an explicit message when someone combines
count + args in a way which is not valid, which was the primary
issue, and then also improves the diagnostic output for cases
where someone passes the wrong type of arguments to point to the
invalid args expression and the type of the field.

Notably, this does not resolve most issues with named arguments as
those are typically errors caused by calling non-existent builders
and builder functions, which requires a different strategy.

Closes jam1garner#112.
@csnover
Copy link
Collaborator

csnover commented Aug 24, 2022

There is now an explicit error message explaining the problem and what to do instead. Thanks for your report!

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

No branches or pull requests

3 participants