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

Box<[T]> should have an IntoIter implementation. #59878

Closed
emilio opened this issue Apr 11, 2019 · 12 comments
Closed

Box<[T]> should have an IntoIter implementation. #59878

emilio opened this issue Apr 11, 2019 · 12 comments
Labels
A-iterators Area: Iterators A-maybe-future-edition Something we may consider for a future edition. C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@emilio
Copy link
Contributor

emilio commented Apr 11, 2019

Right now the only way to move out of a Box<[T]> is using into_vec, which is a bit inconvenient. Is there any reason Box<[T]> shouldn't implement IntoIter?

@jonas-schievink jonas-schievink added A-iterators Area: Iterators C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Apr 11, 2019
@SimonSapin
Copy link
Contributor

Unfortunately this has the same issue as #25725: boxed_slice.into_iter() works today through auto-deref (then auto-ref?) to <&[T] as IntoIterator>::into_iter, so adding this impl would change the meaning of (some) existing code.

Whatever we end up deciding should probably apply to both.

CC @rust-lang/libs

@SimonSapin
Copy link
Contributor

Unlike with arrays though, this case has a work-around in converting though Vec<T>.

@scottmcm
Copy link
Member

So we could add the implementation today, using Vec::from(self).into_iter() to make a vec::IntoIter? Seems like there wouldn't even be a need to make a new iterator type...

@cuviper
Copy link
Member

cuviper commented Apr 16, 2019

The user could work around this by going through Vec, and the nice thing is that this has no real cost -- it's just a bit awkward.

But if std did this, we would run into the same problems as arrays, where calling into_iter() today on a Box<[T]> gets you a slice::Iter. So it's a breaking change to add IntoIterator -- technically minor and allowable, but needs caution.

@SimonSapin
Copy link
Contributor

@scottmcm Yes, the implementation is easy and does not require a new type. The concern is only in the change being breaking.

@LukasKalbertodt
Copy link
Member

@SimonSapin Do you think it would be a good idea to add a future-incompatibility lint for this, like we did for the array.into_iter() problem (#66145)? In particular, because you said "Whatever we end up deciding should probably apply to both." This was suggested here.

@cuviper
Copy link
Member

cuviper commented Mar 28, 2021

FWIW, I just tried adding Box<[T]> and Box<[T; N]> to see what would break, and they fail immediately:

// library/alloc/src/boxed.rs

#[stable(feature = "boxed_slice_into_iter", since = "1.53.0")]
impl<T, A> IntoIterator for Box<[T], A> {
    type Item = T;
    type IntoIter = crate::vec::IntoIter<T, A>;

    #[inline]
    fn into_iter(self) -> Self::IntoIter {
        todo!()
    }
}

#[stable(feature = "boxed_array_into_iter", since = "1.53.0")]
impl<T, A, const N: usize> IntoIterator for Box<[T; N], A> {
    type Item = T;
    type IntoIter = crate::vec::IntoIter<T, A>;

    #[inline]
    fn into_iter(self) -> Self::IntoIter {
        todo!()
    }
}
error[E0119]: conflicting implementations of trait `core::iter::IntoIterator` for type `boxed::Box<[_], _>`:
    --> library/alloc/src/boxed.rs:1578:1
     |
1578 | impl<T, A> IntoIterator for Box<[T], A> {
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
     = note: conflicting implementation in crate `core`:
             - impl<I> IntoIterator for I
               where I: Iterator;
     = note: upstream crates may add a new impl of trait `core::iter::Iterator` for type `[_]` in future versions

error[E0119]: conflicting implementations of trait `core::iter::IntoIterator` for type `boxed::Box<[_; _], _>`:
    --> library/alloc/src/boxed.rs:1589:1
     |
1589 | impl<T, A, const N: usize> IntoIterator for Box<[T; N], A> {
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
     = note: conflicting implementation in crate `core`:
             - impl<I> IntoIterator for I
               where I: Iterator;
     = note: upstream crates may add a new impl of trait `core::iter::Iterator` for type `[_; _]` in future versions

This has to do with impl<I: Iterator + ?Sized, A> Iterator for Box<I, A> to complete the conflict, although we know that we would never add direct Iterator for [T] or [T; N]. I don't know if we could hack around that with a lang item or something.

@joshtriplett
Copy link
Member

We discussed this in the @rust-lang/libs meeting today.

Our consensus was that we'd be happy to see this impl, as long as doing so was possible and wouldn't break backwards compatibility. It's possible that the machinery proposed for supporting [].into_iter() might help here; if that's the case, and this would potentially benefit from the edition transition, we'd love to see an implementation making use of that.

The coherence issue @cuviper reported seems orthogonal to that, though.

@m-ou-se
Copy link
Member

m-ou-se commented Jun 10, 2021

@yaahc This might be another good use case for your negative trait implementation feature. E.g. impl !Iterator for [_; _]; to promise we will never add the trait implementation that rustc is worried about.

@btzy
Copy link

btzy commented Jul 5, 2021

Are there any updates on this? It does seem like a waste of the edition update if this isn't going into Rust 2021. There're a number of places in our codebase where we're doing Vec::from(boxed_slice).into_iter() currently, which would benefit from this.

I wouldn't mind spending some time to get this working, if we're just waiting for someone to get around to writing the implementation. (But I've never contributed to Rust before, so I might need some guidance.)

@yaahc
Copy link
Member

yaahc commented Aug 6, 2021

Are there any updates on this? It does seem like a waste of the edition update if this isn't going into Rust 2021. There're a number of places in our codebase where we're doing Vec::from(boxed_slice).into_iter() currently, which would benefit from this.

I wouldn't mind spending some time to get this working, if we're just waiting for someone to get around to writing the implementation. (But I've never contributed to Rust before, so I might need some guidance.)

I believe the edition cutoff has already passed quite a few months ago, so I doubt it's possible for this to make it into the 2021 edition but there may be some ways we can make it work backwards compatibly as @joshtriplett mentioned. AFAIK this is just waiting on someone to get around to writing the implementation so if this is something you're interested in please take a crack at it @btzy!

@jhpratt jhpratt added the A-maybe-future-edition Something we may consider for a future edition. label Mar 23, 2024
bors added a commit to rust-lang-ci/rust that referenced this issue May 21, 2024
…fleLapkin

Add `IntoIterator` for `Box<[T]>` + edition 2024-specific lints

* Adds a similar method probe opt-out mechanism to the `[T;N]: IntoIterator` implementation for edition 2021.
* Adjusts the relevant lints (shadowed `.into_iter()` calls, new source of method ambiguity).
* Adds some tests.
* Took the liberty to rework the logic in the `ARRAY_INTO_ITER` lint, since it was kind of confusing.

Based mostly off of rust-lang#116607.

ACP: rust-lang/libs-team#263
References rust-lang#59878
Tracking for Rust 2024: rust-lang#123759

Crater run was done here: rust-lang#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`.
@traviscross
Copy link
Contributor

This has now happened in:

...and is expected to stabilize with Rust 1.80.

So we can close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-iterators Area: Iterators A-maybe-future-edition Something we may consider for a future edition. C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
Status: Needs help: Impl
Development

No branches or pull requests