Implement Index and IndexMut like collections.Counter.__getitem__

[qryxip]

This may be a breaking change because the Index implementation shadows <<Counter<_, _> as Deref>::Target as Index<_>>::index. (The new implementation returns zero instead of panics)

[coriolinus]

Hi, thanks for the PR! I agree that this is technically a breaking change because of the shadowing you mentioned. I also agree that this change increases compatibility with collections.Counter, and is unlikely to break anyone's code. However, it's no big deal to bump the minor version to properly handle the breakage.

There are some questions below about implementation details which I'd like to talk about before merging. Additionally, I'd like to see a bit of documentation demonstrating how to explicitly deref into the interior map for cases where a user wants to panic on a missing key. Despite those caveats, this looks pretty plausible to me.

[coriolinus]

I haven't previously played around much with the Borrow trait. I think this reads that anything whose borrow is of type Q can be used as an index. This makes things generic over both char and &char in the example. Is this correct?

If yes, great, but I'd like to see an example in the tests. If no, please help me understand what the usage of Borrow does here.

[qryxip]

No. T: Borrow<T> and &'_T: Borrow<T> for all T but not T: Borrow<&'_ T>.

impl<T, Q, N> Index<Q> for Counter<T, N>
where
    T: Hash + Eq + Borrow<Q>,
    Q: Hash + Eq,
    N: Zero,
{
    type Output = N;

    /// Index in immutable contexts
    ///
    /// ```
    /// # use counter::Counter;
    /// let counter = Counter::<_>::init("aabbcc".chars());
    /// assert_eq!(counter[&'a'], 2);
    /// assert_eq!(counter[&'b'], 2);
    /// assert_eq!(counter[&'c'], 2);
    /// assert_eq!(counter[&'d'], 0);
    /// ```
    fn index(&self, key: Q) -> &N {
        self.map.get(&key).unwrap_or(&self.zero)
    }
}

error[E0308]: mismatched types
 --> src/lib.rs:562:9
  |
6 | counter[&'c'] += 1;
  |         ^^^^ expected char, found &char
  |
  = note: expected type `char`
             found type `&char`

https://doc.rust-lang.org/stable/std/borrow/trait.Borrow.html#examples

[qryxip]

It is possible to accept both of char and &chars like defaultmap::DefaultHashMap::get.

[qryxip]

It was not possible.

error[E0207]: the type parameter `Q` is not constrained by the impl trait, self type, or predicates
   --> src/lib.rs:539:9
    |
539 | impl<T, Q, R, N> Index<R> for Counter<T, N>
    |         ^ unconstrained type parameter

error: aborting due to previous error

For more information about this error, try `rustc --explain E0207`.

[coriolinus]

Why no Q for IndexMut?

[qryxip]

To use HashMap::entry with Clone::clone. But I got a better solution. I modified the part to use ToOwned instead of Clone.

[coriolinus]

Why use self.zero here? It looks like this would also work, for a smaller struct size:

Suggested change

	self.map.get(key).unwrap_or(&self.zero)
	self.map.get(key).unwrap_or(&N::zero())

[qryxip]

No. The zero must be a struct field or static variable. And in the current Rust, we cannot declare generic static variables. Moreover Zero::zero is not a const fn.

error[E0515]: cannot return value referencing temporary value
   --> src/lib.rs:548:9
    |
548 |         self.map.get(key).unwrap_or(&N::zero())
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^---------^
    |         |                            |
    |         |                            temporary value created here
    |         returns a value referencing data owned by the current function

error: aborting due to previous error

For more information about this error, try `rustc --explain E0515`.

As an alternative, we can use RefCell to modify the inner HashMap for &self.

[qryxip]

Update the docstrings.

[coriolinus]

Acknowledged; this is on my plate to look at. Between the holidays and various other projects, I don't have a ton of discretionary time right now. I'm hoping to be able to get back to this within a week or so.

[coriolinus]

I'm very sorry that it's taken this long to get back to you, but I've finally had a chance to look in detail and understand your code. This is a good change, and I'm merging it now. Thanks for the contribution!