-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
46f8d36
Lint ready
Samarth1696 73039f6
test cases added including some edge cases
Samarth1696 6f01273
dogfood test passed
Samarth1696 0f99aa9
all tests passed
Samarth1696 bed4441
refactored the code
Samarth1696 c6c7408
error notations added
Samarth1696 d43acb8
Added checks for binary expr and added different test cases for unfix…
Samarth1696 af3346a
Check for get method and new test case in unfixable
Samarth1696 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 | ||
&& 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 | ||
Samarth1696 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
&& 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) | ||
llogiq marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
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, | ||
} | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 | ||
} | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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()); | ||
Samarth1696 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
//~^ 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 | ||
} | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 | ||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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) {} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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 missy.into::<NonZeroXX>()
. Do you want to tackle that case, too?There was a problem hiding this comment.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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 theunfixable
test cases:It can be added in the future optimizations.