diff --git a/CHANGELOG.md b/CHANGELOG.md index b2e9f6d1dd3e..5288f26f4831 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6482,6 +6482,7 @@ Released 2018-09-13 [`manual_retain`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_retain [`manual_rotate`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_rotate [`manual_saturating_arithmetic`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_saturating_arithmetic +[`manual_sign_check`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_sign_check [`manual_slice_fill`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_slice_fill [`manual_slice_size_calculation`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_slice_size_calculation [`manual_split_once`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_split_once diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 375d179681da..c1bc591735ce 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -594,6 +594,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::operators::INTEGER_DIVISION_INFO, crate::operators::MANUAL_IS_MULTIPLE_OF_INFO, crate::operators::MANUAL_MIDPOINT_INFO, + crate::operators::MANUAL_SIGN_CHECK_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_sign_check.rs b/clippy_lints/src/operators/manual_sign_check.rs new file mode 100644 index 000000000000..c07cc4bfb2b7 --- /dev/null +++ b/clippy_lints/src/operators/manual_sign_check.rs @@ -0,0 +1,67 @@ +use clippy_utils::consts::{ConstEvalCtxt, Constant}; +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::source::snippet_with_applicability; +use rustc_errors::Applicability; +use rustc_hir::{BinOpKind, Expr}; +use rustc_lint::LateContext; + +use super::MANUAL_SIGN_CHECK; + +pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, op: BinOpKind, left: &'tcx Expr<'_>, right: &'tcx Expr<'_>) { + let (float_expr, direction) = if is_zero(cx, left) && is_float(cx, right) { + (right, Side::Right) + } else if is_zero(cx, right) && is_float(cx, left) { + (left, Side::Left) + } else { + return; + }; + + let mut applicability = Applicability::MachineApplicable; + let float_snippet = snippet_with_applicability(cx, float_expr.span, "..", &mut applicability); + let (method, negate) = match (direction, op) { + (Side::Left, BinOpKind::Lt) | (Side::Right, BinOpKind::Gt) => ("is_sign_negative", false), + (Side::Left, BinOpKind::Le) | (Side::Right, BinOpKind::Ge) => ("is_sign_positive", true), + (Side::Left, BinOpKind::Gt) | (Side::Right, BinOpKind::Lt) => ("is_sign_positive", false), + (Side::Left, BinOpKind::Ge) | (Side::Right, BinOpKind::Le) => ("is_sign_negative", true), + _ => return, + }; + + let suggestion = if negate { + format!("!{float_snippet}.{method}()") + } else { + format!("{float_snippet}.{method}()") + }; + + span_lint_and_sugg( + cx, + MANUAL_SIGN_CHECK, + float_expr.span.to(right.span), + "checking the sign of a floating point number by comparing it to zero", + format!("consider using `{method}` for clarity and performance"), + suggestion, + applicability, + ); +} + +enum Side { + Left, + Right, +} + +fn is_zero(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { + let ecx = ConstEvalCtxt::new(cx); + let Some(constant) = ecx.eval(expr) else { + return false; + }; + + match constant { + // FIXME(f16_f128): add when equality check is available on all platforms + Constant::F32(f) => f == 0.0, + Constant::F64(f) => f == 0.0, + _ => false, + } +} + +fn is_float(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { + cx.typeck_results().expr_ty(expr).is_floating_point() +} diff --git a/clippy_lints/src/operators/mod.rs b/clippy_lints/src/operators/mod.rs index aaea4ff11fc3..2db95bc29363 100644 --- a/clippy_lints/src/operators/mod.rs +++ b/clippy_lints/src/operators/mod.rs @@ -13,6 +13,7 @@ mod identity_op; mod integer_division; mod manual_is_multiple_of; mod manual_midpoint; +mod manual_sign_check; mod misrefactored_assign_op; mod modulo_arithmetic; mod modulo_one; @@ -860,6 +861,40 @@ declare_clippy_lint! { "manual implementation of `.is_multiple_of()`" } +declare_clippy_lint! { + /// ### What it does + /// Checks for manually checking the sign of a floating point number by comparing it to zero with `<`, `>`, `<=`, or `>=`. + /// + /// ### Why is this bad? + /// Floating point numbers have a dedicated sign bit that can be checked with bitwise operators which uses far fewer + /// CPU cycles. `std` exposes the `is_sign_positive` and `is_sign_negative` methods for this purpose. + /// + /// ### Caveats + /// The `is_sign_positive` and `is_sign_negative` methods do not behave the same as the comparison operators in some + /// cases. Behavior with `NaN` is unspecified, and use with negative zero will return `true`, whereas `-0.0 < 0.0 + /// == false`. + /// + /// ### Example + /// ```rust + /// let foo: f32 = 1.0; + /// if foo < 0.0 { + /// println!("negative"); + /// } + /// ``` + /// + /// Use instead: + /// ```rust + /// let foo: f32 = 1.0; + /// if foo.is_sign_negative() { + /// println!("negative"); + /// } + /// ``` + #[clippy::version = "1.92.0"] + pub MANUAL_SIGN_CHECK, + pedantic, + "checking the sign of a floating point number by comparing it to zero, which is slow" +} + pub struct Operators { arithmetic_context: numeric_arithmetic::Context, verbose_bit_mask_threshold: u64, @@ -906,6 +941,7 @@ impl_lint_pass!(Operators => [ SELF_ASSIGNMENT, MANUAL_MIDPOINT, MANUAL_IS_MULTIPLE_OF, + MANUAL_SIGN_CHECK, ]); impl<'tcx> LateLintPass<'tcx> for Operators { @@ -944,6 +980,7 @@ impl<'tcx> LateLintPass<'tcx> for Operators { rhs, self.modulo_arithmetic_allow_comparison_to_zero, ); + manual_sign_check::check(cx, op.node, lhs, rhs); }, ExprKind::AssignOp(op, lhs, rhs) => { let bin_op = op.node.into(); diff --git a/tests/ui/manual_sign_check.fixed b/tests/ui/manual_sign_check.fixed new file mode 100644 index 000000000000..414fedff5336 --- /dev/null +++ b/tests/ui/manual_sign_check.fixed @@ -0,0 +1,17 @@ +#![allow(clippy::needless_if)] +#![warn(clippy::manual_sign_check)] + +fn main() { + let x: f32 = 1.0; + + if x.is_sign_negative() {} + //~^ manual_sign_check + if !x.is_sign_positive() {} + //~^ manual_sign_check + if x.is_sign_positive() {} + //~^ manual_sign_check + if !x.is_sign_negative() {} + //~^ manual_sign_check + if x == 0.0 {} + if x < 1.0 {} +} diff --git a/tests/ui/manual_sign_check.rs b/tests/ui/manual_sign_check.rs new file mode 100644 index 000000000000..84c7bd5d5c08 --- /dev/null +++ b/tests/ui/manual_sign_check.rs @@ -0,0 +1,17 @@ +#![allow(clippy::needless_if)] +#![warn(clippy::manual_sign_check)] + +fn main() { + let x: f32 = 1.0; + + if x < 0.0 {} + //~^ manual_sign_check + if x <= 0.0 {} + //~^ manual_sign_check + if x > 0.0 {} + //~^ manual_sign_check + if x >= 0.0 {} + //~^ manual_sign_check + if x == 0.0 {} + if x < 1.0 {} +} diff --git a/tests/ui/manual_sign_check.stderr b/tests/ui/manual_sign_check.stderr new file mode 100644 index 000000000000..adbfc381e1c3 --- /dev/null +++ b/tests/ui/manual_sign_check.stderr @@ -0,0 +1,29 @@ +error: checking the sign of a floating point number by comparing it to zero + --> tests/ui/manual_sign_check.rs:7:8 + | +LL | if x < 0.0 {} + | ^^^^^^^ help: consider using `is_sign_negative` for clarity and performance: `x.is_sign_negative()` + | + = note: `-D clippy::manual-sign-check` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::manual_sign_check)]` + +error: checking the sign of a floating point number by comparing it to zero + --> tests/ui/manual_sign_check.rs:9:8 + | +LL | if x <= 0.0 {} + | ^^^^^^^^ help: consider using `is_sign_positive` for clarity and performance: `!x.is_sign_positive()` + +error: checking the sign of a floating point number by comparing it to zero + --> tests/ui/manual_sign_check.rs:11:8 + | +LL | if x > 0.0 {} + | ^^^^^^^ help: consider using `is_sign_positive` for clarity and performance: `x.is_sign_positive()` + +error: checking the sign of a floating point number by comparing it to zero + --> tests/ui/manual_sign_check.rs:13:8 + | +LL | if x >= 0.0 {} + | ^^^^^^^^ help: consider using `is_sign_negative` for clarity and performance: `!x.is_sign_negative()` + +error: aborting due to 4 previous errors +