Skip to content

Add ?Sized bounds to all the RangeBounds impls #61584

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Add ?Sized bounds to all the RangeBounds impls #61584

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions src/libcore/ops/range.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -850,7 +850,7 @@ impl<T> RangeBounds<T> for (Bound<T>, Bound<T>) {
}

#[stable(feature = "collections_range", since = "1.28.0")]
impl<'a, T: ?Sized + 'a> RangeBounds<T> for (Bound<&'a T>, Bound<&'a T>) {
impl<T: ?Sized> RangeBounds<T> for (Bound<&T>, Bound<&T>) {
fn start_bound(&self) -> Bound<&T> {
self.0
}
Expand All @@ -861,7 +861,7 @@ impl<'a, T: ?Sized + 'a> RangeBounds<T> for (Bound<&'a T>, Bound<&'a T>) {
}

#[stable(feature = "collections_range", since = "1.28.0")]
impl<T> RangeBounds<T> for RangeFrom<&T> {
impl<T: ?Sized> RangeBounds<T> for RangeFrom<&T> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it technically a breaking change to expand the scope of a blanket impl? e.g. if someone had already implemented this for their own unsized Foo, that would now conflict.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. We usually categorize it as a so-called “minor” breaking change: https://rust-lang.github.io/rfcs/1105-api-evolution.html#minor-change-implementing-any-non-fundamental-trait

Copy link
Member

@cuviper cuviper Jun 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's minor regarding dispatch ambiguity, but blanket impls cause actual conflicts.

https://rust-lang.github.io/rfcs/1023-rebalancing-coherence.html
https://rust-lang.github.io/rfcs/2451-re-rebalancing-coherence.html

As such, RFC #1105 is amended to remove the statement that implementing a non-fundamental trait is a minor breaking change, and states that adding any blanket impl for an existing trait is a major breaking change, using the definition of blanket impl given above.

That doesn't address expanding a blanket impl by relaxing constraints, as here, but I think the problem is the same and should be considered a major breaking change. It's particularly relevant that Sized is a fundamental trait, so coherence allows child crates to assume that their unsized type will never overlap with this current Sized impl. (Hmm, well, maybe that's beside the point for a crate's local types.)

The reason I said "technically" before was that I doubt anyone would actually have a conflicting impl in this specific case, but it's possible.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Getting more and more off-topic, but: I think we should not have RFCs that say “RFC X is amended to…” without actually applying the change to the other file. More generally, “facts” that are out of date should not be found on https://rust-lang.github.io/rfcs/.)

fn start_bound(&self) -> Bound<&T> {
Included(self.start)
}
Expand All @@ -871,7 +871,7 @@ impl<T> RangeBounds<T> for RangeFrom<&T> {
}

#[stable(feature = "collections_range", since = "1.28.0")]
impl<T> RangeBounds<T> for RangeTo<&T> {
impl<T: ?Sized> RangeBounds<T> for RangeTo<&T> {
fn start_bound(&self) -> Bound<&T> {
Unbounded
}
Expand All @@ -881,7 +881,7 @@ impl<T> RangeBounds<T> for RangeTo<&T> {
}

#[stable(feature = "collections_range", since = "1.28.0")]
impl<T> RangeBounds<T> for Range<&T> {
impl<T: ?Sized> RangeBounds<T> for Range<&T> {
fn start_bound(&self) -> Bound<&T> {
Included(self.start)
}
Expand All @@ -891,7 +891,7 @@ impl<T> RangeBounds<T> for Range<&T> {
}

#[stable(feature = "collections_range", since = "1.28.0")]
impl<T> RangeBounds<T> for RangeInclusive<&T> {
impl<T: ?Sized> RangeBounds<T> for RangeInclusive<&T> {
fn start_bound(&self) -> Bound<&T> {
Included(self.start)
}
Expand All @@ -901,7 +901,7 @@ impl<T> RangeBounds<T> for RangeInclusive<&T> {
}

#[stable(feature = "collections_range", since = "1.28.0")]
impl<T> RangeBounds<T> for RangeToInclusive<&T> {
impl<T: ?Sized> RangeBounds<T> for RangeToInclusive<&T> {
fn start_bound(&self) -> Bound<&T> {
Unbounded
}
Expand Down