dimforge / dimforge/nalgebra

Hash for Matrix doesn't match `Borrow<[[T; R]; C]>` semantics

Open
#1,352 0 comments 0 reactions 0 assignees View on GitHub
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.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.