Have Vec use slice's implementations of Index and IndexMut #47832

fintelia · 2018-01-28T19:37:34Z

This PR simplifies the implementation of Index and IndexMut on Vec, and in the process enables indexing Vec by any user types that implement SliceIndex.

The stability annotations probably need to be changed, but I wasn't sure of the right way to do that. It also wasn't completely clear to me if this change could break any existing code.

rust-highfive · 2018-01-28T19:37:39Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @kennytm (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

bluss · 2018-01-28T20:54:44Z

src/liballoc/vec.rs


    #[inline]
-    fn index(&self, index: usize) -> &T {
+    fn index(&self, index: I) -> &Self::Output {
        // NB built-in indexing via `&[T]`
        &(**self)[index]


This is not built in indexing anymore, so the comment can be updated.

dtolnay

Seems reasonable to me.

dtolnay · 2018-01-29T03:17:46Z

Looks like this changes an error message that is breaking a UI test.

+ error[E0277]: the trait bound `i32: std::slice::SliceIndex<[{integer}]>` is not satisfied
- error[E0277]: the trait bound `std::vec::Vec<{integer}>: std::ops::Index<i32>` is not satisfied
    --> $DIR/index-help.rs:13:5
     |
  13 |     x[0i32]; //~ ERROR E0277
+    |     ^^^^^^^ slice indices are of type `usize` or ranges of `usize`
-    |     ^^^^^^^ vector indices are of type `usize` or ranges of `usize`
     |
+    = help: the trait `std::slice::SliceIndex<[{integer}]>` is not implemented for `i32`
+    = note: required because of the requirements on the impl of `std::ops::Index<i32>` for `[{integer}]`
-    = help: the trait `std::ops::Index<i32>` is not implemented for `std::vec::Vec<{integer}>`

kennytm · 2018-01-31T16:46:46Z

@fintelia You need to update the wordings in compile-fail/integral-indexing.rs too.

rust/src/test/compile-fail/integral-indexing.rs

Lines 16 to 19 in 560a2f4

    
           v[3u8];  //~ERROR : std::ops::Index<u8>` is not satisfied 
        
           v[3i8];  //~ERROR : std::ops::Index<i8>` is not satisfied 
        
           v[3u32]; //~ERROR : std::ops::Index<u32>` is not satisfied 
        
           v[3i32]; //~ERROR : std::ops::Index<i32>` is not satisfied

[01:02:28] ---- [compile-fail] compile-fail/integral-indexing.rs stdout ----
[01:02:28] 	
[01:02:28] error: /checkout/src/test/compile-fail/integral-indexing.rs:16: unexpected error: '16:5: 16:11: the trait bound `u8: std::slice::SliceIndex<[isize]>` is not satisfied [E0277]'
[01:02:28] 
[01:02:28] error: /checkout/src/test/compile-fail/integral-indexing.rs:17: unexpected error: '17:5: 17:11: the trait bound `i8: std::slice::SliceIndex<[isize]>` is not satisfied [E0277]'
[01:02:28] 
[01:02:28] error: /checkout/src/test/compile-fail/integral-indexing.rs:18: unexpected error: '18:5: 18:12: the trait bound `u32: std::slice::SliceIndex<[isize]>` is not satisfied [E0277]'
[01:02:28] 
[01:02:28] error: /checkout/src/test/compile-fail/integral-indexing.rs:19: unexpected error: '19:5: 19:12: the trait bound `i32: std::slice::SliceIndex<[isize]>` is not satisfied [E0277]'
[01:02:28] 
[01:02:28] error: /checkout/src/test/compile-fail/integral-indexing.rs:16: expected error not found: : std::ops::Index<u8>` is not satisfied
[01:02:28] 
[01:02:28] error: /checkout/src/test/compile-fail/integral-indexing.rs:17: expected error not found: : std::ops::Index<i8>` is not satisfied
[01:02:28] 
[01:02:28] error: /checkout/src/test/compile-fail/integral-indexing.rs:18: expected error not found: : std::ops::Index<u32>` is not satisfied
[01:02:28] 
[01:02:28] error: /checkout/src/test/compile-fail/integral-indexing.rs:19: expected error not found: : std::ops::Index<i32>` is not satisfied
[01:02:28] 
[01:02:28] error: 4 unexpected errors found, 4 expected errors not found

kennytm · 2018-02-01T10:45:41Z

@bors try - we may need crater.

While the change looks reasonable, I'm afraid this will break downstream crates due to conflicting implementation. In particular, the following compiles today:

use std::ops::Index;

struct S;
struct X;
struct A;
struct B;

impl Index<S> for [X] {
    type Output = A;
    fn index(&self, _: S) -> &A {
        &A
    }
}

impl Index<S> for Vec<X> {
    type Output = B;
    fn index(&self, _: S) -> &B {
        &B
    }
}

I'm nominating this to @rust-lang/libs to decide whether to merge this or not.

bors · 2018-02-01T10:45:53Z

⌛ Trying commit 7f4e9c9 with merge 03a61b1...

Have Vec use slice's implementations of Index and IndexMut This PR simplifies the implementation of Index and IndexMut on Vec, and in the process enables indexing Vec by any user types that implement SliceIndex. The stability annotations probably need to be changed, but I wasn't sure of the right way to do that. It also wasn't completely clear to me if this change could break any existing code.

bors · 2018-02-01T12:29:20Z

☀️ Test successful - status-travis
State: approved= try=True

aidanhs · 2018-02-08T17:54:19Z

Crater run started.

aidanhs · 2018-02-14T00:05:36Z

Crater results: http://cargobomb-reports.s3.amazonaws.com/pr-47382/index.html. 'Blacklisted' crates (spurious failures etc) can be found here. If you see any spurious failures not on the list, please make a PR against that file.

There's at least one legit failure (in "kirillkh.xword_constructor....").

kennytm · 2018-02-14T07:09:08Z

Looks like the rest are spurious.

The offending lines for the real regression:

https://github.com/kirillkh/xword_constructor/blob/a0c41e39d0fbe016dec80429df8b32fc39579fe5/src/common.rs#L198-L214

impl Index<PlacementId> for [ScoredMove] {
    type Output = ScoredMove;

	#[inline]
    fn index(&self, index: PlacementId) -> &Self::Output {
		self.index(index.0 as usize)
    }
}

impl Index<PlacementId> for Vec<ScoredMove> {
    type Output = ScoredMove;

	#[inline]
    fn index(&self, index: PlacementId) -> &Self::Output {
		self.index(index.0 as usize)
    }
}

The good news is that, the output type of the two impls are the same, so this can be fixed by simply #[cfg]'ing out the Vec impl.

sfackler · 2018-02-14T17:08:15Z

The code there also relies on specialization, so we have more wiggle room to break things since that's unstable.

fintelia · 2018-02-28T04:13:25Z

@kennytm what are the next steps on this?

alexcrichton · 2018-02-28T05:17:51Z

I've tagged this as waiting-on-crater

alexcrichton · 2018-02-28T05:18:18Z

er wait sorry, I mixed this up with something else, we decided in @rust-lang/libs triage today to merge this!

@fintelia want to rebase and I'll r+?

sfackler · 2018-02-28T05:20:06Z

src/liballoc/vec.rs

@@ -1542,146 +1542,26 @@ impl<T: Hash> Hash for Vec<T> {

 #[stable(feature = "rust1", since = "1.0.0")]
 #[rustc_on_unimplemented = "vector indices are of type `usize` or ranges of `usize`"]
-impl<T> Index<usize> for Vec<T> {
-    type Output = T;
+impl<T, I> Index<I> for Vec<T> where [T]: Index<I> {


Can we change these bounds to be the same as those for [T] rather than delegating to T's impl explicitly?

Are you saying you'd prefer something like this?

impl<T, I> IndexMut for Vec<T> where I: ::core::slice::SliceIndex<[T]>, { #[inline] fn index_mut(&mut self, index: I) -> &mut Self::Output { IndexMut::index_mut(&mut **self, index) } }

@fintelia indeed!

Why? The one currently in the PR means that if a crate implements Index<TheirType> for [T] they automatically get the impl for Vec<T> too, which seems convenient.

It's a weird layer of double-indirection that isn't used very commonly in trait bounds. It is apparently different enough that specialization treats it differently as that one regression indicates.

kennytm · 2018-03-04T10:52:19Z

@bors r+

Seems like all concerns are addressed. Thanks for your patience 😃

bors · 2018-03-04T10:52:20Z

📌 Commit 370df40 has been approved by kennytm

bors · 2018-03-04T12:30:33Z

⌛ Testing commit 370df40 with merge 1e1bfc7...

Have Vec use slice's implementations of Index and IndexMut This PR simplifies the implementation of Index and IndexMut on Vec, and in the process enables indexing Vec by any user types that implement SliceIndex. The stability annotations probably need to be changed, but I wasn't sure of the right way to do that. It also wasn't completely clear to me if this change could break any existing code.

bors · 2018-03-04T15:15:55Z

☀️ Test successful - status-appveyor, status-travis
Approved by: kennytm
Pushing 1e1bfc7 to master...

Have `String` use `SliceIndex` impls from `str` This PR simplifies the implementation of `Index` and `IndexMut` on `String`, and in the process enables indexing `String` by any user types that implement `SliceIndex<str>`. Similar to rust-lang#47832 r? libs Not sure if this warrants a crater run.

Rollup merge of rust-lang#120291 - pitaj:string-sliceindex, r=Amanieu Have `String` use `SliceIndex` impls from `str` This PR simplifies the implementation of `Index` and `IndexMut` on `String`, and in the process enables indexing `String` by any user types that implement `SliceIndex<str>`. Similar to rust-lang#47832 r? libs Not sure if this warrants a crater run.

rust-highfive assigned kennytm Jan 28, 2018

kennytm 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 Jan 28, 2018

bluss reviewed Jan 28, 2018

View reviewed changes

dtolnay approved these changes Jan 29, 2018

View reviewed changes

kennytm added I-nominated S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Feb 1, 2018

arazabishov added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Feb 1, 2018

Mark-Simulacrum removed the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Feb 1, 2018

alexcrichton added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed I-nominated S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Feb 28, 2018

alexcrichton added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 28, 2018

alexcrichton removed the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Feb 28, 2018

sfackler reviewed Feb 28, 2018

View reviewed changes

fintelia added 5 commits March 2, 2018 23:25

Have Vec use slice's implementations of Index and IndexMut

46fef76

Update UI test

2db05f0

Update comments

45bdf9c

Update compile fail test error messages

ae73a21

Don't have Vec<T> delegate to [T]'s bounds for indexing

370df40

fintelia force-pushed the vec-index branch from 7f4e9c9 to 370df40 Compare March 3, 2018 04:26

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 4, 2018

bors merged commit 370df40 into rust-lang:master Mar 4, 2018

This was referenced Mar 4, 2018

Put back ui json check #48684

Merged

Stabilize inclusive range (..=) #47813

Merged

kennytm mentioned this pull request May 17, 2018

Can't compile brotli with rustc 1.27.0-nightly (2f2a11dfc 2018-05-16) #50825

Closed

pitaj mentioned this pull request Jan 24, 2024

Have String use SliceIndex impls from str #120291

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Have Vec use slice's implementations of Index<I> and IndexMut<I> #47832

Have Vec use slice's implementations of Index<I> and IndexMut<I> #47832

fintelia commented Jan 28, 2018 •

edited

Loading

rust-highfive commented Jan 28, 2018

bluss Jan 28, 2018

dtolnay left a comment

dtolnay commented Jan 29, 2018

kennytm commented Jan 31, 2018

kennytm commented Feb 1, 2018

bors commented Feb 1, 2018

bors commented Feb 1, 2018

aidanhs commented Feb 8, 2018

aidanhs commented Feb 14, 2018 •

edited

Loading

kennytm commented Feb 14, 2018

sfackler commented Feb 14, 2018

fintelia commented Feb 28, 2018 •

edited

Loading

alexcrichton commented Feb 28, 2018

alexcrichton commented Feb 28, 2018

sfackler Feb 28, 2018

fintelia Feb 28, 2018

alexcrichton Feb 28, 2018

dtolnay Feb 28, 2018

sfackler Mar 3, 2018

kennytm commented Mar 4, 2018

bors commented Mar 4, 2018

bors commented Mar 4, 2018

bors commented Mar 4, 2018

Have Vec use slice's implementations of Index<I> and IndexMut<I> #47832

Have Vec use slice's implementations of Index<I> and IndexMut<I> #47832

Conversation

fintelia commented Jan 28, 2018 • edited Loading

rust-highfive commented Jan 28, 2018

bluss Jan 28, 2018

Choose a reason for hiding this comment

dtolnay left a comment

Choose a reason for hiding this comment

dtolnay commented Jan 29, 2018

kennytm commented Jan 31, 2018

kennytm commented Feb 1, 2018

bors commented Feb 1, 2018

bors commented Feb 1, 2018

aidanhs commented Feb 8, 2018

aidanhs commented Feb 14, 2018 • edited Loading

kennytm commented Feb 14, 2018

sfackler commented Feb 14, 2018

fintelia commented Feb 28, 2018 • edited Loading

alexcrichton commented Feb 28, 2018

alexcrichton commented Feb 28, 2018

sfackler Feb 28, 2018

Choose a reason for hiding this comment

fintelia Feb 28, 2018

Choose a reason for hiding this comment

alexcrichton Feb 28, 2018

Choose a reason for hiding this comment

dtolnay Feb 28, 2018

Choose a reason for hiding this comment

sfackler Mar 3, 2018

Choose a reason for hiding this comment

kennytm commented Mar 4, 2018

bors commented Mar 4, 2018

bors commented Mar 4, 2018

bors commented Mar 4, 2018

fintelia commented Jan 28, 2018 •

edited

Loading

aidanhs commented Feb 14, 2018 •

edited

Loading

fintelia commented Feb 28, 2018 •

edited

Loading