-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
Add IntoIterator
for Box<[T]>
+ edition 2024-specific lints
#124097
Conversation
Needs a T-compiler review of the impl first, then can go into FCP. |
0aafa34
to
6fcbd7a
Compare
@@ -184,7 +184,7 @@ impl<'a, T> IntoIterator for &'a P<[T]> { | |||
type Item = &'a T; | |||
type IntoIter = slice::Iter<'a, T>; | |||
fn into_iter(self) -> Self::IntoIter { | |||
self.ptr.into_iter() | |||
self.ptr.iter() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very funny to see somewhere in the compiler where we were relying on this behavior.
This comment has been minimized.
This comment has been minimized.
I looked over the PR and I think we can kick off FCP in tandem with code review. The implementation includes a full migration that behaves as I'd expect. @rust-lang/lang Are we okay with applying the same method resolution hack used for slices in 2021 to boxed slices in 2024? @rust-lang/libs-api Are we okay with insta-stabilizing @rfcbot fcp merge |
Team member @tmandry has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. 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! cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. |
The |
Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
For FCP folks, see some of the discussion in #116607 -- apparently reusing Also, this PR doesn't implement If anyone has a strong opinion, I could either add to this PR, or (ideally) add to this PR in a follow-up. It would only cause a breaking change when the user is relying on the exact |
This comment has been minimized.
This comment has been minimized.
My personal opinion here is that it would be desirable to have a heap-based iterator here, since using the array implementation would copy the entire array onto the stack before iterating. It also helps avoid the weird discrepancy between Worth noting that a lot of people will use |
…=<try> [crate] Add `Box<[T; N]>: IntoIterator` without any method dispatch hacks **Unlike** `Box<[T]>` (rust-lang#116607 (comment)), there's a much higher chance that this will not conflict with existing usages since it produces an iterator with the same type before/after this change, but let's test that theory with crater. Ideally we have fewer migrations that are tangled up in hacks like `rustc_skip_during_method_dispatch`, so if this crater comes back clean, I'd strongly suggest landing this as-is. As for the rationale for having this impl at all, I agree (as `@clarfonthey` pointed out in rust-lang#124097 (comment)) that it is generally better for any user to not require moving the array *out* of the box just to turn it into an iterator.
@rustbot author |
…not-notable, r=fmease rustdoc: Negative impls are not notable In rust-lang#124097, we add `impl !Iterator for [T]` for coherence reasons, and since `Iterator` is a [notable trait](https://github.com/rust-lang/rust/blob/8387315ab3c26a57a1f53a90f188f0bc88514bca/library/core/src/iter/traits/iterator.rs#L40), this means that all `-> &[_]` now are tagged with a `!Iterator` impl as a notable trait. I "fixed" the failing tests in that PR with 6cbbb8b, where I just blessed the tests, since I didn't want to mix these changes with that PR; however, don't believe negative impls are notable, and this PR aims to prevent these impls from being mentioned. In the standard library, we use negative impls purely to guide coherence. They're not really a signal of anything useful to the end-user. If there ever is a case that we want negative impls to be mentioned as notable, this really should be an opt-in feature.
☔ The latest upstream changes (presumably #125006) made this pull request unmergeable. Please resolve the merge conflicts. |
6cbbb8b
to
917bb83
Compare
@bors r=WaffleLapkin |
☀️ Test successful - checks-actions |
Finished benchmarking commit (e8fbd99): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results (primary -2.1%, secondary -3.4%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (secondary -4.0%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeResults (secondary 0.0%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 669.761s -> 671.391s (0.24%) |
@rustbot labels +relnotes We discussed in the edition call today that this should probably go in the release notes for Rust 1.80. |
[T;N]: IntoIterator
implementation for edition 2021..into_iter()
calls, new source of method ambiguity).ARRAY_INTO_ITER
lint, since it was kind of confusing.Based mostly off of #116607.
ACP: rust-lang/libs-team#263
References #59878
Tracking for Rust 2024: #123759
Crater run was done here: #116607 (comment)
Consensus afaict was that there is too much breakage, so let's do this in an edition-dependent way much like
[T; N]: IntoIterator
.