Skip to content
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

RFC: generalized-index. #2473

Closed
wants to merge 4 commits into from

Conversation

Projects
None yet
@phaazon
Copy link
Contributor

phaazon commented Jun 13, 2018

Rendered.

phaazon added some commits Jun 13, 2018

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Jun 13, 2018

Adding T-lang since this touches #[lang = "index"].

@matthewjasper

This comment has been minimized.

Copy link

matthewjasper commented Jun 13, 2018

How does this affect IndexMut?

@durka

This comment has been minimized.

Copy link
Contributor

durka commented Jun 13, 2018

I think you need to better address backwards compatibility. The fact that it's easy to rewrite old impls against the new trait isn't quite enough -- all those impls currently exist, and even new editions of the compiler need to be able to use them. Perhaps a new trait should be introduced with the more flexible definition, and a blanket impl can be provided (is this possible?) to cover the old Index. Once that's wired up the [] syntax could be changed over to the new trait.

@Gilnaa

This comment has been minimized.

Copy link

Gilnaa commented Jun 13, 2018

Is it possible to avoid breakage using some kind of hack?

Like changing the lang item's name to "IndexMove" and having some catchall impl that translates from the old trait to the new?

impl<T: Index> IndexMove for T {}

I'm pretty uncomfortable with this breakage, although I agree that the proposed trait is better than what we currently have.

@CAD97

This comment has been minimized.

Copy link

CAD97 commented Jun 13, 2018

I believe the following is a possible solution without breakage, but purposely using bad naming:

  • New trait, NewIndex<'a, Idx> created, index(&'a self, idx: Idx) -> Self::Output
  • [] indexing syntax translates to NewIndex::index instead of Index::index
  • Automatic translation impl from Index to NewIndex:
impl <'a, Idx, T: Index<Idx>> NewIndex<'a, Idx> for T {
    type Output = &'a <Self as Index<Idx>>::Output;
    fn index(&'a self, idx: Idx) -> Self::Output {
        Index::index(self, idx)
    }
}

In terms of the name, IndexMove scares me because I'd expect that to move out of self rather than borrowing self.

@Emerentius

This comment was marked as off-topic.

Copy link

Emerentius commented Jun 13, 2018

It's obvious that breaking changes such as this do not fly and the author knows this. This RFC's purpose is to signal a need and to stir up discussion. The limitations of indexing have been a longstanding pain point.

@Gilnaa

This comment has been minimized.

Copy link

Gilnaa commented Jun 13, 2018

@emerenitus can you give an example for a place where this is needed

@phaazon

This comment has been minimized.

Copy link
Contributor Author

phaazon commented Jun 13, 2018

I think I should alter the RFC to do the same thing with IndexMut, indeed!

@Gilnaa

This comment has been minimized.

Copy link

Gilnaa commented Jun 13, 2018

Does IndexMut make any sense in a move context?

@phaazon

This comment has been minimized.

Copy link
Contributor Author

phaazon commented Jun 13, 2018

@Gilnaa it does:

fn index(&'a mut self, idx: Idx) -> Self::Output

@Gilnaa

This comment has been minimized.

Copy link

Gilnaa commented Jun 13, 2018

Okay, but why. In what context is this useful?

@phaazon

This comment has been minimized.

Copy link
Contributor Author

phaazon commented Jun 13, 2018

@durka, the blanket definition you talk about looks like a default implementation (specialization), like so:

impl<'a, T, I> NewIndex<I> for T where T: Index<I> {
  default fn index(…)…
}
@phaazon

This comment has been minimized.

Copy link
Contributor Author

phaazon commented Jun 13, 2018

@Gilnaa in the context where you want to return an object that borrows, instead of a direct reference. Both immutable and mutable situations might happen. Not related to indexing, but you already do this with Ref and RefMut with RefCell for instance.

Add the universal impl and NewTrait.
Also, add the unresolved question about `IndexMut`.
@phaazon

This comment was marked as off-topic.

Copy link
Contributor Author

phaazon commented Jun 13, 2018

@Emerentius I have no idea what you’re talking about. Nor even how I should react. I don’t write RFCs just for the sake of it. I write RFCs because I have an idea I want to share with people and if consensus is met, the idea is merged. Every RFC that I did came from a situation that I had found problematic in Rust.

Please don’t label my actions as stiring up.

@Gilnaa

This comment was marked as off-topic.

Copy link

Gilnaa commented Jun 13, 2018

I don't think "stiring up" in this context is bad. I think he just meant "to make conversation"

@phaazon

This comment has been minimized.

Copy link
Contributor Author

phaazon commented Jun 13, 2018

Anyone sees specificities with IndexMut? Should I treat it a separate way, or should I just say “we also need this for IndexMut”?

@warlord500

This comment has been minimized.

Copy link

warlord500 commented Jun 13, 2018

however in way, this change makes slices in rust proper a lot less special because of this.
some one can write a slice struct, that acts exactly like the current rust slice type.
I hate that rust slices are special cases.

is it possible that this could be consider with the edition? change to a different edition will break code.
I wonder what exactly are the limits of possible breaking changes can happen on editions.

operator, like the `[[]]` operator or even `[move idx]` construct. This would then require adding a
new trait, like `IndexMove`, to make the whole design work.

## How not to introduce a breaking change

This comment has been minimized.

@sfackler

sfackler Jun 13, 2018

Member

Not making a breaking change to the language is not an alternative, it's the only acceptable approach.

This comment has been minimized.

@phaazon

phaazon Jun 13, 2018

Author Contributor

Right, I’ll move that in another section. 💃

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Jun 13, 2018

however in way, this change makes slices in rust proper a lot less special because of this.
some one can write a slice struct, that acts exactly like the current rust slice type.

Slices already use the existing Index trait, they have no special behavior with respect to indexing syntax.

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Jun 13, 2018

Nominating for next lang team meeting due to urgency of the RFC wrt. editions.

@Centril Centril added the I-nominated label Jun 13, 2018

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Jun 13, 2018

foo[bar] currently desugars to *foo.index(bar) (note the dereference of the returned value). How is that supposed to be handled with this new trait definition? Would the compiler only insert that if the output type is a reference? If it implements Deref?

@warlord500

This comment has been minimized.

Copy link

warlord500 commented Jun 13, 2018

slices arent special in the sense that the act differently for the index trait. its that its current impossible to write a struct that acts nearly the exact same as a slice. in essence how do you write that a reference to
this type is always two pointers wide? thus &foo is the same size as &[u8] where foo is an ordinary struct

# Unresolved questions
[unresolved]: #unresolved-questions

- Should this RFC also concern `IndexMut`?

This comment has been minimized.

@sfackler

sfackler Jun 13, 2018

Member

Yes, this RFC also has to cover IndexMut - that trait extends Index and reuses its output type. That setup will not work with this proposed change.

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Jun 13, 2018

As written, this RFC cannot be accepted, because it proposes a breaking change to std based on the definitions laid out in RFC 1105. It also cannot be enabled (as written) by the edition/epoch changes introduced in RFC 2052, because it does not allow us to change the definition of any existing item in libstd.

There are two active internals threads that are discussing the possibility of enabling different kinds of index operations, including attempts to address the backward compatibility issues:

Because accepting this RFC as written would be a violation of our breaking change guarantees laid out in previous RFCs, I'm going to close this for procedural reasons: we can't, according to our rules, consider this proposal as written. I would encourage anyone who wants to work on a proposal in this space to participate in one of the internals threads I linked to, and a non-breaking RFC can be posted based on that discussion.


It's obvious that breaking changes such as this do not fly and the author knows this. This RFC's purpose is to signal a need and to stir up discussion. The limitations of indexing have been a longstanding pain point.

@phaazon says that this was not their intention, so this portion of my comment is not directed at them. But I want to make it clear for all users that opening an RFC you know cannot be accepted would be a misuse of the RFC process: you should only open an RFC if you believe in good faith that your RFC can and should be accepted. Nothing @phaazon has posted suggests that this RFC was not opened in good faith, but its important to establish for everyone that this would not be how to bring attention to an issue you care about.

@phaazon

This comment has been minimized.

Copy link
Contributor Author

phaazon commented Jun 13, 2018

@withoutboats couldn’t we just rewrite / move the RFC with comments from people in this thread? The proposed situation with NewIndex wouldn’t be a breaking change, would it?

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Jun 13, 2018

@phaazon Yea, an RFC along those lines could be considered, because it wouldn't be a breaking change. I think it would be ideal to work out the details on one of the internals threads I linked to & start a new RFC pull request for a proposal with a non-breaking change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.