Rename RangeArgument to RangeBounds, move it and Bound to libcore#49163

SimonSapin · 2018-03-19T08:45:50Z

As proposed in the tracking issue: #30877

Changes to stable items:

core::ops::Bound / std::ops::Bound is new
std::collections::Bound is a deprecated reexport of it (does this actually cause a warning?)

Changes to unstable items

alloc::Bound is gone
alloc::range::RangeArgument is moved to core::ops::RangeBounds / std::ops::RangeBounds
alloc::range is gone
std::collections::range::RangeArgument is deprecated reexport, to be removed later
std::collections::range is deprecated, to be removed later
impl RangeBounds<T> for Range{,From,To,Inclusive,ToInclusive}<&T> are added

The idea of replacing this trait with a type to be used with Into<_> is left for future consideration / work.

(Fixes rust-lang/rust-clippy#2552.)

clarfonthey · 2018-03-19T16:10:14Z

I think that adding impl<C, T> RangeBounds<T> for C where C: RangeBounds<&T> might be good so that things like "abc".."xyz" will work properly.

I remember something along those lines was what I was trying to solve in previous PRs, but with limited success when using Into. I might also just be tired and misreading the code so if that's the case, please ignore this.

SimonSapin · 2018-03-19T16:36:46Z

I tried adding this:

impl<'a, R, T: ?Sized + 'a> RangeBounds<T> for R where R: RangeBounds<&'a T> {
    // …
}

and got:

error[E0275]: overflow evaluating the requirement `ops::range::RangeFull: ops::range::RangeBounds<&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&_>`
  |
  = help: consider adding a `#![recursion_limit="128"]` attribute to your crate
  = note: required because of the requirements on the impl of `ops::range::RangeBounds<&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&_>` for `ops::range::RangeFull`
  = note: required because of the requirements on the impl of `ops::range::RangeBounds<&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&_>` for `ops::range::RangeFull`
[…]
  = note: required because of the requirements on the impl of `ops::range::RangeBounds<&&_>` for `ops::range::RangeFull`
  = note: required because of the requirements on the impl of `ops::range::RangeBounds<&_>` for `ops::range::RangeFull`

error: aborting due to previous error

clarfonthey · 2018-03-19T20:40:04Z

Oh, huh. I'm a bit confused why that's failing, considering how AsRef has a similar implementation…

bors · 2018-03-19T20:40:06Z

@clarcharr: 🔑 Insufficient privileges: Not in reviewers

bors · 2018-03-19T20:40:07Z

@clarcharr: 🔑 Insufficient privileges: not in try users

SimonSapin · 2018-03-19T21:37:26Z

AsRef has an impl for &T, not for T where /*…*/. The latter is more general and has more potential overlap with other impls.

SimonSapin · 2018-03-20T18:48:20Z

Added impl RangeBounds<T> for Range{,From,To,Inclusive,ToInclusive}<&T>

SimonSapin · 2018-03-22T14:11:00Z

r? @alexcrichton

SimonSapin · 2018-03-22T21:06:12Z

@clarcharr You comment #44518 (comment) suggests that the thing with ranges of strings is not just adding & to some types, but also String v.s. str.

I’ve just pushed another commit that generalizes the impls to:

impl<T: ?Sized> RangeBounds<T> for RangeFull
impl<T: ?Sized, U: Borrow<T>> RangeBounds<T> for RangeFrom<U>
impl<T: ?Sized, U: Borrow<T>> RangeBounds<T> for RangeTo<U>
impl<T: ?Sized, U: Borrow<T>> RangeBounds<T> for Range<U>
impl<T: ?Sized, U: Borrow<T>> RangeBounds<T> for RangeInclusive<U>
impl<T: ?Sized, U: Borrow<T>> RangeBounds<T> for RangeToInclusive<U>
impl<T: ?Sized, U: Borrow<T>> RangeBounds<T> for (Bound<U>, Bound<U>)

Does this seem like a good idea?

SimonSapin · 2018-03-22T21:08:28Z

(As a reminder: T and &T both implement Borrow<T>, only the latter implements AsRef<T>. Trying to have two sets of impls for Range*<T> and Range*<U> where U: AsRef<T> causes an overlap error because some type could implement AsRef<Foo> for Foo.)

SimonSapin · 2018-03-22T23:49:16Z

Ah, this was probably the issue you meant:

---- btree/map.rs - btree::map::BTreeMap<K, V>::range_mut (line 818) stdout ----
	error[E0283]: type annotations required: cannot resolve `_: std::cmp::Ord`
 --> btree/map.rs:824:25
  |
9 | for (_, balance) in map.range_mut("B".."Cheryl") {
  |                         ^^^^^^^^^

thread 'rustc' panicked at 'couldn't compile the test', librustdoc/test.rs:296:13
note: Run with `RUST_BACKTRACE=1` for a backtrace.


failures:
    btree/map.rs - btree::map::BTreeMap<K, V>::range_mut (line 818)

I think that code has two possible concrete types (str and String) for some type parameter. Both would end up producing the same behavior, but the compiler is still unhappy about the ambiguity.

Undoing that commit.

clarfonthey · 2018-03-23T04:20:10Z

Ah, yeah, that's why I could never complete the original PR.

It's truly unfortunate that RangeArgument exists the way it does, as it can't really be changed despite being unstable. It'd be better if we could return the start and end by value, as Into<RangeBounds> would, but that would result in different behaviour and not-so-subtle differences from what we've got right now.

I guess that the best option is to just stabilise it as-is and deal with what we've got. I appreciate the name change and the effort, though!

alexcrichton

Looks great to me, thanks @SimonSapin!

alexcrichton · 2018-03-23T18:33:56Z

src/libstd/collections/mod.rs

We may want to also tag this with #[doc(hidden)] to prevent it showing up in the docs

Sounds good, done.

alexcrichton · 2018-03-23T18:36:14Z

@rfcbot fcp merge

Since this is touching and moving stable items I'd like to get some more eyes on this, but I'm not expecting anything surprising here

rfcbot · 2018-03-23T18:36:15Z

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), 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!

See this document for info about what commands tagged team members can give me.

SimonSapin · 2018-03-23T18:43:06Z

If we’re doing the FCP dance, should we also propose to stabilize? If we decide on a dedicated trait (over Into<_>) this PR fixes the remaining issues that I could find in the tracking issue.

alexcrichton · 2018-03-23T18:44:28Z

I'd personally prefer to land this first and then have a separate discussion about the various bounds/impls/such

rfcbot · 2018-03-27T19:28:06Z

🔔 This is now entering its final comment period, as per the review above. 🔔

bors · 2018-03-28T22:18:09Z

💔 Test failed - status-travis

kennytm · 2018-03-29T07:47:17Z

This has broken clippy's ui/needless_pass_by_value.rs test. It has printed nothing.

SimonSapin · 2018-03-29T09:15:45Z

I can reproduce this but I have no idea why it happens :(

The stable reexport `std::collections::Bound` is now deprecated. Another deprecated reexport could be added in `alloc`, but that crate is unstable.

These unstable items are deprecated: * The `std::collections::range::RangeArgument` reexport * The `std::collections::range` module.

…tions

Manishearth · 2018-03-29T11:14:59Z

Rebased and fixed. I'll deal with getting the clippy stuff landed correctly.

@bors r=alexcrichton

bors · 2018-03-29T11:15:00Z

📌 Commit 6c9b3cc has been approved by alexcrichton

bors · 2018-03-29T11:34:24Z

⌛ Testing commit 6c9b3cc with merge ae544ee...

Rename RangeArgument to RangeBounds, move it and Bound to libcore As proposed in the tracking issue: #30877 Changes to *stable* items: * `core::ops::Bound` / `std::ops::Bound` is new * `std::collections::Bound` is a deprecated reexport of it (does this actually cause a warning?) Changes to *unstable* items * `alloc::Bound` is gone * `alloc::range::RangeArgument` is moved to `core::ops::RangeBounds` / `std::ops::RangeBounds` * `alloc::range` is gone * `std::collections::range::RangeArgument` is deprecated reexport, to be removed later * `std::collections::range` is deprecated, to be removed later * `impl RangeBounds<T> for Range{,From,To,Inclusive,ToInclusive}<&T>` are added The idea of replacing this trait with a type to be used with `Into<_>` is left for future consideration / work. (Fixes rust-lang/rust-clippy#2552.)

bors · 2018-03-29T14:16:41Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing ae544ee to master...

kennytm-githubbot · 2018-03-29T14:18:15Z

📣 Toolstate changed by #49163!

Tested on commit ae544ee.
Direct link to PR: #49163

💔 clippy-driver on windows: test-pass → test-fail (cc @Manishearth @llogiq @mcarton @oli-obk).
💔 clippy-driver on linux: test-pass → test-fail (cc @Manishearth @llogiq @mcarton @oli-obk).

@Manishearth

Tested on commit rust-lang/rust@ae544ee. Direct link to PR: <rust-lang/rust#49163> 💔 clippy-driver on windows: test-pass → test-fail (cc @Manishearth @llogiq @mcarton @oli-obk). 💔 clippy-driver on linux: test-pass → test-fail (cc @Manishearth @llogiq @mcarton @oli-obk).

Manishearth · 2018-03-29T14:20:56Z

Unsure why that didn't work, but it let it land, so that's good enough for now. Will fix later.

stabilize RangeBounds collections_range #30877 The FCP for #30877 closed last month, with the decision to: 1. move from `collections::range::RangeArgument` to `ops::RangeBounds`, and 2. rename `start()` and `end()` to `start_bounds()` and `end_bounds()`. Simon Sapin already moved it to `ops::RangeBounds` in #49163. I renamed the functions, and removed the old `collections::range::RangeArgument` alias. This is my first Rust PR, please let me know if I can improve anything. This passes all tests for me, except the `clippy` tool (which uses `RangeArgument::start()`). I considered deprecating `start()` and `end()` instead of removing them, but the contribution guidelines indicate we can break `clippy` temporarily. I thought it was best to remove the functions, since we're worried about name collisions with `Range::start` and `end`. Closes #30877.

SimonSapin added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 19, 2018

SimonSapin assigned dtolnay Mar 19, 2018

This was referenced Mar 19, 2018

alloc::range::RangeArgument → core::ops::RangeBounds rust-lang/rust-clippy#2552

Closed

Tracking issue for RangeBounds #30877

Closed

SimonSapin force-pushed the range-bounds branch from a3037f5 to 3f64375 Compare March 19, 2018 10:05

rust-highfive assigned alexcrichton and unassigned dtolnay Mar 22, 2018

SimonSapin force-pushed the range-bounds branch from 8663ca7 to fa7ed65 Compare March 22, 2018 23:49

alexcrichton reviewed Mar 23, 2018

View reviewed changes

rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Mar 23, 2018

rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Mar 27, 2018

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 28, 2018

kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 29, 2018

SimonSapin and others added 6 commits March 29, 2018 13:12

Move alloc::Bound to {core,std}::ops

c3a6397

The stable reexport `std::collections::Bound` is now deprecated. Another deprecated reexport could be added in `alloc`, but that crate is unstable.

Move RangeArguments to {core::std}::ops and rename to RangeBounds

16d3ba1

These unstable items are deprecated: * The `std::collections::range::RangeArgument` reexport * The `std::collections::range` module.

impl RangeBounds<T> for Range{,From,To,Inclusive,ToInclusive}<&T>

124453e

Hide the deprecated std::collections::range module from docs

6960a96

Hide the Bound type in docs at its deprecated location in std::collec…

3542ff8

…tions

Update clippy

6c9b3cc

Manishearth force-pushed the range-bounds branch from 62309b8 to 6c9b3cc Compare March 29, 2018 11:14

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 29, 2018

bors merged commit 6c9b3cc into rust-lang:master Mar 29, 2018

bors mentioned this pull request Mar 29, 2018

Remove all unstable placement features #48333

Merged

SimonSapin deleted the range-bounds branch March 29, 2018 14:28

emilyalbini mentioned this pull request Apr 19, 2018

Inference regression in 1.26 #49944

Closed

coryshrmn mentioned this pull request May 24, 2018

stabilize RangeBounds collections_range #30877 #51033

Merged

Uh oh!

Conversation

SimonSapin commented Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clarfonthey commented Mar 19, 2018

Uh oh!

SimonSapin commented Mar 19, 2018

Uh oh!

clarfonthey commented Mar 19, 2018

Uh oh!

bors commented Mar 19, 2018

Uh oh!

bors commented Mar 19, 2018

Uh oh!

SimonSapin commented Mar 19, 2018

Uh oh!

SimonSapin commented Mar 20, 2018

Uh oh!

SimonSapin commented Mar 22, 2018

Uh oh!

SimonSapin commented Mar 22, 2018

Uh oh!

SimonSapin commented Mar 22, 2018

Uh oh!

SimonSapin commented Mar 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clarfonthey commented Mar 23, 2018

Uh oh!

alexcrichton left a comment

Choose a reason for hiding this comment

Uh oh!

alexcrichton Mar 23, 2018

Choose a reason for hiding this comment

Uh oh!

SimonSapin Mar 23, 2018

Choose a reason for hiding this comment

Uh oh!

alexcrichton commented Mar 23, 2018

Uh oh!

rfcbot commented Mar 23, 2018 • edited by withoutboats Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SimonSapin commented Mar 23, 2018

Uh oh!

alexcrichton commented Mar 23, 2018

Uh oh!

rfcbot commented Mar 27, 2018

Uh oh!

bors commented Mar 28, 2018

Uh oh!

kennytm commented Mar 29, 2018

Uh oh!

SimonSapin commented Mar 29, 2018

Uh oh!

Manishearth commented Mar 29, 2018

Uh oh!

bors commented Mar 29, 2018

Uh oh!

bors commented Mar 29, 2018

Uh oh!

bors commented Mar 29, 2018

Uh oh!

kennytm-githubbot commented Mar 29, 2018

Uh oh!

Manishearth commented Mar 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

SimonSapin commented Mar 19, 2018 •

edited

Loading

SimonSapin commented Mar 22, 2018 •

edited

Loading

rfcbot commented Mar 23, 2018 •

edited by withoutboats

Loading