Added checks for binary expr and added different test cases for unfix…

…able cases
rust-lang · Sep 7, 2024 · d43acb8 · d43acb8
1 parent c6c7408
commit d43acb8
Show file tree

Hide file tree

Showing 6 changed files with 115 additions and 109 deletions.
diff --git a/clippy_lints/src/non_zero_suggestions.rs b/clippy_lints/src/non_zero_suggestions.rs
@@ -1,5 +1,6 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::source::snippet;
+use rustc_ast::ast::BinOpKind;
 use rustc_errors::Applicability;
 use rustc_hir::{Expr, ExprKind};
 use rustc_lint::{LateContext, LateLintPass};
@@ -48,23 +49,42 @@ declare_lint_pass!(NonZeroSuggestions => [NON_ZERO_SUGGESTIONS]);
 
 impl<'tcx> LateLintPass<'tcx> for NonZeroSuggestions {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
-        if let ExprKind::Call(func, [arg]) = expr.kind
-            && let ExprKind::Path(qpath) = &func.kind
-            && let Some(def_id) = cx.qpath_res(qpath, func.hir_id).opt_def_id()
-            && let ExprKind::MethodCall(rcv_path, receiver, _, _) = &arg.kind
+        if let ExprKind::Binary(op, _, rhs) = expr.kind
+            && matches!(op.node, BinOpKind::Div | BinOpKind::Rem)
         {
-            let fn_name = cx.tcx.item_name(def_id);
-            let target_ty = cx.typeck_results().expr_ty(expr);
-            let receiver_ty = cx.typeck_results().expr_ty(receiver);
+            check_non_zero_conversion(cx, rhs, Applicability::MachineApplicable);
+        } else {
+            // Check if the parent expression is a binary operation
+            let parent_is_binary = cx.tcx.hir().parent_iter(expr.hir_id).any(|(_, node)| {
+                matches!(node, rustc_hir::Node::Expr(parent_expr) if matches!(parent_expr.kind, ExprKind::Binary(..)))
+            });
 
-            if let ty::Adt(adt_def, _) = receiver_ty.kind()
-                && adt_def.is_struct()
-                && cx.tcx.get_diagnostic_name(adt_def.did()) == Some(sym::NonZero)
-            {
-                if let Some(target_non_zero_type) = get_target_non_zero_type(target_ty) {
-                    let arg_snippet = get_arg_snippet(cx, arg, rcv_path);
-                    suggest_non_zero_conversion(cx, expr, fn_name, target_non_zero_type, &arg_snippet);
-                }
+            if !parent_is_binary {
+                check_non_zero_conversion(cx, expr, Applicability::MaybeIncorrect);
+            }
+        }
+    }
+}
+
+fn check_non_zero_conversion(cx: &LateContext<'_>, expr: &Expr<'_>, applicability: Applicability) {
+    // Check if the expression is a function call with one argument
+    if let ExprKind::Call(func, [arg]) = expr.kind
+        && let ExprKind::Path(qpath) = &func.kind
+        && let Some(def_id) = cx.qpath_res(qpath, func.hir_id).opt_def_id()
+        && let ExprKind::MethodCall(rcv_path, receiver, _, _) = &arg.kind
+    {
+        let fn_name = cx.tcx.item_name(def_id);
+        let target_ty = cx.typeck_results().expr_ty(expr);
+        let receiver_ty = cx.typeck_results().expr_ty(receiver);
+
+        // Check if the receiver type is a NonZero type
+        if let ty::Adt(adt_def, _) = receiver_ty.kind()
+            && adt_def.is_struct()
+            && cx.tcx.get_diagnostic_name(adt_def.did()) == Some(sym::NonZero)
+        {
+            if let Some(target_non_zero_type) = get_target_non_zero_type(target_ty) {
+                let arg_snippet = get_arg_snippet(cx, arg, rcv_path);
+                suggest_non_zero_conversion(cx, expr, fn_name, target_non_zero_type, &arg_snippet, applicability);
             }
         }
     }
@@ -85,6 +105,7 @@ fn suggest_non_zero_conversion(
     fn_name: rustc_span::Symbol,
     target_non_zero_type: &str,
     arg_snippet: &str,
+    applicability: Applicability,
 ) {
     let suggestion = format!("{target_non_zero_type}::{fn_name}({arg_snippet})");
     span_lint_and_sugg(
@@ -94,7 +115,7 @@ fn suggest_non_zero_conversion(
         format!("consider using `{target_non_zero_type}::{fn_name}()` for more efficient and type-safe conversion"),
         "replace with",
         suggestion,
-        Applicability::MachineApplicable,
+        applicability,
     );
 }
 

diff --git a/tests/ui/non_zero_suggestions.fixed b/tests/ui/non_zero_suggestions.fixed
@@ -1,5 +1,4 @@
 #![warn(clippy::non_zero_suggestions)]
-
 use std::num::{NonZeroI16, NonZeroI8, NonZeroU16, NonZeroU32, NonZeroU64, NonZeroU8, NonZeroUsize};
 
 fn main() {
@@ -19,43 +18,33 @@ fn main() {
     let r3 = a / NonZeroU32::from(b);
     //~^ ERROR: consider using `NonZeroU32::from()` for more efficient and type-safe conversion
 
-    // I8 -> I16
-    let c: i16 = 25;
-    let d = NonZeroI8::new(3).unwrap();
-    let r4 = NonZeroI16::from(d);
-    //~^ ERROR: consider using `NonZeroI16::from()` for more efficient and type-safe conversion
-
-    // Different operations
-    let m: u64 = 400;
-    let n = NonZeroU32::new(20).unwrap();
-    let r5 = m / NonZeroU64::from(n);
+    let x = NonZeroU64::from(NonZeroU32::new(5).unwrap());
     //~^ ERROR: consider using `NonZeroU64::from()` for more efficient and type-safe conversion
 
-    /// Edge cases
-    // Using the max value of a type
-    let max_u32 = NonZeroU32::new(u32::MAX).unwrap();
-    let r6 = NonZeroU64::from(max_u32);
-    //~^ ERROR: consider using `NonZeroU64::from()` for more efficient and type-safe conversion
+    /// Negative test cases (lint should not trigger)
+    // Left hand side expressions should not be triggered
+    let c: u32 = 50;
+    let d = NonZeroU16::new(5).unwrap();
+    let r4 = u32::from(b.get()) / a;
 
-    // Chained method calls
-    let _ = NonZeroU64::from(NonZeroU32::new(10).unwrap());
-    //~^ ERROR: consider using `NonZeroU64::from()` for more efficient and type-safe conversion
+    // Should not trigger for any other operand other than `/` and `%`
+    let r5 = a + u32::from(b.get());
+    let r6 = a - u32::from(b.get());
 
-    /// Negative test cases (lint should not trigger)
     // Same size types
     let e: u32 = 200;
     let f = NonZeroU32::new(20).unwrap();
-    let r10 = e / f.get();
+    let r7 = e / f.get();
 
     // Smaller to larger, but not NonZero
     let g: u64 = 1000;
     let h: u32 = 50;
-    let r11 = g / u64::from(h);
+    let r8 = g / u64::from(h);
 
     // Using From correctly
     let k: u64 = 300;
     let l = NonZeroU32::new(15).unwrap();
-    let r12 = k / NonZeroU64::from(l);
+    let r9 = k / NonZeroU64::from(l);
 }
 
 // Additional function to test the lint in a different context
@@ -64,19 +53,13 @@ fn divide_numbers(x: u64, y: NonZeroU32) -> u64 {
     //~^ ERROR: consider using `NonZeroU64::from()` for more efficient and type-safe conversion
 }
 
-fn no_bin_exp(x: u64, y: NonZeroU32) -> u64 {
-    NonZeroU64::from(y)
-    //~^ ERROR: consider using `NonZeroU64::from()` for more efficient and type-safe conversion
-}
-
-fn some_fn_that_only_takes_u64(_: u64) {}
-
 struct Calculator {
     value: u64,
 }
 
 impl Calculator {
     fn divide(&self, divisor: NonZeroU32) -> u64 {
         self.value / NonZeroU64::from(divisor)
+        //~^ ERROR: consider using `NonZeroU64::from()` for more efficient and type-safe conversion
     }
 }
diff --git a/tests/ui/non_zero_suggestions.rs b/tests/ui/non_zero_suggestions.rs
@@ -1,5 +1,4 @@
 #![warn(clippy::non_zero_suggestions)]
-
 use std::num::{NonZeroI16, NonZeroI8, NonZeroU16, NonZeroU32, NonZeroU64, NonZeroU8, NonZeroUsize};
 
 fn main() {
@@ -19,43 +18,33 @@ fn main() {
     let r3 = a / u32::from(b.get());
     //~^ ERROR: consider using `NonZeroU32::from()` for more efficient and type-safe conversion
 
-    // I8 -> I16
-    let c: i16 = 25;
-    let d = NonZeroI8::new(3).unwrap();
-    let r4 = i16::from(d.get());
-    //~^ ERROR: consider using `NonZeroI16::from()` for more efficient and type-safe conversion
-
-    // Different operations
-    let m: u64 = 400;
-    let n = NonZeroU32::new(20).unwrap();
-    let r5 = m / u64::from(n.get());
+    let x = u64::from(NonZeroU32::new(5).unwrap().get());
     //~^ ERROR: consider using `NonZeroU64::from()` for more efficient and type-safe conversion
 
-    /// Edge cases
-    // Using the max value of a type
-    let max_u32 = NonZeroU32::new(u32::MAX).unwrap();
-    let r6 = u64::from(max_u32.get());
-    //~^ ERROR: consider using `NonZeroU64::from()` for more efficient and type-safe conversion
+    /// Negative test cases (lint should not trigger)
+    // Left hand side expressions should not be triggered
+    let c: u32 = 50;
+    let d = NonZeroU16::new(5).unwrap();
+    let r4 = u32::from(b.get()) / a;
 
-    // Chained method calls
-    let _ = u64::from(NonZeroU32::new(10).unwrap().get());
-    //~^ ERROR: consider using `NonZeroU64::from()` for more efficient and type-safe conversion
+    // Should not trigger for any other operand other than `/` and `%`
+    let r5 = a + u32::from(b.get());
+    let r6 = a - u32::from(b.get());
 
-    /// Negative test cases (lint should not trigger)
     // Same size types
     let e: u32 = 200;
     let f = NonZeroU32::new(20).unwrap();
-    let r10 = e / f.get();
+    let r7 = e / f.get();
 
     // Smaller to larger, but not NonZero
     let g: u64 = 1000;
     let h: u32 = 50;
-    let r11 = g / u64::from(h);
+    let r8 = g / u64::from(h);
 
     // Using From correctly
     let k: u64 = 300;
     let l = NonZeroU32::new(15).unwrap();
-    let r12 = k / NonZeroU64::from(l);
+    let r9 = k / NonZeroU64::from(l);
 }
 
 // Additional function to test the lint in a different context
@@ -64,19 +53,13 @@ fn divide_numbers(x: u64, y: NonZeroU32) -> u64 {
     //~^ ERROR: consider using `NonZeroU64::from()` for more efficient and type-safe conversion
 }
 
-fn no_bin_exp(x: u64, y: NonZeroU32) -> u64 {
-    u64::from(y.get())
-    //~^ ERROR: consider using `NonZeroU64::from()` for more efficient and type-safe conversion
-}
-
-fn some_fn_that_only_takes_u64(_: u64) {}
-
 struct Calculator {
     value: u64,
 }
 
 impl Calculator {
     fn divide(&self, divisor: NonZeroU32) -> u64 {
         self.value / u64::from(divisor.get())
+        //~^ ERROR: consider using `NonZeroU64::from()` for more efficient and type-safe conversion
     }
 }
diff --git a/tests/ui/non_zero_suggestions.stderr b/tests/ui/non_zero_suggestions.stderr
@@ -1,5 +1,5 @@
 error: consider using `NonZeroU64::from()` for more efficient and type-safe conversion
-  --> tests/ui/non_zero_suggestions.rs:10:18
+  --> tests/ui/non_zero_suggestions.rs:9:18
    |
 LL |     let r1 = x / u64::from(y.get());
    |                  ^^^^^^^^^^^^^^^^^^ help: replace with: `NonZeroU64::from(y)`
@@ -8,58 +8,34 @@ LL |     let r1 = x / u64::from(y.get());
    = help: to override `-D warnings` add `#[allow(clippy::non_zero_suggestions)]`
 
 error: consider using `NonZeroU64::from()` for more efficient and type-safe conversion
-  --> tests/ui/non_zero_suggestions.rs:13:18
+  --> tests/ui/non_zero_suggestions.rs:12:18
    |
 LL |     let r2 = x % u64::from(y.get());
    |                  ^^^^^^^^^^^^^^^^^^ help: replace with: `NonZeroU64::from(y)`
 
 error: consider using `NonZeroU32::from()` for more efficient and type-safe conversion
-  --> tests/ui/non_zero_suggestions.rs:19:18
+  --> tests/ui/non_zero_suggestions.rs:18:18
    |
 LL |     let r3 = a / u32::from(b.get());
    |                  ^^^^^^^^^^^^^^^^^^ help: replace with: `NonZeroU32::from(b)`
 
-error: consider using `NonZeroI16::from()` for more efficient and type-safe conversion
-  --> tests/ui/non_zero_suggestions.rs:25:14
-   |
-LL |     let r4 = i16::from(d.get());
-   |              ^^^^^^^^^^^^^^^^^^ help: replace with: `NonZeroI16::from(d)`
-
-error: consider using `NonZeroU64::from()` for more efficient and type-safe conversion
-  --> tests/ui/non_zero_suggestions.rs:31:18
-   |
-LL |     let r5 = m / u64::from(n.get());
-   |                  ^^^^^^^^^^^^^^^^^^ help: replace with: `NonZeroU64::from(n)`
-
 error: consider using `NonZeroU64::from()` for more efficient and type-safe conversion
-  --> tests/ui/non_zero_suggestions.rs:37:14
+  --> tests/ui/non_zero_suggestions.rs:21:13
    |
-LL |     let r6 = u64::from(max_u32.get());
-   |              ^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `NonZeroU64::from(max_u32)`
+LL |     let x = u64::from(NonZeroU32::new(5).unwrap().get());
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `NonZeroU64::from(NonZeroU32::new(5).unwrap())`
 
 error: consider using `NonZeroU64::from()` for more efficient and type-safe conversion
-  --> tests/ui/non_zero_suggestions.rs:41:13
-   |
-LL |     let _ = u64::from(NonZeroU32::new(10).unwrap().get());
-   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `NonZeroU64::from(NonZeroU32::new(10).unwrap())`
-
-error: consider using `NonZeroU64::from()` for more efficient and type-safe conversion
-  --> tests/ui/non_zero_suggestions.rs:63:9
+  --> tests/ui/non_zero_suggestions.rs:52:9
    |
 LL |     x / u64::from(y.get())
    |         ^^^^^^^^^^^^^^^^^^ help: replace with: `NonZeroU64::from(y)`
 
 error: consider using `NonZeroU64::from()` for more efficient and type-safe conversion
-  --> tests/ui/non_zero_suggestions.rs:68:5
-   |
-LL |     u64::from(y.get())
-   |     ^^^^^^^^^^^^^^^^^^ help: replace with: `NonZeroU64::from(y)`
-
-error: consider using `NonZeroU64::from()` for more efficient and type-safe conversion
-  --> tests/ui/non_zero_suggestions.rs:80:22
+  --> tests/ui/non_zero_suggestions.rs:62:22
    |
 LL |         self.value / u64::from(divisor.get())
    |                      ^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `NonZeroU64::from(divisor)`
 
-error: aborting due to 10 previous errors
+error: aborting due to 6 previous errors
 
diff --git a/tests/ui/non_zero_suggestions_unfixable.rs b/tests/ui/non_zero_suggestions_unfixable.rs
@@ -0,0 +1,20 @@
+#![warn(clippy::non_zero_suggestions)]
+//@no-rustfix
+use std::num::{NonZeroI16, NonZeroI8, NonZeroU16, NonZeroU32, NonZeroU64, NonZeroU8, NonZeroUsize};
+
+fn main() {
+    let x: u64 = u64::from(NonZeroU32::new(5).unwrap().get());
+    //~^ ERROR: consider using `NonZeroU64::from()` for more efficient and type-safe conversion
+
+    let n = NonZeroU32::new(20).unwrap();
+    let y = u64::from(n.get());
+    //~^ ERROR: consider using `NonZeroU64::from()` for more efficient and type-safe conversion
+    some_fn_that_only_takes_u64(y);
+}
+
+fn return_non_zero(x: u64, y: NonZeroU32) -> u64 {
+    u64::from(y.get())
+    //~^ ERROR: consider using `NonZeroU64::from()` for more efficient and type-safe conversion
+}
+
+fn some_fn_that_only_takes_u64(_: u64) {}
diff --git a/tests/ui/non_zero_suggestions_unfixable.stderr b/tests/ui/non_zero_suggestions_unfixable.stderr
@@ -0,0 +1,23 @@
+error: consider using `NonZeroU64::from()` for more efficient and type-safe conversion
+  --> tests/ui/non_zero_suggestions_unfixable.rs:6:18
+   |
+LL |     let x: u64 = u64::from(NonZeroU32::new(5).unwrap().get());
+   |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `NonZeroU64::from(NonZeroU32::new(5).unwrap())`
+   |
+   = note: `-D clippy::non-zero-suggestions` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::non_zero_suggestions)]`
+
+error: consider using `NonZeroU64::from()` for more efficient and type-safe conversion
+  --> tests/ui/non_zero_suggestions_unfixable.rs:10:13
+   |
+LL |     let y = u64::from(n.get());
+   |             ^^^^^^^^^^^^^^^^^^ help: replace with: `NonZeroU64::from(n)`
+
+error: consider using `NonZeroU64::from()` for more efficient and type-safe conversion
+  --> tests/ui/non_zero_suggestions_unfixable.rs:16:5
+   |
+LL |     u64::from(y.get())
+   |     ^^^^^^^^^^^^^^^^^^ help: replace with: `NonZeroU64::from(y)`
+
+error: aborting due to 3 previous errors
+