-
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
Expand upper bounds on RangeBounds impls #64327
Conversation
r? @kennytm (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 |
This has broken type-inference of some existing tests.
|
Yup I'm looking into it. Is there an easy way to run that test case? Edit: Nevermind, I found what I need. |
I think we need a crater run but the queue is super long for now 😕 |
Hm, yeah, inference breakage here is concerning. @bors try to get a build for crater |
⌛ Trying commit c1539ab with merge b19afc12c4f781e95f168f2fdea339c972f14ae2... |
☀️ Try build successful - checks-azure |
@craterbot run mode=check-only |
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
How long do crater runs typically take? I'm curious as to how much longer this PR may be in the queue. I see it's second to the top. |
I think the current average for a check run is around 4 days, but I'm not sure. |
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🎉 Experiment
|
cc #61584, an earlier attempt at this |
Seems like acceptable breakage to me (a single crates.io regression which will probably be fixed soon...). r? @dtolnay to assess this. |
@@ -904,7 +904,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) | |||
} |
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.
Was this change supposed to apply to the RangeBounds<T> impls for RangeInclusive<&T> and RangeToInclusive<&T> (below) as well?
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.
I agree that the T: ?Sized
impls are more correct and it would be great to have that, but I am not thrilled about introducing even more need for turbofish on range and range_mut.
Some questions to dig into what is going on here:
-
Is there any defensible language change that might allow us to eliminate the ambiguity when using
"from".."to"
as a range into a BTreeMap<&str, _> or (separately) a BTreeMap<String, _>? Possibly some way to guide the inference ofT
when the other type parameters are known, or some way to say that any choice ofT
that satisfies the bounds is fine even if it's not known to be the unique possible choice. -
If range and range_mut didn't exist yet, is there a better less generic signature for them that wouldn't be ambiguous in common cases?
I don't know who these questions are directed to but I am hesitant to merge this change as currently implemented. The amount of breakage is small and would probably be justifiable for a change that made "from".."to"
"just work" without turbofishes, but as it is, I don't think landing this is the right tradeoff.
The workaround is to use map.range::<str, _>((Bound::Included("from"), Bound::Excluded("to")))
which has the right performance characteristics, or map.range("from".to_owned().."to".to_owned())
which is more concise.
Ping from triage: @kennethbgoodin any updates on this? |
It sounds like the consensus is that this change isn't the best approach -- should I close this PR? |
Per issue #64027 adds
?Sized
to allRangeBounds<&T>
impls.