impl PartialEq for comparing primitive integer {u,i}N with NonZero{U,I}N

[inodentry]

It should be possible to directly compare primitive integers with their NonZero equivalents:

let x = 10u8;
let y = NonZeroU8::new(x).unwrap();

// this should work:
if x == y {
    // ...
}

Currently, such code produces a type mismatch compiler error due to PartialEq not being implemented for these types, to allow it.

Such a comparison seems trivial. It should be non-controversial to add support for it?

[leonardo-m]

Do you have an use case?

[inodentry]

Yes, I came here after being surprised by this in my own project.

I have an enum where I use NonZeroU8 to take advantage of the size packing optimization to keep it as one byte:

enum PlayerId {
    Spectator,
    Player(NonZeroU8),
}

This is useful for correctness (because I don't consider 0 to be a valid player id in my multiplayer game), for it to be used in binary messages (have it optimized to a no-op), and to save memory (in arrays containing many of them, or to not bloat the stack). Basically, the standard advantages of the NonZero types (what motivated them being added to Rust).

However, it is quite annoying to use in practice, because I cannot compare it with arbitrary u8 (which includes literals!).

I have to suffer the endless papercuts with type conversions (having to type u8::from(id) everywhere), because I want NonZeroU8 in my enum (for semantic correctness and memory layout).

I am sure many people have other similar applications where using a NonZero type makes sense.

It seems silly to not support PartialEq for directly comparing these types. A NonZeroU8 is fully compatible and effectively equivalent to an u8.

I mean, it's a papercut, not a major blocker, but it's silly that I have to deal with it. It makes using these standard wrapper types a hassle and unergonomic.

[leonardo-m]

(having to type u8::from(id) everywhere)

There's also the option of using id.get()

[slerpyyy]

Implementations of PartialEq which compare values of different types are fairly rare in the std crate and may be surprising to some users. I definitely see the appeal to having this impl in your case, but I'm not convinced adding abstractions to the language which hide safety wrappers is such a good idea in general.

That being said, how about you add these abstractions on your end directly?

Playground

[inodentry]

Thanks for the suggestion. You are right that there are things I can do on my end to improve the experience.

I guess adding such "conveniences" to the language is more controversial than I imagined.

[inodentry]

Implementations of PartialEq which compare values of different types are fairly rare in the std crate and may be surprising to some users.

I went to look at the docs to find examples, and found that there are already some such implementations:

• Path, PathBuf, OsString, OsStr can all be compared with each other (which makes sense since the underlying string is the same)
• ditto for slices, arrays, Vecs, VecDeques
• OsString/OsStr can be compared with &str, which seems more controversial, as str is guaranteed utf-8, but os strings may not be. This is an example of a convenience abstraction that may hide underlying details important for safety.

That considered, I don't see why comparing NonZero{I,U}* with {i,u}* is any different in that regard. It seems obvious enough that it shouldn't be surprising, and I can't think of how it could actually compromise safety.

[cuviper]

Having more PartialEq will break type inference in some cases, for example:

struct Foo;

impl PartialEq<Foo> for u32 {
    fn eq(&self, _: &Foo) -> bool {
        false
    }
}

fn main() {
    let x = 42_u32;
    let y = 0u8;
    
    assert!(x != y.into());
}

error[E0283]: type annotations needed
  --> src/main.rs:13:15
   |
13 |     assert!(x != y.into());
   |               ^^ -------- this method call resolves to `T`
   |               |
   |               cannot infer type
   |
   = note: cannot satisfy `u32: PartialEq<_>`

I ran into this scenario myself -- rust-num/num-bigint#150.

[osa1]

Would it be possible to run crater with this change to get an idea on how much breakage this would cause?

[slerpyyy]

I added an implementation (#81266) in case you to try things out.

Looking at the CI, it seems things are already falling appart:

error[E0283]: type annotations needed
    --> /cargo/registry/src/github.com-1ecc6299db9ec823/miniz_oxide-0.4.0/src/deflate/core.rs:1871:43
     |
1871 |         let far_and_small = cur_match_len == MIN_MATCH_LEN.into() && cur_match_dist >= 8 * 1024;
     |                                           ^^ -------------------- this method call resolves to `T`
     |                                           |
     |                                           cannot infer type
     |
     = note: cannot satisfy `u32: PartialEq<_>`

error[E0283]: type annotations needed
    --> /cargo/registry/src/github.com-1ecc6299db9ec823/miniz_oxide-0.4.0/src/deflate/core.rs:2037:43
     |
2037 |                         || (cur_match_len == MIN_MATCH_LEN.into() && cur_match_dist >= 8 * 1024)
     |                                           ^^ -------------------- this method call resolves to `T`
     |                                           |
     |                                           cannot infer type
     |
     = note: cannot satisfy `u32: PartialEq<_>`

error: aborting due to 2 previous errors

[jyn514]

I'm going to close this issue since adding the impl would cause a lot of inferrence issues.