Added TryFromCtx impl for &[u8] with no ctx.

[Frostie314159]

Hi, this PR adds an implementation of TryFromCtx for &[u8], with no context. Instead, the whole input slice is converted. This is useful, if a struct has a variable length body and users may further specify that type through a generic.

[m4b]

In principle I agree this is extremely useful; however, and perhaps I'm misremembering, but i recall many years ago, when i tried to add multiple ctx's for a single type, the compiler at client invocation site, was not able to properly infer which context to use, for e.g., pread (not pread_with); i decided this was not an acceptable ergonomic decision for users (myself primarily at the time) so I did not add this.

Maybe there is a solution around this (without changing all of scroll), but I don't know. I realize this may be disappointing, but I hope you can see why the ambiguous compile error is problematic/not desirable, from both backwards compat standpoints as well as ergonomics?

[m4b]

so the problem with having two contexts is that now e.g., this function call becomes ambiguous:

    const buf: &[u8] = &[0x1, 0x2, 0x3, 0x4];
    let res = buf.pread::<&[u8]>(0);

You should see an error like this after a bunch of generic vomit:

error[E0283]: type annotations needed
   --> tests/api.rs:388:19
    |
388 |     let res = buf.pread::<&[u8]>(0);
    |                   ^^^^^
    |
    = note: multiple `impl`s satisfying `&[u8]: TryFromCtx<'_, _>` found in the `scroll` crate:
            - impl<'a> TryFromCtx<'a, usize> for &'a [u8];
            - impl<'a> TryFromCtx<'a> for &'a [u8];

I'm very surprised we do not have a test in either api.rs or somewhere in lib.rs to ensure that existing preads of e.g., &[u8] are compilable. I believe introducing this could theoretically break downstream users as any code that did a pread of &[u8] will no longer compile.

I swear we had tests specifically to test that just pread worked fine on &[u8] but don't see any tests failing...

[m4b]

I'd also like to learn more about your usecase, could perhaps post a snippet of the type of pread you're attempting/want to do, along with the type, etc.

also thanks for all your work on this crate so far and following up on issues ! :)

[Frostie314159]

My usecase for this is, that in my library I have a type ManagementFrame, which is generic over its body and there are multiple type aliases, which define for example a BeaconFrame as a ManagementFrame<BeaconFrameBody>. This is all well and good, as long as we actually have a strongly typed version of that body, that implements TryFromCtx<()>. In other situations, it's better to have something more generic, like ManagementFrame<&[u8]>, which currently doesn't work through the same generic implementation of TryFromCtx. There are other usecases for this, for example strongly typing certain data frames.
Regarding the implications for Pread, I was surprised, that plain pread just uses a default ctx, which in most cases is just () (It's in the docs, I know, but it still surprised me). Perhaps, we should consider either of two options here:

• Change pread's behavior, to assume a unit type as ctx (Potentially a very breaking change, but maybe more intuitive) and create something like pread_default
• Create a pread_without, which assumes a unit type as ctx (Not breaking at all, but less intuitive IMO)

[m4b]

Getting back to this now, so:

Regarding the implications for Pread, I was surprised, that plain pread just uses a default ctx, which in most cases is just () (It's in the docs, I know, but it still surprised me)

Maybe I'm misunderstanding you, but pread doesn't use a "default" ctx, insofar as it only requires the Ctx: Default bound if you invoke pread<T>; if you invoke pread_with<T>(ctx) this does not require Ctx: Default.

That being said, none of this actually matters w.r.t. the ambiguous trait dispatch. The heart of the issue is that when someone implements > 1 TryFromCtx, e.g. TryFromCtx<Ctx1> for T and TryFromCtx<Ctx2> for T, due to how rust dispatches to trait methods, it can't resolve which of those it should pick; so it requires the user to pick whichTryFromCtx to use via the UFCS (or whatever it's called these days).

This was a conscious decision to not burden the user with this, or to have the Ctx appear in the generic bound of a pread invocation, e.g., I originally experimented with various forms of something like pread::<T, Ctx1>() but I decided I didn't like the inelegance of that second generic parameter, and it being fairly onerous on the user (me at the time).

The original design goals of scroll were roughly, ergonomic user api (not really ergonomic for library author(s) 😆 ), very generic (see how far could take it), offset based, allow user defined types, support deriving.

I don't think it's in the cards that scroll will ever change in that regard (somewhat ergonomic user api), but if you have some idea theoretically how the existing design could potentially be redesigned (while keeping most/all of the ergonomics of current design) to allow multiple TryFromCtx impls, I would be interested (even if it only ever is theoretical, different generic designs are interesting in and of themselves); however just realistically, if it is a bunch of breaking changes, or significantly diverges from the design of the library it's unlikely to get merged, just to be straight with you :)

======================

I'd like to understand your usecase a bit more/solve it though; is it possible for you to paste maybe a simple repro with what you'd like to do/expect?

Maybe something like:

let data = bytes.pread::<Foo<Bar>>(0)?;
let data2 = bytes.pread::<Foo<&[u8]>>()?; // does not work today?

I'm also curious if you tested out this patch on your specific usecase, and how you did not hit the ambiguous trait dispatch error i posted above?

Anyway, this was a long comment, thanks for reading this far, and hope you're having a good day/night :)