jam1garner / binrw

A Rust crate for helping parse and rebuild binary data using ✨macro magic✨.
https://binrw.rs
MIT License
620 stars 36 forks source link

Passing arguments to Vec<...> requires Clone trait #256

Closed marxin closed 6 months ago

marxin commented 6 months ago

First of all, thanks for the great parsing library. Have a question: I'm writing a parser for DNS packets where DNS question section can contain a Question that references a previous one. Thus, I would like to keep a mapping in a parent object:

#[binrw]
struct Data {
    #[brw(ignore)]
    mapping: HashMap<u64, u64>,

    num_items: u64,
    #[br(args{ count: num_items as usize, inner: (& mut mapping,) })]
    items: Vec<Item>,
}

#[binrw]
#[br(import(mapping: & mut HashMap<u64, u64>))]
struct Item {
    field1: u32,
}

In the future, I would like to use and modify the mapping map in the parse_with function. But I can't compile this code due to:

error[E0277]: the trait bound `for<'a> &'a mut HashMap<u64, u64>: Clone` is not satisfied in `(&'a mut HashMap<u64, u64>,)`
  --> src/lib.rs:52:12
   |
52 |     items: Vec<Item>,
   |            ^^^^^^^^^ within `(&'a mut HashMap<u64, u64>,)`, the trait `for<'a> Clone` is not implemented for `&'a mut HashMap<u64, u64>`, which is required by `Vec<Item>: BinRead`
   |
   = help: the trait `Clone` is implemented for `HashMap<K, V, S>`
   = note: `for<'a> Clone` is implemented for `&'a HashMap<u64, u64>`, but not for `&'a mut HashMap<u64, u64>`
   = note: required because it appears within the type `(&'a mut HashMap<u64, u64>,)`
   = note: required for `Vec<Item>` to implement `BinRead`

Is it really needed the Clone attribute? Note the issue goes away if I change items: Vec<Item> to item: Item.

marxin commented 6 months ago
    mapping: HashMap<u64, u64>,

Apparently, one possible solution would be using Rc<Mutex<HashMap<u64, u64>.

csnover commented 6 months ago

Thanks for your report! In the helper function that is used for parsing collections, arguments get moved into the reader function on each iteration, and I’m unaware of a way to do this less restrictively than by requiring the Clone trait. I’d be glad to see a PR if it is possible though :-)

Instead, I believe you should use parse_with with a function that accepts &mut HashMap<u64, u64> and returns a Vec<Item>. (See #[binrw::parser].) The Rust compiler is more capable of reasoning about intrafunction lifetimes, and you presumably need some way to call whatever methods on HashMap anyway to set up your mapping.

marxin commented 6 months ago

Thanks for the reply. Both approaches are feasible for my use case. Anyway, thank you for the nice library!