Added new `non_zero_suggestions` lint #13167

Samarth1696 · 2024-07-26T13:43:10Z

Created lint based on the suggestions in #7291

Followed [lint naming conventions][lint_naming]
Added passing UI tests (including committed .stderr file)
cargo test passes locally
Executed cargo dev update_lints
Added lint documentation
Run cargo dev fmt

changelog: new [non_zero_suggestions] lint

rustbot · 2024-07-26T13:43:16Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @dswij (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

J-ZhengLi

👋 welcome~

I'm just stopping by and left some comments.

(btw, I didn't go through the details, only pointing out something rather obvious~)

clippy_lints/src/non_zero_suggestions.rs

tests/ui/non_zero_suggestions.rs

J-ZhengLi · 2024-07-26T14:46:12Z

clippy_lints/src/non_zero_suggestions.rs

+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
+        if let ExprKind::Call(func, [arg]) = expr.kind {
+            if let ExprKind::Path(qpath) = &func.kind {


maybe check if the expr is a binary expression first? Therefore the below example doesn't cause error after applying lint suggestion:

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

First I thought like that only, based on the suggestion from the issue but then I thought do we need it?

Do we need to check for binary expr?

I think yes? Notice that the tests are failing because the lint shouldn't suggestion a fix to a non-binary expr:

error[E0308]: mismatched types --> tests/ui/non_zero_suggestions.fixed:68:5 | LL | fn no_bin_exp(x: u64, y: NonZeroU32) -> u64 { | --- expected `u64` because of return type LL | NonZeroU64::from(y) | ^^^^^^^^^^^^^^^^^^^ expected `u64`, found `NonZero<u64>` | = note: expected type `u64` found struct `std::num::NonZero<u64>` help: call `Into::into` on this expression to convert `std::num::NonZero<u64>` into `u64` | LL | NonZeroU64::from(y).into() | +++++++

In this case, you shouldn't give a MachineApplicable suggestion at least, as it's telling rustfix to apply that suggestion because it's "always correct", but that's not the case since there could be type difference. I think the original issue specifically limit the cases to binary expr was because that they wanted to play safe, as the resulting type of an integer that add|sub|div|mul|... another NonZero type is still the integer type itself (example), so it can always being replaced to the NonZero type directly.

I think you could try adjusting the applicability base on whether the expr is binary or not, if yes, you can continue to offer a MachineApplicable suggestion as it is. But if not, you should state the applicability as MaybeIncorrect, so when a user runs clippy with --fix, it wouldn't cause any suggestion-cause-error issue.
For example, you could try something like this:

let mut appllicability = Applicability::MaybeIncorrect; if let ExprKind::Binary(_, _, rhs) = expr.kind { applicability = Applicability::MachineApplicable; check_for_fn(...); } else { check_for_fn(...); }

If you decided to do this, you may split the test cases to non-fixable and fixable tests, because in clippy, the suggestion will always get applied no matter the applicability. (There are some lints that does this, search files that end with fixable for hints)

I guess you are right. Limiting the lint to binary expressions would be better. It avoids complex cases and aligns with the original intent of the issue. I'll focus on implementing this change and add relevant test cases. I'll update the PR soon with these changes.

tests/ui/non_zero_suggestions.rs

Samarth1696 · 2024-07-29T17:02:35Z

Thank you very much for your thorough review and helpful suggestions, @J-ZhengLi ^^. Actually this is my first PR towards Open Source. 😅

J-ZhengLi · 2024-07-30T01:34:25Z

Thank you very much for your thorough review and helpful suggestions, @J-ZhengLi ^^. Actually this is my first PR towards Open Source. 😅

❤️ Already doing pretty good then, keep it up~

Samarth1696 · 2024-08-01T12:09:53Z

All fixed! can you check? @J-ZhengLi

J-ZhengLi · 2024-08-30T00:59:19Z

r? clippy

could someone have the time to take a look at this?

llogiq

This already looks good, I merely have a suggestion for an optional extension. If you don't want to tackle that, too, that's alright, we can merge after a rebase and open an issue describing the extension so it doesn't get lost.

llogiq · 2024-08-30T04:51:35Z

clippy_lints/src/non_zero_suggestions.rs

+
+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


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

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?

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

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.

clippy_lints/src/non_zero_suggestions.rs

rustbot · 2024-08-30T10:24:18Z

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust-clippy.git master
$ git push --force-with-lease

The following commits are merge commits:

87f0986

clippy_lints/src/non_zero_suggestions.rs

rustbot · 2024-08-31T06:53:05Z

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust-clippy.git master
$ git push --force-with-lease

The following commits are merge commits:

88c8dc6

Samarth1696 · 2024-08-31T07:03:11Z

@rustbot ready

llogiq · 2024-09-03T17:01:10Z

Can you remove the merge commit and instead rebase on current master?

Samarth1696 · 2024-09-04T08:15:20Z

It's ready!

bors · 2024-09-05T23:27:34Z

☔ The latest upstream changes (presumably #12987) made this pull request unmergeable. Please resolve the merge conflicts.

llogiq · 2024-09-06T21:12:45Z

r=me after rebase.

…able cases

llogiq · 2024-09-07T11:20:28Z

@bors r+

bors · 2024-09-07T11:20:30Z

📌 Commit af3346a has been approved by llogiq

It is now in the queue for this repository.

bors · 2024-09-07T11:21:38Z

⌛ Testing commit af3346a with merge d9c4523...

bors · 2024-09-07T11:29:04Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing d9c4523 to master...

rustbot assigned dswij Jul 26, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 26, 2024

J-ZhengLi reviewed Jul 26, 2024

View reviewed changes

tests/ui/non_zero_suggestions.rs Show resolved Hide resolved

rustbot assigned llogiq and unassigned dswij Aug 30, 2024

llogiq approved these changes Aug 30, 2024

View reviewed changes

rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Aug 30, 2024

y21 reviewed Aug 30, 2024

View reviewed changes

clippy_lints/src/non_zero_suggestions.rs Show resolved Hide resolved

Samarth1696 force-pushed the tests branch from 87f0986 to af268c8 Compare August 31, 2024 06:48

Samarth1696 requested review from y21 and llogiq August 31, 2024 07:05

Samarth1696 force-pushed the tests branch from 88c8dc6 to 1520e6a Compare September 4, 2024 08:04

rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Sep 4, 2024

llogiq approved these changes Sep 6, 2024

View reviewed changes

Samarth1696 force-pushed the tests branch from 1520e6a to 1dff0f5 Compare September 7, 2024 07:35

Samarth1696 added 8 commits September 7, 2024 13:21

Lint ready

46f8d36

test cases added including some edge cases

73039f6

dogfood test passed

6f01273

all tests passed

0f99aa9

refactored the code

bed4441

error notations added

c6c7408

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

d43acb8

…able cases

Check for get method and new test case in unfixable

af3346a

Samarth1696 force-pushed the tests branch from 1dff0f5 to af3346a Compare September 7, 2024 07:51

bors merged commit d9c4523 into rust-lang:master Sep 7, 2024
11 checks passed

Samarth1696 deleted the tests branch September 7, 2024 11:37

Samarth1696 restored the tests branch September 7, 2024 12:24

y21 mentioned this pull request Oct 19, 2024

Suggest the usage of NonZeroU#::from where appropriate #7291

Closed

DylanBulfin mentioned this pull request Nov 30, 2024

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

Open

Added new non_zero_suggestions lint #13167

Added new non_zero_suggestions lint #13167

Uh oh!

Conversation

Samarth1696 commented Jul 26, 2024 • edited by llogiq Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jul 26, 2024

Uh oh!

J-ZhengLi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

J-ZhengLi Jul 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Samarth1696 commented Jul 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

J-ZhengLi commented Jul 30, 2024

Uh oh!

Samarth1696 commented Aug 1, 2024

Uh oh!

J-ZhengLi commented Aug 30, 2024

Uh oh!

llogiq left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rustbot commented Aug 30, 2024

Uh oh!

Uh oh!

rustbot commented Aug 31, 2024

Uh oh!

Samarth1696 commented Aug 31, 2024

Uh oh!

llogiq commented Sep 3, 2024

Uh oh!

Samarth1696 commented Sep 4, 2024

Uh oh!

bors commented Sep 5, 2024

Uh oh!

llogiq commented Sep 6, 2024

Uh oh!

llogiq commented Sep 7, 2024

Uh oh!

bors commented Sep 7, 2024

Uh oh!

bors commented Sep 7, 2024

Uh oh!

bors commented Sep 7, 2024

Uh oh!

Uh oh!

Uh oh!

Added new `non_zero_suggestions` lint #13167

Added new `non_zero_suggestions` lint #13167

Samarth1696 commented Jul 26, 2024 •

edited by llogiq

Loading

J-ZhengLi left a comment •

edited

Loading

J-ZhengLi Jul 29, 2024 •

edited

Loading

Samarth1696 commented Jul 29, 2024 •

edited

Loading