jam1garner / binrw

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

Higher ranked lifetime (for <'a>) makes API difficult to use #227

Closed wangbj closed 9 months ago

wangbj commented 9 months ago

Considering below example:

fn read_buf<B, T>(buf: B) -> Result<T, binrw::Error>
  where B: AsRef<[u8]>,
  T: BinRead,
  for<'r> <T as BinRead>::Args<'r>: Default,
{
  let mut cursor = io::Cursor::new(buf.as_ref());

  let res: T = BinRead::read_ne_args(&mut cursor, Default::default())?;

  Ok(res)
}

fn main() -> anyhow::Result<()> {

  let r = read_buf::<_, u8>(b"1234")?; // This works!   (RHS type annotation)
  let r: u8 = read_buf(b"1234")?; // This doesn't work! (LHS type annotation)

  Ok(())
}

The new API is harder to use, as the caller now have to insert explicit type annotation on the right hand side (RHS). My intuition is that T on the LHS (can be inferred by the compiler) is not necessarily the same as the explicit T on the RHS, because T::Args<’a> could have different lifetimes (higher ranked, hence T could be different too?).

While I understand the urge to get rid of Arg:Clone, the new API has higher mental overhead, Is it possible to go back to the old API (before v0.11) without the higher kinded lifetime (for<'a> Self::Args<'a>)?

(I think clap-v3 has a similar issue by introducing Cow everywhere, but it was later fixed in v4, by allowing Cow<'static, _>), see clap ChangeLog.

csnover commented 9 months ago

Hi, thanks for your feedback!

You can just specify 'r in the function signature instead of using for <'r> and both call formats will work. The equivalent to the old API would be to just use 'static as the lifetime instead of 'r. I will revise the old release notes to include this information.

There are many upstream bugs so I don’t know which are directly applicable to the problem you encountered here.

Thank you again,