generic slice/container indexing using RangeArgument

[durka]

So, while I was working on #30884 (which is still missing indexing) I came up with the crazy idea to allow indexing things with any R: RangeArgument<usize> instead of special-casing Range, RangeTo, RangeFrom and RangeFull (which already implement RangeArgument).

At first it sounds like it would break all type inference everywhere. But, in my test it doesn't, so it seems like a crater run would be interesting to see if there are type inference regressions that we didn't think of.

It does require moving RangeArgument to core::ops (so that slices can use it) and marking it #[fundamental] so that it's possible to write the Index impls in libcollections (see the commit comment).

TL;DR @brson this is crazy, but crater me maybe?

cc @bluss @eddyb

[rust-highfive]

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

[durka]

Also, if there are alternatives to making RangeArgument fundamental that still allow these impls, that would be nice... otherwise we can't stabilize RangeArgument until we're sure that the core range data structures are fully baked.

One idea is to simply impl RangeArgument<usize> for usize and combine even more impls, but then it would need an associated type to specify what indexing returns (scalar vs slice) and that sounds hairy...

[brson]

I'll run crater.

I don't understand what this is doing but it does look crazy.

[durka]

Yay! Basically it replaces a bunch of Index<Range*> with blanket
impls for anything implementing RangeArgument (which is currently
unstable). This was simple (for now) because the current API of
RangeArgument is essentially a copy of Range with the fields changed to
methods.
On Jan 28, 2016 19:30, "Brian Anderson" [email protected] wrote:

I'll run crater.

I don't understand what this is doing but it does look crazy.

—
Reply to this email directly or view it on GitHub
#31191 (comment).

[aturon]

@durka

I'm curious about the motivation for this change (aside from cutting down on the number of Index impls needed) -- are there additional impls of RangeArgument you have in mind beyond the ones for range notation?

[durka]

@aturon

I guess it is mostly to reduce the number of impls and code duplication/potential bug sites. With the addition of inclusive range syntax, the new range structs will also need to impl RangeArgument (though it needs to be redesigned somewhat for that to be possible...). Besides that... I agree the benefits aren't super clear. It provides another way for libraries to define indexing on slices/collections with just one impl (but I'm not sure what the implications of #[fundamental] are on who's allowed to do that).

[brson]

Crater report.

[durka]

No regressions!

[durka]

@aturon I suppose another argument is API consistency. You can drain with any R: RangeArgument, so why not index?

[aturon]

@durka Thanks for the explanation, and the PR! I'm not opposed to this change, was just curious where it was coming from.

I'm going to cc @rust-lang/libs, and bring this up at the libs team meeting tomorrow where we'll make a final decision. Sorry for the delay, but changes to core APIs like this get extra scrutiny.

[Gankra]

I'm not particularly jazzed about pushing RangeArgument more into the spotlight, since it's kinda a perpetual API-design chaos-vortex that drives great men to madness (weirdly, great women are only driven to Denny's, and the vortex doesn't seem to wield its malice against those who identify otherwise).

But in all seriousness this seems basically fine, modulo whether we expect drain/indexing/etc to diverge at all. I don't... think so...? Obviously questions need to be answered about RangeInclusive, and the other managerie like (x,y] that we may well adopt for e.g. BTree.

[durka]

Oh I'm not at all worried about the delay. This was more of a "what-if" than a forceful proposal from me. Though I do like the API consistency angle.

To be clear, it does break code that was using #![feature(collections_range)] -- trivially, by moving RangeArgument, and then in the unlikely case that someone wrote struct S; impl RangeArgument for S { ... } impl Index<S> for [T] { ... }. But to my knowledge (bolstered by crater, though I don't like to take that as proof) code that isn't using the feature doesn't break. And there's the weird #[fundamental] thing, but hopefully the libs team understands the implications of that better than I do.

[alexcrichton]

The libs team discussed this during triage today and some thoughts were:

• The change in error messages seems a little unfortunate
• The addition of #[fundamental] is a blocker for this today

There's unfortunately not really a path to stabilization for #[fundamental], so we're trying to avoid all uses of it if we can. It seems that this does indeed need to infer, however, that usize does not implement RangeArgument. This could in theory be a private trait to libcollections, but that seems a bit unfortunate, I'm not really sure what the best outcome of that is...

[bors]

☔ The latest upstream changes (presumably #31646) made this pull request unmergeable. Please resolve the merge conflicts.