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

Implement direct usize indexing #132

Merged
merged 2 commits into from
Aug 7, 2020
Merged

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Jun 11, 2020

where
    IndexMap<K, V, S>: IndexMut<usize, Output = V>,
    IndexSet<T, S>: Index<usize, Output = T>,

This allows map[i] and set[i] indexing to access values directly,
panicking if the index is out of bounds, similar to slices.

On maps, this somewhat overlaps with Index<&Q> + IndexMut<&Q> where
Q: Equivalent<K>. The reference makes this indexing unambiguous, but
it could be confusing to users if the key type is also an integer.

@cuviper
Copy link
Member Author

cuviper commented Jun 11, 2020

In my experiments in rustc, replacing HashMap,Vec combinations with indexmap, I've found it annoying to replace direct vec[i] indexing with get_index(i).unwrap().1. I didn't find anywhere that we've considered Index<usize> before, but it would make this a lot easier.

@cuviper
Copy link
Member Author

cuviper commented Jun 11, 2020

One downside is that with my experiment of custom index types, I would ideally replace this Index<usize> with Index<Idx> for IndexMap<K, V, S, Idx>, but I can't prove that Index<Idx> and Index<&Q> are distinct. We could still allow Index<usize> with any Idx though.

@bluss
Copy link
Member

bluss commented Jun 28, 2020

I think we should just wait with this, in that case. If we have an indexing operator, IMO it must use the index type that we have parameterized the map with.

@cuviper
Copy link
Member Author

cuviper commented Jul 1, 2020

I brought this up in the forum: https://users.rust-lang.org/t/any-tricks-for-generic-overlap-with-references/45186

I now think the answer is that we shouldn't try to create generic Index<Idx> at all. Instead, we can just add concrete Index<usize> and Index<u32> for maps and sets of the corresponding index type. Then if a user has a newtype index, coherence will still let them write their own Index<MyIndex>, which just needs to call get_index and unwrap/expect.

@bluss
Copy link
Member

bluss commented Jul 2, 2020

That makes sense but

ith Index for IndexMap<K, V, S, Idx>, but I can't prove that Index and Index<&Q> are distinct

Won't we have a trait bound on Idx? If that's a local trait it should be enough, it will prove that no references implement it? (Or maybe that doesn't work because references are fundamental)

@cuviper
Copy link
Member Author

cuviper commented Jul 2, 2020

(Or maybe that doesn't work because references are fundamental)

I think that's it. I shared a simplified playground, if you want to experiment with it:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=161bde1feeaa8c4aaaa864636f2f2ae1

error[E0119]: conflicting implementations of trait `std::ops::Index<&_>` for type `Container<_, &_>`:
[...]
   = note: downstream crates may implement trait `AsIndex` for type `&_`

@cuviper
Copy link
Member Author

cuviper commented Jul 24, 2020

@bluss any more thoughts on this? My plan for custom index types is to just implement each manually, rather than trying to have a blanket impl that potentially conflicts with Index<&Q>.

impl<K, V, S> Index<usize> for IndexMap<K, V, S, usize> {...}
impl<K, V, S> Index<u32> for IndexMap<K, V, S, u32> {...}
// perhaps all unsigned integers
// users can do their own `Index<NewTypeIndex>`

```rust
where
    IndexMap<K, V, S>: IndexMut<usize, Output = V>,
    IndexSet<T, S>: Index<usize, Output = T>,
```

This allows `map[i]` and `set[i]` indexing to access values directly,
panicking if the index is out of bounds, similar to slices.

On maps, this somewhat overlaps with `Index<&Q> + IndexMut<&Q>` where
`Q: Equivalent<K>`. The reference makes this indexing unambiguous, but
it could be confusing to users if the key type is also an integer.
@cuviper
Copy link
Member Author

cuviper commented Aug 7, 2020

Let's go ahead with that plan then...

@cuviper cuviper merged commit 8282873 into indexmap-rs:master Aug 7, 2020
@cuviper cuviper deleted the direct-index branch July 18, 2023 02:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants