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

Tracking Issue for slice::array_chunks #74985

Open
5 tasks
lcnr opened this issue Jul 31, 2020 · 39 comments
Open
5 tasks

Tracking Issue for slice::array_chunks #74985

lcnr opened this issue Jul 31, 2020 · 39 comments
Labels
A-array Area: `[T; N]` A-const-generics Area: const generics (parameters and arguments) A-slice Area: `[T]` C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC F-generic_const_exprs `#![feature(generic_const_exprs)]` Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@lcnr
Copy link
Contributor

lcnr commented Jul 31, 2020

The feature gate for the issue is #![feature(array_chunks)].

Also tracks as_(r)chunks(_mut) in #![feature(slice_as_chunks)].

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also uses as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

Unresolved Questions

Implementation history

@lcnr lcnr added C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC A-const-generics Area: const generics (parameters and arguments) F-const_generics `#![feature(const_generics)]` T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jul 31, 2020
@KodrAus KodrAus added Libs-Tracked Libs issues that are tracked on the team's project board. A-slice Area: `[T]` labels Aug 6, 2020
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 12, 2020
Add `slice::array_chunks_mut`

This follows `array_chunks` from rust-lang#74373 with a mutable version, `array_chunks_mut`. The implementation is identical apart from mutability. The new tests are adaptations of the `chunks_exact_mut` tests, plus an inference test like the one for `array_chunks`.

I reused the unstable feature `array_chunks` and tracking issue rust-lang#74985, but I can separate that if desired.

r? `@withoutboats`
cc `@lcnr`
tmandry added a commit to tmandry/rust that referenced this issue Sep 16, 2020
Add array_windows fn

This mimicks the functionality added by array_chunks, and implements a const-generic form of
`windows`. It makes egregious use of `unsafe`, but by necessity because the array must be
re-interpreted as a slice of arrays, and unlike array_chunks this cannot be done by casting the
original array once, since each time the index is advanced it needs to move one element, not
`N`.

I'm planning on adding more tests, but this should be good enough as a premise for the functionality.
Notably: should there be more functions overwritten for the iterator implementation/in general?

~~I've marked the issue as rust-lang#74985 as there is no corresponding exact issue for `array_windows`, but it's based of off `array_chunks`.~~

Edit: See Issue rust-lang#75027 created by @lcnr for tracking issue

~~Do not merge until I add more tests, please.~~

r? @lcnr
JohnTitor added a commit to JohnTitor/rust that referenced this issue Oct 26, 2020
…lbertodt

Add [T]::as_chunks(_mut)

Allows getting the slices directly, rather than just through an iterator as in `array_chunks(_mut)`.  The constructors for those iterators are then written in terms of these methods, so the iterator constructors no longer have any `unsafe` of their own.

Unstable, of course. rust-lang#74985
@joshlf
Copy link
Contributor

joshlf commented Feb 10, 2021

Is there any intention of providing the reverse versions of these functions?

@thomcc
Copy link
Member

thomcc commented Apr 25, 2021

Is there any intention of providing the reverse versions of these functions?

What would that look like?

@joshlf
Copy link
Contributor

joshlf commented Apr 26, 2021

Same API, but iterating backwards. I have a use case in which I need to convert either a prefix or a suffix of a slice to an array reference. This API is perfect for the prefix use case, and I'd love to be able to use it for the suffix use case as well.

@newpavlov
Copy link
Contributor

@joshlf
Both slice of arrays and custom iterator type would implement the DoubleEndedIterator, so your use case does not need a separate reverse method.

@joshlf
Copy link
Contributor

joshlf commented Apr 26, 2021

That's only true if N evenly divides the length of the slice (see array_chunks docs). Otherwise, the remaining elements are only accessible via the remainder method, and won't be iterated over in the DoubleEndedIterator implementation.

@newpavlov
Copy link
Contributor

Ah, I misunderstood your use case, you want [0, 1, 2, 3, 4, 5, 6] to be split into [4, 5, 6], [1, 2, 3], [0], not into [6], [3, 4, 5], [0, 1, 2]. It indeed requires a separate method.

@joshlf
Copy link
Contributor

joshlf commented Apr 26, 2021

Yep, exactly 😃

@thomcc
Copy link
Member

thomcc commented Apr 27, 2021

Yeah, array_rchunks is definitely worth having.

@sffc
Copy link

sffc commented May 12, 2021

Also useful would be the inverse operation: [[0, 1, 2], [3, 4, 5]] to [0, 1, 2, 3, 4, 5]. (from &[[T; 3]] to &[T])

@ChayimFriedman2
Copy link
Contributor

Do we need to wait until const_evaluatable_unchecked is stabilized so we can deny N = 0 at compile time to stabilize this feature? Perhaps we can just mention in the documentation that currently it panics but it may be uplifted to a compile time error in the future.

@fzyzcjy
Copy link

fzyzcjy commented Oct 7, 2021

Hi, is there any alternatives before this is stable? Thanks!

@CryZe
Copy link
Contributor

CryZe commented Oct 7, 2021

Hi, is there any alternatives before this is stable? Thanks!

Yeah bytemuck::cast_slice and bytemuck::cast_slice_mut can do mostly the same (with a Pod requirement though)

@fzyzcjy
Copy link

fzyzcjy commented Oct 7, 2021

Thanks!

bors added a commit to rust-lang-ci/rust that referenced this issue May 19, 2023
…imulacrum

constify `slice_as_chunks` (unstable)

Tracking issue: rust-lang#74985

Nothing complicated required; just adding `const` to the declarations.
AngelicosPhosphoros added a commit to AngelicosPhosphoros/rust that referenced this issue May 20, 2023
Methods updated:
* `array_windows`
* `array_chunks`
* `array_chunks_mut`
* `as_chunks`
* `as_chunks_mut`
* `as_chunks_unchecked`
* `as_rchunks`
* `as_rchunks_mut`
* `as_chunks_unchecked_mut`

I implemented this using compile time assertions.

Example compilation error:

```
> rustc +stage1 .\improper_array_windows.rs --crate-type=rlib
error[E0080]: evaluation of `core::slice::<impl [u32]>::array_windows::<0>::{constant#0}` failed
    --> J:\rust_lang\library\core\src\slice\mod.rs:1381:13
     |
1381 |             assert!(N != 0, "window size must be non-zero");
     |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the evaluated program panicked at 'window size must be non-zero', J:\rust_lang\library\core\src\slice\mod.rs:1381:13
     |
     = note: this error originates in the macro `$crate::panic::panic_2021` which comes from the expansion of the macro `assert` (in Nightly builds, run with -Z macro-backtrace for more info)

note: the above error was encountered while instantiating `fn core::slice::<impl [u32]>::array_windows::<0>`
 --> .\improper_array_windows.rs:4:14
  |
4 |     for _ in s.array_windows::<0>(){
  |              ^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error
```

I also added doctests and adjusted documentation.

Relates:
rust-lang#74985
rust-lang#75027
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue May 25, 2023
Add slice::{split_,}{first,last}_chunk{,_mut}

This adds to the existing tracking issue for `slice::array_chunks` (rust-lang#74985) under a separate feature, `slice_get_chunk`.

Currently, we have the existing `first`/`last` API for slices:

```rust
impl [T] {
    pub const fn first(&self) -> Option<&T>;
    pub const fn first_mut(&mut self) -> Option<&mut T>;
    pub const fn last(&self) -> Option<&T>;
    pub const fn last_mut(&mut self) -> Option<&mut T>;
    pub const fn split_first(&self) -> Option<(&T, &[T])>;
    pub const fn split_first_mut(&mut self) -> Option<(&mut T, &mut [T])>;
    pub const fn split_last(&self) -> Option<(&T, &[T])>;
    pub const fn split_last_mut(&mut self) -> Option<(&mut T, &mut [T])>;
}
```

This augments it with a `first_chunk`/`last_chunk` API that allows retrieving multiple elements at once:

```rust
impl [T] {
    pub const fn first_chunk<const N: usize>(&self) -> Option<&[T; N]>;
    pub const fn first_chunk_mut<const N: usize>(&mut self) -> Option<&mut [T; N]>;
    pub const fn last_chunk<const N: usize>(&self) -> Option<&[T; N]>;
    pub const fn last_chunk_mut<const N: usize>(&mut self) -> Option<&mut [T; N]>;
    pub const fn split_first_chunk<const N: usize>(&self) -> Option<(&[T; N], &[T])>;
    pub const fn split_first_chunk_mut<const N: usize>(&mut self) -> Option<(&mut [T; N], &mut [T])>;
    pub const fn split_last_chunk<const N: usize>(&self) -> Option<(&[T; N], &[T])>;
    pub const fn split_last_chunk_mut<const N: usize>(&mut self) -> Option<(&mut [T; N], &mut [T])>;
}
```

The code is based off of a copy of the existing API, with the documentation and examples properly modified. Currently, the most common way to perform these kinds of lookups with the existing methods is via `slice.as_chunks::<N>().0[0]` or the worse `slice.as_chunks::<N>().0[slice.len() - N]`, which is substantially less readable than `slice.first_chunk::<N>()` or `slice.last_chunk::<N>()`.

ACP: rust-lang/libs-team#69
@YizhePKU
Copy link

YizhePKU commented Jun 4, 2023

I think there should be a safe version of as_chunks_unchecked.

Usually, an "unchecked" version of a function does the same thing as their checked counterpart. Currently, this is not the case.

I propose we rename the current as_chunks_unchecked to as_chucks_exact_unchecked and then add as_chucks_exact.

@c410-f3r
Copy link
Contributor

c410-f3r commented Aug 7, 2023

What is the status of this feature?

@c410-f3r
Copy link
Contributor

Looks like that checking of N == 0 is the only blocker, right? If so, then we probably already have the answer from the responsible party.

In other words, it seems that officially runtime checks are preferable over compile-time checks so can we proceed with stabilization?

@jhpratt
Copy link
Member

jhpratt commented Oct 17, 2023

Permitting something to compile knowing with 100% certainty that it will panic seems like an odd decision to make, given that we have the capability to prevent it. Post-monomorphization errors aren't ideal, but surely it's better than an unconditional runtime panic?

@jDomantas
Copy link
Contributor

The problem is that it's not "100% certainty that it will panic". There was an example in another thread (#99471 (comment)) of code that is perfectly legal, but adding post-monomorphization error makes it fail to compile:

fn f<const N: usize>() {
    if N == 0 {
        ...
    } else {
        foo.as_chunks::<N>()...
    }
}

fn main() {
    f::<0>();
}

@jhpratt
Copy link
Member

jhpratt commented Oct 17, 2023

Ah, that makes sense. So unless we can solve the halting problem, a runtime error it is…

@mina86
Copy link
Contributor

mina86 commented Oct 17, 2023

If the choice is between stabilising it as is with a runtime error, and keeping it experimental until static if and where N > 0 or N: NonZeroUsize becomes a thing, I vote for keeping this experimental.

@joboet
Copy link
Member

joboet commented Oct 17, 2023

I haven't found the time to work on it for the last few weeks, but I think my pre-RFC for pattern types has a good solution for this when combined with rust-lang/project-const-generics#26.

@suprohub

This comment was marked as spam.

@jhpratt
Copy link
Member

jhpratt commented Apr 30, 2024

Now that inline const is stabilized, we should be able to make compilation fail when N == 0. It's not perfect (as mentioned in #99471, but it would be closer to the desired behavior. Likewise for array_windows (#75027)

@Xiretza
Copy link
Contributor

Xiretza commented Apr 30, 2024

I think the lang team has made it clear repeatedly that they prefer runtime panics over post-mono errors. Personally I don't agree, but I don't think it's worth arguing over anymore.

@jhpratt
Copy link
Member

jhpratt commented Apr 30, 2024

I don't agree either, but it's also not up to me. This issue has it crossed out, but that doesn't appear to be a formal decision in either direction. The docs currently say

This check will most probably get changed to a compile time error before this method gets stabilized.

So it seems to remain an open question?

@c410-f3r
Copy link
Contributor

c410-f3r commented Apr 30, 2024

A reduced example that should compile according to the responsible team.

pub fn foo<const N: usize>() -> i32 {
    const {
        assert!(N != 0);
    }
    1
}

pub fn bar<const N: usize>() -> i32 {
    if N == 0 {
        2
    } else {
        foo::<N>()
    }
}

fn main() {
    assert_eq!(bar::<0>(), 2);
}

Maybe something about the PR that unblocked inline constants (#122568) could help here?

There are then 4 scenarios for stabilization:

  1. Compile-time checks with the current CTFE behavior.
  2. Compile-time checks with a machinery that allows the above snippet.
  3. Tracking Issue for slice::array_chunks #74985 (comment)
  4. Runtime checks, which I also personally disagree.

cc rust-lang/rfcs#3582
cc #122301

@jhpratt
Copy link
Member

jhpratt commented May 1, 2024

Thanks for that example. I've added it to the top comment.

@its-the-shrimp
Copy link
Contributor

I haven't seen it discussed here yet, but another way to tackle the case of N == 0 is to not tackle it and just make the iterator return [T; 0] until the end of time, what do you all think about this?

@scottmcm
Copy link
Member

and just make the iterator return [T; 0] until the end of time

That would mean the iterator can't be ExactSizeIterator, so I think it's unacceptable.

If people want iter::repeat([]) they should use that; I don' think getting that from array_chunks is ever what anyone calling array_chunks actually wants.

@ais523
Copy link

ais523 commented Aug 16, 2024

On the subject of the "what should happen if the const argument is 0" debate, I'd like to present what I think is an analogous situation:

fn divide<const D: u32>(n: u32) -> u32 {
    n / D
}

fn divide_checked<const D: u32>(n: u32) -> Option<u32> {
    if D == 0 {
        None
    } else {
        Some(n / D)
    }
}

fn main() {
    dbg!(divide_checked::<5>(99));
    dbg!(divide_checked::<0>(99));
    dbg!(divide::<5>(99));
    dbg!(divide::<0>(99));
}

Playground.

The current behaviour of Rust in this situation is to compile with no errors or warnings (not even on the call to divide::<0>), and to panic at runtime. In other words, if the divisor is coming from a const generic, there's no check for validity, even though division by 0 is always meaningless, and even though divide::<0> always divides by 0 and can easily be observed at compile time to do so.

Meanwhile, if you just write 99/0 directly, you get a deny-by-default lint, unconditional_panic, which makes sense given that the code can never run successfully.

I don't think the situation of, e.g., [T]::as_chunks::<0> is significantly different from that of division by constant zero – nor do I think there would be any particular benefit from making them act differently. I also suspect that there is unlikely to be a better solution than "[T]::as_chunks::<0> panics at runtime if it is ever called, and produces a deny-by-default lint if you write the 0 explicitly, but not if the 0 is calculated based on a constant generic" – this gives the compile-time error we want in the cases where the statement is obviously meaningless, but allows the code to compile in cases where it might not be called due to expanded generic code.

It's also worth noting an edge case:

let fn_pointer = <[u32]>::as_chunks::<0>;

Do we want to allow this, or not? With the explicit ::<0>, I can see a reasonable argument for throwing the unconditional_panic lint, with a message along the lines of "this function pointer will panic if it is ever called". I can't see a use for writing it directly (although I can see how it could potentially happen in expanded generic code, which could end up never calling the function in cases where the parameter is 0).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-array Area: `[T; N]` A-const-generics Area: const generics (parameters and arguments) A-slice Area: `[T]` C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC F-generic_const_exprs `#![feature(generic_const_exprs)]` Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests