From d43acb803f1bdabfd3761e54bd9509f8b19fb16a Mon Sep 17 00:00:00 2001 From: Samarth1696 Date: Thu, 1 Aug 2024 17:29:07 +0530 Subject: [PATCH] Added checks for binary expr and added different test cases for unfixable cases --- clippy_lints/src/non_zero_suggestions.rs | 53 +++++++++++++------ tests/ui/non_zero_suggestions.fixed | 45 +++++----------- tests/ui/non_zero_suggestions.rs | 45 +++++----------- tests/ui/non_zero_suggestions.stderr | 44 ++++----------- tests/ui/non_zero_suggestions_unfixable.rs | 20 +++++++ .../ui/non_zero_suggestions_unfixable.stderr | 23 ++++++++ 6 files changed, 118 insertions(+), 112 deletions(-) create mode 100644 tests/ui/non_zero_suggestions_unfixable.rs create mode 100644 tests/ui/non_zero_suggestions_unfixable.stderr diff --git a/clippy_lints/src/non_zero_suggestions.rs b/clippy_lints/src/non_zero_suggestions.rs index 1624c365ee94..808de147d722 100644 --- 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 index d7e6b19edc1f..9851063782e7 100644 --- 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); - //~^ 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 - - // Chained method calls - let _ = NonZeroU64::from(NonZeroU32::new(10).unwrap()); + let x = NonZeroU64::from(NonZeroU32::new(5).unwrap()); //~^ 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; + + // Should not trigger for any other operand other than `/` and `%` + let r5 = a + u32::from(b.get()); + let r6 = a - u32::from(b.get()); + // 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,13 +53,6 @@ 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, } @@ -78,5 +60,6 @@ struct Calculator { 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 index 8f256dabcb84..1605c459248c 100644 --- 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()); - //~^ 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 - - // Chained method calls - let _ = u64::from(NonZeroU32::new(10).unwrap().get()); + let x = u64::from(NonZeroU32::new(5).unwrap().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; + + // Should not trigger for any other operand other than `/` and `%` + let r5 = a + u32::from(b.get()); + let r6 = a - u32::from(b.get()); + // 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,13 +53,6 @@ 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, } @@ -78,5 +60,6 @@ struct Calculator { 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 index b231c8d216ed..7a57f7983be7 100644 --- 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 +error: consider using `NonZeroU64::from()` for more efficient and type-safe conversion + --> tests/ui/non_zero_suggestions.rs:21:13 | -LL | let r4 = i16::from(d.get()); - | ^^^^^^^^^^^^^^^^^^ help: replace with: `NonZeroI16::from(d)` +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: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 - | -LL | let r6 = u64::from(max_u32.get()); - | ^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `NonZeroU64::from(max_u32)` - -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 new file mode 100644 index 000000000000..71f5f94dcc7b --- /dev/null +++ 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 new file mode 100644 index 000000000000..5e22fddbb1a4 --- /dev/null +++ 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 +