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.

Sign up for GitHub

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

Jump to bottom

Remove IndexMut impl from various collections to prepare for possible IndexSet in the future #23448

Closed
nikomatsakis opened this issue Mar 17, 2015 · 10 comments · Fixed by #23559
Closed

Remove IndexMut impl from various collections to prepare for possible IndexSet in the future #23448

nikomatsakis opened this issue Mar 17, 2015 · 10 comments · Fixed by #23559
Labels
I-needs-decision Issue: In need of a decision.

Comments

@nikomatsakis
Copy link
Contributor

I believe we intend to eventually extend the index traits with the ability to override the x[y] = z syntax specifically using a trait like IndexSet. This would allow hashmaps to be used like map[key] = value and have that insert the key, unlike today where map[key] = value will panic if key is not already present.

To pave the way, the simplest thing is to remove the IndexMut operator from those collections where we would anticipate growing on demand.

I believe that would be btree, hash_map, and possible vec_deque and vec_map.

Thoughts?

cc @aturon @alexcrichton @gankro
@nikomatsakis
Copy link
Contributor Author

triage: P-backcompat-libs I-needs-decision (1.0 beta)

@nikomatsakis
Copy link
Contributor Author

triage: P-backcompat-libs (1.0 beta)

@nikomatsakis nikomatsakis added the I-needs-decision Issue: In need of a decision. label Mar 17, 2015
@aturon
Copy link
Member

aturon commented Mar 17, 2015

I was just starting to prep a PR to resolve this by removing the impl, as you suggested. :)

@alexcrichton
Copy link
Member

Don't we want to keep the implementation on HashMap for example to do something like let a = &mut map[key]?

@aturon
Copy link
Member

aturon commented Mar 17, 2015

@alexcrichton Depending on the exact way that IndexSet is done, it may not be possible to provide both modes of use. That is, we want to be able to write map[key] = value -- which we cannot today -- but it's not clear that you can do that and also support examples like the one you gave. In particular, that involves adjusting inference to determine whether or not a deref is in play.

For now, removing these impls will give us the most freedom when we do incorporate IndexSet.

@Gankra
Copy link
Contributor

Gankra commented Mar 17, 2015

It seems weird to have this and insert -- insert has a return value, though. This would be a super-annoying ergonomic regression in the interim. What's the expected timeline for something like IndexSet? 1.1?

@Gankra
Copy link
Contributor

Gankra commented Mar 17, 2015

Oh actually upon reflection most usages I know are Index, not IndexMut. Not as awful a regression.

@mahkoh
Copy link
Contributor

mahkoh commented Mar 17, 2015

@aturon

Depending on the exact way that IndexSet is done, it may not be possible to provide both modes of use.

It should be possible to move in, out, and take references with the [] operator. If that's not possible with the current design then the current design has to be changed instead of committing to yet another subpar design until 2.0.

aturon added a commit to aturon/rust that referenced this issue Mar 20, 2015
@aturon
Future-proof indexing on maps: remove IndexMut
5fe0bb7 
This commit removes the `IndexMut` impls on `HashMap` and `BTreeMap`, in
order to future-proof the API against the eventual inclusion of an
`IndexSet` trait.

Ideally, we would eventually be able to support:

```rust
map[owned_key] = val;
map[borrowed_key].mutating_method(arguments);
&mut map[borrowed_key];
```

but to keep the design space as unconstrained as possible, we do not
currently want to support `IndexMut`, in case some other strategy will
eventually be needed.

Code currently using mutating index notation can use `get_mut` instead.

[breaking-change]

Closes rust-lang#23448
@aturon aturon mentioned this issue Mar 20, 2015
Merged
Manishearth added a commit to Manishearth/rust that referenced this issue Mar 21, 2015
@Manishearth
Rollup merge of rust-lang#23559 - aturon:future-proof-map-index, r=Ga…
eec7719 
…nkro

 This commit removes the `IndexMut` impls on `HashMap` and `BTreeMap`, in
order to future-proof the API against the eventual inclusion of an
`IndexSet` trait.

Ideally, we would eventually be able to support:

```rust
map[owned_key] = val;
map[borrowed_key].mutating_method(arguments);
&mut map[borrowed_key];
```

but to keep the design space as unconstrained as possible, we do not
currently want to support `IndexMut`, in case some other strategy will
eventually be needed.

Code currently using mutating index notation can use `get_mut` instead.

[breaking-change]

Closes rust-lang#23448

r? @gankro
Manishearth added a commit to Manishearth/rust that referenced this issue Mar 22, 2015
@Manishearth
Rollup merge of rust-lang#23559 - aturon:future-proof-map-index, r=Ga…
a91eece 
…nkro

 This commit removes the `IndexMut` impls on `HashMap` and `BTreeMap`, in
order to future-proof the API against the eventual inclusion of an
`IndexSet` trait.

Ideally, we would eventually be able to support:

```rust
map[owned_key] = val;
map[borrowed_key].mutating_method(arguments);
&mut map[borrowed_key];
```

but to keep the design space as unconstrained as possible, we do not
currently want to support `IndexMut`, in case some other strategy will
eventually be needed.

Code currently using mutating index notation can use `get_mut` instead.

[breaking-change]

Closes rust-lang#23448

r? @gankro
apasel422 added a commit to apasel422/tree that referenced this issue Mar 24, 2015
@apasel422
remove Map's IndexMut impl
1eeeef4 
CC rust-lang/rust#23448
@apasel422 apasel422 mentioned this issue Jul 5, 2015
Closed
@loggerhead loggerhead mentioned this issue Oct 18, 2016
Closed
@clouds56
Copy link

IndexSet sounds powerful and interesting, is there any RFC for it now?

@pablosichert pablosichert mentioned this issue Mar 26, 2021
Merged
10 tasks
@Danvil
Copy link
Contributor

Danvil commented Jan 31, 2024 •
edited
Loading

It feels odd that HashMap does not support IndexMut. I don't buy the argument that IndexMut would be confused with insert - no other container supports insertion via the [] syntax. Now if you would add some hypothetical IndexSet and support insertion into a HashMap via [], then someone would just get confused that they can't use [] to insert into a Vec.. (doesn't really makes sense, but what if entries are default constructible!) Not even talking about the pandora box of accidentally inserting elements into the container.

Imho we should keep [] consistent as an accessor to get references to data already existing in the container.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
I-needs-decision Issue: In need of a decision.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Future-proof indexing on maps: remove IndexMut aturon/rust
7 participants
@alexcrichton @nikomatsakis @Danvil @aturon @clouds56 @Gankra @mahkoh