non_zero_suggestions is triggered when converting from a nonzero value to a larger unsigned value

[zacknewman]

Summary

T::from is getting confused with G::from when T is an unsigned integer (e.g., u64) and G is the nonzero version (e.g., NonZeroU64). For example converting a NonZeroU32 into a u64 triggers the lint incorrectly.

Lint Name

non_zero_suggestions

Reproducer

I tried this code:

#![deny(clippy::non_zero_suggestions)]
use core::num::NonZeroU32;
fn main() {
    _ = u64::from(NonZeroU32::new(10).unwrap().get());
}

I saw this happen:

error: consider using `NonZeroU64::from()` for more efficient and type-safe conversion
 --> src/main.rs:4:9
  |
4 |     _ = u64::from(NonZeroU32::new(10).unwrap().get());
  |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `NonZeroU64::from(NonZeroU32::new(10).unwrap())`
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#non_zero_suggestions
note: the lint level is defined here
 --> src/main.rs:1:9
  |
1 | #![deny(clippy::non_zero_suggestions)]
  |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: could not compile `bug` (bin "bug") due to 1 previous error

I expected to see this happen: Compilation to succeed since u64::from is not the same as NonZeroU64::from.

Version

rustc 1.83.0 (90b35a623 2024-11-26)
binary: rustc
commit-hash: 90b35a6239c3d8bdabc530a6a0816f7ff89a0aaf
commit-date: 2024-11-26
host: x86_64-unknown-linux-gnu
release: 1.83.0
LLVM version: 19.1.1

Additional Labels

No response

[DylanBulfin]

I'm not an expert but this seems like expected behavior to me. What you're doing is creating a much more complicated, checked numeric type, with the overhead that entails, only to immediately convert it to a less-safe native type.

If you do need to do something like this you could instead do either of the below:

let _a: u64 = NonZeroU64::new(10).unwrap().get();
let _b: u64 = NonZeroU64::from(NonZeroU32::new(10).unwrap()).get();

[zacknewman]

You misunderstand that my example is just a minimal reproducible example not a realistic one. The point is if you have a NonZeroU32, x; but need a u64, u64::from(x.get()) should not trigger the warning. You could do NonZeroU64::from(x).get(), but I fail to see how that is better; furthermore the message that is displayed is unequivocally wrong since it recommends something that returns a completely different type (i.e., NonZeroU64 instead of u64).

Here is a more realistic example if you don't see how one doesn't always control the types involved:

use external_crate_i_dont_control::Bar;
use other_crate_i_dont_control as other;
fn main() {
    // `Bar` has a `NonZeroU32` field called `val`.
    // `foo` takes a `u64` and prints it to `stdout`.
    other::foo(Bar::new().val.get().into());
}

other::foo(NonZeroU64::from(Bar::new().val).get()) is not any better; in fact it's more verbose and requires importing NonZeroU64. If there is some reason that it is better that I'm somehow missing, then I'd love to hear the argument; but as stated, that doesn't change the fact that the message is wrong and at a minimum needs to be changed to add .get() that way the recommended change actually works (i.e., it returns a u64 and not a NonZeroU64).

[zacknewman]

One approach to fix this is to ignore temporaries since they are clearly only used once. Even better would be to only recommend converting to NonZeroU64 if the value is captured in an explicit variable and used multiple times (i.e., capturing a NonZeroU64 just to call .get() is basically the same thing as a temporary).

[DylanBulfin]

I see what you mean now, looks like this was noticed during implementation but wasn't seen as worth fixing at the time: #13167 (comment)

I don't think either of the approaches I suggested are better, I just mentioned them since they got around the basic example for me.

As for the suggestion being wrong, I think it's allowed in general for suggestions to be incorrect. There's a concept of 'applicability' of suggestions, and this suggestion is marked as MaybeIncorrect. There was some discussion around that in the PR as well #13167 (comment). It seems like the applicability is only used when determining whether suggestions are auto-applied, not sure why that is.

I also see it's in the Restriction lint group so that might have something to do with it being overly limiting/incorrect.

Leaving this in case any of the context is helpful but I'm very new to Clippy development so I think this is over my head.

[zacknewman]

I see what you mean now, looks like this was noticed during implementation but wasn't seen as worth fixing at the time: #13167 (comment)

Ah, indeed it was noticed. Good to know. I'll let the team close this if they still deem it not worthy to fix.