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
Merged
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 @@ -5917,6 +5917,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_or_max`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_min_or_max
[`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
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -471,6 +471,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_OR_MAX_INFO,
crate::methods::UNNECESSARY_RESULT_MAP_OR_ELSE_INFO,
crate::methods::UNNECESSARY_SORT_BY_INFO,
crate::methods::UNNECESSARY_TO_OWNED_INFO,
Expand Down
30 changes: 30 additions & 0 deletions clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ mod unnecessary_iter_cloned;
mod unnecessary_join;
mod unnecessary_lazy_eval;
mod unnecessary_literal_unwrap;
mod unnecessary_min_or_max;
mod unnecessary_result_map_or_else;
mod unnecessary_sort_by;
mod unnecessary_to_owned;
Expand Down Expand Up @@ -3945,6 +3946,31 @@ declare_clippy_lint! {
"cloning an `Option` via `as_ref().cloned()`"
}

declare_clippy_lint! {
/// ### What it does
/// Checks for unnecessary calls to `min()` or `max()` in the following cases
/// - Either both side is constant
/// - One side is clearly larger than the other, like i32::MIN and an i32 variable
///
/// ### Why is this bad?
///
/// In the aformentioned cases it is not necessary to call `min()` or `max()`
/// to compare values, it may even cause confusion.
///
/// ### Example
/// ```no_run
/// let _ = 0.min(7_u32);
/// ```
/// Use instead:
/// ```no_run
/// let _ = 0;
/// ```
#[clippy::version = "1.78.0"]
pub UNNECESSARY_MIN_OR_MAX,
complexity,
"using 'min()/max()' when there is no need for it"
}

declare_clippy_lint! {
/// ### What it does
/// Checks for usage of `.map_or_else()` "map closure" for `Result` type.
Expand Down Expand Up @@ -4267,6 +4293,7 @@ impl_lint_pass!(Methods => [
UNNECESSARY_GET_THEN_CHECK,
NEEDLESS_CHARACTER_ITERATION,
MANUAL_INSPECT,
UNNECESSARY_MIN_OR_MAX,
]);

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

use super::UNNECESSARY_MIN_OR_MAX;
use clippy_utils::diagnostics::span_lint_and_sugg;

use clippy_utils::consts::{constant, constant_with_source, Constant, ConstantSource, FullInt};
use clippy_utils::source::snippet;

use rustc_errors::Applicability;
use rustc_hir::Expr;
use rustc_lint::LateContext;
use rustc_middle::ty;
use rustc_span::Span;

pub(super) fn check<'tcx>(
cx: &LateContext<'tcx>,
expr: &'tcx Expr<'_>,
name: &str,
recv: &'tcx Expr<'_>,
arg: &'tcx Expr<'_>,
) {
let typeck_results = cx.typeck_results();
if let Some((left, ConstantSource::Local | ConstantSource::CoreConstant)) =
constant_with_source(cx, typeck_results, recv)
&& let Some((right, ConstantSource::Local | ConstantSource::CoreConstant)) =
constant_with_source(cx, typeck_results, arg)
{
let Some(ord) = Constant::partial_cmp(cx.tcx, typeck_results.expr_ty(recv), &left, &right) else {
return;
};

lint(cx, expr, name, recv.span, arg.span, ord);
} else if let Some(extrema) = detect_extrema(cx, recv) {
let ord = match extrema {
Extrema::Minimum => Ordering::Less,
Extrema::Maximum => Ordering::Greater,
};
lint(cx, expr, name, recv.span, arg.span, ord);
} else if let Some(extrema) = detect_extrema(cx, arg) {
let ord = match extrema {
Extrema::Minimum => Ordering::Greater,
Extrema::Maximum => Ordering::Less,
};
lint(cx, expr, name, recv.span, arg.span, ord);
}
}

fn lint(cx: &LateContext<'_>, expr: &Expr<'_>, name: &str, lhs: Span, rhs: Span, order: Ordering) {
let cmp_str = if order.is_ge() { "smaller" } else { "greater" };

let suggested_value = if (name == "min" && order.is_ge()) || (name == "max" && order.is_le()) {
snippet(cx, rhs, "..")
} else {
snippet(cx, lhs, "..")
};

span_lint_and_sugg(
cx,
UNNECESSARY_MIN_OR_MAX,
expr.span,
format!(
"`{}` is never {} than `{}` and has therefore no effect",
snippet(cx, lhs, ".."),
cmp_str,
snippet(cx, rhs, "..")
),
"try",
suggested_value.to_string(),
Applicability::MachineApplicable,
);
}

#[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 (cv.int_value(cx, ty)?, ty.kind()) {
(FullInt::S(i), &ty::Int(ity)) if i == i128::MIN >> (128 - ity.bit_width()?) => Some(Extrema::Minimum),
(FullInt::S(i), &ty::Int(ity)) if i == i128::MAX >> (128 - ity.bit_width()?) => Some(Extrema::Maximum),
(FullInt::U(i), &ty::Uint(uty)) if i == u128::MAX >> (128 - uty.bit_width()?) => Some(Extrema::Maximum),
(FullInt::U(0), &ty::Uint(_)) => Some(Extrema::Minimum),
_ => None,
}
}
17 changes: 15 additions & 2 deletions clippy_utils/src/consts.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#![allow(clippy::float_cmp)]

use crate::macros::HirNode;
use crate::source::{get_source_text, walk_span_to_context};
use crate::{clip, is_direct_expn_of, sext, unsext};

Expand All @@ -15,7 +16,7 @@ use rustc_middle::ty::{self, EarlyBinder, FloatTy, GenericArgsRef, IntTy, List,
use rustc_middle::{bug, mir, span_bug};
use rustc_span::def_id::DefId;
use rustc_span::symbol::{Ident, Symbol};
use rustc_span::SyntaxContext;
use rustc_span::{sym, SyntaxContext};
use rustc_target::abi::Size;
use std::cmp::Ordering;
use std::hash::{Hash, Hasher};
Expand Down Expand Up @@ -302,6 +303,8 @@ pub enum ConstantSource {
Local,
/// The value is dependent on a defined constant.
Constant,
/// The value is dependent on a constant defined in `core` crate.
CoreConstant,
}
impl ConstantSource {
pub fn is_local(&self) -> bool {
Expand Down Expand Up @@ -415,9 +418,19 @@ impl<'a, 'tcx> ConstEvalLateContext<'a, 'tcx> {
ExprKind::ConstBlock(ConstBlock { body, .. }) => self.expr(self.lcx.tcx.hir().body(body).value),
ExprKind::DropTemps(e) => self.expr(e),
ExprKind::Path(ref qpath) => {
let is_core_crate = if let Some(def_id) = self.lcx.qpath_res(qpath, e.hir_id()).opt_def_id() {
self.lcx.tcx.crate_name(def_id.krate) == sym::core
} else {
false
};
self.fetch_path_and_apply(qpath, e.hir_id, self.typeck_results.expr_ty(e), |this, result| {
let result = mir_to_const(this.lcx, result)?;
this.source = ConstantSource::Constant;
// If source is already Constant we wouldn't want to override it with CoreConstant
this.source = if is_core_crate && !matches!(this.source, ConstantSource::Constant) {
ConstantSource::CoreConstant
} else {
ConstantSource::Constant
};
Some(result)
})
},
Expand Down
1 change: 1 addition & 0 deletions tests/ui/auxiliary/external_consts.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
pub const MAGIC_NUMBER: i32 = 1;
1 change: 1 addition & 0 deletions tests/ui/cast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#![allow(
clippy::cast_abs_to_unsigned,
clippy::no_effect,
clippy::unnecessary_min_or_max,
clippy::unnecessary_operation,
clippy::unnecessary_literal_unwrap,
clippy::identity_op
Expand Down
Loading
Loading