-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Comments
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`
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
…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
Is there any intention of providing the reverse versions of these functions? |
What would that look like? |
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. |
@joshlf |
That's only true if |
Ah, I misunderstood your use case, you want |
Yep, exactly 😃 |
Yeah, array_rchunks is definitely worth having. |
Also useful would be the inverse operation: |
Do we need to wait until |
Hi, is there any alternatives before this is stable? Thanks! |
Yeah |
Thanks! |
That would mean the iterator can't be If people want |
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));
} The current behaviour of Rust in this situation is to compile with no errors or warnings (not even on the call to Meanwhile, if you just write I don't think the situation of, e.g., 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 |
The remaining issue here is what to do on #74985 (comment) it is mentioned above that apparently the lang team prefers runtime panics over post-mono errors, but I don't see any links to discussion about this or justification for why. Runtime errors do have the advantage that they won't be triggered in dead code. The arguments for making it a post-mono error are clear: getting errors earlier (and at all if the code is not tested). Post mono errors won't error in Other comments above make additional arguments in favor of post-mono errors, and the documentation of the methods also intended to make it a post-mono error eventually. |
@rustbot labels +I-lang-nominated I'm not sure whether what was mentioned about our lang preferences is actually true, so let's nominate to discuss. cc @rust-lang/lang |
I don't remember what exactly the argument against post-mono errors was but I do remember thinking "that does makes sense", even though I prefer an earlier error. I want to say @BurntSushi may have originally expressed the concern? If we do go with the runtime panic, we should document that a future Rust version is allowed to reject |
I think #74985 (comment) is a good example why a runtime check is desirable, so users can be flexible in their own APIs around this. If I write |
That’s current limitation in expressiveness of the language, no? I can easily imagine: impl [T] {
fn as_chunks<const N: NonZeroUsize>() -> (&[[T; N]], &[T]);
}
fn foo<const N: usize>(slice: &[T]) -> Option<i32> {
let chunks = const if let Some(N) = NonZeroUsize::new(N) {
slice.as_chunks::<N>().0;
} else {
return None;
};
todo!()
} That would require a few things from the language:
To me it seems that the question is whether we want to wait for all that or stabilise. |
Maybe neither here nor there, but note that the following works: const fn divide<const D: u64>(n: u64) -> u64 {
const { assert!(D > 0) }; // <--- Post-mono assertion.
n / D
}
const fn divide_checked<const D: u64>(n: u64) -> Option<u64> {
match D {
0 => None,
1.. => Some(divide::<D>(n)),
}
}
fn main() {
dbg!({const { divide_checked::<1>(1) }});
dbg!({const { divide_checked::<0>(1) }}); // <--- Look here.
dbg!(divide_checked::<1>(1));
} That is, if the function within which we want to compile-time assert is parametric over a type or const generic, is const, and we're always in a const context when we'd see the "bad" generic argument for it, then something like what we want here does work currently. (The question of whether we'll guarantee to not later disallow this is #122301.) Interestingly, this is the odd case of a call that we allow in a const context but disallow outside of one. |
From the options in #74985 (comment) we would like a combination of point 2 and 3. Runtime panics are not great because they fail it compiles, it works or make illegal states non-representatble principles, for precisely the thing that we're making legible to the type system. The current CTFE approach, i.e. post-mono errors, is suboptimal because it would not be forwards-compatible with pattern types or where bounds. It would also bet difficult to work with for a more-generic caller. The const-trick from #122301 ¹² would help with the latter issue but not with the former. It's also a somewhat obscure trick. So we'd like So the second desired feature is a way for generic callers to locally discharge the restriction. E.g. // std function
fn chunks<const N: usize>()
where
const { N > 0 },
{
// ...
}
// user code
fn use_chunks<const N: usize>() {
where N > 0 {
chunks<N>();
} else {
fallback();
}
} ¹ That issue was actually motivated by this one, but it turned out insufficient. |
I agree with you that this would be a nice way to implement this. But I think this feature is so incredibly far off that we really shouldn't be blocking this very useful helper (that replaced chunks_exact, which has a lot more room for bugs) for literal decades because of one of its weird error edge cases. |
I would suggest going with a post mono error, as that has significant advantages. The only disadvantage is that it may impede flexibility. After stabilizing, in the post mono error, we could link to a tracking issue where people can complain if their use case was blocked by this inflexibility, and we should be able to make it a runtime error in case it's a problem (since that's backwards compatible as long as we document that people shouldn't rely on it being a post mono error). |
Would any of the people involved in the current state of the relevant const generic bounds involving pattern (types?)... I'm not entirely sure if I should be asking @oli-obk or @BoxyUwU... be able to weigh in on the projected development cycle for such a feature as libs-api is asking? @Noratrieb has projected "literal decades" and I have no reason to doubt such a guess, currently, but are we talking 1 or 10? |
Uh, please do not put pattern types into the consideration for any API decisions. They are an internal language feature and will stay so for the forseeable future. Getting them anywhere production ready is definitely off far enough that waiting for them is at present like waiting for specialization to be stable. If you'd like to know more, join https://rust-lang.zulipchat.com/#narrow/channel/481660-t-lang.2Fpattern-types |
I think there is basically no universe in which we get One thing that could be interesting is adding an unstable attribute for const generics that requires the argument to be fully concrete, i.e. forbid |
Ok then our options are
I was gesturing at a general category of features to solve our problems by limiting the range of valid const values. If |
I think just a mono time error seems fine. There are enough APIs that we would like to improve with better const checks, this one isn't particularly special. We can just ship it and handle it however we'll handle these in the future. Even a panic doesn't seem bad, we do that for lots of APIs in libstd |
Which stable ones are you thinking of? There are a bunch of array ones like this one that have been kept unstable due to this issue.
I don't think anyone has mentioned a plan how to handle these other than living with the limitation. |
I think this might be worth looking into, if it's not too difficult to implement. |
for the record I don't expect the lang feature for concrete calls to take very long. it's the kind of thing that could probably be implemented in a day or so. but the transitioning off it to a proper type system feature for requiring |
@rustbot labels -I-lang-nominated We discussed this in lang triage today. In particular, it had been asserted and then repeated in this thread that the lang team prefers runtime panics over post-mono errors. That's not the case. We like errors to happen as soon as possible. So we prefer pre-mono errors over post-mono errors when possible, but similarly we prefer post-mono errors over runtime errors when that is possible and practical. It's of course for libs-api to decide what's practical in this case. Regarding what counsel we can provide on the language direction and timeline here, probably all of us want something in the spirit of |
Sounds good. It seems like our preferred paths are the slightly hacky As the compiler magic that prevents jamming in random expressions is
It seems like the logical first move would be the enforced concrete arguments, then? |
One of the truly magical things about for [a, b, c] in slice.array_chunks() {
// do stuff with a, b, and c
} If we want a solution that enforces a non zero uint, we should take care that it doesn't break inference. EDIT: Also, the inferred length should count as "concrete", if we implement a concrete enforcing attribute. |
I would say that lang wants compile-time checks where feasible, but also wants to avoid nuisance compilation failures that you can't suppress even though the thing it's complaining about is only in dead code. TBH I'm tempted to just give this a runtime assert and a custom lint that's deny-by-default if it notices I'm reminded of the runtime assert in https://doc.rust-lang.org/nightly/std/primitive.slice.html#method.as_flattened, where I just think it's fine, and better than blocking arrays-of-ZSTs entirely in post-mono. (Though admittedly that one requires the combination of two unlikely things, not just one.) |
Maybe just give chunk size 0 a well-defined behavior such as never evaluating the loop expression? And there can be a runtime panic in debug mode like for integer overflow. I'd like to avoid a runtime panic being unconditionally compiled into code for N > 0. If we can assume that the compiler will eliminate it, maybe that's fine. |
It’s an if on a const expression, so surely it will be optimized out. |
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
RestrictMakeN
to values greater than 0 at compile time instead of using a runtime panic.<[T]>::array_*
methods fail to compile on 0 len arrays #99471N == 0
is unreachable due to conditionals Tracking Issue for slice::array_chunks #74985 (comment)Unresolved Questions
[T]::array_chunks
really need to be an iterator? #76354Implementation history
slice::array_chunks
to std #74373slice::array_chunks_mut
#75021as_chunks{_mut}
in Add [T]::as_chunks(_mut) #76635as_rchunks{_mut}
and unchecked versions in Addas_rchunks
(and friends) to slices #78818as_(r)chunk
in constifyslice_as_chunks
(unstable) #111453The text was updated successfully, but these errors were encountered: