From 00042a86aa9d20f51ae980dd7c983e43c899b3db Mon Sep 17 00:00:00 2001 From: Samuel Tardieu Date: Tue, 17 Dec 2024 22:43:19 +0100 Subject: [PATCH 1/3] Add `is_float_literal` utility --- clippy_utils/src/lib.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 0d9502c50db6..ff32bba6a5ed 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -1750,6 +1750,17 @@ pub fn is_integer_literal(expr: &Expr<'_>, value: u128) -> bool { false } +/// Checks whether the given expression is a constant literal of the given value. +pub fn is_float_literal(expr: &Expr<'_>, value: f64) -> bool { + if let ExprKind::Lit(spanned) = expr.kind + && let LitKind::Float(v, _) = spanned.node + { + v.as_str().parse() == Ok(value) + } else { + false + } +} + /// Returns `true` if the given `Expr` has been coerced before. /// /// Examples of coercions can be found in the Nomicon at From d8e4023c14c6fd820ffb1232a6da746a04c62c34 Mon Sep 17 00:00:00 2001 From: Samuel Tardieu Date: Tue, 17 Dec 2024 22:43:19 +0100 Subject: [PATCH 2/3] New lint: `manual_midpoint` --- CHANGELOG.md | 1 + book/src/lint_configuration.md | 1 + clippy_config/src/conf.rs | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/operators/manual_midpoint.rs | 58 +++++++++++++++++ clippy_lints/src/operators/mod.rs | 32 ++++++++++ clippy_utils/src/msrvs.rs | 1 + tests/ui/manual_midpoint.fixed | 63 +++++++++++++++++++ tests/ui/manual_midpoint.rs | 63 +++++++++++++++++++ tests/ui/manual_midpoint.stderr | 59 +++++++++++++++++ tests/ui/option_if_let_else.fixed | 1 + tests/ui/option_if_let_else.rs | 1 + tests/ui/option_if_let_else.stderr | 50 +++++++-------- tests/ui/suspicious_operation_groupings.fixed | 2 +- tests/ui/suspicious_operation_groupings.rs | 2 +- 15 files changed, 309 insertions(+), 27 deletions(-) create mode 100644 clippy_lints/src/operators/manual_midpoint.rs create mode 100644 tests/ui/manual_midpoint.fixed create mode 100644 tests/ui/manual_midpoint.rs create mode 100644 tests/ui/manual_midpoint.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index a1ea3ce8f79d..322afb8bf742 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5762,6 +5762,7 @@ Released 2018-09-13 [`manual_main_separator_str`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_main_separator_str [`manual_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_map [`manual_memcpy`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_memcpy +[`manual_midpoint`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_midpoint [`manual_next_back`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_next_back [`manual_non_exhaustive`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_non_exhaustive [`manual_ok_err`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_ok_err diff --git a/book/src/lint_configuration.md b/book/src/lint_configuration.md index 0e264cdcd4a3..5a485384f1df 100644 --- a/book/src/lint_configuration.md +++ b/book/src/lint_configuration.md @@ -768,6 +768,7 @@ The minimum rust version that the project supports. Defaults to the `rust-versio * [`manual_hash_one`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_hash_one) * [`manual_is_ascii_check`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_is_ascii_check) * [`manual_let_else`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_let_else) +* [`manual_midpoint`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_midpoint) * [`manual_non_exhaustive`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_non_exhaustive) * [`manual_option_as_slice`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_option_as_slice) * [`manual_pattern_char_comparison`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_pattern_char_comparison) diff --git a/clippy_config/src/conf.rs b/clippy_config/src/conf.rs index cac4408fff0e..77837f8e7a3a 100644 --- a/clippy_config/src/conf.rs +++ b/clippy_config/src/conf.rs @@ -623,6 +623,7 @@ define_Conf! { manual_hash_one, manual_is_ascii_check, manual_let_else, + manual_midpoint, manual_non_exhaustive, manual_option_as_slice, manual_pattern_char_comparison, diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 1f00fb82c6db..b571b226dc9e 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -604,6 +604,7 @@ pub static LINTS: &[&crate::LintInfo] = &[ crate::operators::IMPOSSIBLE_COMPARISONS_INFO, crate::operators::INEFFECTIVE_BIT_MASK_INFO, crate::operators::INTEGER_DIVISION_INFO, + crate::operators::MANUAL_MIDPOINT_INFO, crate::operators::MISREFACTORED_ASSIGN_OP_INFO, crate::operators::MODULO_ARITHMETIC_INFO, crate::operators::MODULO_ONE_INFO, diff --git a/clippy_lints/src/operators/manual_midpoint.rs b/clippy_lints/src/operators/manual_midpoint.rs new file mode 100644 index 000000000000..d4f1282a7542 --- /dev/null +++ b/clippy_lints/src/operators/manual_midpoint.rs @@ -0,0 +1,58 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::msrvs::{self, Msrv}; +use clippy_utils::sugg::Sugg; +use clippy_utils::{is_float_literal, is_integer_literal}; +use rustc_ast::BinOpKind; +use rustc_errors::Applicability; +use rustc_hir::{Expr, ExprKind}; +use rustc_lint::LateContext; +use rustc_middle::ty; + +use super::MANUAL_MIDPOINT; + +pub(super) fn check<'tcx>( + cx: &LateContext<'tcx>, + expr: &'tcx Expr<'_>, + op: BinOpKind, + left: &'tcx Expr<'_>, + right: &'tcx Expr<'_>, + msrv: &Msrv, +) { + if msrv.meets(msrvs::UINT_FLOAT_MIDPOINT) + && !left.span.from_expansion() + && !right.span.from_expansion() + && op == BinOpKind::Div + && (is_integer_literal(right, 2) || is_float_literal(right, 2.0)) + && let Some((ll_expr, lr_expr)) = add_operands(left) + && add_operands(ll_expr).is_none() && add_operands(lr_expr).is_none() + && let left_ty = cx.typeck_results().expr_ty_adjusted(ll_expr) + && let right_ty = cx.typeck_results().expr_ty_adjusted(lr_expr) + && left_ty == right_ty + // Do not lint on `(_+1)/2` and `(1+_)/2`, it is likely a `div_ceil()` operation + && !is_integer_literal(ll_expr, 1) && !is_integer_literal(lr_expr, 1) + // FIXME: Also lint on signed integers when rust-lang/rust#134340 is merged + && matches!(left_ty.kind(), ty::Uint(_) | ty::Float(_)) + { + let mut app = Applicability::MachineApplicable; + let left_sugg = Sugg::hir_with_context(cx, ll_expr, expr.span.ctxt(), "..", &mut app); + let right_sugg = Sugg::hir_with_context(cx, lr_expr, expr.span.ctxt(), "..", &mut app); + let sugg = format!("{left_ty}::midpoint({left_sugg}, {right_sugg})"); + span_lint_and_sugg( + cx, + MANUAL_MIDPOINT, + expr.span, + "manual implementation of `midpoint` which can overflow", + format!("use `{left_ty}::midpoint` instead"), + sugg, + app, + ); + } +} + +/// Return the left and right operands if `expr` represents an addition +fn add_operands<'e, 'tcx>(expr: &'e Expr<'tcx>) -> Option<(&'e Expr<'tcx>, &'e Expr<'tcx>)> { + match expr.kind { + ExprKind::Binary(op, left, right) if op.node == BinOpKind::Add => Some((left, right)), + _ => None, + } +} diff --git a/clippy_lints/src/operators/mod.rs b/clippy_lints/src/operators/mod.rs index 9ad32c2bd396..badda1330777 100644 --- a/clippy_lints/src/operators/mod.rs +++ b/clippy_lints/src/operators/mod.rs @@ -11,6 +11,7 @@ mod float_cmp; mod float_equality_without_abs; mod identity_op; mod integer_division; +mod manual_midpoint; mod misrefactored_assign_op; mod modulo_arithmetic; mod modulo_one; @@ -24,6 +25,7 @@ mod verbose_bit_mask; pub(crate) mod arithmetic_side_effects; use clippy_config::Conf; +use clippy_utils::msrvs::Msrv; use rustc_hir::{Body, Expr, ExprKind, UnOp}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::impl_lint_pass; @@ -834,10 +836,35 @@ declare_clippy_lint! { "explicit self-assignment" } +declare_clippy_lint! { + /// ### What it does + /// Checks for manual implementation of `midpoint`. + /// + /// ### Why is this bad? + /// Using `(x + y) / 2` might cause an overflow on the intermediate + /// addition result. + /// + /// ### Example + /// ```no_run + /// # let a: u32 = 0; + /// let c = (a + 10) / 2; + /// ``` + /// Use instead: + /// ```no_run + /// # let a: u32 = 0; + /// let c = u32::midpoint(a, 10); + /// ``` + #[clippy::version = "1.87.0"] + pub MANUAL_MIDPOINT, + correctness, + "manual implementation of `midpoint` which can overflow" +} + pub struct Operators { arithmetic_context: numeric_arithmetic::Context, verbose_bit_mask_threshold: u64, modulo_arithmetic_allow_comparison_to_zero: bool, + msrv: Msrv, } impl Operators { pub fn new(conf: &'static Conf) -> Self { @@ -845,6 +872,7 @@ impl Operators { arithmetic_context: numeric_arithmetic::Context::default(), verbose_bit_mask_threshold: conf.verbose_bit_mask_threshold, modulo_arithmetic_allow_comparison_to_zero: conf.allow_comparison_to_zero, + msrv: conf.msrv.clone(), } } } @@ -876,6 +904,7 @@ impl_lint_pass!(Operators => [ NEEDLESS_BITWISE_BOOL, PTR_EQ, SELF_ASSIGNMENT, + MANUAL_MIDPOINT, ]); impl<'tcx> LateLintPass<'tcx> for Operators { @@ -893,6 +922,7 @@ impl<'tcx> LateLintPass<'tcx> for Operators { identity_op::check(cx, e, op.node, lhs, rhs); needless_bitwise_bool::check(cx, e, op.node, lhs, rhs); ptr_eq::check(cx, e, op.node, lhs, rhs); + manual_midpoint::check(cx, e, op.node, lhs, rhs, &self.msrv); } self.arithmetic_context.check_binary(cx, e, op.node, lhs, rhs); bit_mask::check(cx, e, op.node, lhs, rhs); @@ -943,6 +973,8 @@ impl<'tcx> LateLintPass<'tcx> for Operators { fn check_body_post(&mut self, cx: &LateContext<'tcx>, b: &Body<'_>) { self.arithmetic_context.body_post(cx, b); } + + extract_msrv_attr!(LateContext); } fn macro_with_not_op(e: &Expr<'_>) -> bool { diff --git a/clippy_utils/src/msrvs.rs b/clippy_utils/src/msrvs.rs index 2e8bcfaa7af0..395cf0a789b1 100644 --- a/clippy_utils/src/msrvs.rs +++ b/clippy_utils/src/msrvs.rs @@ -19,6 +19,7 @@ macro_rules! msrv_aliases { // names may refer to stabilized feature flags or library items msrv_aliases! { + 1,85,0 { UINT_FLOAT_MIDPOINT } 1,84,0 { CONST_OPTION_AS_SLICE } 1,83,0 { CONST_EXTERN_FN, CONST_FLOAT_BITS_CONV, CONST_FLOAT_CLASSIFY, CONST_MUT_REFS, CONST_UNWRAP } 1,82,0 { IS_NONE_OR, REPEAT_N, RAW_REF_OP } diff --git a/tests/ui/manual_midpoint.fixed b/tests/ui/manual_midpoint.fixed new file mode 100644 index 000000000000..89fd710b31bc --- /dev/null +++ b/tests/ui/manual_midpoint.fixed @@ -0,0 +1,63 @@ +#![warn(clippy::manual_midpoint)] + +macro_rules! mac { + ($a: expr, $b: expr) => { + ($a + $b) / 2 + }; +} + +macro_rules! add { + ($a: expr, $b: expr) => { + ($a + $b) + }; +} + +macro_rules! two { + () => { + 2 + }; +} + +#[clippy::msrv = "1.84"] +fn older_msrv() { + let a: u32 = 10; + let _ = (a + 5) / 2; +} + +#[clippy::msrv = "1.85"] +fn main() { + let a: u32 = 10; + let _ = u32::midpoint(a, 5); //~ ERROR: manual implementation of `midpoint` + + let f: f32 = 10.0; + let _ = f32::midpoint(f, 5.0); //~ ERROR: manual implementation of `midpoint` + + let _: u32 = 5 + u32::midpoint(8, 8) + 2; //~ ERROR: manual implementation of `midpoint` + let _: u32 = const { u32::midpoint(8, 8) }; //~ ERROR: manual implementation of `midpoint` + let _: f64 = const { f64::midpoint(8.0f64, 8.) }; //~ ERROR: manual implementation of `midpoint` + let _: u32 = u32::midpoint(u32::default(), u32::default()); //~ ERROR: manual implementation of `midpoint` + let _: u32 = u32::midpoint(two!(), two!()); //~ ERROR: manual implementation of `midpoint` + + // Do not lint in presence of an addition with more than 2 operands + let _: u32 = (10 + 20 + 30) / 2; + + // Do not lint if whole or part is coming from a macro + let _ = mac!(10, 20); + let _: u32 = add!(10u32, 20u32) / 2; + let _: u32 = (10 + 20) / two!(); + + // Do not lint if a literal is not present + let _ = (f + 5.0) / (1.0 + 1.0); + + // Do not lint on signed integer types + let i: i32 = 10; + let _ = (i + 5) / 2; + + // Do not lint on (x+1)/2 or (1+x)/2 as this looks more like a `div_ceil()` operation + let _ = (i + 1) / 2; + let _ = (1 + i) / 2; + + // But if we see (x+1.0)/2.0 or (x+1.0)/2.0, it is probably a midpoint operation + let _ = f32::midpoint(f, 1.0); //~ ERROR: manual implementation of `midpoint` + let _ = f32::midpoint(1.0, f); //~ ERROR: manual implementation of `midpoint` +} diff --git a/tests/ui/manual_midpoint.rs b/tests/ui/manual_midpoint.rs new file mode 100644 index 000000000000..577219a867ce --- /dev/null +++ b/tests/ui/manual_midpoint.rs @@ -0,0 +1,63 @@ +#![warn(clippy::manual_midpoint)] + +macro_rules! mac { + ($a: expr, $b: expr) => { + ($a + $b) / 2 + }; +} + +macro_rules! add { + ($a: expr, $b: expr) => { + ($a + $b) + }; +} + +macro_rules! two { + () => { + 2 + }; +} + +#[clippy::msrv = "1.84"] +fn older_msrv() { + let a: u32 = 10; + let _ = (a + 5) / 2; +} + +#[clippy::msrv = "1.85"] +fn main() { + let a: u32 = 10; + let _ = (a + 5) / 2; //~ ERROR: manual implementation of `midpoint` + + let f: f32 = 10.0; + let _ = (f + 5.0) / 2.0; //~ ERROR: manual implementation of `midpoint` + + let _: u32 = 5 + (8 + 8) / 2 + 2; //~ ERROR: manual implementation of `midpoint` + let _: u32 = const { (8 + 8) / 2 }; //~ ERROR: manual implementation of `midpoint` + let _: f64 = const { (8.0f64 + 8.) / 2. }; //~ ERROR: manual implementation of `midpoint` + let _: u32 = (u32::default() + u32::default()) / 2; //~ ERROR: manual implementation of `midpoint` + let _: u32 = (two!() + two!()) / 2; //~ ERROR: manual implementation of `midpoint` + + // Do not lint in presence of an addition with more than 2 operands + let _: u32 = (10 + 20 + 30) / 2; + + // Do not lint if whole or part is coming from a macro + let _ = mac!(10, 20); + let _: u32 = add!(10u32, 20u32) / 2; + let _: u32 = (10 + 20) / two!(); + + // Do not lint if a literal is not present + let _ = (f + 5.0) / (1.0 + 1.0); + + // Do not lint on signed integer types + let i: i32 = 10; + let _ = (i + 5) / 2; + + // Do not lint on (x+1)/2 or (1+x)/2 as this looks more like a `div_ceil()` operation + let _ = (i + 1) / 2; + let _ = (1 + i) / 2; + + // But if we see (x+1.0)/2.0 or (x+1.0)/2.0, it is probably a midpoint operation + let _ = (f + 1.0) / 2.0; //~ ERROR: manual implementation of `midpoint` + let _ = (1.0 + f) / 2.0; //~ ERROR: manual implementation of `midpoint` +} diff --git a/tests/ui/manual_midpoint.stderr b/tests/ui/manual_midpoint.stderr new file mode 100644 index 000000000000..e968ac3f7551 --- /dev/null +++ b/tests/ui/manual_midpoint.stderr @@ -0,0 +1,59 @@ +error: manual implementation of `midpoint` which can overflow + --> tests/ui/manual_midpoint.rs:30:13 + | +LL | let _ = (a + 5) / 2; + | ^^^^^^^^^^^ help: use `u32::midpoint` instead: `u32::midpoint(a, 5)` + | + = note: `-D clippy::manual-midpoint` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::manual_midpoint)]` + +error: manual implementation of `midpoint` which can overflow + --> tests/ui/manual_midpoint.rs:33:13 + | +LL | let _ = (f + 5.0) / 2.0; + | ^^^^^^^^^^^^^^^ help: use `f32::midpoint` instead: `f32::midpoint(f, 5.0)` + +error: manual implementation of `midpoint` which can overflow + --> tests/ui/manual_midpoint.rs:35:22 + | +LL | let _: u32 = 5 + (8 + 8) / 2 + 2; + | ^^^^^^^^^^^ help: use `u32::midpoint` instead: `u32::midpoint(8, 8)` + +error: manual implementation of `midpoint` which can overflow + --> tests/ui/manual_midpoint.rs:36:26 + | +LL | let _: u32 = const { (8 + 8) / 2 }; + | ^^^^^^^^^^^ help: use `u32::midpoint` instead: `u32::midpoint(8, 8)` + +error: manual implementation of `midpoint` which can overflow + --> tests/ui/manual_midpoint.rs:37:26 + | +LL | let _: f64 = const { (8.0f64 + 8.) / 2. }; + | ^^^^^^^^^^^^^^^^^^ help: use `f64::midpoint` instead: `f64::midpoint(8.0f64, 8.)` + +error: manual implementation of `midpoint` which can overflow + --> tests/ui/manual_midpoint.rs:38:18 + | +LL | let _: u32 = (u32::default() + u32::default()) / 2; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `u32::midpoint` instead: `u32::midpoint(u32::default(), u32::default())` + +error: manual implementation of `midpoint` which can overflow + --> tests/ui/manual_midpoint.rs:39:18 + | +LL | let _: u32 = (two!() + two!()) / 2; + | ^^^^^^^^^^^^^^^^^^^^^ help: use `u32::midpoint` instead: `u32::midpoint(two!(), two!())` + +error: manual implementation of `midpoint` which can overflow + --> tests/ui/manual_midpoint.rs:61:13 + | +LL | let _ = (f + 1.0) / 2.0; + | ^^^^^^^^^^^^^^^ help: use `f32::midpoint` instead: `f32::midpoint(f, 1.0)` + +error: manual implementation of `midpoint` which can overflow + --> tests/ui/manual_midpoint.rs:62:13 + | +LL | let _ = (1.0 + f) / 2.0; + | ^^^^^^^^^^^^^^^ help: use `f32::midpoint` instead: `f32::midpoint(1.0, f)` + +error: aborting due to 9 previous errors + diff --git a/tests/ui/option_if_let_else.fixed b/tests/ui/option_if_let_else.fixed index 56b313244e36..f5a869cf2831 100644 --- a/tests/ui/option_if_let_else.fixed +++ b/tests/ui/option_if_let_else.fixed @@ -4,6 +4,7 @@ clippy::equatable_if_let, clippy::let_unit_value, clippy::redundant_locals, + clippy::manual_midpoint, clippy::manual_unwrap_or_default, clippy::manual_unwrap_or )] diff --git a/tests/ui/option_if_let_else.rs b/tests/ui/option_if_let_else.rs index 4a4bb141ed54..d48272e618ac 100644 --- a/tests/ui/option_if_let_else.rs +++ b/tests/ui/option_if_let_else.rs @@ -4,6 +4,7 @@ clippy::equatable_if_let, clippy::let_unit_value, clippy::redundant_locals, + clippy::manual_midpoint, clippy::manual_unwrap_or_default, clippy::manual_unwrap_or )] diff --git a/tests/ui/option_if_let_else.stderr b/tests/ui/option_if_let_else.stderr index f872ac991840..9eb41f81a539 100644 --- a/tests/ui/option_if_let_else.stderr +++ b/tests/ui/option_if_let_else.stderr @@ -1,5 +1,5 @@ error: use Option::map_or instead of an if let/else - --> tests/ui/option_if_let_else.rs:12:5 + --> tests/ui/option_if_let_else.rs:13:5 | LL | / if let Some(x) = string { LL | | @@ -13,19 +13,19 @@ LL | | } = help: to override `-D warnings` add `#[allow(clippy::option_if_let_else)]` error: use Option::map_or instead of an if let/else - --> tests/ui/option_if_let_else.rs:31:13 + --> tests/ui/option_if_let_else.rs:32:13 | LL | let _ = if let Some(s) = *string { s.len() } else { 0 }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `string.map_or(0, |s| s.len())` error: use Option::map_or instead of an if let/else - --> tests/ui/option_if_let_else.rs:33:13 + --> tests/ui/option_if_let_else.rs:34:13 | LL | let _ = if let Some(s) = &num { s } else { &0 }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `num.as_ref().map_or(&0, |s| s)` error: use Option::map_or instead of an if let/else - --> tests/ui/option_if_let_else.rs:35:13 + --> tests/ui/option_if_let_else.rs:36:13 | LL | let _ = if let Some(s) = &mut num { | _____________^ @@ -47,13 +47,13 @@ LL ~ }); | error: use Option::map_or instead of an if let/else - --> tests/ui/option_if_let_else.rs:42:13 + --> tests/ui/option_if_let_else.rs:43:13 | LL | let _ = if let Some(ref s) = num { s } else { &0 }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `num.as_ref().map_or(&0, |s| s)` error: use Option::map_or instead of an if let/else - --> tests/ui/option_if_let_else.rs:44:13 + --> tests/ui/option_if_let_else.rs:45:13 | LL | let _ = if let Some(mut s) = num { | _____________^ @@ -75,7 +75,7 @@ LL ~ }); | error: use Option::map_or instead of an if let/else - --> tests/ui/option_if_let_else.rs:51:13 + --> tests/ui/option_if_let_else.rs:52:13 | LL | let _ = if let Some(ref mut s) = num { | _____________^ @@ -97,7 +97,7 @@ LL ~ }); | error: use Option::map_or instead of an if let/else - --> tests/ui/option_if_let_else.rs:61:5 + --> tests/ui/option_if_let_else.rs:62:5 | LL | / if let Some(x) = arg { LL | | @@ -118,7 +118,7 @@ LL + }) | error: use Option::map_or_else instead of an if let/else - --> tests/ui/option_if_let_else.rs:75:13 + --> tests/ui/option_if_let_else.rs:76:13 | LL | let _ = if let Some(x) = arg { | _____________^ @@ -131,7 +131,7 @@ LL | | }; | |_____^ help: try: `arg.map_or_else(side_effect, |x| x)` error: use Option::map_or_else instead of an if let/else - --> tests/ui/option_if_let_else.rs:85:13 + --> tests/ui/option_if_let_else.rs:86:13 | LL | let _ = if let Some(x) = arg { | _____________^ @@ -154,7 +154,7 @@ LL ~ }, |x| x * x * x * x); | error: use Option::map_or_else instead of an if let/else - --> tests/ui/option_if_let_else.rs:119:13 + --> tests/ui/option_if_let_else.rs:120:13 | LL | / if let Some(idx) = s.find('.') { LL | | @@ -165,7 +165,7 @@ LL | | } | |_____________^ help: try: `s.find('.').map_or_else(|| vec![s.to_string()], |idx| vec![s[..idx].to_string(), s[idx..].to_string()])` error: use Option::map_or_else instead of an if let/else - --> tests/ui/option_if_let_else.rs:131:5 + --> tests/ui/option_if_let_else.rs:132:5 | LL | / if let Ok(binding) = variable { LL | | @@ -189,13 +189,13 @@ LL + }) | error: use Option::map_or instead of an if let/else - --> tests/ui/option_if_let_else.rs:156:13 + --> tests/ui/option_if_let_else.rs:157:13 | LL | let _ = if let Some(x) = optional { x + 2 } else { 5 }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `optional.map_or(5, |x| x + 2)` error: use Option::map_or instead of an if let/else - --> tests/ui/option_if_let_else.rs:167:13 + --> tests/ui/option_if_let_else.rs:168:13 | LL | let _ = if let Some(x) = Some(0) { | _____________^ @@ -217,13 +217,13 @@ LL ~ }); | error: use Option::map_or instead of an if let/else - --> tests/ui/option_if_let_else.rs:196:13 + --> tests/ui/option_if_let_else.rs:197:13 | LL | let _ = if let Some(x) = Some(0) { s.len() + x } else { s.len() }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Some(0).map_or(s.len(), |x| s.len() + x)` error: use Option::map_or instead of an if let/else - --> tests/ui/option_if_let_else.rs:201:13 + --> tests/ui/option_if_let_else.rs:202:13 | LL | let _ = if let Some(x) = Some(0) { | _____________^ @@ -245,7 +245,7 @@ LL ~ }); | error: use Option::map_or instead of an if let/else - --> tests/ui/option_if_let_else.rs:241:13 + --> tests/ui/option_if_let_else.rs:242:13 | LL | let _ = match s { | _____________^ @@ -256,7 +256,7 @@ LL | | }; | |_____^ help: try: `s.map_or(1, |string| string.len())` error: use Option::map_or instead of an if let/else - --> tests/ui/option_if_let_else.rs:246:13 + --> tests/ui/option_if_let_else.rs:247:13 | LL | let _ = match Some(10) { | _____________^ @@ -267,7 +267,7 @@ LL | | }; | |_____^ help: try: `Some(10).map_or(5, |a| a + 1)` error: use Option::map_or instead of an if let/else - --> tests/ui/option_if_let_else.rs:253:13 + --> tests/ui/option_if_let_else.rs:254:13 | LL | let _ = match res { | _____________^ @@ -278,7 +278,7 @@ LL | | }; | |_____^ help: try: `res.map_or(1, |a| a + 1)` error: use Option::map_or instead of an if let/else - --> tests/ui/option_if_let_else.rs:258:13 + --> tests/ui/option_if_let_else.rs:259:13 | LL | let _ = match res { | _____________^ @@ -289,13 +289,13 @@ LL | | }; | |_____^ help: try: `res.map_or(1, |a| a + 1)` error: use Option::map_or instead of an if let/else - --> tests/ui/option_if_let_else.rs:263:13 + --> tests/ui/option_if_let_else.rs:264:13 | LL | let _ = if let Ok(a) = res { a + 1 } else { 5 }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `res.map_or(5, |a| a + 1)` error: use Option::map_or instead of an if let/else - --> tests/ui/option_if_let_else.rs:281:17 + --> tests/ui/option_if_let_else.rs:282:17 | LL | let _ = match initial { | _________________^ @@ -306,7 +306,7 @@ LL | | }; | |_________^ help: try: `initial.as_ref().map_or(42, |value| do_something(value))` error: use Option::map_or instead of an if let/else - --> tests/ui/option_if_let_else.rs:289:17 + --> tests/ui/option_if_let_else.rs:290:17 | LL | let _ = match initial { | _________________^ @@ -317,7 +317,7 @@ LL | | }; | |_________^ help: try: `initial.as_mut().map_or(42, |value| do_something2(value))` error: use Option::map_or_else instead of an if let/else - --> tests/ui/option_if_let_else.rs:313:24 + --> tests/ui/option_if_let_else.rs:314:24 | LL | let mut _hashmap = if let Some(hm) = &opt { | ________________________^ @@ -329,7 +329,7 @@ LL | | }; | |_____^ help: try: `opt.as_ref().map_or_else(HashMap::new, |hm| hm.clone())` error: use Option::map_or_else instead of an if let/else - --> tests/ui/option_if_let_else.rs:320:19 + --> tests/ui/option_if_let_else.rs:321:19 | LL | let mut _hm = if let Some(hm) = &opt { hm.clone() } else { new_map!() }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `opt.as_ref().map_or_else(|| new_map!(), |hm| hm.clone())` diff --git a/tests/ui/suspicious_operation_groupings.fixed b/tests/ui/suspicious_operation_groupings.fixed index 37054222bd62..fa680e537d30 100644 --- a/tests/ui/suspicious_operation_groupings.fixed +++ b/tests/ui/suspicious_operation_groupings.fixed @@ -1,7 +1,7 @@ //@compile-flags: -Zdeduplicate-diagnostics=yes #![warn(clippy::suspicious_operation_groupings)] -#![allow(dead_code, unused_parens, clippy::eq_op)] +#![allow(dead_code, unused_parens, clippy::eq_op, clippy::manual_midpoint)] struct Vec3 { x: f64, diff --git a/tests/ui/suspicious_operation_groupings.rs b/tests/ui/suspicious_operation_groupings.rs index f4d1c3c70b56..4ffee640e8bd 100644 --- a/tests/ui/suspicious_operation_groupings.rs +++ b/tests/ui/suspicious_operation_groupings.rs @@ -1,7 +1,7 @@ //@compile-flags: -Zdeduplicate-diagnostics=yes #![warn(clippy::suspicious_operation_groupings)] -#![allow(dead_code, unused_parens, clippy::eq_op)] +#![allow(dead_code, unused_parens, clippy::eq_op, clippy::manual_midpoint)] struct Vec3 { x: f64, From 296587cdfc8bcdd98a6b5f7d1399bfca69fe7b37 Mon Sep 17 00:00:00 2001 From: Samuel Tardieu Date: Fri, 21 Feb 2025 22:23:54 +0100 Subject: [PATCH 3/3] `manual_midpoint`: support signed integer types as well `midpoint()` for integer types has been stabilized in Rust 1.87. --- clippy_lints/src/operators/manual_midpoint.rs | 16 ++++++++---- clippy_utils/src/msrvs.rs | 1 + tests/ui/manual_midpoint.fixed | 12 +++++++++ tests/ui/manual_midpoint.rs | 12 +++++++++ tests/ui/manual_midpoint.stderr | 26 ++++++++++++------- 5 files changed, 52 insertions(+), 15 deletions(-) diff --git a/clippy_lints/src/operators/manual_midpoint.rs b/clippy_lints/src/operators/manual_midpoint.rs index d4f1282a7542..61ef5670a5ad 100644 --- a/clippy_lints/src/operators/manual_midpoint.rs +++ b/clippy_lints/src/operators/manual_midpoint.rs @@ -6,7 +6,7 @@ use rustc_ast::BinOpKind; use rustc_errors::Applicability; use rustc_hir::{Expr, ExprKind}; use rustc_lint::LateContext; -use rustc_middle::ty; +use rustc_middle::ty::{self, Ty}; use super::MANUAL_MIDPOINT; @@ -18,8 +18,7 @@ pub(super) fn check<'tcx>( right: &'tcx Expr<'_>, msrv: &Msrv, ) { - if msrv.meets(msrvs::UINT_FLOAT_MIDPOINT) - && !left.span.from_expansion() + if !left.span.from_expansion() && !right.span.from_expansion() && op == BinOpKind::Div && (is_integer_literal(right, 2) || is_float_literal(right, 2.0)) @@ -30,8 +29,7 @@ pub(super) fn check<'tcx>( && left_ty == right_ty // Do not lint on `(_+1)/2` and `(1+_)/2`, it is likely a `div_ceil()` operation && !is_integer_literal(ll_expr, 1) && !is_integer_literal(lr_expr, 1) - // FIXME: Also lint on signed integers when rust-lang/rust#134340 is merged - && matches!(left_ty.kind(), ty::Uint(_) | ty::Float(_)) + && is_midpoint_implemented(left_ty, msrv) { let mut app = Applicability::MachineApplicable; let left_sugg = Sugg::hir_with_context(cx, ll_expr, expr.span.ctxt(), "..", &mut app); @@ -56,3 +54,11 @@ fn add_operands<'e, 'tcx>(expr: &'e Expr<'tcx>) -> Option<(&'e Expr<'tcx>, &'e E _ => None, } } + +fn is_midpoint_implemented(ty: Ty<'_>, msrv: &Msrv) -> bool { + match ty.kind() { + ty::Uint(_) | ty::Float(_) => msrv.meets(msrvs::UINT_FLOAT_MIDPOINT), + ty::Int(_) => msrv.meets(msrvs::INT_MIDPOINT), + _ => false, + } +} diff --git a/clippy_utils/src/msrvs.rs b/clippy_utils/src/msrvs.rs index 395cf0a789b1..ebcf9a5247f4 100644 --- a/clippy_utils/src/msrvs.rs +++ b/clippy_utils/src/msrvs.rs @@ -19,6 +19,7 @@ macro_rules! msrv_aliases { // names may refer to stabilized feature flags or library items msrv_aliases! { + 1,87,0 { INT_MIDPOINT } 1,85,0 { UINT_FLOAT_MIDPOINT } 1,84,0 { CONST_OPTION_AS_SLICE } 1,83,0 { CONST_EXTERN_FN, CONST_FLOAT_BITS_CONV, CONST_FLOAT_CLASSIFY, CONST_MUT_REFS, CONST_UNWRAP } diff --git a/tests/ui/manual_midpoint.fixed b/tests/ui/manual_midpoint.fixed index 89fd710b31bc..0b088a1af6e3 100644 --- a/tests/ui/manual_midpoint.fixed +++ b/tests/ui/manual_midpoint.fixed @@ -1,4 +1,5 @@ #![warn(clippy::manual_midpoint)] +#![feature(num_midpoint_signed)] macro_rules! mac { ($a: expr, $b: expr) => { @@ -61,3 +62,14 @@ fn main() { let _ = f32::midpoint(f, 1.0); //~ ERROR: manual implementation of `midpoint` let _ = f32::midpoint(1.0, f); //~ ERROR: manual implementation of `midpoint` } + +#[clippy::msrv = "1.86"] +fn older_signed_midpoint(i: i32) { + // Do not lint + let _ = (i + 10) / 2; +} + +#[clippy::msrv = "1.87"] +fn signed_midpoint(i: i32) { + let _ = i32::midpoint(i, 10); //~ ERROR: manual implementation of `midpoint` +} diff --git a/tests/ui/manual_midpoint.rs b/tests/ui/manual_midpoint.rs index 577219a867ce..6f957f88a43a 100644 --- a/tests/ui/manual_midpoint.rs +++ b/tests/ui/manual_midpoint.rs @@ -1,4 +1,5 @@ #![warn(clippy::manual_midpoint)] +#![feature(num_midpoint_signed)] macro_rules! mac { ($a: expr, $b: expr) => { @@ -61,3 +62,14 @@ fn main() { let _ = (f + 1.0) / 2.0; //~ ERROR: manual implementation of `midpoint` let _ = (1.0 + f) / 2.0; //~ ERROR: manual implementation of `midpoint` } + +#[clippy::msrv = "1.86"] +fn older_signed_midpoint(i: i32) { + // Do not lint + let _ = (i + 10) / 2; +} + +#[clippy::msrv = "1.87"] +fn signed_midpoint(i: i32) { + let _ = (i + 10) / 2; //~ ERROR: manual implementation of `midpoint` +} diff --git a/tests/ui/manual_midpoint.stderr b/tests/ui/manual_midpoint.stderr index e968ac3f7551..54dca4d26f49 100644 --- a/tests/ui/manual_midpoint.stderr +++ b/tests/ui/manual_midpoint.stderr @@ -1,5 +1,5 @@ error: manual implementation of `midpoint` which can overflow - --> tests/ui/manual_midpoint.rs:30:13 + --> tests/ui/manual_midpoint.rs:31:13 | LL | let _ = (a + 5) / 2; | ^^^^^^^^^^^ help: use `u32::midpoint` instead: `u32::midpoint(a, 5)` @@ -8,52 +8,58 @@ LL | let _ = (a + 5) / 2; = help: to override `-D warnings` add `#[allow(clippy::manual_midpoint)]` error: manual implementation of `midpoint` which can overflow - --> tests/ui/manual_midpoint.rs:33:13 + --> tests/ui/manual_midpoint.rs:34:13 | LL | let _ = (f + 5.0) / 2.0; | ^^^^^^^^^^^^^^^ help: use `f32::midpoint` instead: `f32::midpoint(f, 5.0)` error: manual implementation of `midpoint` which can overflow - --> tests/ui/manual_midpoint.rs:35:22 + --> tests/ui/manual_midpoint.rs:36:22 | LL | let _: u32 = 5 + (8 + 8) / 2 + 2; | ^^^^^^^^^^^ help: use `u32::midpoint` instead: `u32::midpoint(8, 8)` error: manual implementation of `midpoint` which can overflow - --> tests/ui/manual_midpoint.rs:36:26 + --> tests/ui/manual_midpoint.rs:37:26 | LL | let _: u32 = const { (8 + 8) / 2 }; | ^^^^^^^^^^^ help: use `u32::midpoint` instead: `u32::midpoint(8, 8)` error: manual implementation of `midpoint` which can overflow - --> tests/ui/manual_midpoint.rs:37:26 + --> tests/ui/manual_midpoint.rs:38:26 | LL | let _: f64 = const { (8.0f64 + 8.) / 2. }; | ^^^^^^^^^^^^^^^^^^ help: use `f64::midpoint` instead: `f64::midpoint(8.0f64, 8.)` error: manual implementation of `midpoint` which can overflow - --> tests/ui/manual_midpoint.rs:38:18 + --> tests/ui/manual_midpoint.rs:39:18 | LL | let _: u32 = (u32::default() + u32::default()) / 2; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `u32::midpoint` instead: `u32::midpoint(u32::default(), u32::default())` error: manual implementation of `midpoint` which can overflow - --> tests/ui/manual_midpoint.rs:39:18 + --> tests/ui/manual_midpoint.rs:40:18 | LL | let _: u32 = (two!() + two!()) / 2; | ^^^^^^^^^^^^^^^^^^^^^ help: use `u32::midpoint` instead: `u32::midpoint(two!(), two!())` error: manual implementation of `midpoint` which can overflow - --> tests/ui/manual_midpoint.rs:61:13 + --> tests/ui/manual_midpoint.rs:62:13 | LL | let _ = (f + 1.0) / 2.0; | ^^^^^^^^^^^^^^^ help: use `f32::midpoint` instead: `f32::midpoint(f, 1.0)` error: manual implementation of `midpoint` which can overflow - --> tests/ui/manual_midpoint.rs:62:13 + --> tests/ui/manual_midpoint.rs:63:13 | LL | let _ = (1.0 + f) / 2.0; | ^^^^^^^^^^^^^^^ help: use `f32::midpoint` instead: `f32::midpoint(1.0, f)` -error: aborting due to 9 previous errors +error: manual implementation of `midpoint` which can overflow + --> tests/ui/manual_midpoint.rs:74:13 + | +LL | let _ = (i + 10) / 2; + | ^^^^^^^^^^^^ help: use `i32::midpoint` instead: `i32::midpoint(i, 10)` + +error: aborting due to 10 previous errors