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

implemented unnecessary min #12061

Closed
wants to merge 4 commits into from
Closed
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 @@ -5672,6 +5672,7 @@ Released 2018-09-13
[`unnecessary_lazy_evaluations`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_lazy_evaluations
[`unnecessary_literal_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_literal_unwrap
[`unnecessary_map_on_constructor`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_map_on_constructor
[`unnecessary_min`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_min
[`unnecessary_mut_passed`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_mut_passed
[`unnecessary_operation`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_operation
[`unnecessary_owned_empty_strings`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_owned_empty_strings
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.

[There are over 650 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
[There are over 700 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)

Lints are divided into categories, each with a default [lint level](https://doc.rust-lang.org/rustc/lints/levels.html).
You can choose how much Clippy is supposed to ~~annoy~~ help you by changing the lint level by category.
Expand Down
2 changes: 1 addition & 1 deletion book/src/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
A collection of lints to catch common mistakes and improve your
[Rust](https://github.com/rust-lang/rust) code.

[There are over 650 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
[There are over 700 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)

Lints are divided into categories, each with a default [lint
level](https://doc.rust-lang.org/rustc/lints/levels.html). You can choose how
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 @@ -450,6 +450,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
crate::methods::UNNECESSARY_JOIN_INFO,
crate::methods::UNNECESSARY_LAZY_EVALUATIONS_INFO,
crate::methods::UNNECESSARY_LITERAL_UNWRAP_INFO,
crate::methods::UNNECESSARY_MIN_INFO,
crate::methods::UNNECESSARY_SORT_BY_INFO,
crate::methods::UNNECESSARY_TO_OWNED_INFO,
crate::methods::UNWRAP_OR_DEFAULT_INFO,
Expand Down
24 changes: 24 additions & 0 deletions clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ mod unnecessary_iter_cloned;
mod unnecessary_join;
mod unnecessary_lazy_eval;
mod unnecessary_literal_unwrap;
mod unnecessary_min;
mod unnecessary_sort_by;
mod unnecessary_to_owned;
mod unwrap_expect_used;
Expand Down Expand Up @@ -3887,6 +3888,27 @@ declare_clippy_lint! {
"splitting a trimmed string at hard-coded newlines"
}

declare_clippy_lint! {
/// ### What it does
/// Checks for unnecessary calls to `min()`
///
/// ### Why is this bad?
///
/// In these cases it is not necessary to call `min()`
/// ### Example
/// ```no_run
/// let _ = 0.min(7_u32);
/// ```
/// Use instead:
/// ```no_run
/// let _ = 0;
/// ```
#[clippy::version = "1.77.0"]
pub UNNECESSARY_MIN,
complexity,
"using 'min()' when there is no need for it"
}

pub struct Methods {
avoid_breaking_exported_api: bool,
msrv: Msrv,
Expand Down Expand Up @@ -4043,6 +4065,7 @@ impl_lint_pass!(Methods => [
ITER_FILTER_IS_OK,
MANUAL_IS_VARIANT_AND,
STR_SPLIT_AT_NEWLINE,
UNNECESSARY_MIN,
]);

/// Extracts a method call name, args, and `Span` of the method name.
Expand Down Expand Up @@ -4324,6 +4347,7 @@ impl Methods {
Some(("bytes", recv2, [], _, _)) => bytes_count_to_len::check(cx, expr, recv, recv2),
_ => {},
},
("min", [arg]) => unnecessary_min::check(cx, expr, recv, arg),
("drain", ..) => {
if let Node::Stmt(Stmt { hir_id: _, kind, .. }) = cx.tcx.hir().get_parent(expr.hir_id)
&& matches!(kind, StmtKind::Semi(_))
Expand Down
111 changes: 111 additions & 0 deletions clippy_lints/src/methods/unnecessary_min.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
use std::cmp::Ordering;

use super::UNNECESSARY_MIN;
use clippy_utils::diagnostics::span_lint_and_sugg;

use clippy_utils::consts::{constant, Constant};
use clippy_utils::source::snippet;
use clippy_utils::{clip, int_bits, unsext};
use hir::Expr;

use rustc_errors::Applicability;
use rustc_hir as hir;
use rustc_lint::LateContext;

use rustc_middle::ty;
use rustc_span::Span;

pub fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, recv: &'tcx Expr<'_>, arg: &'tcx Expr<'_>) {
if both_are_constant(cx, expr, recv, arg) {
return;
}
one_extrema(cx, expr, recv, arg);
}
fn lint(cx: &LateContext<'_>, expr: &Expr<'_>, sugg: Span, other: Span) {
let msg = format!(
"`{}` is never greater than `{}` and has therefore no effect",
snippet(cx, sugg, "Not yet implemented"),
snippet(cx, other, "Not yet implemented")
);
span_lint_and_sugg(
cx,
UNNECESSARY_MIN,
expr.span,
&msg,
"try",
snippet(cx, sugg, "Not yet implemented").to_string(),
Applicability::MachineApplicable,
);
}

fn try_to_eval<'tcx>(
cx: &LateContext<'tcx>,
recv: &'tcx Expr<'_>,
arg: &'tcx Expr<'_>,
) -> (Option<Constant<'tcx>>, Option<Constant<'tcx>>) {
(
(constant(cx, cx.typeck_results(), recv)),
(constant(cx, cx.typeck_results(), arg)),
)
}
#[derive(Debug)]
enum Extrema {
Minimum,
Maximum,
}
fn detect_extrema<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<Extrema> {
let ty = cx.typeck_results().expr_ty(expr);

let cv = constant(cx, cx.typeck_results(), expr)?;

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,
}
}
fn both_are_constant<'tcx>(
cx: &LateContext<'tcx>,
expr: &'tcx Expr<'_>,
recv: &'tcx Expr<'_>,
arg: &'tcx Expr<'_>,
) -> bool {
let ty = cx.typeck_results().expr_ty(recv);
if let (Some(left), Some(right)) = try_to_eval(cx, recv, arg)
&& let Some(ord) = Constant::partial_cmp(cx.tcx, ty, &left, &right)
{
let (sugg, other) = match ord {
Ordering::Less => (recv.span, arg.span),
Ordering::Equal | Ordering::Greater => (arg.span, recv.span),
};

lint(cx, expr, sugg, other);
return true;
}
false
}
fn one_extrema<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, recv: &'tcx Expr<'_>, arg: &'tcx Expr<'_>) -> bool {
if let Some(extrema) = detect_extrema(cx, recv) {
match extrema {
Extrema::Minimum => lint(cx, expr, recv.span, arg.span),
Extrema::Maximum => lint(cx, expr, arg.span, recv.span),
}
return true;
} else if let Some(extrema) = detect_extrema(cx, arg) {
match extrema {
Extrema::Minimum => lint(cx, expr, arg.span, recv.span),
Extrema::Maximum => lint(cx, expr, recv.span, arg.span),
}
return true;
}

false
}
11 changes: 10 additions & 1 deletion tests/ui/cast.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,15 @@ help: ... or use `try_from` and handle the error accordingly
LL | i8::try_from((-99999999999i64).min(1));
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

error: `(-99999999999i64)` is never greater than `1` and has therefore no effect
--> $DIR/cast.rs:179:5
|
LL | (-99999999999i64).min(1) as i8;
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `(-99999999999i64)`
|
= note: `-D clippy::unnecessary-min` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::unnecessary_min)]`

error: casting `u64` to `u8` may truncate the value
--> $DIR/cast.rs:193:5
|
Expand Down Expand Up @@ -444,5 +453,5 @@ help: ... or use `try_from` and handle the error accordingly
LL | let c = u8::try_from(q / 1000);
| ~~~~~~~~~~~~~~~~~~~~~~

error: aborting due to 51 previous errors
error: aborting due to 52 previous errors

53 changes: 53 additions & 0 deletions tests/ui/unnecessary_min.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
#![allow(unused)]
#![warn(clippy::unnecessary_min)]
fn main() {
const A: i64 = 45;
const B: i64 = -1;
const C: i64 = const_fn(B);
let _ = (A * B); // Both are constants
let _ = B; // Both are constants
let _ = B; // Both are constants

let _ = (-6_i32); // Both are Literals
let _ = 6; // Both are Literals

let _ = 6; // Both are Literals

let _ = 0; // unsigned with zero
let _ = 0_u32; // unsigned with zero

let _ = i32::MIN; // singed MIN
let _ = i32::MIN; // singed MIN

let _ = 42; // singed MAX
let _ = 42; // singed MAX

let _ = 0; // unsigned with zero and function

let _ = 0; // unsigned with zero and function

let _ = i64::MIN; // signed with MIN and function

let _ = i64::MIN; // signed with MIN and function

let _ = test_i64(); // signed with MAX and function
let _ = test_i64(); // signed with MAX and function

let mut min = u32::MAX;
for _ in 0..1000 {
min = min.min(random_u32()); // shouldn't lint
}
}
fn test_usize() -> usize {
42
}
fn test_i64() -> i64 {
42
}
const fn const_fn(input: i64) -> i64 {
-2 * input
}
fn random_u32() -> u32 {
// random number generator
0
}
53 changes: 53 additions & 0 deletions tests/ui/unnecessary_min.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
#![allow(unused)]
#![warn(clippy::unnecessary_min)]
fn main() {
const A: i64 = 45;
const B: i64 = -1;
const C: i64 = const_fn(B);
let _ = (A * B).min(B); // Both are constants
let _ = C.min(B); // Both are constants
let _ = B.min(C); // Both are constants
Comment on lines +7 to +9
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know whether clippy has a policy for these sorts of things, but linting using the values of constants makes me wary. It would be possible to change A, B, or C in such a way that the use of min or max do actually do something, especially if it's something like min(CONST_LIMIT, user_input).

Copy link
Author

Choose a reason for hiding this comment

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

I don't know whether clippy has a policy for these sorts of things, but linting using the values of constants makes me wary. It would be possible to change A, B, or C in such a way that the use of min or max do actually do something

I don't think i get your point.
In your commented block of code all "variables" are constants so afaik it is not possible that one of them can change.
In the case of

min(CONST_LIMIT, user_input)

there should be only a lint emitted if CONST_LIMIT is either the minimum or maximum value. If it is the minimum value, then the result will always be the minumum regardless of the user_input and if it is the maximum value then the result will always be the user_input.

Or did you mean something like that?:

let mut m = u32::MAX;
for _ in 0..1000{
   m = m.min(user_input);
}

In this situation nothing should be linted.
I'll add a test to confirm it.

thank you for your feedback. I appreciate it.

Copy link
Author

Choose a reason for hiding this comment

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

i added the test


let _ = (-6_i32).min(9); // Both are Literals
let _ = 9_u32.min(6); // Both are Literals

let _ = 6.min(7_u8); // Both are Literals

let _ = 0.min(7_u8); // unsigned with zero
let _ = 7.min(0_u32); // unsigned with zero

let _ = i32::MIN.min(42); // singed MIN
let _ = 42.min(i32::MIN); // singed MIN

let _ = i32::MAX.min(42); // singed MAX
let _ = 42.min(i32::MAX); // singed MAX

let _ = 0.min(test_usize()); // unsigned with zero and function

let _ = test_usize().min(0); // unsigned with zero and function

let _ = i64::MIN.min(test_i64()); // signed with MIN and function

let _ = test_i64().min(i64::MIN); // signed with MIN and function

let _ = i64::MAX.min(test_i64()); // signed with MAX and function
let _ = test_i64().min(i64::MAX); // signed with MAX and function

let mut min = u32::MAX;
for _ in 0..1000 {
min = min.min(random_u32()); // shouldn't lint
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to see tests using the u32::min(a, b) syntax to be sure that the suggestions are correct.

It might be worth adding a test for things like u32::min(CONST, { some_math * abc - 3 }) or other complex expressions that could be placed in the call.

Copy link
Author

Choose a reason for hiding this comment

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

i'll add that.
Thank you.

Copy link
Author

Choose a reason for hiding this comment

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

i tried your suggestion and it doesn't lint it.
It seems like it currently only works as a method with the . like a.min(b).

fn test_usize() -> usize {
42
}
fn test_i64() -> i64 {
42
}
const fn const_fn(input: i64) -> i64 {
-2 * input
}
fn random_u32() -> u32 {
// random number generator
0
}
Loading