From ab1281f54a94bf59d0fab2941c0c642158fae122 Mon Sep 17 00:00:00 2001 From: Morten Lohne Date: Mon, 29 May 2023 21:39:05 +0200 Subject: [PATCH 01/11] fix: Make ConstEvalLateContext::new() public, to match the 'constant_context()' function that it replaced --- clippy_utils/src/consts.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_utils/src/consts.rs b/clippy_utils/src/consts.rs index 061086c4fc20..63dfd2145b84 100644 --- a/clippy_utils/src/consts.rs +++ b/clippy_utils/src/consts.rs @@ -329,7 +329,7 @@ pub struct ConstEvalLateContext<'a, 'tcx> { } impl<'a, 'tcx> ConstEvalLateContext<'a, 'tcx> { - fn new(lcx: &'a LateContext<'tcx>, typeck_results: &'a ty::TypeckResults<'tcx>) -> Self { + pub fn new(lcx: &'a LateContext<'tcx>, typeck_results: &'a ty::TypeckResults<'tcx>) -> Self { Self { lcx, typeck_results, From 046d3df35eccafd73f84c47bb3b45a3cec33bcf4 Mon Sep 17 00:00:00 2001 From: Morten Lohne Date: Mon, 29 May 2023 21:40:28 +0200 Subject: [PATCH 02/11] New lints: `impossible_double_const_comparisons` and `ineffective_double_const_comparisons` --- CHANGELOG.md | 2 + clippy_lints/src/declared_lints.rs | 2 + .../src/operators/double_const_comparison.rs | 204 ++++++++++++++++++ clippy_lints/src/operators/mod.rs | 41 ++++ tests/ui/double_const_comparisons.rs | 52 +++++ tests/ui/double_const_comparisons.stderr | 180 ++++++++++++++++ tests/ui/range_contains.fixed | 2 + tests/ui/range_contains.rs | 2 + tests/ui/range_contains.stderr | 42 ++-- 9 files changed, 506 insertions(+), 21 deletions(-) create mode 100644 clippy_lints/src/operators/double_const_comparison.rs create mode 100644 tests/ui/double_const_comparisons.rs create mode 100644 tests/ui/double_const_comparisons.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ef225559795..3f9ec3a4df90 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4897,6 +4897,7 @@ Released 2018-09-13 [`implicit_return`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_return [`implicit_saturating_add`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_saturating_add [`implicit_saturating_sub`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_saturating_sub +[`impossible_double_const_comparisons`]: https://rust-lang.github.io/rust-clippy/master/index.html#impossible_double_const_comparisons [`imprecise_flops`]: https://rust-lang.github.io/rust-clippy/master/index.html#imprecise_flops [`inconsistent_digit_grouping`]: https://rust-lang.github.io/rust-clippy/master/index.html#inconsistent_digit_grouping [`inconsistent_struct_constructor`]: https://rust-lang.github.io/rust-clippy/master/index.html#inconsistent_struct_constructor @@ -4905,6 +4906,7 @@ Released 2018-09-13 [`index_refutable_slice`]: https://rust-lang.github.io/rust-clippy/master/index.html#index_refutable_slice [`indexing_slicing`]: https://rust-lang.github.io/rust-clippy/master/index.html#indexing_slicing [`ineffective_bit_mask`]: https://rust-lang.github.io/rust-clippy/master/index.html#ineffective_bit_mask +[`ineffective_double_const_comparisons`]: https://rust-lang.github.io/rust-clippy/master/index.html#ineffective_double_const_comparisons [`inefficient_to_string`]: https://rust-lang.github.io/rust-clippy/master/index.html#inefficient_to_string [`infallible_destructuring_match`]: https://rust-lang.github.io/rust-clippy/master/index.html#infallible_destructuring_match [`infinite_iter`]: https://rust-lang.github.io/rust-clippy/master/index.html#infinite_iter diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index ea66c760a2f8..897e6c7bd107 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -518,7 +518,9 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::operators::FLOAT_CMP_CONST_INFO, crate::operators::FLOAT_EQUALITY_WITHOUT_ABS_INFO, crate::operators::IDENTITY_OP_INFO, + crate::operators::IMPOSSIBLE_DOUBLE_CONST_COMPARISONS_INFO, crate::operators::INEFFECTIVE_BIT_MASK_INFO, + crate::operators::INEFFECTIVE_DOUBLE_CONST_COMPARISONS_INFO, crate::operators::INTEGER_DIVISION_INFO, crate::operators::MISREFACTORED_ASSIGN_OP_INFO, crate::operators::MODULO_ARITHMETIC_INFO, diff --git a/clippy_lints/src/operators/double_const_comparison.rs b/clippy_lints/src/operators/double_const_comparison.rs new file mode 100644 index 000000000000..d029a3d4ebe2 --- /dev/null +++ b/clippy_lints/src/operators/double_const_comparison.rs @@ -0,0 +1,204 @@ +#![allow(clippy::match_same_arms)] + +use std::cmp::Ordering; + +use clippy_utils::consts; +use clippy_utils::consts::{ConstEvalLateContext, Constant}; +use if_chain::if_chain; +use rustc_hir::{BinOpKind, Expr, ExprKind}; +use rustc_lint::LateContext; +use rustc_middle::ty::{layout::HasTyCtxt, Ty, TypeckResults}; +use rustc_span::source_map::{Span, Spanned}; + +use clippy_utils::diagnostics::span_lint_and_note; +use clippy_utils::source::snippet; +use clippy_utils::SpanlessEq; + +use super::IMPOSSIBLE_DOUBLE_CONST_COMPARISONS; +use super::INEFFECTIVE_DOUBLE_CONST_COMPARISONS; + +// Extract a comparison between a const and non-const +// Flip yoda conditionals, turnings expressions like `42 < x` into `x > 42` +fn comparison_to_const<'tcx>( + ctx: &mut ConstEvalLateContext<'_, 'tcx>, + typeck: &TypeckResults<'tcx>, + expr: &'tcx Expr<'tcx>, +) -> Option<(CmpOp, &'tcx Expr<'tcx>, &'tcx Expr<'tcx>, Constant, Ty<'tcx>)> { + if_chain! { + if let ExprKind::Binary(operator, left, right) = expr.kind; + if let Ok(cmp_op) = CmpOp::try_from(operator.node); + then { + match (ctx.expr(left), ctx.expr(right)) { + (Some(_), Some(_)) => None, + (_, Some(con)) => Some((cmp_op, left, right, con, typeck.expr_ty(right))), + (Some(con), _) => Some((cmp_op.reverse(), right, left, con, typeck.expr_ty(left))), + _ => None, + } + } else { + None + } + } +} + +pub(super) fn check<'tcx>( + cx: &LateContext<'tcx>, + and_op: Spanned, + left_cond: &'tcx Expr<'tcx>, + right_cond: &'tcx Expr<'tcx>, + span: Span, +) { + if_chain! { + // Ensure that the binary operator is && + if and_op.node == BinOpKind::And; + + let typeck_results = cx.typeck_results(); + let mut const_context = consts::ConstEvalLateContext::new(cx, typeck_results); + + // Check that both operands to '&&' compare a non-literal to a literal + if let Some((left_cmp_op, left_expr, left_const_expr, left_const, left_type)) = + comparison_to_const(&mut const_context, typeck_results, left_cond); + if let Some((right_cmp_op, right_expr, right_const_expr, right_const, right_type)) = + comparison_to_const(&mut const_context, typeck_results, right_cond); + + if left_type == right_type; + + // Check that the same expression is compared in both comparisons + if SpanlessEq::new(cx).eq_expr(left_expr, right_expr); + + if !left_expr.can_have_side_effects(); + + // Compare the two constant expressions + if let Some(ordering) = Constant::partial_cmp(cx.tcx(), left_type, &left_const, &right_const); + + // Rule out the `x >= 42 && x <= 42` corner case immediately + // Mostly to simplify the implementation, but it is also covered by `clippy::double_comparisons` + if !matches!( + (&left_cmp_op, &right_cmp_op, ordering), + (CmpOp::Le | CmpOp::Ge, CmpOp::Le | CmpOp::Ge, Ordering::Equal) + ); + + then { + if left_cmp_op.direction() == right_cmp_op.direction() { + let lhs_str = snippet(cx, left_cond.span, ""); + let rhs_str = snippet(cx, right_cond.span, ""); + // We already know that either side of `&&` has no effect, + // but emit a different error message depending on which side it is + if left_side_is_useless(left_cmp_op, ordering) { + span_lint_and_note( + cx, + INEFFECTIVE_DOUBLE_CONST_COMPARISONS, + span, + "left-hand side of `&&` operator has no effect", + Some(left_cond.span.until(right_cond.span)), + &format!("`if `{rhs_str}` evaluates to true, {lhs_str}` will always evaluate to true as well"), + ); + } else { + span_lint_and_note( + cx, + INEFFECTIVE_DOUBLE_CONST_COMPARISONS, + span, + "right-hand side of `&&` operator has no effect", + Some(and_op.span.to(right_cond.span)), + &format!("`if `{lhs_str}` evaluates to true, {rhs_str}` will always evaluate to true as well"), + ); + } + // We could autofix this error but choose not to, + // because code triggering this lint probably not behaving correctly in the first place + } + else if !comparison_is_possible(left_cmp_op.direction(), ordering) { + let expr_str = snippet(cx, left_expr.span, ""); + let lhs_str = snippet(cx, left_const_expr.span, ""); + let rhs_str = snippet(cx, right_const_expr.span, ""); + let note = match ordering { + Ordering::Less => format!("since `{lhs_str}` < `{rhs_str}`, the expression evaluates to false for any value of `{expr_str}`"), + Ordering::Equal => format!("`{expr_str}` cannot simultaneously be greater than and less than `{lhs_str}`"), + Ordering::Greater => format!("since `{lhs_str}` > `{rhs_str}`, the expression evaluates to false for any value of `{expr_str}`"), + }; + span_lint_and_note( + cx, + IMPOSSIBLE_DOUBLE_CONST_COMPARISONS, + span, + "boolean expression will never evaluate to 'true'", + None, + ¬e, + ); + }; + } + } +} + +fn left_side_is_useless(left_cmp_op: CmpOp, ordering: Ordering) -> bool { + // Special-case for equal constants with an inclusive comparison + if ordering == Ordering::Equal { + match left_cmp_op { + CmpOp::Lt | CmpOp::Gt => false, + CmpOp::Le | CmpOp::Ge => true, + } + } else { + match (left_cmp_op.direction(), ordering) { + (CmpOpDirection::Lesser, Ordering::Less) => false, + (CmpOpDirection::Lesser, Ordering::Equal) => false, + (CmpOpDirection::Lesser, Ordering::Greater) => true, + (CmpOpDirection::Greater, Ordering::Less) => true, + (CmpOpDirection::Greater, Ordering::Equal) => false, + (CmpOpDirection::Greater, Ordering::Greater) => false, + } + } +} + +fn comparison_is_possible(left_cmp_direction: CmpOpDirection, ordering: Ordering) -> bool { + match (left_cmp_direction, ordering) { + (CmpOpDirection::Lesser, Ordering::Less | Ordering::Equal) => false, + (CmpOpDirection::Lesser, Ordering::Greater) => true, + (CmpOpDirection::Greater, Ordering::Greater | Ordering::Equal) => false, + (CmpOpDirection::Greater, Ordering::Less) => true, + } +} + +#[derive(PartialEq, Eq, Clone, Copy)] +enum CmpOpDirection { + Lesser, + Greater, +} + +#[derive(Clone, Copy)] +enum CmpOp { + Lt, + Le, + Ge, + Gt, +} + +impl CmpOp { + fn reverse(self) -> Self { + match self { + CmpOp::Lt => CmpOp::Gt, + CmpOp::Le => CmpOp::Ge, + CmpOp::Ge => CmpOp::Le, + CmpOp::Gt => CmpOp::Lt, + } + } + + fn direction(self) -> CmpOpDirection { + match self { + CmpOp::Lt => CmpOpDirection::Lesser, + CmpOp::Le => CmpOpDirection::Lesser, + CmpOp::Ge => CmpOpDirection::Greater, + CmpOp::Gt => CmpOpDirection::Greater, + } + } +} + +impl TryFrom for CmpOp { + type Error = (); + + fn try_from(bin_op: BinOpKind) -> Result { + match bin_op { + BinOpKind::Lt => Ok(CmpOp::Lt), + BinOpKind::Le => Ok(CmpOp::Le), + BinOpKind::Ge => Ok(CmpOp::Ge), + BinOpKind::Gt => Ok(CmpOp::Gt), + _ => Err(()), + } + } +} diff --git a/clippy_lints/src/operators/mod.rs b/clippy_lints/src/operators/mod.rs index 2cf15adda01a..41efcef86c8a 100644 --- a/clippy_lints/src/operators/mod.rs +++ b/clippy_lints/src/operators/mod.rs @@ -3,6 +3,7 @@ mod assign_op_pattern; mod bit_mask; mod cmp_owned; mod double_comparison; +mod double_const_comparison; mod duration_subsec; mod eq_op; mod erasing_op; @@ -298,6 +299,43 @@ declare_clippy_lint! { "unnecessary double comparisons that can be simplified" } +declare_clippy_lint! { + /// ### What it does + /// Checks for double comparisons that can never succeed + /// + /// ### Why is this bad? + /// The whole expression can be replaced by `false`, + /// which is probably not the programmer's intention + /// + /// ### Example + /// ```rust + /// status_code <= 400 && status_code > 500; + /// ``` + #[clippy::version = "1.71.0"] + pub IMPOSSIBLE_DOUBLE_CONST_COMPARISONS, + correctness, + "default lint description" +} + +declare_clippy_lint! { + /// ### What it does + /// Checks for ineffective double comparisons against constants + /// + /// ### Why is this bad? + /// Only one of the comparisons has any effect on the result + /// The programmer probably intended to flip one of the comparison operators, + /// or compare a different value entirely + /// + /// ### Example + /// ```rust + /// status_code <= 400 && status_code < 500; + /// ``` + #[clippy::version = "1.71.0"] + pub INEFFECTIVE_DOUBLE_CONST_COMPARISONS, + correctness, + "default lint description" +} + declare_clippy_lint! { /// ### What it does /// Checks for calculation of subsecond microseconds or milliseconds @@ -742,6 +780,8 @@ impl_lint_pass!(Operators => [ INEFFECTIVE_BIT_MASK, VERBOSE_BIT_MASK, DOUBLE_COMPARISONS, + IMPOSSIBLE_DOUBLE_CONST_COMPARISONS, + INEFFECTIVE_DOUBLE_CONST_COMPARISONS, DURATION_SUBSEC, EQ_OP, OP_REF, @@ -786,6 +826,7 @@ impl<'tcx> LateLintPass<'tcx> for Operators { bit_mask::check(cx, e, op.node, lhs, rhs); verbose_bit_mask::check(cx, e, op.node, lhs, rhs, self.verbose_bit_mask_threshold); double_comparison::check(cx, op.node, lhs, rhs, e.span); + double_const_comparison::check(cx, op, lhs, rhs, e.span); duration_subsec::check(cx, e, op.node, lhs, rhs); float_equality_without_abs::check(cx, e, op.node, lhs, rhs); integer_division::check(cx, e, op.node, lhs, rhs); diff --git a/tests/ui/double_const_comparisons.rs b/tests/ui/double_const_comparisons.rs new file mode 100644 index 000000000000..0f57fbba1eb2 --- /dev/null +++ b/tests/ui/double_const_comparisons.rs @@ -0,0 +1,52 @@ +#![allow(unused)] +#![warn(clippy::impossible_double_const_comparisons)] +#![warn(clippy::ineffective_double_const_comparisons)] +#![allow(clippy::no_effect)] +#![allow(clippy::short_circuit_statement)] +#![allow(clippy::manual_range_contains)] + +const STATUS_BAD_REQUEST: u16 = 400; +const STATUS_SERVER_ERROR: u16 = 500; + +fn main() { + let status_code = 500; // Value doesn't matter for the lint + + status_code >= 400 && status_code < 500; // Correct + status_code <= 400 && status_code > 500; + status_code > 500 && status_code < 400; + status_code < 500 && status_code > 500; + + // More complex expressions + status_code < { 400 } && status_code > { 500 }; + status_code < STATUS_BAD_REQUEST && status_code > STATUS_SERVER_ERROR; + status_code <= u16::MIN + 1 && status_code > STATUS_SERVER_ERROR; + status_code < STATUS_SERVER_ERROR && status_code > STATUS_SERVER_ERROR; + + // Yoda conditions + 500 <= status_code && 600 > status_code; // Correct + 500 <= status_code && status_code <= 600; // Correct + 500 >= status_code && 600 < status_code; // Incorrect + 500 >= status_code && status_code > 600; // Incorrect + + // Expressions where one of the sides has no effect + status_code < 200 && status_code <= 299; + status_code > 200 && status_code >= 299; + + status_code >= 500 && status_code > 500; // Useless left + status_code > 500 && status_code >= 500; // Useless right + status_code <= 500 && status_code < 500; // Useless left + status_code < 500 && status_code <= 500; // Useless right + + // Other types + let name = "Steve"; + name < "Jennifer" && name > "Shannon"; + + let numbers = [1, 2]; + numbers < [3, 4] && numbers > [5, 6]; + + let letter = 'a'; + letter < 'b' && letter > 'c'; + + let area = 42.0; + area < std::f32::consts::E && area > std::f32::consts::PI; +} diff --git a/tests/ui/double_const_comparisons.stderr b/tests/ui/double_const_comparisons.stderr new file mode 100644 index 000000000000..76cfc82c0a4a --- /dev/null +++ b/tests/ui/double_const_comparisons.stderr @@ -0,0 +1,180 @@ +error: boolean expression will never evaluate to 'true' + --> $DIR/double_const_comparisons.rs:15:5 + | +LL | status_code <= 400 && status_code > 500; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: since `400` < `500`, the expression evaluates to false for any value of `status_code` + = note: `-D clippy::impossible-double-const-comparisons` implied by `-D warnings` + +error: boolean expression will never evaluate to 'true' + --> $DIR/double_const_comparisons.rs:16:5 + | +LL | status_code > 500 && status_code < 400; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: since `500` > `400`, the expression evaluates to false for any value of `status_code` + +error: boolean expression will never evaluate to 'true' + --> $DIR/double_const_comparisons.rs:17:5 + | +LL | status_code < 500 && status_code > 500; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `status_code` cannot simultaneously be greater than and less than `500` + +error: boolean expression will never evaluate to 'true' + --> $DIR/double_const_comparisons.rs:20:5 + | +LL | status_code < { 400 } && status_code > { 500 }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: since `{ 400 }` < `{ 500 }`, the expression evaluates to false for any value of `status_code` + +error: boolean expression will never evaluate to 'true' + --> $DIR/double_const_comparisons.rs:21:5 + | +LL | status_code < STATUS_BAD_REQUEST && status_code > STATUS_SERVER_ERROR; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: since `STATUS_BAD_REQUEST` < `STATUS_SERVER_ERROR`, the expression evaluates to false for any value of `status_code` + +error: boolean expression will never evaluate to 'true' + --> $DIR/double_const_comparisons.rs:22:5 + | +LL | status_code <= u16::MIN + 1 && status_code > STATUS_SERVER_ERROR; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: since `u16::MIN + 1` < `STATUS_SERVER_ERROR`, the expression evaluates to false for any value of `status_code` + +error: boolean expression will never evaluate to 'true' + --> $DIR/double_const_comparisons.rs:23:5 + | +LL | status_code < STATUS_SERVER_ERROR && status_code > STATUS_SERVER_ERROR; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `status_code` cannot simultaneously be greater than and less than `STATUS_SERVER_ERROR` + +error: boolean expression will never evaluate to 'true' + --> $DIR/double_const_comparisons.rs:28:5 + | +LL | 500 >= status_code && 600 < status_code; // Incorrect + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: since `500` < `600`, the expression evaluates to false for any value of `status_code` + +error: boolean expression will never evaluate to 'true' + --> $DIR/double_const_comparisons.rs:29:5 + | +LL | 500 >= status_code && status_code > 600; // Incorrect + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: since `500` < `600`, the expression evaluates to false for any value of `status_code` + +error: right-hand side of `&&` operator has no effect + --> $DIR/double_const_comparisons.rs:32:5 + | +LL | status_code < 200 && status_code <= 299; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +note: `if `status_code < 200` evaluates to true, status_code <= 299` will always evaluate to true as well + --> $DIR/double_const_comparisons.rs:32:23 + | +LL | status_code < 200 && status_code <= 299; + | ^^^^^^^^^^^^^^^^^^^^^ + = note: `-D clippy::ineffective-double-const-comparisons` implied by `-D warnings` + +error: left-hand side of `&&` operator has no effect + --> $DIR/double_const_comparisons.rs:33:5 + | +LL | status_code > 200 && status_code >= 299; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +note: `if `status_code >= 299` evaluates to true, status_code > 200` will always evaluate to true as well + --> $DIR/double_const_comparisons.rs:33:5 + | +LL | status_code > 200 && status_code >= 299; + | ^^^^^^^^^^^^^^^^^^^^^ + +error: left-hand side of `&&` operator has no effect + --> $DIR/double_const_comparisons.rs:35:5 + | +LL | status_code >= 500 && status_code > 500; // Useless left + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +note: `if `status_code > 500` evaluates to true, status_code >= 500` will always evaluate to true as well + --> $DIR/double_const_comparisons.rs:35:5 + | +LL | status_code >= 500 && status_code > 500; // Useless left + | ^^^^^^^^^^^^^^^^^^^^^^ + +error: right-hand side of `&&` operator has no effect + --> $DIR/double_const_comparisons.rs:36:5 + | +LL | status_code > 500 && status_code >= 500; // Useless right + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +note: `if `status_code > 500` evaluates to true, status_code >= 500` will always evaluate to true as well + --> $DIR/double_const_comparisons.rs:36:23 + | +LL | status_code > 500 && status_code >= 500; // Useless right + | ^^^^^^^^^^^^^^^^^^^^^ + +error: left-hand side of `&&` operator has no effect + --> $DIR/double_const_comparisons.rs:37:5 + | +LL | status_code <= 500 && status_code < 500; // Useless left + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +note: `if `status_code < 500` evaluates to true, status_code <= 500` will always evaluate to true as well + --> $DIR/double_const_comparisons.rs:37:5 + | +LL | status_code <= 500 && status_code < 500; // Useless left + | ^^^^^^^^^^^^^^^^^^^^^^ + +error: right-hand side of `&&` operator has no effect + --> $DIR/double_const_comparisons.rs:38:5 + | +LL | status_code < 500 && status_code <= 500; // Useless right + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +note: `if `status_code < 500` evaluates to true, status_code <= 500` will always evaluate to true as well + --> $DIR/double_const_comparisons.rs:38:23 + | +LL | status_code < 500 && status_code <= 500; // Useless right + | ^^^^^^^^^^^^^^^^^^^^^ + +error: boolean expression will never evaluate to 'true' + --> $DIR/double_const_comparisons.rs:42:5 + | +LL | name < "Jennifer" && name > "Shannon"; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: since `"Jennifer"` < `"Shannon"`, the expression evaluates to false for any value of `name` + +error: boolean expression will never evaluate to 'true' + --> $DIR/double_const_comparisons.rs:45:5 + | +LL | numbers < [3, 4] && numbers > [5, 6]; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: since `[3, 4]` < `[5, 6]`, the expression evaluates to false for any value of `numbers` + +error: boolean expression will never evaluate to 'true' + --> $DIR/double_const_comparisons.rs:48:5 + | +LL | letter < 'b' && letter > 'c'; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: since `'b'` < `'c'`, the expression evaluates to false for any value of `letter` + +error: boolean expression will never evaluate to 'true' + --> $DIR/double_const_comparisons.rs:51:5 + | +LL | area < std::f32::consts::E && area > std::f32::consts::PI; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: since `std::f32::consts::E` < `std::f32::consts::PI`, the expression evaluates to false for any value of `area` + +error: aborting due to 19 previous errors + diff --git a/tests/ui/range_contains.fixed b/tests/ui/range_contains.fixed index 0a92ee7c8dd3..429d86082911 100644 --- a/tests/ui/range_contains.fixed +++ b/tests/ui/range_contains.fixed @@ -5,6 +5,8 @@ #![allow(clippy::no_effect)] #![allow(clippy::short_circuit_statement)] #![allow(clippy::unnecessary_operation)] +#![allow(clippy::impossible_double_const_comparisons)] +#![allow(clippy::ineffective_double_const_comparisons)] fn main() { let x = 9_i32; diff --git a/tests/ui/range_contains.rs b/tests/ui/range_contains.rs index 7a83be609571..e47e29481471 100644 --- a/tests/ui/range_contains.rs +++ b/tests/ui/range_contains.rs @@ -5,6 +5,8 @@ #![allow(clippy::no_effect)] #![allow(clippy::short_circuit_statement)] #![allow(clippy::unnecessary_operation)] +#![allow(clippy::impossible_double_const_comparisons)] +#![allow(clippy::ineffective_double_const_comparisons)] fn main() { let x = 9_i32; diff --git a/tests/ui/range_contains.stderr b/tests/ui/range_contains.stderr index ea34023a4664..1265db695bfb 100644 --- a/tests/ui/range_contains.stderr +++ b/tests/ui/range_contains.stderr @@ -1,5 +1,5 @@ error: manual `Range::contains` implementation - --> $DIR/range_contains.rs:13:5 + --> $DIR/range_contains.rs:15:5 | LL | x >= 8 && x < 12; | ^^^^^^^^^^^^^^^^ help: use: `(8..12).contains(&x)` @@ -7,121 +7,121 @@ LL | x >= 8 && x < 12; = note: `-D clippy::manual-range-contains` implied by `-D warnings` error: manual `Range::contains` implementation - --> $DIR/range_contains.rs:14:5 + --> $DIR/range_contains.rs:16:5 | LL | x < 42 && x >= 21; | ^^^^^^^^^^^^^^^^^ help: use: `(21..42).contains(&x)` error: manual `Range::contains` implementation - --> $DIR/range_contains.rs:15:5 + --> $DIR/range_contains.rs:17:5 | LL | 100 > x && 1 <= x; | ^^^^^^^^^^^^^^^^^ help: use: `(1..100).contains(&x)` error: manual `RangeInclusive::contains` implementation - --> $DIR/range_contains.rs:18:5 + --> $DIR/range_contains.rs:20:5 | LL | x >= 9 && x <= 99; | ^^^^^^^^^^^^^^^^^ help: use: `(9..=99).contains(&x)` error: manual `RangeInclusive::contains` implementation - --> $DIR/range_contains.rs:19:5 + --> $DIR/range_contains.rs:21:5 | LL | x <= 33 && x >= 1; | ^^^^^^^^^^^^^^^^^ help: use: `(1..=33).contains(&x)` error: manual `RangeInclusive::contains` implementation - --> $DIR/range_contains.rs:20:5 + --> $DIR/range_contains.rs:22:5 | LL | 999 >= x && 1 <= x; | ^^^^^^^^^^^^^^^^^^ help: use: `(1..=999).contains(&x)` error: manual `!Range::contains` implementation - --> $DIR/range_contains.rs:23:5 + --> $DIR/range_contains.rs:25:5 | LL | x < 8 || x >= 12; | ^^^^^^^^^^^^^^^^ help: use: `!(8..12).contains(&x)` error: manual `!Range::contains` implementation - --> $DIR/range_contains.rs:24:5 + --> $DIR/range_contains.rs:26:5 | LL | x >= 42 || x < 21; | ^^^^^^^^^^^^^^^^^ help: use: `!(21..42).contains(&x)` error: manual `!Range::contains` implementation - --> $DIR/range_contains.rs:25:5 + --> $DIR/range_contains.rs:27:5 | LL | 100 <= x || 1 > x; | ^^^^^^^^^^^^^^^^^ help: use: `!(1..100).contains(&x)` error: manual `!RangeInclusive::contains` implementation - --> $DIR/range_contains.rs:28:5 + --> $DIR/range_contains.rs:30:5 | LL | x < 9 || x > 99; | ^^^^^^^^^^^^^^^ help: use: `!(9..=99).contains(&x)` error: manual `!RangeInclusive::contains` implementation - --> $DIR/range_contains.rs:29:5 + --> $DIR/range_contains.rs:31:5 | LL | x > 33 || x < 1; | ^^^^^^^^^^^^^^^ help: use: `!(1..=33).contains(&x)` error: manual `!RangeInclusive::contains` implementation - --> $DIR/range_contains.rs:30:5 + --> $DIR/range_contains.rs:32:5 | LL | 999 < x || 1 > x; | ^^^^^^^^^^^^^^^^ help: use: `!(1..=999).contains(&x)` error: manual `Range::contains` implementation - --> $DIR/range_contains.rs:45:5 + --> $DIR/range_contains.rs:47:5 | LL | y >= 0. && y < 1.; | ^^^^^^^^^^^^^^^^^ help: use: `(0. ..1.).contains(&y)` error: manual `!RangeInclusive::contains` implementation - --> $DIR/range_contains.rs:46:5 + --> $DIR/range_contains.rs:48:5 | LL | y < 0. || y > 1.; | ^^^^^^^^^^^^^^^^ help: use: `!(0. ..=1.).contains(&y)` error: manual `RangeInclusive::contains` implementation - --> $DIR/range_contains.rs:49:5 + --> $DIR/range_contains.rs:51:5 | LL | x >= -10 && x <= 10; | ^^^^^^^^^^^^^^^^^^^ help: use: `(-10..=10).contains(&x)` error: manual `RangeInclusive::contains` implementation - --> $DIR/range_contains.rs:51:5 + --> $DIR/range_contains.rs:53:5 | LL | y >= -3. && y <= 3.; | ^^^^^^^^^^^^^^^^^^^ help: use: `(-3. ..=3.).contains(&y)` error: manual `RangeInclusive::contains` implementation - --> $DIR/range_contains.rs:56:30 + --> $DIR/range_contains.rs:58:30 | LL | (x >= 0) && (x <= 10) && (z >= 0) && (z <= 10); | ^^^^^^^^^^^^^^^^^^^^^ help: use: `(0..=10).contains(&z)` error: manual `RangeInclusive::contains` implementation - --> $DIR/range_contains.rs:56:5 + --> $DIR/range_contains.rs:58:5 | LL | (x >= 0) && (x <= 10) && (z >= 0) && (z <= 10); | ^^^^^^^^^^^^^^^^^^^^^ help: use: `(0..=10).contains(&x)` error: manual `!Range::contains` implementation - --> $DIR/range_contains.rs:57:29 + --> $DIR/range_contains.rs:59:29 | LL | (x < 0) || (x >= 10) || (z < 0) || (z >= 10); | ^^^^^^^^^^^^^^^^^^^^ help: use: `!(0..10).contains(&z)` error: manual `!Range::contains` implementation - --> $DIR/range_contains.rs:57:5 + --> $DIR/range_contains.rs:59:5 | LL | (x < 0) || (x >= 10) || (z < 0) || (z >= 10); | ^^^^^^^^^^^^^^^^^^^^ help: use: `!(0..10).contains(&x)` error: manual `Range::contains` implementation - --> $DIR/range_contains.rs:76:5 + --> $DIR/range_contains.rs:78:5 | LL | x >= 8 && x < 35; | ^^^^^^^^^^^^^^^^ help: use: `(8..35).contains(&x)` From e16a2ac0c65c233ab52f61cdb38ad76ea53b11f4 Mon Sep 17 00:00:00 2001 From: Morten Lohne Date: Mon, 29 May 2023 23:49:54 +0200 Subject: [PATCH 03/11] Add descriptions for 'impossible_double_const_comparisons' and 'ineffective_double_const_comparisons' --- clippy_lints/src/operators/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/operators/mod.rs b/clippy_lints/src/operators/mod.rs index 41efcef86c8a..25e1fce1980b 100644 --- a/clippy_lints/src/operators/mod.rs +++ b/clippy_lints/src/operators/mod.rs @@ -314,7 +314,7 @@ declare_clippy_lint! { #[clippy::version = "1.71.0"] pub IMPOSSIBLE_DOUBLE_CONST_COMPARISONS, correctness, - "default lint description" + "double comparisons that will never evaluate to `true`" } declare_clippy_lint! { @@ -333,7 +333,7 @@ declare_clippy_lint! { #[clippy::version = "1.71.0"] pub INEFFECTIVE_DOUBLE_CONST_COMPARISONS, correctness, - "default lint description" + "double comparisons where one of them can be removed" } declare_clippy_lint! { From 08e1333fa6467290b9951fc28f0616db3b13c51d Mon Sep 17 00:00:00 2001 From: Morten Lohne Date: Thu, 1 Jun 2023 23:50:33 +0200 Subject: [PATCH 04/11] Add missing variable decl to doc comment --- clippy_lints/src/operators/mod.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/operators/mod.rs b/clippy_lints/src/operators/mod.rs index 25e1fce1980b..ef76812d9dd1 100644 --- a/clippy_lints/src/operators/mod.rs +++ b/clippy_lints/src/operators/mod.rs @@ -309,7 +309,8 @@ declare_clippy_lint! { /// /// ### Example /// ```rust - /// status_code <= 400 && status_code > 500; + /// # let status_code = 200; + /// if status_code <= 400 && status_code > 500 {} /// ``` #[clippy::version = "1.71.0"] pub IMPOSSIBLE_DOUBLE_CONST_COMPARISONS, @@ -328,7 +329,8 @@ declare_clippy_lint! { /// /// ### Example /// ```rust - /// status_code <= 400 && status_code < 500; + /// # let status_code = 200; + /// if status_code <= 400 && status_code < 500 {} /// ``` #[clippy::version = "1.71.0"] pub INEFFECTIVE_DOUBLE_CONST_COMPARISONS, From 8f40d09e0f08273e8edbb22493d845029a92d58f Mon Sep 17 00:00:00 2001 From: Morten Lohne Date: Sat, 3 Jun 2023 13:36:12 +0200 Subject: [PATCH 05/11] Add tests for const comparisons that compare two different types --- tests/ui/double_const_comparisons.rs | 41 ++++++++++ tests/ui/double_const_comparisons.stderr | 100 +++++++++++++++++------ 2 files changed, 115 insertions(+), 26 deletions(-) diff --git a/tests/ui/double_const_comparisons.rs b/tests/ui/double_const_comparisons.rs index 0f57fbba1eb2..de01d8e0c368 100644 --- a/tests/ui/double_const_comparisons.rs +++ b/tests/ui/double_const_comparisons.rs @@ -8,8 +8,37 @@ const STATUS_BAD_REQUEST: u16 = 400; const STATUS_SERVER_ERROR: u16 = 500; +struct Status { + code: u16, +} + +impl PartialEq for Status { + fn eq(&self, other: &u16) -> bool { + self.code == *other + } +} + +impl PartialOrd for Status { + fn partial_cmp(&self, other: &u16) -> Option { + self.code.partial_cmp(other) + } +} + +impl PartialEq for u16 { + fn eq(&self, other: &Status) -> bool { + *self == other.code + } +} + +impl PartialOrd for u16 { + fn partial_cmp(&self, other: &Status) -> Option { + self.partial_cmp(&other.code) + } +} + fn main() { let status_code = 500; // Value doesn't matter for the lint + let status = Status { code: status_code }; status_code >= 400 && status_code < 500; // Correct status_code <= 400 && status_code > 500; @@ -22,12 +51,24 @@ fn main() { status_code <= u16::MIN + 1 && status_code > STATUS_SERVER_ERROR; status_code < STATUS_SERVER_ERROR && status_code > STATUS_SERVER_ERROR; + // Comparing two different types, via the `impl PartialOrd for Status` + status < { 400 } && status > { 500 }; + status < STATUS_BAD_REQUEST && status > STATUS_SERVER_ERROR; + status <= u16::MIN + 1 && status > STATUS_SERVER_ERROR; + status < STATUS_SERVER_ERROR && status > STATUS_SERVER_ERROR; + // Yoda conditions 500 <= status_code && 600 > status_code; // Correct 500 <= status_code && status_code <= 600; // Correct 500 >= status_code && 600 < status_code; // Incorrect 500 >= status_code && status_code > 600; // Incorrect + // Yoda conditions, comparing two different types + 500 <= status && 600 > status; // Correct + 500 <= status && status <= 600; // Correct + 500 >= status && 600 < status; // Incorrect + 500 >= status && status > 600; // Incorrect + // Expressions where one of the sides has no effect status_code < 200 && status_code <= 299; status_code > 200 && status_code >= 299; diff --git a/tests/ui/double_const_comparisons.stderr b/tests/ui/double_const_comparisons.stderr index 76cfc82c0a4a..3d8ef3c6d641 100644 --- a/tests/ui/double_const_comparisons.stderr +++ b/tests/ui/double_const_comparisons.stderr @@ -1,5 +1,5 @@ error: boolean expression will never evaluate to 'true' - --> $DIR/double_const_comparisons.rs:15:5 + --> $DIR/double_const_comparisons.rs:44:5 | LL | status_code <= 400 && status_code > 500; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -8,7 +8,7 @@ LL | status_code <= 400 && status_code > 500; = note: `-D clippy::impossible-double-const-comparisons` implied by `-D warnings` error: boolean expression will never evaluate to 'true' - --> $DIR/double_const_comparisons.rs:16:5 + --> $DIR/double_const_comparisons.rs:45:5 | LL | status_code > 500 && status_code < 400; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -16,7 +16,7 @@ LL | status_code > 500 && status_code < 400; = note: since `500` > `400`, the expression evaluates to false for any value of `status_code` error: boolean expression will never evaluate to 'true' - --> $DIR/double_const_comparisons.rs:17:5 + --> $DIR/double_const_comparisons.rs:46:5 | LL | status_code < 500 && status_code > 500; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -24,7 +24,7 @@ LL | status_code < 500 && status_code > 500; = note: `status_code` cannot simultaneously be greater than and less than `500` error: boolean expression will never evaluate to 'true' - --> $DIR/double_const_comparisons.rs:20:5 + --> $DIR/double_const_comparisons.rs:49:5 | LL | status_code < { 400 } && status_code > { 500 }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -32,7 +32,7 @@ LL | status_code < { 400 } && status_code > { 500 }; = note: since `{ 400 }` < `{ 500 }`, the expression evaluates to false for any value of `status_code` error: boolean expression will never evaluate to 'true' - --> $DIR/double_const_comparisons.rs:21:5 + --> $DIR/double_const_comparisons.rs:50:5 | LL | status_code < STATUS_BAD_REQUEST && status_code > STATUS_SERVER_ERROR; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -40,7 +40,7 @@ LL | status_code < STATUS_BAD_REQUEST && status_code > STATUS_SERVER_ERROR; = note: since `STATUS_BAD_REQUEST` < `STATUS_SERVER_ERROR`, the expression evaluates to false for any value of `status_code` error: boolean expression will never evaluate to 'true' - --> $DIR/double_const_comparisons.rs:22:5 + --> $DIR/double_const_comparisons.rs:51:5 | LL | status_code <= u16::MIN + 1 && status_code > STATUS_SERVER_ERROR; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -48,7 +48,7 @@ LL | status_code <= u16::MIN + 1 && status_code > STATUS_SERVER_ERROR; = note: since `u16::MIN + 1` < `STATUS_SERVER_ERROR`, the expression evaluates to false for any value of `status_code` error: boolean expression will never evaluate to 'true' - --> $DIR/double_const_comparisons.rs:23:5 + --> $DIR/double_const_comparisons.rs:52:5 | LL | status_code < STATUS_SERVER_ERROR && status_code > STATUS_SERVER_ERROR; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -56,7 +56,39 @@ LL | status_code < STATUS_SERVER_ERROR && status_code > STATUS_SERVER_ERROR; = note: `status_code` cannot simultaneously be greater than and less than `STATUS_SERVER_ERROR` error: boolean expression will never evaluate to 'true' - --> $DIR/double_const_comparisons.rs:28:5 + --> $DIR/double_const_comparisons.rs:55:5 + | +LL | status < { 400 } && status > { 500 }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: since `{ 400 }` < `{ 500 }`, the expression evaluates to false for any value of `status` + +error: boolean expression will never evaluate to 'true' + --> $DIR/double_const_comparisons.rs:56:5 + | +LL | status < STATUS_BAD_REQUEST && status > STATUS_SERVER_ERROR; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: since `STATUS_BAD_REQUEST` < `STATUS_SERVER_ERROR`, the expression evaluates to false for any value of `status` + +error: boolean expression will never evaluate to 'true' + --> $DIR/double_const_comparisons.rs:57:5 + | +LL | status <= u16::MIN + 1 && status > STATUS_SERVER_ERROR; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: since `u16::MIN + 1` < `STATUS_SERVER_ERROR`, the expression evaluates to false for any value of `status` + +error: boolean expression will never evaluate to 'true' + --> $DIR/double_const_comparisons.rs:58:5 + | +LL | status < STATUS_SERVER_ERROR && status > STATUS_SERVER_ERROR; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `status` cannot simultaneously be greater than and less than `STATUS_SERVER_ERROR` + +error: boolean expression will never evaluate to 'true' + --> $DIR/double_const_comparisons.rs:63:5 | LL | 500 >= status_code && 600 < status_code; // Incorrect | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -64,88 +96,104 @@ LL | 500 >= status_code && 600 < status_code; // Incorrect = note: since `500` < `600`, the expression evaluates to false for any value of `status_code` error: boolean expression will never evaluate to 'true' - --> $DIR/double_const_comparisons.rs:29:5 + --> $DIR/double_const_comparisons.rs:64:5 | LL | 500 >= status_code && status_code > 600; // Incorrect | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: since `500` < `600`, the expression evaluates to false for any value of `status_code` +error: boolean expression will never evaluate to 'true' + --> $DIR/double_const_comparisons.rs:69:5 + | +LL | 500 >= status && 600 < status; // Incorrect + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: since `500` < `600`, the expression evaluates to false for any value of `status` + +error: boolean expression will never evaluate to 'true' + --> $DIR/double_const_comparisons.rs:70:5 + | +LL | 500 >= status && status > 600; // Incorrect + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: since `500` < `600`, the expression evaluates to false for any value of `status` + error: right-hand side of `&&` operator has no effect - --> $DIR/double_const_comparisons.rs:32:5 + --> $DIR/double_const_comparisons.rs:73:5 | LL | status_code < 200 && status_code <= 299; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | note: `if `status_code < 200` evaluates to true, status_code <= 299` will always evaluate to true as well - --> $DIR/double_const_comparisons.rs:32:23 + --> $DIR/double_const_comparisons.rs:73:23 | LL | status_code < 200 && status_code <= 299; | ^^^^^^^^^^^^^^^^^^^^^ = note: `-D clippy::ineffective-double-const-comparisons` implied by `-D warnings` error: left-hand side of `&&` operator has no effect - --> $DIR/double_const_comparisons.rs:33:5 + --> $DIR/double_const_comparisons.rs:74:5 | LL | status_code > 200 && status_code >= 299; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | note: `if `status_code >= 299` evaluates to true, status_code > 200` will always evaluate to true as well - --> $DIR/double_const_comparisons.rs:33:5 + --> $DIR/double_const_comparisons.rs:74:5 | LL | status_code > 200 && status_code >= 299; | ^^^^^^^^^^^^^^^^^^^^^ error: left-hand side of `&&` operator has no effect - --> $DIR/double_const_comparisons.rs:35:5 + --> $DIR/double_const_comparisons.rs:76:5 | LL | status_code >= 500 && status_code > 500; // Useless left | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | note: `if `status_code > 500` evaluates to true, status_code >= 500` will always evaluate to true as well - --> $DIR/double_const_comparisons.rs:35:5 + --> $DIR/double_const_comparisons.rs:76:5 | LL | status_code >= 500 && status_code > 500; // Useless left | ^^^^^^^^^^^^^^^^^^^^^^ error: right-hand side of `&&` operator has no effect - --> $DIR/double_const_comparisons.rs:36:5 + --> $DIR/double_const_comparisons.rs:77:5 | LL | status_code > 500 && status_code >= 500; // Useless right | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | note: `if `status_code > 500` evaluates to true, status_code >= 500` will always evaluate to true as well - --> $DIR/double_const_comparisons.rs:36:23 + --> $DIR/double_const_comparisons.rs:77:23 | LL | status_code > 500 && status_code >= 500; // Useless right | ^^^^^^^^^^^^^^^^^^^^^ error: left-hand side of `&&` operator has no effect - --> $DIR/double_const_comparisons.rs:37:5 + --> $DIR/double_const_comparisons.rs:78:5 | LL | status_code <= 500 && status_code < 500; // Useless left | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | note: `if `status_code < 500` evaluates to true, status_code <= 500` will always evaluate to true as well - --> $DIR/double_const_comparisons.rs:37:5 + --> $DIR/double_const_comparisons.rs:78:5 | LL | status_code <= 500 && status_code < 500; // Useless left | ^^^^^^^^^^^^^^^^^^^^^^ error: right-hand side of `&&` operator has no effect - --> $DIR/double_const_comparisons.rs:38:5 + --> $DIR/double_const_comparisons.rs:79:5 | LL | status_code < 500 && status_code <= 500; // Useless right | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | note: `if `status_code < 500` evaluates to true, status_code <= 500` will always evaluate to true as well - --> $DIR/double_const_comparisons.rs:38:23 + --> $DIR/double_const_comparisons.rs:79:23 | LL | status_code < 500 && status_code <= 500; // Useless right | ^^^^^^^^^^^^^^^^^^^^^ error: boolean expression will never evaluate to 'true' - --> $DIR/double_const_comparisons.rs:42:5 + --> $DIR/double_const_comparisons.rs:83:5 | LL | name < "Jennifer" && name > "Shannon"; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -153,7 +201,7 @@ LL | name < "Jennifer" && name > "Shannon"; = note: since `"Jennifer"` < `"Shannon"`, the expression evaluates to false for any value of `name` error: boolean expression will never evaluate to 'true' - --> $DIR/double_const_comparisons.rs:45:5 + --> $DIR/double_const_comparisons.rs:86:5 | LL | numbers < [3, 4] && numbers > [5, 6]; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -161,7 +209,7 @@ LL | numbers < [3, 4] && numbers > [5, 6]; = note: since `[3, 4]` < `[5, 6]`, the expression evaluates to false for any value of `numbers` error: boolean expression will never evaluate to 'true' - --> $DIR/double_const_comparisons.rs:48:5 + --> $DIR/double_const_comparisons.rs:89:5 | LL | letter < 'b' && letter > 'c'; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -169,12 +217,12 @@ LL | letter < 'b' && letter > 'c'; = note: since `'b'` < `'c'`, the expression evaluates to false for any value of `letter` error: boolean expression will never evaluate to 'true' - --> $DIR/double_const_comparisons.rs:51:5 + --> $DIR/double_const_comparisons.rs:92:5 | LL | area < std::f32::consts::E && area > std::f32::consts::PI; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: since `std::f32::consts::E` < `std::f32::consts::PI`, the expression evaluates to false for any value of `area` -error: aborting due to 19 previous errors +error: aborting due to 25 previous errors From b5ef66f442229ada09a83454b47a9671c2d098c9 Mon Sep 17 00:00:00 2001 From: Morten Lohne Date: Sat, 10 Jun 2023 14:29:24 +0200 Subject: [PATCH 06/11] Optimize by doing a cheap check for double binary expression first --- .../src/operators/double_const_comparison.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/operators/double_const_comparison.rs b/clippy_lints/src/operators/double_const_comparison.rs index d029a3d4ebe2..80db455d2e64 100644 --- a/clippy_lints/src/operators/double_const_comparison.rs +++ b/clippy_lints/src/operators/double_const_comparison.rs @@ -51,14 +51,19 @@ pub(super) fn check<'tcx>( // Ensure that the binary operator is && if and_op.node == BinOpKind::And; - let typeck_results = cx.typeck_results(); - let mut const_context = consts::ConstEvalLateContext::new(cx, typeck_results); + // Check that both operands to '&&' are themselves a binary operation + // The `comparison_to_const` step also checks this, so this step is just an optimization + if let ExprKind::Binary(_, _, _) = left_cond.kind; + if let ExprKind::Binary(_, _, _) = right_cond.kind; + + let typeck = cx.typeck_results(); + let mut const_context = consts::ConstEvalLateContext::new(cx, typeck); // Check that both operands to '&&' compare a non-literal to a literal if let Some((left_cmp_op, left_expr, left_const_expr, left_const, left_type)) = - comparison_to_const(&mut const_context, typeck_results, left_cond); + comparison_to_const(&mut const_context, typeck, left_cond); if let Some((right_cmp_op, right_expr, right_const_expr, right_const, right_type)) = - comparison_to_const(&mut const_context, typeck_results, right_cond); + comparison_to_const(&mut const_context, typeck, right_cond); if left_type == right_type; From 1d61fc1b0a02bb8f4a597279f03e897de0e521d1 Mon Sep 17 00:00:00 2001 From: Morten Lohne Date: Sat, 10 Jun 2023 15:58:46 +0200 Subject: [PATCH 07/11] Rename 'impossible_double_const_comparisons' -> 'impossible_comparisons' and 'ineffective_double_const_comparisons' -> 'redundant_comparisons', after discussion on Zulip --- CHANGELOG.md | 4 +- clippy_lints/src/declared_lints.rs | 4 +- ...nst_comparison.rs => const_comparisons.rs} | 10 +-- clippy_lints/src/operators/mod.rs | 12 ++-- ...st_comparisons.rs => const_comparisons.rs} | 4 +- ...risons.stderr => const_comparisons.stderr} | 66 +++++++++---------- tests/ui/range_contains.fixed | 4 +- tests/ui/range_contains.rs | 4 +- 8 files changed, 54 insertions(+), 54 deletions(-) rename clippy_lints/src/operators/{double_const_comparison.rs => const_comparisons.rs} (96%) rename tests/ui/{double_const_comparisons.rs => const_comparisons.rs} (96%) rename tests/ui/{double_const_comparisons.stderr => const_comparisons.stderr} (83%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f9ec3a4df90..71671273c57b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4897,7 +4897,7 @@ Released 2018-09-13 [`implicit_return`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_return [`implicit_saturating_add`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_saturating_add [`implicit_saturating_sub`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_saturating_sub -[`impossible_double_const_comparisons`]: https://rust-lang.github.io/rust-clippy/master/index.html#impossible_double_const_comparisons +[`impossible_comparisons`]: https://rust-lang.github.io/rust-clippy/master/index.html#impossible_comparisons [`imprecise_flops`]: https://rust-lang.github.io/rust-clippy/master/index.html#imprecise_flops [`inconsistent_digit_grouping`]: https://rust-lang.github.io/rust-clippy/master/index.html#inconsistent_digit_grouping [`inconsistent_struct_constructor`]: https://rust-lang.github.io/rust-clippy/master/index.html#inconsistent_struct_constructor @@ -4906,7 +4906,6 @@ Released 2018-09-13 [`index_refutable_slice`]: https://rust-lang.github.io/rust-clippy/master/index.html#index_refutable_slice [`indexing_slicing`]: https://rust-lang.github.io/rust-clippy/master/index.html#indexing_slicing [`ineffective_bit_mask`]: https://rust-lang.github.io/rust-clippy/master/index.html#ineffective_bit_mask -[`ineffective_double_const_comparisons`]: https://rust-lang.github.io/rust-clippy/master/index.html#ineffective_double_const_comparisons [`inefficient_to_string`]: https://rust-lang.github.io/rust-clippy/master/index.html#inefficient_to_string [`infallible_destructuring_match`]: https://rust-lang.github.io/rust-clippy/master/index.html#infallible_destructuring_match [`infinite_iter`]: https://rust-lang.github.io/rust-clippy/master/index.html#infinite_iter @@ -5193,6 +5192,7 @@ Released 2018-09-13 [`redundant_closure`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure [`redundant_closure_call`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure_call [`redundant_closure_for_method_calls`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure_for_method_calls +[`redundant_comparisons`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_comparisons [`redundant_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_else [`redundant_feature_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_feature_names [`redundant_field_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_field_names diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 897e6c7bd107..db114abfc869 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -518,9 +518,8 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::operators::FLOAT_CMP_CONST_INFO, crate::operators::FLOAT_EQUALITY_WITHOUT_ABS_INFO, crate::operators::IDENTITY_OP_INFO, - crate::operators::IMPOSSIBLE_DOUBLE_CONST_COMPARISONS_INFO, + crate::operators::IMPOSSIBLE_COMPARISONS_INFO, crate::operators::INEFFECTIVE_BIT_MASK_INFO, - crate::operators::INEFFECTIVE_DOUBLE_CONST_COMPARISONS_INFO, crate::operators::INTEGER_DIVISION_INFO, crate::operators::MISREFACTORED_ASSIGN_OP_INFO, crate::operators::MODULO_ARITHMETIC_INFO, @@ -528,6 +527,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::operators::NEEDLESS_BITWISE_BOOL_INFO, crate::operators::OP_REF_INFO, crate::operators::PTR_EQ_INFO, + crate::operators::REDUNDANT_COMPARISONS_INFO, crate::operators::SELF_ASSIGNMENT_INFO, crate::operators::VERBOSE_BIT_MASK_INFO, crate::option_env_unwrap::OPTION_ENV_UNWRAP_INFO, diff --git a/clippy_lints/src/operators/double_const_comparison.rs b/clippy_lints/src/operators/const_comparisons.rs similarity index 96% rename from clippy_lints/src/operators/double_const_comparison.rs rename to clippy_lints/src/operators/const_comparisons.rs index 80db455d2e64..357981c10ad0 100644 --- a/clippy_lints/src/operators/double_const_comparison.rs +++ b/clippy_lints/src/operators/const_comparisons.rs @@ -14,8 +14,8 @@ use clippy_utils::diagnostics::span_lint_and_note; use clippy_utils::source::snippet; use clippy_utils::SpanlessEq; -use super::IMPOSSIBLE_DOUBLE_CONST_COMPARISONS; -use super::INEFFECTIVE_DOUBLE_CONST_COMPARISONS; +use super::IMPOSSIBLE_COMPARISONS; +use super::REDUNDANT_COMPARISONS; // Extract a comparison between a const and non-const // Flip yoda conditionals, turnings expressions like `42 < x` into `x > 42` @@ -91,7 +91,7 @@ pub(super) fn check<'tcx>( if left_side_is_useless(left_cmp_op, ordering) { span_lint_and_note( cx, - INEFFECTIVE_DOUBLE_CONST_COMPARISONS, + REDUNDANT_COMPARISONS, span, "left-hand side of `&&` operator has no effect", Some(left_cond.span.until(right_cond.span)), @@ -100,7 +100,7 @@ pub(super) fn check<'tcx>( } else { span_lint_and_note( cx, - INEFFECTIVE_DOUBLE_CONST_COMPARISONS, + REDUNDANT_COMPARISONS, span, "right-hand side of `&&` operator has no effect", Some(and_op.span.to(right_cond.span)), @@ -121,7 +121,7 @@ pub(super) fn check<'tcx>( }; span_lint_and_note( cx, - IMPOSSIBLE_DOUBLE_CONST_COMPARISONS, + IMPOSSIBLE_COMPARISONS, span, "boolean expression will never evaluate to 'true'", None, diff --git a/clippy_lints/src/operators/mod.rs b/clippy_lints/src/operators/mod.rs index ef76812d9dd1..fa8e4c00a974 100644 --- a/clippy_lints/src/operators/mod.rs +++ b/clippy_lints/src/operators/mod.rs @@ -2,8 +2,8 @@ mod absurd_extreme_comparisons; mod assign_op_pattern; mod bit_mask; mod cmp_owned; +mod const_comparisons; mod double_comparison; -mod double_const_comparison; mod duration_subsec; mod eq_op; mod erasing_op; @@ -313,7 +313,7 @@ declare_clippy_lint! { /// if status_code <= 400 && status_code > 500 {} /// ``` #[clippy::version = "1.71.0"] - pub IMPOSSIBLE_DOUBLE_CONST_COMPARISONS, + pub IMPOSSIBLE_COMPARISONS, correctness, "double comparisons that will never evaluate to `true`" } @@ -333,7 +333,7 @@ declare_clippy_lint! { /// if status_code <= 400 && status_code < 500 {} /// ``` #[clippy::version = "1.71.0"] - pub INEFFECTIVE_DOUBLE_CONST_COMPARISONS, + pub REDUNDANT_COMPARISONS, correctness, "double comparisons where one of them can be removed" } @@ -782,8 +782,8 @@ impl_lint_pass!(Operators => [ INEFFECTIVE_BIT_MASK, VERBOSE_BIT_MASK, DOUBLE_COMPARISONS, - IMPOSSIBLE_DOUBLE_CONST_COMPARISONS, - INEFFECTIVE_DOUBLE_CONST_COMPARISONS, + IMPOSSIBLE_COMPARISONS, + REDUNDANT_COMPARISONS, DURATION_SUBSEC, EQ_OP, OP_REF, @@ -828,7 +828,7 @@ impl<'tcx> LateLintPass<'tcx> for Operators { bit_mask::check(cx, e, op.node, lhs, rhs); verbose_bit_mask::check(cx, e, op.node, lhs, rhs, self.verbose_bit_mask_threshold); double_comparison::check(cx, op.node, lhs, rhs, e.span); - double_const_comparison::check(cx, op, lhs, rhs, e.span); + const_comparisons::check(cx, op, lhs, rhs, e.span); duration_subsec::check(cx, e, op.node, lhs, rhs); float_equality_without_abs::check(cx, e, op.node, lhs, rhs); integer_division::check(cx, e, op.node, lhs, rhs); diff --git a/tests/ui/double_const_comparisons.rs b/tests/ui/const_comparisons.rs similarity index 96% rename from tests/ui/double_const_comparisons.rs rename to tests/ui/const_comparisons.rs index de01d8e0c368..8e265c9141c1 100644 --- a/tests/ui/double_const_comparisons.rs +++ b/tests/ui/const_comparisons.rs @@ -1,6 +1,6 @@ #![allow(unused)] -#![warn(clippy::impossible_double_const_comparisons)] -#![warn(clippy::ineffective_double_const_comparisons)] +#![warn(clippy::impossible_comparisons)] +#![warn(clippy::redundant_comparisons)] #![allow(clippy::no_effect)] #![allow(clippy::short_circuit_statement)] #![allow(clippy::manual_range_contains)] diff --git a/tests/ui/double_const_comparisons.stderr b/tests/ui/const_comparisons.stderr similarity index 83% rename from tests/ui/double_const_comparisons.stderr rename to tests/ui/const_comparisons.stderr index 3d8ef3c6d641..90e6db647621 100644 --- a/tests/ui/double_const_comparisons.stderr +++ b/tests/ui/const_comparisons.stderr @@ -1,14 +1,14 @@ error: boolean expression will never evaluate to 'true' - --> $DIR/double_const_comparisons.rs:44:5 + --> $DIR/const_comparisons.rs:44:5 | LL | status_code <= 400 && status_code > 500; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: since `400` < `500`, the expression evaluates to false for any value of `status_code` - = note: `-D clippy::impossible-double-const-comparisons` implied by `-D warnings` + = note: `-D clippy::impossible-comparisons` implied by `-D warnings` error: boolean expression will never evaluate to 'true' - --> $DIR/double_const_comparisons.rs:45:5 + --> $DIR/const_comparisons.rs:45:5 | LL | status_code > 500 && status_code < 400; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -16,7 +16,7 @@ LL | status_code > 500 && status_code < 400; = note: since `500` > `400`, the expression evaluates to false for any value of `status_code` error: boolean expression will never evaluate to 'true' - --> $DIR/double_const_comparisons.rs:46:5 + --> $DIR/const_comparisons.rs:46:5 | LL | status_code < 500 && status_code > 500; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -24,7 +24,7 @@ LL | status_code < 500 && status_code > 500; = note: `status_code` cannot simultaneously be greater than and less than `500` error: boolean expression will never evaluate to 'true' - --> $DIR/double_const_comparisons.rs:49:5 + --> $DIR/const_comparisons.rs:49:5 | LL | status_code < { 400 } && status_code > { 500 }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -32,7 +32,7 @@ LL | status_code < { 400 } && status_code > { 500 }; = note: since `{ 400 }` < `{ 500 }`, the expression evaluates to false for any value of `status_code` error: boolean expression will never evaluate to 'true' - --> $DIR/double_const_comparisons.rs:50:5 + --> $DIR/const_comparisons.rs:50:5 | LL | status_code < STATUS_BAD_REQUEST && status_code > STATUS_SERVER_ERROR; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -40,7 +40,7 @@ LL | status_code < STATUS_BAD_REQUEST && status_code > STATUS_SERVER_ERROR; = note: since `STATUS_BAD_REQUEST` < `STATUS_SERVER_ERROR`, the expression evaluates to false for any value of `status_code` error: boolean expression will never evaluate to 'true' - --> $DIR/double_const_comparisons.rs:51:5 + --> $DIR/const_comparisons.rs:51:5 | LL | status_code <= u16::MIN + 1 && status_code > STATUS_SERVER_ERROR; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -48,7 +48,7 @@ LL | status_code <= u16::MIN + 1 && status_code > STATUS_SERVER_ERROR; = note: since `u16::MIN + 1` < `STATUS_SERVER_ERROR`, the expression evaluates to false for any value of `status_code` error: boolean expression will never evaluate to 'true' - --> $DIR/double_const_comparisons.rs:52:5 + --> $DIR/const_comparisons.rs:52:5 | LL | status_code < STATUS_SERVER_ERROR && status_code > STATUS_SERVER_ERROR; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -56,7 +56,7 @@ LL | status_code < STATUS_SERVER_ERROR && status_code > STATUS_SERVER_ERROR; = note: `status_code` cannot simultaneously be greater than and less than `STATUS_SERVER_ERROR` error: boolean expression will never evaluate to 'true' - --> $DIR/double_const_comparisons.rs:55:5 + --> $DIR/const_comparisons.rs:55:5 | LL | status < { 400 } && status > { 500 }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -64,7 +64,7 @@ LL | status < { 400 } && status > { 500 }; = note: since `{ 400 }` < `{ 500 }`, the expression evaluates to false for any value of `status` error: boolean expression will never evaluate to 'true' - --> $DIR/double_const_comparisons.rs:56:5 + --> $DIR/const_comparisons.rs:56:5 | LL | status < STATUS_BAD_REQUEST && status > STATUS_SERVER_ERROR; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -72,7 +72,7 @@ LL | status < STATUS_BAD_REQUEST && status > STATUS_SERVER_ERROR; = note: since `STATUS_BAD_REQUEST` < `STATUS_SERVER_ERROR`, the expression evaluates to false for any value of `status` error: boolean expression will never evaluate to 'true' - --> $DIR/double_const_comparisons.rs:57:5 + --> $DIR/const_comparisons.rs:57:5 | LL | status <= u16::MIN + 1 && status > STATUS_SERVER_ERROR; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -80,7 +80,7 @@ LL | status <= u16::MIN + 1 && status > STATUS_SERVER_ERROR; = note: since `u16::MIN + 1` < `STATUS_SERVER_ERROR`, the expression evaluates to false for any value of `status` error: boolean expression will never evaluate to 'true' - --> $DIR/double_const_comparisons.rs:58:5 + --> $DIR/const_comparisons.rs:58:5 | LL | status < STATUS_SERVER_ERROR && status > STATUS_SERVER_ERROR; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -88,7 +88,7 @@ LL | status < STATUS_SERVER_ERROR && status > STATUS_SERVER_ERROR; = note: `status` cannot simultaneously be greater than and less than `STATUS_SERVER_ERROR` error: boolean expression will never evaluate to 'true' - --> $DIR/double_const_comparisons.rs:63:5 + --> $DIR/const_comparisons.rs:63:5 | LL | 500 >= status_code && 600 < status_code; // Incorrect | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -96,7 +96,7 @@ LL | 500 >= status_code && 600 < status_code; // Incorrect = note: since `500` < `600`, the expression evaluates to false for any value of `status_code` error: boolean expression will never evaluate to 'true' - --> $DIR/double_const_comparisons.rs:64:5 + --> $DIR/const_comparisons.rs:64:5 | LL | 500 >= status_code && status_code > 600; // Incorrect | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -104,7 +104,7 @@ LL | 500 >= status_code && status_code > 600; // Incorrect = note: since `500` < `600`, the expression evaluates to false for any value of `status_code` error: boolean expression will never evaluate to 'true' - --> $DIR/double_const_comparisons.rs:69:5 + --> $DIR/const_comparisons.rs:69:5 | LL | 500 >= status && 600 < status; // Incorrect | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -112,7 +112,7 @@ LL | 500 >= status && 600 < status; // Incorrect = note: since `500` < `600`, the expression evaluates to false for any value of `status` error: boolean expression will never evaluate to 'true' - --> $DIR/double_const_comparisons.rs:70:5 + --> $DIR/const_comparisons.rs:70:5 | LL | 500 >= status && status > 600; // Incorrect | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -120,80 +120,80 @@ LL | 500 >= status && status > 600; // Incorrect = note: since `500` < `600`, the expression evaluates to false for any value of `status` error: right-hand side of `&&` operator has no effect - --> $DIR/double_const_comparisons.rs:73:5 + --> $DIR/const_comparisons.rs:73:5 | LL | status_code < 200 && status_code <= 299; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | note: `if `status_code < 200` evaluates to true, status_code <= 299` will always evaluate to true as well - --> $DIR/double_const_comparisons.rs:73:23 + --> $DIR/const_comparisons.rs:73:23 | LL | status_code < 200 && status_code <= 299; | ^^^^^^^^^^^^^^^^^^^^^ - = note: `-D clippy::ineffective-double-const-comparisons` implied by `-D warnings` + = note: `-D clippy::redundant-comparisons` implied by `-D warnings` error: left-hand side of `&&` operator has no effect - --> $DIR/double_const_comparisons.rs:74:5 + --> $DIR/const_comparisons.rs:74:5 | LL | status_code > 200 && status_code >= 299; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | note: `if `status_code >= 299` evaluates to true, status_code > 200` will always evaluate to true as well - --> $DIR/double_const_comparisons.rs:74:5 + --> $DIR/const_comparisons.rs:74:5 | LL | status_code > 200 && status_code >= 299; | ^^^^^^^^^^^^^^^^^^^^^ error: left-hand side of `&&` operator has no effect - --> $DIR/double_const_comparisons.rs:76:5 + --> $DIR/const_comparisons.rs:76:5 | LL | status_code >= 500 && status_code > 500; // Useless left | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | note: `if `status_code > 500` evaluates to true, status_code >= 500` will always evaluate to true as well - --> $DIR/double_const_comparisons.rs:76:5 + --> $DIR/const_comparisons.rs:76:5 | LL | status_code >= 500 && status_code > 500; // Useless left | ^^^^^^^^^^^^^^^^^^^^^^ error: right-hand side of `&&` operator has no effect - --> $DIR/double_const_comparisons.rs:77:5 + --> $DIR/const_comparisons.rs:77:5 | LL | status_code > 500 && status_code >= 500; // Useless right | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | note: `if `status_code > 500` evaluates to true, status_code >= 500` will always evaluate to true as well - --> $DIR/double_const_comparisons.rs:77:23 + --> $DIR/const_comparisons.rs:77:23 | LL | status_code > 500 && status_code >= 500; // Useless right | ^^^^^^^^^^^^^^^^^^^^^ error: left-hand side of `&&` operator has no effect - --> $DIR/double_const_comparisons.rs:78:5 + --> $DIR/const_comparisons.rs:78:5 | LL | status_code <= 500 && status_code < 500; // Useless left | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | note: `if `status_code < 500` evaluates to true, status_code <= 500` will always evaluate to true as well - --> $DIR/double_const_comparisons.rs:78:5 + --> $DIR/const_comparisons.rs:78:5 | LL | status_code <= 500 && status_code < 500; // Useless left | ^^^^^^^^^^^^^^^^^^^^^^ error: right-hand side of `&&` operator has no effect - --> $DIR/double_const_comparisons.rs:79:5 + --> $DIR/const_comparisons.rs:79:5 | LL | status_code < 500 && status_code <= 500; // Useless right | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | note: `if `status_code < 500` evaluates to true, status_code <= 500` will always evaluate to true as well - --> $DIR/double_const_comparisons.rs:79:23 + --> $DIR/const_comparisons.rs:79:23 | LL | status_code < 500 && status_code <= 500; // Useless right | ^^^^^^^^^^^^^^^^^^^^^ error: boolean expression will never evaluate to 'true' - --> $DIR/double_const_comparisons.rs:83:5 + --> $DIR/const_comparisons.rs:83:5 | LL | name < "Jennifer" && name > "Shannon"; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -201,7 +201,7 @@ LL | name < "Jennifer" && name > "Shannon"; = note: since `"Jennifer"` < `"Shannon"`, the expression evaluates to false for any value of `name` error: boolean expression will never evaluate to 'true' - --> $DIR/double_const_comparisons.rs:86:5 + --> $DIR/const_comparisons.rs:86:5 | LL | numbers < [3, 4] && numbers > [5, 6]; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -209,7 +209,7 @@ LL | numbers < [3, 4] && numbers > [5, 6]; = note: since `[3, 4]` < `[5, 6]`, the expression evaluates to false for any value of `numbers` error: boolean expression will never evaluate to 'true' - --> $DIR/double_const_comparisons.rs:89:5 + --> $DIR/const_comparisons.rs:89:5 | LL | letter < 'b' && letter > 'c'; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -217,7 +217,7 @@ LL | letter < 'b' && letter > 'c'; = note: since `'b'` < `'c'`, the expression evaluates to false for any value of `letter` error: boolean expression will never evaluate to 'true' - --> $DIR/double_const_comparisons.rs:92:5 + --> $DIR/const_comparisons.rs:92:5 | LL | area < std::f32::consts::E && area > std::f32::consts::PI; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/tests/ui/range_contains.fixed b/tests/ui/range_contains.fixed index 429d86082911..47c5248118ea 100644 --- a/tests/ui/range_contains.fixed +++ b/tests/ui/range_contains.fixed @@ -5,8 +5,8 @@ #![allow(clippy::no_effect)] #![allow(clippy::short_circuit_statement)] #![allow(clippy::unnecessary_operation)] -#![allow(clippy::impossible_double_const_comparisons)] -#![allow(clippy::ineffective_double_const_comparisons)] +#![allow(clippy::impossible_comparisons)] +#![allow(clippy::redundant_comparisons)] fn main() { let x = 9_i32; diff --git a/tests/ui/range_contains.rs b/tests/ui/range_contains.rs index e47e29481471..a35315a649d8 100644 --- a/tests/ui/range_contains.rs +++ b/tests/ui/range_contains.rs @@ -5,8 +5,8 @@ #![allow(clippy::no_effect)] #![allow(clippy::short_circuit_statement)] #![allow(clippy::unnecessary_operation)] -#![allow(clippy::impossible_double_const_comparisons)] -#![allow(clippy::ineffective_double_const_comparisons)] +#![allow(clippy::impossible_comparisons)] +#![allow(clippy::redundant_comparisons)] fn main() { let x = 9_i32; From 964644692324c41fae286208417086c45b5c878e Mon Sep 17 00:00:00 2001 From: Morten Lohne Date: Sun, 6 Aug 2023 13:29:50 +0200 Subject: [PATCH 08/11] Add lifetime parameter to 'Constant', after rebasing on upstream --- clippy_lints/src/operators/const_comparisons.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/operators/const_comparisons.rs b/clippy_lints/src/operators/const_comparisons.rs index 357981c10ad0..16ac964e93a8 100644 --- a/clippy_lints/src/operators/const_comparisons.rs +++ b/clippy_lints/src/operators/const_comparisons.rs @@ -7,15 +7,15 @@ use clippy_utils::consts::{ConstEvalLateContext, Constant}; use if_chain::if_chain; use rustc_hir::{BinOpKind, Expr, ExprKind}; use rustc_lint::LateContext; -use rustc_middle::ty::{layout::HasTyCtxt, Ty, TypeckResults}; +use rustc_middle::ty::layout::HasTyCtxt; +use rustc_middle::ty::{Ty, TypeckResults}; use rustc_span::source_map::{Span, Spanned}; use clippy_utils::diagnostics::span_lint_and_note; use clippy_utils::source::snippet; use clippy_utils::SpanlessEq; -use super::IMPOSSIBLE_COMPARISONS; -use super::REDUNDANT_COMPARISONS; +use super::{IMPOSSIBLE_COMPARISONS, REDUNDANT_COMPARISONS}; // Extract a comparison between a const and non-const // Flip yoda conditionals, turnings expressions like `42 < x` into `x > 42` @@ -23,7 +23,7 @@ fn comparison_to_const<'tcx>( ctx: &mut ConstEvalLateContext<'_, 'tcx>, typeck: &TypeckResults<'tcx>, expr: &'tcx Expr<'tcx>, -) -> Option<(CmpOp, &'tcx Expr<'tcx>, &'tcx Expr<'tcx>, Constant, Ty<'tcx>)> { +) -> Option<(CmpOp, &'tcx Expr<'tcx>, &'tcx Expr<'tcx>, Constant<'tcx>, Ty<'tcx>)> { if_chain! { if let ExprKind::Binary(operator, left, right) = expr.kind; if let Ok(cmp_op) = CmpOp::try_from(operator.node); From 0e064d5d0480bf471c26a4566a9e3a09751a6262 Mon Sep 17 00:00:00 2001 From: Morten Lohne Date: Sun, 6 Aug 2023 13:48:28 +0200 Subject: [PATCH 09/11] Replace ConstEvalLateContext::new() with two calls to constant() to simplify the code, after PR suggestion --- clippy_lints/src/operators/const_comparisons.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/clippy_lints/src/operators/const_comparisons.rs b/clippy_lints/src/operators/const_comparisons.rs index 16ac964e93a8..77043b0a3eb0 100644 --- a/clippy_lints/src/operators/const_comparisons.rs +++ b/clippy_lints/src/operators/const_comparisons.rs @@ -2,8 +2,7 @@ use std::cmp::Ordering; -use clippy_utils::consts; -use clippy_utils::consts::{ConstEvalLateContext, Constant}; +use clippy_utils::consts::{constant, Constant}; use if_chain::if_chain; use rustc_hir::{BinOpKind, Expr, ExprKind}; use rustc_lint::LateContext; @@ -20,7 +19,7 @@ use super::{IMPOSSIBLE_COMPARISONS, REDUNDANT_COMPARISONS}; // Extract a comparison between a const and non-const // Flip yoda conditionals, turnings expressions like `42 < x` into `x > 42` fn comparison_to_const<'tcx>( - ctx: &mut ConstEvalLateContext<'_, 'tcx>, + cx: &LateContext<'tcx>, typeck: &TypeckResults<'tcx>, expr: &'tcx Expr<'tcx>, ) -> Option<(CmpOp, &'tcx Expr<'tcx>, &'tcx Expr<'tcx>, Constant<'tcx>, Ty<'tcx>)> { @@ -28,7 +27,7 @@ fn comparison_to_const<'tcx>( if let ExprKind::Binary(operator, left, right) = expr.kind; if let Ok(cmp_op) = CmpOp::try_from(operator.node); then { - match (ctx.expr(left), ctx.expr(right)) { + match (constant(cx, typeck, left), constant(cx, typeck, right)) { (Some(_), Some(_)) => None, (_, Some(con)) => Some((cmp_op, left, right, con, typeck.expr_ty(right))), (Some(con), _) => Some((cmp_op.reverse(), right, left, con, typeck.expr_ty(left))), @@ -57,13 +56,12 @@ pub(super) fn check<'tcx>( if let ExprKind::Binary(_, _, _) = right_cond.kind; let typeck = cx.typeck_results(); - let mut const_context = consts::ConstEvalLateContext::new(cx, typeck); // Check that both operands to '&&' compare a non-literal to a literal if let Some((left_cmp_op, left_expr, left_const_expr, left_const, left_type)) = - comparison_to_const(&mut const_context, typeck, left_cond); + comparison_to_const(cx, typeck, left_cond); if let Some((right_cmp_op, right_expr, right_const_expr, right_const, right_type)) = - comparison_to_const(&mut const_context, typeck, right_cond); + comparison_to_const(cx, typeck, right_cond); if left_type == right_type; From 3157b96a5bc352336889399c68d0248eb67073c0 Mon Sep 17 00:00:00 2001 From: Morten Lohne Date: Sun, 6 Aug 2023 13:49:17 +0200 Subject: [PATCH 10/11] Provide fallback code snippets, if the snippet is not available --- clippy_lints/src/operators/const_comparisons.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/operators/const_comparisons.rs b/clippy_lints/src/operators/const_comparisons.rs index 77043b0a3eb0..abe8df195434 100644 --- a/clippy_lints/src/operators/const_comparisons.rs +++ b/clippy_lints/src/operators/const_comparisons.rs @@ -82,8 +82,8 @@ pub(super) fn check<'tcx>( then { if left_cmp_op.direction() == right_cmp_op.direction() { - let lhs_str = snippet(cx, left_cond.span, ""); - let rhs_str = snippet(cx, right_cond.span, ""); + let lhs_str = snippet(cx, left_cond.span, ""); + let rhs_str = snippet(cx, right_cond.span, ""); // We already know that either side of `&&` has no effect, // but emit a different error message depending on which side it is if left_side_is_useless(left_cmp_op, ordering) { @@ -109,9 +109,9 @@ pub(super) fn check<'tcx>( // because code triggering this lint probably not behaving correctly in the first place } else if !comparison_is_possible(left_cmp_op.direction(), ordering) { - let expr_str = snippet(cx, left_expr.span, ""); - let lhs_str = snippet(cx, left_const_expr.span, ""); - let rhs_str = snippet(cx, right_const_expr.span, ""); + let expr_str = snippet(cx, left_expr.span, ".."); + let lhs_str = snippet(cx, left_const_expr.span, ""); + let rhs_str = snippet(cx, right_const_expr.span, ""); let note = match ordering { Ordering::Less => format!("since `{lhs_str}` < `{rhs_str}`, the expression evaluates to false for any value of `{expr_str}`"), Ordering::Equal => format!("`{expr_str}` cannot simultaneously be greater than and less than `{lhs_str}`"), From d62804624441c2f7bf3de45cd2c9b5c411b2d6a6 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Sun, 6 Aug 2023 17:19:43 +0000 Subject: [PATCH 11/11] Update clippy_lints/src/operators/mod.rs Co-authored-by: Catherine Flores --- clippy_lints/src/operators/mod.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/operators/mod.rs b/clippy_lints/src/operators/mod.rs index fa8e4c00a974..4635e1164cd3 100644 --- a/clippy_lints/src/operators/mod.rs +++ b/clippy_lints/src/operators/mod.rs @@ -320,12 +320,12 @@ declare_clippy_lint! { declare_clippy_lint! { /// ### What it does - /// Checks for ineffective double comparisons against constants + /// Checks for ineffective double comparisons against constants. /// /// ### Why is this bad? - /// Only one of the comparisons has any effect on the result - /// The programmer probably intended to flip one of the comparison operators, - /// or compare a different value entirely + /// Only one of the comparisons has any effect on the result, the programmer + /// probably intended to flip one of the comparison operators, or compare a + /// different value entirely. /// /// ### Example /// ```rust