Hash for Matrix doesn't match `Borrow<[[T; R]; C]>` semantics
- Dominant language
- Rust
- Stars
- 4.8k
- Forks
- 565
- PR merge metrics
- No merged PRs in 30d
Description
`Borrow for Self` implies that trait implementations on `Self` and `T` behave identically, in notable particular `Eq`, `Ord`, and `Hash`. The `Hash` implementation for `Matrix` is different from the one for arrays, resulting in e.g. the hash of `Matrix3` differing from that of the same matrix `Borrow::borrow`ed as `[[i32; 3]; 3]`. [[playground]](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=dcb216c33b8d91f144292dcd7ca17431)
```rust
use nalgebra; // 0.32.3
use std::borrow::Borrow;
use std::hash::{BuildHasher, Hash};
use std::collections::hash_map::RandomState;
fn main() {
let s = RandomState::new();
let v = nalgebra::matrix![3, 1, 4; 1, 5, 9; 2, 6, 5];
assert_eq!(s.hash_one(&v), s.hash_one(<_ as Borrow<[_; 3]>>::borrow(&v))); // fails, should pass
}
```
Either the `Borrow` implementation should be removed or the `Hash` implementation should be changed to try to match the array `Hash` implementation. I think the only way to do so properly[^x] would be to hash the `data` field directly; `ArrayStorage` has the properly equivalent `Hash` implementation, so it's a matter of requiring all `impl RawStorage` to provide a `Hash` impl. I think the way to do so in a non-API-breaking manner would be to add a copy of the `Hash::hash` method into the `RawStorage` trait and use that, as a default of the current behavior can be kept which `ArrayStorage` uses its derived `Hash` implementation. (This will result in hashing *slightly* more data, since R gets prefixed to each column individually, but that should be marginal for `ArrayStorage`-backed `Matrix`.)
[^x]: The presence of `Hash::hash_slice` as a customization point means any and all attempts to mirror the `Hash` behavior will fail in an edge case, short of constructing `&[[T; R]]` or `&[&[T]]` (which would require allocation in the general case).
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.