Make <[T]>::array_* methods fail to compile on 0 len arrays

[AngelicosPhosphoros]

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:
#74985
#75027

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[AngelicosPhosphoros]

There are also few other methods, would change them too.

[lcnr]

while it results in a better user experience than runtime panic, this solution has the issue that it only panics during monomorphization, so check builds compile successfully, even for N == 0.

This also means that fn foo<const N: usize>() { let _ = [1, 2, 3, 4].array_chunks::<N>(); } compiles. If we then later change the N == 0 bound to an actual where-clause on array_chunks, foo will stop compiling.

So this doesn't allow us to stabilize these methods as is and add a 0 check as a where clause later.

@rustbot label +T-libs-api -T-libs

[lcnr]

r? rust-lang/libs

[AngelicosPhosphoros]

while it results in a better user experience than runtime panic, this solution has the issue that it only panics during monomorphization, so check builds compile successfully, even for N == 0.

Ooops, really rustc +stage1 .\improper_array_windows.rs --crate-type=rlib --emit=metadata -Z no-codegen doesn't complain.
However, this would prevent nightly users from writing some code that we would broke later by adding bound.

[AngelicosPhosphoros]

Added this check to more methods.

[AngelicosPhosphoros]

Also relevant: #99682

[AngelicosPhosphoros]

@joshtriplett

Hi! Do I need to do something with this PR?

[bors]

☔ The latest upstream changes (presumably #100759) made this pull request unmergeable. Please resolve the merge conflicts.

[nbdd0121]

Might just do const { assert!(N != 0); }?

[the8472]

As other reviewers already mentioned, asserts in inline const blocks work now and should be used.

@rustbot author

[AngelicosPhosphoros]

@joshtriplett @the8472
Sorry for delay, I was forced to leave my country and had been some time without suitable equipment to work.

I switched to inline const blocks as requested.

[AngelicosPhosphoros]

@rustbot label -S-waiting-on-author +S-waiting-on-review

[AngelicosPhosphoros]

@scottmcm Old docs for the methods mentioned that in future any instantiations with N == 0 would stop compile. My assertions ensure that we wouldn't broke someones code later when we add premonomorphisation check.

Also, we can always remove checks later if we wish because it would not be breaking changes.

[nbdd0121]

Ideally this would be a where N > 0 bound. But the compile time assert is not a proper substitute for that because bound can be infectious while the assert only fires for concrete instantiations.

Where bound would stop you writing if N != 0 { foo.as_chunks::<N>() } else { ... } at all.

[the8472]

Ideally this would be a where N > 0 bound. But the compile time assert is not a proper substitute for that because bound can be infectious while the assert only fires for concrete instantiations.

Where bound would stop you writing if N != 0 { foo.as_chunks::<N>() } else { ... } at all.

You can then have a separate implementation for N = 0. Assuming the trait solver can then figure out that for each N there exists exactly one implementation.

[CAD97]

The full-fat solution to working with generic instantiation errors is some sort of static if (C++ calls it "constexpr if"), e.g.

static if N != 0 {
    foo.as_chunks::<N>() // ...
} else {
    // ...
}

where each arm of the static if is only instantiated if the controlling condition (which is constant evaluated) says to take that arm.

With arbitrary requirements and conditions, we probably can't prevent instantiation errors (post-mono errors), but we could probably utilize pattern restricted types to effect here (i.e. instead of N: usize, N: usize is 1..). (Though that might have the additional effect of delaying these APIs by perhaps a decade or more...)

[bors]

☔ The latest upstream changes (presumably #111453) made this pull request unmergeable. Please resolve the merge conflicts.

[AngelicosPhosphoros]

I would like to merge this PR already because it just gather merge conflicts as it stays.

So I like to address all concerns in a single place:

1. Compilation error happens only during monomorphization (e.g. cargo build vs cargo check). True, but compile error during monomorphization is still better than runtime panic.
2. If we decide later to support case which @scottmcm brought up (if N != 0 { my_slice.array_xxx::<N>() } else { do something else }), we can always relax this check later. Removing const from assertions will not break code with N != 0. And const assert added now would prevent breaking users code later if we decide that we don't support this usecase.
3. About allowing N==0 and making infinite loop from it. I am against this proposition for 2 reasons:
   • It is just confusing. Infinite loops are uncommon thing so they should be made only using loop {}. Making iteration over chunks infinite loop would be surprising for programmer almost always.
   • It contradicts existing behaviour of windows(n) and chunks(n) methods so should not be allowed for sake of consistency.

TLDR: We cannot broke any valid code by adding this, we can always remove this later and it would prevent breaking more code if we keep this behaviour.

[joshtriplett]

I don't feel comfortable unilaterally approving this.

cc @rust-lang/libs-api

[BurntSushi]

Let's FCP this perhaps?

@rfcbot fcp merge

[rfcbot]

Team member @BurntSushi has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se

Concerns:

• monomorphization errors (Make <[T]>::array_* methods fail to compile on 0 len arrays #99471 (comment))

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[BurntSushi]

@rfcbot concern monomorphization errors

True, but compile error during monomorphization is still better than runtime panic.

I don't agree generally.

I don't think we should be litigating whether we want to start introducing more monomorphization errors in this PR. It should probably be done as a separate decision, albeit I'm not sure by what process and by what team.

[dtolnay]

I agree with #99471 (comment) and I would strongly prefer not to accept this PR. The following is fine and correct code; we do not need to make it fail to compile.

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

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

[BurntSushi]

Given @dtolnay's comment above, the FCP seems unnecessary.

@rfcbot fcp cancel

[rfcbot]

@BurntSushi proposal cancelled.

[the8472]

I'd like fn as_chunks::<N>(&self) where N > 0. But this would require constructs such as the static if mentioned in this comment. As long as we don't have the tools to make that usable compile-time enforcement is more of a hindrance.

[AngelicosPhosphoros]

True, but compile error during monomorphization is still better than runtime panic.

I don't agree generally.

Maybe allow such compile errors for unstable functions/traits/structs until we implement bounds on constant generics?

[dtolnay]

Thanks anyway for the PR! This was worth exploring.