Unnecessary call to min/max method by l0ngvh · Pull Request #12368 · rust-lang/rust-clippy

l0ngvh · 2024-02-28T04:41:53Z

Continuation of #12061
Fix #11901 and #11924

This implementation only deal with literal int, like i32::MAX, -6_i32, 0

changelog: added lint [unnecessary_min_max]

rustbot · 2024-02-28T04:41:58Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Alexendoo (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

bors · 2024-03-22T13:36:16Z

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

Alexendoo · 2024-03-22T14:15:13Z

clippy_lints/src/methods/unnecessary_min_max.rs

+    match (ty.kind(), cv) {
+        (&ty::Uint(_), Constant::Int(0)) => Some(Extrema::Minimum),
+        (&ty::Int(ity), Constant::Int(i)) if i == unsext(cx.tcx, i128::MIN >> (128 - int_bits(cx.tcx, ity)), ity) => {
+            Some(Extrema::Minimum)
+        },
+
+        (&ty::Int(ity), Constant::Int(i)) if i == unsext(cx.tcx, i128::MAX >> (128 - int_bits(cx.tcx, ity)), ity) => {
+            Some(Extrema::Maximum)
+        },
+        (&ty::Uint(uty), Constant::Int(i)) if i == clip(cx.tcx, u128::MAX, uty) => Some(Extrema::Maximum),
+
+        _ => None,
+    }


unsext/clip/int_bits handle usize/isize but for this lint we wouldn't want that (excluding 0usize) to ensure x.min(i32::MAX as isize) doesn't lint on 32 bit targets

cv.int_value(...)? could replace the unsexts with ity/uty.bit_width() instead of int_bits to exclude *size

ensure x.min(i32::MAX as isize) doesn't lint on 32 bit targets

I don't really understand this. If x is of i32, it shouldn't be able to be compared to i32::MAX as isize?

In that example it would be that x is an isize, it wasn't a reference to the existing i32 x in the test file

tests/ui/cast.stderr

Alexendoo · 2024-03-26T18:02:06Z

tests/ui/unnecessary_min_max.fixed

+        min = min.min(random_u32());
+    }
+
+    let _ = (u32::MIN as isize).min(42);


An implementation detail of constant but I don't think it handles as casts, you'd need to either put these in a const or inline the value as e.g. 2147483647isize with a comment

Alexendoo · 2024-03-27T13:55:56Z

clippy_lints/src/methods/unnecessary_min_max.rs

+}
+
+fn lint(cx: &LateContext<'_>, expr: &Expr<'_>, name: &str, lhs: Span, rhs: Span, order: Ordering) {
+    let cmp_str = if (name == "min" && order.is_ge()) || (name == "max" && order.is_ge()) {


I believe this can be

Suggested change

let cmp_str = if (name == "min" && order.is_ge()) || (name == "max" && order.is_ge()) {

let cmp_str = if order.is_ge() {

With that & squashed commits I think we're good to go

Alexendoo · 2024-03-30T15:37:34Z

zulip thread: https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/FCP.20.60unnecessary_min_max.60

bors · 2024-04-04T23:51:58Z

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

Alexendoo · 2024-04-23T14:46:31Z

clippy_lints/src/methods/unnecessary_min_or_max.rs

+fn is_external_const(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
+    let ExprKind::Path(ref qpath) = expr.kind else {
+        return false;
+    };
+
+    let Some(def_id) = cx.qpath_res(qpath, expr.hir_id()).opt_def_id() else {
+        return false;
+    };
+    !def_id.is_local()
+}


This won't catch some indirect usage, e.g. EXTERNAL_CONST + 1. You can use constant_with_source but it doesn't currently track if the constant is local or not. That's probably fine though as the same logic of it may only happen to be 0 applies to local constants equally

A good addition would be to have the const evaluation distinguish consts in core, it would be a shame to lose those

I agree. A significant purpose of this lint rule is to detect incorrect usage of i32::MIN. I will see what I can do here.

Alexendoo · 2024-04-28T12:55:20Z

clippy_utils/src/consts.rs

+    /// Simple constant folding to determine if an expression depends on an external crate
+    /// excluding crates listed in exceptions
+    pub fn expr_is_external(&mut self, e: &Expr<'_>, exceptions: &[Symbol]) -> bool {


Let's add a new variant and check it in expr/fetch_path_and_apply rather than revisiting after the fact. It can be just specific to core as that's all we're using it for currently. ConstantSource::CoreConstant or similar

bors · 2024-05-03T09:34:34Z

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

Alexendoo · 2024-05-06T12:08:59Z

clippy_utils/src/consts.rs

+    /// The value is dependent on a constant defined in current crate of `core` crate.
+    LocalOrCoreCrateConstant,


Let's keep it just core constants, local ones may have "arbitrary" values just as remote ones do while the ones in core are pretty straight forward

Alexendoo · 2024-05-06T12:10:28Z

clippy_config/src/conf.rs

+    /// Lint: UNNECESSARY_MIN_OR_MAX.
+    ///
+    /// Whether to also run the lints on external defined consts.
+    (allowed_external_crates: bool = false),


I think it would be fine to have no config, it seems like it would be very rare that an external crate would redefine a constant to be MIN/MAX in a way that the user wants to trigger the lint

bors · 2024-06-03T09:14:41Z

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

rustbot · 2024-06-03T09:43:13Z

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:

aa83de2

Alexendoo

Sorry I forgot to press submit review and left this as pending 😅

Alexendoo · 2024-05-17T13:35:03Z

clippy_utils/src/consts.rs

+                    this.source = if is_core_crate {
+                        ConstantSource::CoreConstant
+                    } else {
+                        ConstantSource::Constant
+                    };


If source is already Constant we wouldn't want to override it with CoreConstant

Sorry for taking so long to come back to this 🙇 I have pushed the new changes, could you give it another go?

bors · 2024-06-16T23:06:15Z

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

Alexendoo · 2024-06-20T13:22:12Z

clippy_lints/src/methods/unnecessary_min_or_max.rs

+    if let (
+        Some((left, ConstantSource::Local | ConstantSource::CoreConstant)),
+        Some((right, ConstantSource::Local | ConstantSource::CoreConstant)),
+    ) = (
+        constant_with_source(cx, typeck_results, recv),
+        constant_with_source(cx, typeck_results, arg),
+    ) {


Let's go with if let Some(..) == constant_with_source(..) && let Some(..) = constant_with_source(..) so we don't have to evaluate arg as a constant if recv isn't one

Alexendoo · 2024-06-20T14:11:49Z

Great, thanks!

@bors r+

bors · 2024-06-20T14:11:52Z

📌 Commit 2f9f204 has been approved by Alexendoo

It is now in the queue for this repository.

bors · 2024-06-20T14:12:59Z

⌛ Testing commit 2f9f204 with merge 3e84ca8...

bors · 2024-06-20T14:22:03Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Alexendoo
Pushing 3e84ca8 to master...

rustbot assigned Alexendoo Feb 28, 2024

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

Alexendoo reviewed Mar 22, 2024

View reviewed changes

l0ngvh force-pushed the unnecessary-min branch 2 times, most recently from 5d840b6 to 2a22b1c Compare March 25, 2024 01:03

l0ngvh requested a review from Alexendoo March 25, 2024 04:33

Alexendoo reviewed Mar 26, 2024

View reviewed changes

l0ngvh requested a review from Alexendoo March 27, 2024 04:26

Alexendoo reviewed Mar 27, 2024

View reviewed changes

l0ngvh force-pushed the unnecessary-min branch 2 times, most recently from 894ea82 to 961adc9 Compare March 27, 2024 22:47

l0ngvh requested a review from Alexendoo March 27, 2024 22:47

Alexendoo approved these changes Mar 30, 2024

View reviewed changes

Alexendoo added S-final-comment-period Status: final comment period it will be merged unless new objections are raised (~1 week) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Mar 30, 2024

xFrednet mentioned this pull request Apr 1, 2024

implemented unnecessary min #12061

Closed

l0ngvh force-pushed the unnecessary-min branch from 882f85b to 7a091b3 Compare April 4, 2024 06:00

l0ngvh force-pushed the unnecessary-min branch 3 times, most recently from 5160ac7 to d7d6b41 Compare April 9, 2024 09:10

l0ngvh force-pushed the unnecessary-min branch from b4b77cd to 2a688ad Compare April 23, 2024 08:58

Alexendoo reviewed Apr 23, 2024

View reviewed changes

l0ngvh force-pushed the unnecessary-min branch 2 times, most recently from fc2d35c to f446e4e Compare April 28, 2024 08:31

l0ngvh requested a review from Alexendoo April 28, 2024 08:48

Alexendoo reviewed Apr 28, 2024

View reviewed changes

l0ngvh force-pushed the unnecessary-min branch 2 times, most recently from 9d33968 to 31f5010 Compare April 29, 2024 03:33

l0ngvh requested a review from Alexendoo April 29, 2024 03:46

Alexendoo reviewed May 6, 2024

View reviewed changes

l0ngvh force-pushed the unnecessary-min branch from 31f5010 to e0f3cfb Compare May 17, 2024 00:08

l0ngvh requested a review from Alexendoo May 17, 2024 00:15

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 Jun 3, 2024

l0ngvh force-pushed the unnecessary-min branch from aa83de2 to 0bd0af2 Compare June 3, 2024 09:49

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 Jun 3, 2024

Alexendoo reviewed Jun 3, 2024

View reviewed changes

l0ngvh force-pushed the unnecessary-min branch 3 times, most recently from 141c346 to e9501b8 Compare June 20, 2024 04:21

Alexendoo reviewed Jun 20, 2024

View reviewed changes

feat: unnecessary_min_max lint

2f9f204

l0ngvh force-pushed the unnecessary-min branch from e9501b8 to 2f9f204 Compare June 20, 2024 13:57

l0ngvh requested a review from Alexendoo June 20, 2024 13:57

Alexendoo approved these changes Jun 20, 2024

View reviewed changes

bors merged commit 3e84ca8 into rust-lang:master Jun 20, 2024

y21 mentioned this pull request Sep 11, 2024

unnecessary call to min function #11924

Closed

	let cmp_str = if (name == "min" && order.is_ge()) \|\| (name == "max" && order.is_ge()) {
	let cmp_str = if order.is_ge() {

		/// The value is dependent on a constant defined in current crate of `core` crate.
		LocalOrCoreCrateConstant,

Conversation

l0ngvh commented Feb 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Feb 28, 2024

Uh oh!

bors commented Mar 22, 2024

Uh oh!

Alexendoo Mar 22, 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!

l0ngvh Mar 27, 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!

Alexendoo Mar 26, 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!

Choose a reason for hiding this comment

Uh oh!

Alexendoo commented Mar 30, 2024

Uh oh!

bors commented Apr 4, 2024

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!

bors commented May 3, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented Jun 3, 2024

Uh oh!

rustbot commented Jun 3, 2024

Uh oh!

Alexendoo 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!

bors commented Jun 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Alexendoo commented Jun 20, 2024

Uh oh!

bors commented Jun 20, 2024

Uh oh!

bors commented Jun 20, 2024

Uh oh!

bors commented Jun 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

l0ngvh commented Feb 28, 2024 •

edited

Loading

Alexendoo Mar 22, 2024 •

edited

Loading

l0ngvh Mar 27, 2024 •

edited

Loading

Alexendoo Mar 26, 2024 •

edited

Loading