Skip to content

Added new non_zero_suggestions lint #13167

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Sep 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5773,6 +5773,7 @@ Released 2018-09-13
[`non_minimal_cfg`]: https://rust-lang.github.io/rust-clippy/master/index.html#non_minimal_cfg
[`non_octal_unix_permissions`]: https://rust-lang.github.io/rust-clippy/master/index.html#non_octal_unix_permissions
[`non_send_fields_in_send_ty`]: https://rust-lang.github.io/rust-clippy/master/index.html#non_send_fields_in_send_ty
[`non_zero_suggestions`]: https://rust-lang.github.io/rust-clippy/master/index.html#non_zero_suggestions
[`nonminimal_bool`]: https://rust-lang.github.io/rust-clippy/master/index.html#nonminimal_bool
[`nonsensical_open_options`]: https://rust-lang.github.io/rust-clippy/master/index.html#nonsensical_open_options
[`nonstandard_macro_braces`]: https://rust-lang.github.io/rust-clippy/master/index.html#nonstandard_macro_braces
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -557,6 +557,7 @@ pub static LINTS: &[&crate::LintInfo] = &[
crate::non_expressive_names::SIMILAR_NAMES_INFO,
crate::non_octal_unix_permissions::NON_OCTAL_UNIX_PERMISSIONS_INFO,
crate::non_send_fields_in_send_ty::NON_SEND_FIELDS_IN_SEND_TY_INFO,
crate::non_zero_suggestions::NON_ZERO_SUGGESTIONS_INFO,
crate::nonstandard_macro_braces::NONSTANDARD_MACRO_BRACES_INFO,
crate::octal_escapes::OCTAL_ESCAPES_INFO,
crate::only_used_in_recursion::ONLY_USED_IN_RECURSION_INFO,
Expand Down
2 changes: 2 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,7 @@ mod non_copy_const;
mod non_expressive_names;
mod non_octal_unix_permissions;
mod non_send_fields_in_send_ty;
mod non_zero_suggestions;
mod nonstandard_macro_braces;
mod octal_escapes;
mod only_used_in_recursion;
Expand Down Expand Up @@ -940,5 +941,6 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
store.register_late_pass(|_| Box::new(pointers_in_nomem_asm_block::PointersInNomemAsmBlock));
store.register_late_pass(move |_| Box::new(manual_div_ceil::ManualDivCeil::new(conf)));
store.register_late_pass(|_| Box::new(manual_is_power_of_two::ManualIsPowerOfTwo));
store.register_late_pass(|_| Box::new(non_zero_suggestions::NonZeroSuggestions));
// add lints here, do not remove this comment, it's used in `new_lint`
}
143 changes: 143 additions & 0 deletions clippy_lints/src/non_zero_suggestions.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
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};
use rustc_middle::ty::{self, Ty};
use rustc_session::declare_lint_pass;
use rustc_span::symbol::sym;

declare_clippy_lint! {
/// ### What it does
/// Checks for conversions from `NonZero` types to regular integer types,
/// and suggests using `NonZero` types for the target as well.
///
/// ### Why is this bad?
/// Converting from `NonZero` types to regular integer types and then back to `NonZero`
/// types is less efficient and loses the type-safety guarantees provided by `NonZero` types.
/// Using `NonZero` types consistently can lead to more optimized code and prevent
/// certain classes of errors related to zero values.
///
/// ### Example
/// ```no_run
/// use std::num::{NonZeroU32, NonZeroU64};
///
/// fn example(x: u64, y: NonZeroU32) {
/// // Bad: Converting NonZeroU32 to u64 unnecessarily
/// let r1 = x / u64::from(y.get());
/// let r2 = x % u64::from(y.get());
/// }
/// ```
/// Use instead:
/// ```no_run
/// use std::num::{NonZeroU32, NonZeroU64};
///
/// fn example(x: u64, y: NonZeroU32) {
/// // Good: Preserving the NonZero property
/// let r1 = x / NonZeroU64::from(y);
/// let r2 = x % NonZeroU64::from(y);
/// }
/// ```
#[clippy::version = "1.81.0"]
pub NON_ZERO_SUGGESTIONS,
restriction,
"suggests using `NonZero#` from `u#` or `i#` for more efficient and type-safe conversions"
}

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::Binary(op, _, rhs) = expr.kind
&& matches!(op.node, BinOpKind::Div | BinOpKind::Rem)
{
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 !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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This catches NonZeroXX::from(x) but would miss y.into::<NonZeroXX>(). Do you want to tackle that case, too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the into method of NonZero does not take generic arguments. I am not sure of it. was it changed in the new update? Can you provide me with an example?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use std::num::{NonZeroU32, NonZeroU64};
fn main() {
    let x = NonZeroU32::try_from(1).unwrap();
    let _y: NonZeroU64 = x.into();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see.. we can add that to the unfixable test cases. It is a little tricky to tackle. That's why I added this one in the unfixable test cases:

    let x: u64 = u64::from(NonZeroU32::new(5).unwrap().get());

It can be added in the future optimizations.

&& 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
&& rcv_path.ident.name.as_str() == "get"
{
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);
}
}
}
}

fn get_arg_snippet(cx: &LateContext<'_>, arg: &Expr<'_>, rcv_path: &rustc_hir::PathSegment<'_>) -> String {
let arg_snippet = snippet(cx, arg.span, "..");
if let Some(index) = arg_snippet.rfind(&format!(".{}", rcv_path.ident.name)) {
arg_snippet[..index].trim().to_string()
} else {
arg_snippet.to_string()
}
}

fn suggest_non_zero_conversion(
cx: &LateContext<'_>,
expr: &Expr<'_>,
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(
cx,
NON_ZERO_SUGGESTIONS,
expr.span,
format!("consider using `{target_non_zero_type}::{fn_name}()` for more efficient and type-safe conversion"),
"replace with",
suggestion,
applicability,
);
}

fn get_target_non_zero_type(ty: Ty<'_>) -> Option<&'static str> {
match ty.kind() {
ty::Uint(uint_ty) => Some(match uint_ty {
ty::UintTy::U8 => "NonZeroU8",
ty::UintTy::U16 => "NonZeroU16",
ty::UintTy::U32 => "NonZeroU32",
ty::UintTy::U64 => "NonZeroU64",
ty::UintTy::U128 => "NonZeroU128",
ty::UintTy::Usize => "NonZeroUsize",
}),
ty::Int(int_ty) => Some(match int_ty {
ty::IntTy::I8 => "NonZeroI8",
ty::IntTy::I16 => "NonZeroI16",
ty::IntTy::I32 => "NonZeroI32",
ty::IntTy::I64 => "NonZeroI64",
ty::IntTy::I128 => "NonZeroI128",
ty::IntTy::Isize => "NonZeroIsize",
}),
_ => None,
}
}
65 changes: 65 additions & 0 deletions tests/ui/non_zero_suggestions.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
#![warn(clippy::non_zero_suggestions)]
use std::num::{NonZeroI16, NonZeroI8, NonZeroU16, NonZeroU32, NonZeroU64, NonZeroU8, NonZeroUsize};

fn main() {
/// Positive test cases (lint should trigger)
// U32 -> U64
let x: u64 = 100;
let y = NonZeroU32::new(10).unwrap();
let r1 = x / NonZeroU64::from(y);
//~^ ERROR: consider using `NonZeroU64::from()` for more efficient and type-safe conversion

let r2 = x % NonZeroU64::from(y);
//~^ ERROR: consider using `NonZeroU64::from()` for more efficient and type-safe conversion

// U16 -> U32
let a: u32 = 50;
let b = NonZeroU16::new(5).unwrap();
let r3 = a / NonZeroU32::from(b);
//~^ ERROR: consider using `NonZeroU32::from()` for more efficient and type-safe conversion

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 r7 = e / f.get();

// Smaller to larger, but not NonZero
let g: u64 = 1000;
let h: u32 = 50;
let r8 = g / u64::from(h);

// Using From correctly
let k: u64 = 300;
let l = NonZeroU32::new(15).unwrap();
let r9 = k / NonZeroU64::from(l);
}

// Additional function to test the lint in a different context
fn divide_numbers(x: u64, y: NonZeroU32) -> u64 {
x / NonZeroU64::from(y)
//~^ ERROR: consider using `NonZeroU64::from()` for more efficient and type-safe conversion
}

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
}
}
65 changes: 65 additions & 0 deletions tests/ui/non_zero_suggestions.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
#![warn(clippy::non_zero_suggestions)]
use std::num::{NonZeroI16, NonZeroI8, NonZeroU16, NonZeroU32, NonZeroU64, NonZeroU8, NonZeroUsize};

fn main() {
/// Positive test cases (lint should trigger)
// U32 -> U64
let x: u64 = 100;
let y = NonZeroU32::new(10).unwrap();
let r1 = x / u64::from(y.get());
//~^ ERROR: consider using `NonZeroU64::from()` for more efficient and type-safe conversion

let r2 = x % u64::from(y.get());
//~^ ERROR: consider using `NonZeroU64::from()` for more efficient and type-safe conversion

// U16 -> U32
let a: u32 = 50;
let b = NonZeroU16::new(5).unwrap();
let r3 = a / u32::from(b.get());
//~^ ERROR: consider using `NonZeroU32::from()` for more efficient and type-safe conversion

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 r7 = e / f.get();

// Smaller to larger, but not NonZero
let g: u64 = 1000;
let h: u32 = 50;
let r8 = g / u64::from(h);

// Using From correctly
let k: u64 = 300;
let l = NonZeroU32::new(15).unwrap();
let r9 = k / NonZeroU64::from(l);
}

// Additional function to test the lint in a different context
fn divide_numbers(x: u64, y: NonZeroU32) -> u64 {
x / u64::from(y.get())
//~^ ERROR: consider using `NonZeroU64::from()` for more efficient and type-safe conversion
}

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
}
}
41 changes: 41 additions & 0 deletions tests/ui/non_zero_suggestions.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
error: consider using `NonZeroU64::from()` for more efficient and type-safe conversion
--> tests/ui/non_zero_suggestions.rs:9:18
|
LL | let r1 = x / u64::from(y.get());
| ^^^^^^^^^^^^^^^^^^ help: replace with: `NonZeroU64::from(y)`
|
= 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.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:18:18
|
LL | let r3 = a / u32::from(b.get());
| ^^^^^^^^^^^^^^^^^^ help: replace with: `NonZeroU32::from(b)`

error: consider using `NonZeroU64::from()` for more efficient and type-safe conversion
--> tests/ui/non_zero_suggestions.rs:21:13
|
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: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:62:22
|
LL | self.value / u64::from(divisor.get())
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `NonZeroU64::from(divisor)`

error: aborting due to 6 previous errors

23 changes: 23 additions & 0 deletions tests/ui/non_zero_suggestions_unfixable.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
#![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);

let m = NonZeroU32::try_from(1).unwrap();
let _z: NonZeroU64 = m.into();
}

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) {}
23 changes: 23 additions & 0 deletions tests/ui/non_zero_suggestions_unfixable.stderr
Original file line number Diff line number Diff line change
@@ -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:19:5
|
LL | u64::from(y.get())
| ^^^^^^^^^^^^^^^^^^ help: replace with: `NonZeroU64::from(y)`

error: aborting due to 3 previous errors

Loading