Skip to content
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

Unnecessary call to min/max method #12368

Merged
merged 1 commit into from
Jun 20, 2024

Conversation

vohoanglong0107
Copy link
Contributor

@vohoanglong0107 vohoanglong0107 commented Feb 28, 2024

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
Copy link
Collaborator

rustbot commented Feb 28, 2024

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

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

bors commented Mar 22, 2024

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

Comment on lines 90 to 87
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,
}
Copy link
Member

@Alexendoo Alexendoo Mar 22, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@vohoanglong0107 vohoanglong0107 Mar 27, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
@vohoanglong0107 vohoanglong0107 force-pushed the unnecessary-min branch 2 times, most recently from 5d840b6 to 2a22b1c Compare March 25, 2024 01:03
min = min.min(random_u32());
}

let _ = (u32::MIN as isize).min(42);
Copy link
Member

@Alexendoo Alexendoo Mar 26, 2024

Choose a reason for hiding this comment

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

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

}

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()) {
Copy link
Member

Choose a reason for hiding this comment

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

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() {

Copy link
Member

Choose a reason for hiding this comment

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

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

@vohoanglong0107 vohoanglong0107 force-pushed the unnecessary-min branch 2 times, most recently from 894ea82 to 961adc9 Compare March 27, 2024 22:47
@Alexendoo 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
@Alexendoo
Copy link
Member

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

@bors
Copy link
Collaborator

bors commented Apr 4, 2024

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

Comment on lines 95 to 100
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()
}
Copy link
Member

Choose a reason for hiding this comment

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

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

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 agree. A significant purpose of this lint rule is to detect incorrect usage of i32::MIN. I will see what I can do here.

@vohoanglong0107 vohoanglong0107 force-pushed the unnecessary-min branch 2 times, most recently from fc2d35c to f446e4e Compare April 28, 2024 08:31
Comment on lines 533 to 535
/// 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 {
Copy link
Member

Choose a reason for hiding this comment

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

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

@vohoanglong0107 vohoanglong0107 force-pushed the unnecessary-min branch 2 times, most recently from 9d33968 to 31f5010 Compare April 29, 2024 03:33
@bors
Copy link
Collaborator

bors commented May 3, 2024

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

Comment on lines 306 to 307
/// The value is dependent on a constant defined in current crate of `core` crate.
LocalOrCoreCrateConstant,
Copy link
Member

Choose a reason for hiding this comment

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

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

Comment on lines 616 to 619
/// Lint: UNNECESSARY_MIN_OR_MAX.
///
/// Whether to also run the lints on external defined consts.
(allowed_external_crates: bool = false),
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Collaborator

bors commented Jun 3, 2024

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

@rustbot
Copy link
Collaborator

rustbot commented Jun 3, 2024

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:

@rustbot 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
@rustbot 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
Copy link
Member

@Alexendoo Alexendoo left a comment

Choose a reason for hiding this comment

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

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

Comment on lines 428 to 433
this.source = if is_core_crate {
ConstantSource::CoreConstant
} else {
ConstantSource::Constant
};
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@bors
Copy link
Collaborator

bors commented Jun 16, 2024

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

@vohoanglong0107 vohoanglong0107 force-pushed the unnecessary-min branch 3 times, most recently from 141c346 to e9501b8 Compare June 20, 2024 04:21
Comment on lines 23 to 29
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),
) {
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

Great, thanks!

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 20, 2024

📌 Commit 2f9f204 has been approved by Alexendoo

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jun 20, 2024

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

@bors
Copy link
Collaborator

bors commented Jun 20, 2024

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

@bors bors merged commit 3e84ca8 into rust-lang:master Jun 20, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-final-comment-period Status: final comment period it will be merged unless new objections are raised (~1 week)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unnecessary call to max function
4 participants