libs-team icon indicating copy to clipboard operation
libs-team copied to clipboard

ACP: Provide `TryFrom<&[T]>` implementation for `&[[T; N]]`

Open mina86 opened this issue 2 years ago • 9 comments

Proposal

Problem statement

Provide TryFrom<&[T]> implementation for &[[T; N]] as a more convenient wrapper around [T]::as_chunks.

Motivation, use-cases

Unstable [T]::as_chunks method allows converting &[T] into &[[T; N]]. However, to handle slices whose length doesn’t divide by N the method returns (chunks, remainder) tuple. If one wants to assert an empty remainder they may need to resort to destructuring the tuple and introducing if statement. Similarly, it doesn’t mesh nicely with method chaining. A chain of methods needs to be stopped to verify the second element of the returned tuple before proceeding.

Introducing TryFrom<&[T]> implementation for &[[T; N]] offers a Result-based interface for chunking a slice. Result integrates with many interfaces of the language which results in more concise code.

Compare:

// Before:

fn pairs(slice: &[u8]) -> Result<&[[u8; 2]]> {
    let (chunks, remainder) = slice.as_chunks::<2>();
    if remainder.is_empty() {
        Ok(chuns)
    } else {
        Err(SomeError)
    }
}

// Alternatively with let-else:

fn pairs(slice: &[u8]) -> Result<&[[u8; 2]]> {
    let (chunks, []) = slice.as_chunks::<2>() else {
        return Err(SomeError);
    };
    Ok(chunks)
}

// After, more concise with a single expression:

fn pairs(slice: &[u8]) -> Result<&[[u8; 2]]> {
    slice.try_into().map_err(|_| SomeError)
}
// Before, with use of let-else:

let bytes = get_bytes_in_a_vector();
let (chunks, []) = bytes.as_chunks() else {
    return Err(SomeError.into());
};
let u64s = chunks
    .into_iter()
    .map(|chunk| u64::from_le_bytes(*bytes))
    .collect::<Vec<_>>()

// After, single statement with no need for temporary variables:

let u64s = get_bytes_in_a_vector()
    .as_slice()
    .try_into::<&[[u8; 8]]>(bytes)
    .map_err(|_| SomeError)?
    .into_iter()
    .map(|chunk| u64::from_le_bytes(*bytes))
    .collect::<Vec<_>>();

Solution sketches

Solution is implemented in https://github.com/rust-lang/rust/pull/105316 and has the form:

impl<'a, T, const N: usize> TryFrom<&'a [T]> for &'a [[T; N]] {
    type Error = TryFromSliceError;

    fn try_from(slice: &'a [T]) -> Result<Self, Self::Error> {
        let (chunks, remainder) = slice.as_chunks::<N>();
        if remainder.is_empty() {
            Ok(chunks)
        } else {
            Err(TryFromSliceError(()))
        }
    }
}

mina86 avatar Apr 01 '23 15:04 mina86

Not sure why this is more useful than the method that returns both. At least with as_chunks, you can iterate over the chunks that do exist, even if it doesn't fit evenly.

dead-claudia avatar Apr 06 '23 09:04 dead-claudia

It’s useful if length of the slice not being divisible by the length of a chunk is an error. If you want to go through all the available chunks regardless of remainder than indeed you wouldn’t use this.

mina86 avatar Apr 06 '23 17:04 mina86

How do you propose that this impl handles the N == 0 case?

scottmcm avatar Apr 06 '23 17:04 scottmcm

Ideally it wouldn’t compile (which also makes me realise that suggested solution doesn’t currently do that but that can be addressed with a simple compile-time assert).

mina86 avatar Apr 06 '23 17:04 mina86

@mina86 We reviewed this in today's @rust-lang/libs-api meeting, and there were objections to adding a TryFrom impl like this that changes the "shape" of the array, but we were generally happy with the idea of adding a named method for this (e.g. returning Option).

joshtriplett avatar Mar 05 '24 17:03 joshtriplett

Also, separately, array_chunks might help.

joshtriplett avatar Mar 05 '24 17:03 joshtriplett

This is the objection I shared in the meeting:

I don't think we should have (Try)From implementations that change the "shape" of a data structure like this. Such conversions imply that nested [ and ] are just "noise" that can be ignored, rather than a significant and meaningful part of the data structure.

(If we accept this ACP, would we also have to add all kind of other (Try)From impls that do various forms of flattening and (re-)grouping?)

Having a method for this, however, sounds good to me!

we decided

To clarify, this wasn't a full team decision, but additions like these require team consensus, which cannot be reached when there are objections from team members.

Adding and stabilizing a method for this still requires a team decision (through FCP), but based on the temperature check in the meeting, it seems likely that'd be accepted.

m-ou-se avatar Mar 05 '24 17:03 m-ou-se

The meeting notes mention the possibility of as_chunks_exact, which sounds good to me. It could go under https://github.com/rust-lang/rust/issues/74985 with the rest of the things there, and has the same "needs an answer for N == 0" problem that the rest of them do.

(It's also nice in that it can talk about how as_chunks and as_rchunks give the same results if as_chunks_exact would succeed.)


And +1 to not being fond of this as a trait. I think under the https://doc.rust-lang.org/std/convert/trait.From.html#when-to-implement-from vernacular, it's not "value-preserving" because the "conceptual kind" of the answer is different.

scottmcm avatar Mar 05 '24 22:03 scottmcm

The _exact variant does not necessarily suffer from the N==0 case if it returns an option because we can always return None in that case.

It would make the pairs example even simpler

fn pairs(slice: &[u8]) -> Option<&[[u8; 2]]> {
    slice.as_chunks_exact()
}

the8472 avatar Mar 05 '24 23:03 the8472