-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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 ?Sized bounds to all the RangeBounds impls #61584
Add ?Sized bounds to all the RangeBounds impls #61584
Conversation
r? @TimNN (rust_highfive has picked a reviewer for you, use r? to override) |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
On a closer look I think most of these changes do not make much changes. Sorry for my misleading comment on Reddit. For example, only the last field of a struct can be This immediate problem does not apply when there is only one field, but then that limits how the type can be used. For example, And this is perhaps the important point here: dynamically-sized types (at least for now) need to live behind some kind of pointer indirection. Rather than have dynamically-sized range types, we can have ranges of (statically double-pointer-sized) references to dynamically sized values. We already have impls for ranges of references: https://doc.rust-lang.org/1.35.0/std/ops/trait.RangeBounds.html#implementors but indeed some of them are missing |
Yeah, you are right, I was asking myself if it was possible to have two unsized types in the same struct. My computer and my internet connection are not really helping me and doesn't let me iterate fast enough so I decided to run it on the rust CI :) I will revert all of my changes but the part about |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Do I need to understand that by adding the @SimonSapin Do you think that this dilemma could be fixed by using any specialization? |
I read the signature of the Would it make any sense to ask for a I know that it will be an hard breaking change and it is absolutely not feasible, but I just want to understand if this fix could be valid. |
Is this in response to the latest CI failure?
Let’s work through it. We have this signature: impl<K: Ord, V> BTreeMap<K, V> {
pub fn range_mut<T: ?Sized, R>(&mut self, range: R) -> RangeMut<'_, K, V>
where T: Ord, K: Borrow<T>, R: RangeBounds<T>
} Earlier in the test case there’s The remaining type variable is impl Ord for str {…}
impl<T: ?Sized> Borrow<T> for T {…}
impl<T> RangeBounds<T> for Range<T> {…}
impl<A: ?Sized> Ord for &A where A: Ord {…}
impl<T: ?Sized> Borrow<T> for &T {…}
impl<T /* this part is new: */ : ?Sized> RangeBounds<T> for Range<&T> {…}
This kind of type inference regression when adding or generalizing impls in the standard library is not usual. Sometimes we call it a minor breaking change and move on. But since Now that I’ve typed all this I wonder if we haven’t tried exactly this before and hit the same problem. @rust-lang/libs, does this ring a bell?
In hindsight I think that I think changing the bound from |
@@ -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> { |
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.
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.
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.
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
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.
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.
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.
(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/.)
What is a problem is that even by adding these bounds to relax the |
Isn’t it? When running this test with a rustc/std that have this PR applied, do you see the same error message as on the playground? Anyway, because of the inference regression in use std::ops::Bound::*;
let mut iter = map.range::<[_], _>((Included(key), Unbounded)); |
No, it doesn't fixed the original issue. You are right, this pull request would break too much compilations and there already is a work around using raw The only thing is that it is sad to keep this issue and make people face it again, as it is some kind of mistake in the std library. |
Closing since it looks like there’s no subset or variation of this we can land without breaking Still, thanks for working on this! |
We found that all the
RangeBounds
implementations on the real range types (i.e.RangeFull
,RangeFrom
...) should have some size relaxation. Therefore I added the?Sized
trait bound to all the implementations of theRangeBounds
trait.This pull request emerged after having seen the problem on one of my reddit post.