From caa49c85d6c1d45678d1065c689cc4d168e18dfb Mon Sep 17 00:00:00 2001 From: Yoshitomo Nakanishi Date: Thu, 18 Mar 2021 16:03:56 +0900 Subject: [PATCH] Move absurd_extreme_comparisons to its own module --- .../src/absurd_extreme_comparisons.rs | 173 +++++++++++++++++ clippy_lints/src/lib.rs | 9 +- clippy_lints/src/types/mod.rs | 178 +----------------- 3 files changed, 183 insertions(+), 177 deletions(-) create mode 100644 clippy_lints/src/absurd_extreme_comparisons.rs diff --git a/clippy_lints/src/absurd_extreme_comparisons.rs b/clippy_lints/src/absurd_extreme_comparisons.rs new file mode 100644 index 00000000000..33c720c666e --- /dev/null +++ b/clippy_lints/src/absurd_extreme_comparisons.rs @@ -0,0 +1,173 @@ +use rustc_hir::{BinOpKind, Expr, ExprKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty; +use rustc_session::{declare_lint_pass, declare_tool_lint}; + +use crate::consts::{constant, Constant}; + +use clippy_utils::comparisons::{normalize_comparison, Rel}; +use clippy_utils::diagnostics::span_lint_and_help; +use clippy_utils::source::snippet; +use clippy_utils::ty::is_isize_or_usize; +use clippy_utils::{clip, int_bits, unsext}; + +declare_clippy_lint! { + /// **What it does:** Checks for comparisons where one side of the relation is + /// either the minimum or maximum value for its type and warns if it involves a + /// case that is always true or always false. Only integer and boolean types are + /// checked. + /// + /// **Why is this bad?** An expression like `min <= x` may misleadingly imply + /// that it is possible for `x` to be less than the minimum. Expressions like + /// `max < x` are probably mistakes. + /// + /// **Known problems:** For `usize` the size of the current compile target will + /// be assumed (e.g., 64 bits on 64 bit systems). This means code that uses such + /// a comparison to detect target pointer width will trigger this lint. One can + /// use `mem::sizeof` and compare its value or conditional compilation + /// attributes + /// like `#[cfg(target_pointer_width = "64")] ..` instead. + /// + /// **Example:** + /// + /// ```rust + /// let vec: Vec = Vec::new(); + /// if vec.len() <= 0 {} + /// if 100 > i32::MAX {} + /// ``` + pub ABSURD_EXTREME_COMPARISONS, + correctness, + "a comparison with a maximum or minimum value that is always true or false" +} + +declare_lint_pass!(AbsurdExtremeComparisons => [ABSURD_EXTREME_COMPARISONS]); + +impl<'tcx> LateLintPass<'tcx> for AbsurdExtremeComparisons { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { + if let ExprKind::Binary(ref cmp, ref lhs, ref rhs) = expr.kind { + if let Some((culprit, result)) = detect_absurd_comparison(cx, cmp.node, lhs, rhs) { + if !expr.span.from_expansion() { + let msg = "this comparison involving the minimum or maximum element for this \ + type contains a case that is always true or always false"; + + let conclusion = match result { + AbsurdComparisonResult::AlwaysFalse => "this comparison is always false".to_owned(), + AbsurdComparisonResult::AlwaysTrue => "this comparison is always true".to_owned(), + AbsurdComparisonResult::InequalityImpossible => format!( + "the case where the two sides are not equal never occurs, consider using `{} == {}` \ + instead", + snippet(cx, lhs.span, "lhs"), + snippet(cx, rhs.span, "rhs") + ), + }; + + let help = format!( + "because `{}` is the {} value for this type, {}", + snippet(cx, culprit.expr.span, "x"), + match culprit.which { + ExtremeType::Minimum => "minimum", + ExtremeType::Maximum => "maximum", + }, + conclusion + ); + + span_lint_and_help(cx, ABSURD_EXTREME_COMPARISONS, expr.span, msg, None, &help); + } + } + } + } +} + +enum ExtremeType { + Minimum, + Maximum, +} + +struct ExtremeExpr<'a> { + which: ExtremeType, + expr: &'a Expr<'a>, +} + +enum AbsurdComparisonResult { + AlwaysFalse, + AlwaysTrue, + InequalityImpossible, +} + +fn is_cast_between_fixed_and_target<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool { + if let ExprKind::Cast(ref cast_exp, _) = expr.kind { + let precast_ty = cx.typeck_results().expr_ty(cast_exp); + let cast_ty = cx.typeck_results().expr_ty(expr); + + return is_isize_or_usize(precast_ty) != is_isize_or_usize(cast_ty); + } + + false +} + +fn detect_absurd_comparison<'tcx>( + cx: &LateContext<'tcx>, + op: BinOpKind, + lhs: &'tcx Expr<'_>, + rhs: &'tcx Expr<'_>, +) -> Option<(ExtremeExpr<'tcx>, AbsurdComparisonResult)> { + use AbsurdComparisonResult::{AlwaysFalse, AlwaysTrue, InequalityImpossible}; + use ExtremeType::{Maximum, Minimum}; + // absurd comparison only makes sense on primitive types + // primitive types don't implement comparison operators with each other + if cx.typeck_results().expr_ty(lhs) != cx.typeck_results().expr_ty(rhs) { + return None; + } + + // comparisons between fix sized types and target sized types are considered unanalyzable + if is_cast_between_fixed_and_target(cx, lhs) || is_cast_between_fixed_and_target(cx, rhs) { + return None; + } + + let (rel, normalized_lhs, normalized_rhs) = normalize_comparison(op, lhs, rhs)?; + + let lx = detect_extreme_expr(cx, normalized_lhs); + let rx = detect_extreme_expr(cx, normalized_rhs); + + Some(match rel { + Rel::Lt => { + match (lx, rx) { + (Some(l @ ExtremeExpr { which: Maximum, .. }), _) => (l, AlwaysFalse), // max < x + (_, Some(r @ ExtremeExpr { which: Minimum, .. })) => (r, AlwaysFalse), // x < min + _ => return None, + } + }, + Rel::Le => { + match (lx, rx) { + (Some(l @ ExtremeExpr { which: Minimum, .. }), _) => (l, AlwaysTrue), // min <= x + (Some(l @ ExtremeExpr { which: Maximum, .. }), _) => (l, InequalityImpossible), // max <= x + (_, Some(r @ ExtremeExpr { which: Minimum, .. })) => (r, InequalityImpossible), // x <= min + (_, Some(r @ ExtremeExpr { which: Maximum, .. })) => (r, AlwaysTrue), // x <= max + _ => return None, + } + }, + Rel::Ne | Rel::Eq => return None, + }) +} + +fn detect_extreme_expr<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option> { + let ty = cx.typeck_results().expr_ty(expr); + + let cv = constant(cx, cx.typeck_results(), expr)?.0; + + let which = match (ty.kind(), cv) { + (&ty::Bool, Constant::Bool(false)) | (&ty::Uint(_), Constant::Int(0)) => ExtremeType::Minimum, + (&ty::Int(ity), Constant::Int(i)) if i == unsext(cx.tcx, i128::MIN >> (128 - int_bits(cx.tcx, ity)), ity) => { + ExtremeType::Minimum + }, + + (&ty::Bool, Constant::Bool(true)) => ExtremeType::Maximum, + (&ty::Int(ity), Constant::Int(i)) if i == unsext(cx.tcx, i128::MAX >> (128 - int_bits(cx.tcx, ity)), ity) => { + ExtremeType::Maximum + }, + (&ty::Uint(uty), Constant::Int(i)) if clip(cx.tcx, u128::MAX, uty) == i => ExtremeType::Maximum, + + _ => return None, + }; + Some(ExtremeExpr { which, expr }) +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 3a9236d8735..7b261121f51 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -164,6 +164,7 @@ macro_rules! extract_msrv_attr { mod utils; // begin lints modules, do not remove this comment, it’s used in `update_lints` +mod absurd_extreme_comparisons; mod approx_const; mod arithmetic; mod as_conversions; @@ -558,6 +559,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &utils::internal_lints::PRODUCE_ICE, #[cfg(feature = "internal-lints")] &utils::internal_lints::UNNECESSARY_SYMBOL_STR, + &absurd_extreme_comparisons::ABSURD_EXTREME_COMPARISONS, &approx_const::APPROX_CONSTANT, &arithmetic::FLOAT_ARITHMETIC, &arithmetic::INTEGER_ARITHMETIC, @@ -955,7 +957,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &transmute::WRONG_TRANSMUTE, &transmuting_null::TRANSMUTING_NULL, &try_err::TRY_ERR, - &types::ABSURD_EXTREME_COMPARISONS, &types::BORROWED_BOX, &types::BOX_VEC, &types::IMPLICIT_HASHER, @@ -1112,7 +1113,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box get_last_with_len::GetLastWithLen); store.register_late_pass(|| box drop_forget_ref::DropForgetRef); store.register_late_pass(|| box empty_enum::EmptyEnum); - store.register_late_pass(|| box types::AbsurdExtremeComparisons); + store.register_late_pass(|| box absurd_extreme_comparisons::AbsurdExtremeComparisons); store.register_late_pass(|| box types::InvalidUpcastComparisons); store.register_late_pass(|| box regex::Regex::default()); store.register_late_pass(|| box copies::CopyAndPaste); @@ -1442,6 +1443,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: ]); store.register_group(true, "clippy::all", Some("clippy"), vec![ + LintId::of(&absurd_extreme_comparisons::ABSURD_EXTREME_COMPARISONS), LintId::of(&approx_const::APPROX_CONSTANT), LintId::of(&assertions_on_constants::ASSERTIONS_ON_CONSTANTS), LintId::of(&assign_ops::ASSIGN_OP_PATTERN), @@ -1701,7 +1703,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&transmute::WRONG_TRANSMUTE), LintId::of(&transmuting_null::TRANSMUTING_NULL), LintId::of(&try_err::TRY_ERR), - LintId::of(&types::ABSURD_EXTREME_COMPARISONS), LintId::of(&types::BORROWED_BOX), LintId::of(&types::BOX_VEC), LintId::of(&types::REDUNDANT_ALLOCATION), @@ -1941,6 +1942,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: ]); store.register_group(true, "clippy::correctness", Some("clippy_correctness"), vec![ + LintId::of(&absurd_extreme_comparisons::ABSURD_EXTREME_COMPARISONS), LintId::of(&approx_const::APPROX_CONSTANT), LintId::of(&async_yields_async::ASYNC_YIELDS_ASYNC), LintId::of(&atomic_ordering::INVALID_ATOMIC_ORDERING), @@ -2002,7 +2004,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&transmute::UNSOUND_COLLECTION_TRANSMUTE), LintId::of(&transmute::WRONG_TRANSMUTE), LintId::of(&transmuting_null::TRANSMUTING_NULL), - LintId::of(&types::ABSURD_EXTREME_COMPARISONS), LintId::of(&undropped_manually_drops::UNDROPPED_MANUALLY_DROPS), LintId::of(&unicode::INVISIBLE_CHARACTERS), LintId::of(&unit_return_expecting_ord::UNIT_RETURN_EXPECTING_ORD), diff --git a/clippy_lints/src/types/mod.rs b/clippy_lints/src/types/mod.rs index c73c1c9d92d..0c50ed8348e 100644 --- a/clippy_lints/src/types/mod.rs +++ b/clippy_lints/src/types/mod.rs @@ -13,16 +13,16 @@ use std::cmp::Ordering; use std::collections::BTreeMap; -use clippy_utils::diagnostics::{multispan_sugg, span_lint, span_lint_and_help, span_lint_and_then}; +use clippy_utils::diagnostics::{multispan_sugg, span_lint, span_lint_and_then}; use clippy_utils::source::{snippet, snippet_opt}; -use clippy_utils::ty::{is_isize_or_usize, is_type_diagnostic_item}; +use clippy_utils::ty::is_type_diagnostic_item; use if_chain::if_chain; use rustc_errors::DiagnosticBuilder; use rustc_hir as hir; use rustc_hir::intravisit::{walk_body, walk_expr, walk_ty, FnKind, NestedVisitorMap, Visitor}; use rustc_hir::{ - BinOpKind, Body, Expr, ExprKind, FnDecl, FnRetTy, FnSig, GenericArg, GenericParamKind, HirId, ImplItem, - ImplItemKind, Item, ItemKind, Local, MutTy, QPath, TraitFn, TraitItem, TraitItemKind, TyKind, + Body, Expr, ExprKind, FnDecl, FnRetTy, FnSig, GenericArg, GenericParamKind, HirId, ImplItem, ImplItemKind, Item, + ItemKind, Local, MutTy, QPath, TraitFn, TraitItem, TraitItemKind, TyKind, }; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::hir::map::Map; @@ -37,7 +37,7 @@ use crate::consts::{constant, Constant}; use clippy_utils::paths; -use clippy_utils::{clip, comparisons, differing_macro_contexts, int_bits, match_path, sext, unsext}; +use clippy_utils::{comparisons, differing_macro_contexts, match_path, sext}; declare_clippy_lint! { /// **What it does:** Checks for use of `Box>` anywhere in the code. @@ -552,174 +552,6 @@ fn nested_visit_map(&mut self) -> NestedVisitorMap { } } -declare_clippy_lint! { - /// **What it does:** Checks for comparisons where one side of the relation is - /// either the minimum or maximum value for its type and warns if it involves a - /// case that is always true or always false. Only integer and boolean types are - /// checked. - /// - /// **Why is this bad?** An expression like `min <= x` may misleadingly imply - /// that it is possible for `x` to be less than the minimum. Expressions like - /// `max < x` are probably mistakes. - /// - /// **Known problems:** For `usize` the size of the current compile target will - /// be assumed (e.g., 64 bits on 64 bit systems). This means code that uses such - /// a comparison to detect target pointer width will trigger this lint. One can - /// use `mem::sizeof` and compare its value or conditional compilation - /// attributes - /// like `#[cfg(target_pointer_width = "64")] ..` instead. - /// - /// **Example:** - /// - /// ```rust - /// let vec: Vec = Vec::new(); - /// if vec.len() <= 0 {} - /// if 100 > i32::MAX {} - /// ``` - pub ABSURD_EXTREME_COMPARISONS, - correctness, - "a comparison with a maximum or minimum value that is always true or false" -} - -declare_lint_pass!(AbsurdExtremeComparisons => [ABSURD_EXTREME_COMPARISONS]); - -enum ExtremeType { - Minimum, - Maximum, -} - -struct ExtremeExpr<'a> { - which: ExtremeType, - expr: &'a Expr<'a>, -} - -enum AbsurdComparisonResult { - AlwaysFalse, - AlwaysTrue, - InequalityImpossible, -} - -fn is_cast_between_fixed_and_target<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool { - if let ExprKind::Cast(ref cast_exp, _) = expr.kind { - let precast_ty = cx.typeck_results().expr_ty(cast_exp); - let cast_ty = cx.typeck_results().expr_ty(expr); - - return is_isize_or_usize(precast_ty) != is_isize_or_usize(cast_ty); - } - - false -} - -fn detect_absurd_comparison<'tcx>( - cx: &LateContext<'tcx>, - op: BinOpKind, - lhs: &'tcx Expr<'_>, - rhs: &'tcx Expr<'_>, -) -> Option<(ExtremeExpr<'tcx>, AbsurdComparisonResult)> { - use crate::types::AbsurdComparisonResult::{AlwaysFalse, AlwaysTrue, InequalityImpossible}; - use crate::types::ExtremeType::{Maximum, Minimum}; - use clippy_utils::comparisons::{normalize_comparison, Rel}; - - // absurd comparison only makes sense on primitive types - // primitive types don't implement comparison operators with each other - if cx.typeck_results().expr_ty(lhs) != cx.typeck_results().expr_ty(rhs) { - return None; - } - - // comparisons between fix sized types and target sized types are considered unanalyzable - if is_cast_between_fixed_and_target(cx, lhs) || is_cast_between_fixed_and_target(cx, rhs) { - return None; - } - - let (rel, normalized_lhs, normalized_rhs) = normalize_comparison(op, lhs, rhs)?; - - let lx = detect_extreme_expr(cx, normalized_lhs); - let rx = detect_extreme_expr(cx, normalized_rhs); - - Some(match rel { - Rel::Lt => { - match (lx, rx) { - (Some(l @ ExtremeExpr { which: Maximum, .. }), _) => (l, AlwaysFalse), // max < x - (_, Some(r @ ExtremeExpr { which: Minimum, .. })) => (r, AlwaysFalse), // x < min - _ => return None, - } - }, - Rel::Le => { - match (lx, rx) { - (Some(l @ ExtremeExpr { which: Minimum, .. }), _) => (l, AlwaysTrue), // min <= x - (Some(l @ ExtremeExpr { which: Maximum, .. }), _) => (l, InequalityImpossible), // max <= x - (_, Some(r @ ExtremeExpr { which: Minimum, .. })) => (r, InequalityImpossible), // x <= min - (_, Some(r @ ExtremeExpr { which: Maximum, .. })) => (r, AlwaysTrue), // x <= max - _ => return None, - } - }, - Rel::Ne | Rel::Eq => return None, - }) -} - -fn detect_extreme_expr<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option> { - use crate::types::ExtremeType::{Maximum, Minimum}; - - let ty = cx.typeck_results().expr_ty(expr); - - let cv = constant(cx, cx.typeck_results(), expr)?.0; - - let which = match (ty.kind(), cv) { - (&ty::Bool, Constant::Bool(false)) | (&ty::Uint(_), Constant::Int(0)) => Minimum, - (&ty::Int(ity), Constant::Int(i)) if i == unsext(cx.tcx, i128::MIN >> (128 - int_bits(cx.tcx, ity)), ity) => { - Minimum - }, - - (&ty::Bool, Constant::Bool(true)) => Maximum, - (&ty::Int(ity), Constant::Int(i)) if i == unsext(cx.tcx, i128::MAX >> (128 - int_bits(cx.tcx, ity)), ity) => { - Maximum - }, - (&ty::Uint(uty), Constant::Int(i)) if clip(cx.tcx, u128::MAX, uty) == i => Maximum, - - _ => return None, - }; - Some(ExtremeExpr { which, expr }) -} - -impl<'tcx> LateLintPass<'tcx> for AbsurdExtremeComparisons { - fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - use crate::types::AbsurdComparisonResult::{AlwaysFalse, AlwaysTrue, InequalityImpossible}; - use crate::types::ExtremeType::{Maximum, Minimum}; - - if let ExprKind::Binary(ref cmp, ref lhs, ref rhs) = expr.kind { - if let Some((culprit, result)) = detect_absurd_comparison(cx, cmp.node, lhs, rhs) { - if !expr.span.from_expansion() { - let msg = "this comparison involving the minimum or maximum element for this \ - type contains a case that is always true or always false"; - - let conclusion = match result { - AlwaysFalse => "this comparison is always false".to_owned(), - AlwaysTrue => "this comparison is always true".to_owned(), - InequalityImpossible => format!( - "the case where the two sides are not equal never occurs, consider using `{} == {}` \ - instead", - snippet(cx, lhs.span, "lhs"), - snippet(cx, rhs.span, "rhs") - ), - }; - - let help = format!( - "because `{}` is the {} value for this type, {}", - snippet(cx, culprit.expr.span, "x"), - match culprit.which { - Minimum => "minimum", - Maximum => "maximum", - }, - conclusion - ); - - span_lint_and_help(cx, ABSURD_EXTREME_COMPARISONS, expr.span, msg, None, &help); - } - } - } - } -} - declare_clippy_lint! { /// **What it does:** Checks for comparisons where the relation is always either /// true or false, but where one side has been upcast so that the comparison is -- 2.44.0