diff --git a/clippy_lints/src/floating_point_arithmetic.rs b/clippy_lints/src/floating_point_arithmetic.rs index 29f924ebe1c5..0fcfb7b88497 100644 --- a/clippy_lints/src/floating_point_arithmetic.rs +++ b/clippy_lints/src/floating_point_arithmetic.rs @@ -1,8 +1,8 @@ use crate::consts::{ - constant, Constant, + constant, constant_simple, Constant, Constant::{F32, F64}, }; -use crate::utils::{span_lint_and_sugg, sugg}; +use crate::utils::{higher, span_lint_and_sugg, sugg, SpanlessEq}; use if_chain::if_chain; use rustc::ty; use rustc_errors::Applicability; @@ -72,6 +72,16 @@ declare_clippy_lint! { /// let _ = a.log(E); /// let _ = a.powf(2.0); /// let _ = a * 2.0 + 4.0; + /// let _ = if a < 0.0 { + /// -a + /// } else { + /// a + /// }; + /// let _ = if a < 0.0 { + /// a + /// } else { + /// -a + /// }; /// ``` /// /// is better expressed as @@ -88,6 +98,8 @@ declare_clippy_lint! { /// let _ = a.ln(); /// let _ = a.powi(2); /// let _ = a.mul_add(2.0, 4.0); + /// let _ = a.abs(); + /// let _ = -a.abs(); /// ``` pub SUBOPTIMAL_FLOPS, nursery, @@ -359,6 +371,116 @@ fn check_mul_add(cx: &LateContext<'_, '_>, expr: &Expr<'_>) { } } +/// Returns true iff expr is an expression which tests whether or not +/// test is positive or an expression which tests whether or not test +/// is nonnegative. +/// Used for check-custom-abs function below +fn is_testing_positive(cx: &LateContext<'_, '_>, expr: &Expr<'_>, test: &Expr<'_>) -> bool { + if let ExprKind::Binary(Spanned { node: op, .. }, left, right) = expr.kind { + match op { + BinOpKind::Gt | BinOpKind::Ge => is_zero(cx, right) && are_exprs_equal(cx, left, test), + BinOpKind::Lt | BinOpKind::Le => is_zero(cx, left) && are_exprs_equal(cx, right, test), + _ => false, + } + } else { + false + } +} + +/// See [`is_testing_positive`] +fn is_testing_negative(cx: &LateContext<'_, '_>, expr: &Expr<'_>, test: &Expr<'_>) -> bool { + if let ExprKind::Binary(Spanned { node: op, .. }, left, right) = expr.kind { + match op { + BinOpKind::Gt | BinOpKind::Ge => is_zero(cx, left) && are_exprs_equal(cx, right, test), + BinOpKind::Lt | BinOpKind::Le => is_zero(cx, right) && are_exprs_equal(cx, left, test), + _ => false, + } + } else { + false + } +} + +fn are_exprs_equal(cx: &LateContext<'_, '_>, expr1: &Expr<'_>, expr2: &Expr<'_>) -> bool { + SpanlessEq::new(cx).ignore_fn().eq_expr(expr1, expr2) +} + +/// Returns true iff expr is some zero literal +fn is_zero(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> bool { + match constant_simple(cx, cx.tables, expr) { + Some(Constant::Int(i)) => i == 0, + Some(Constant::F32(f)) => f == 0.0, + Some(Constant::F64(f)) => f == 0.0, + _ => false, + } +} + +/// If the two expressions are negations of each other, then it returns +/// a tuple, in which the first element is true iff expr1 is the +/// positive expressions, and the second element is the positive +/// one of the two expressions +/// If the two expressions are not negations of each other, then it +/// returns None. +fn are_negated<'a>(cx: &LateContext<'_, '_>, expr1: &'a Expr<'a>, expr2: &'a Expr<'a>) -> Option<(bool, &'a Expr<'a>)> { + if let ExprKind::Unary(UnOp::UnNeg, expr1_negated) = &expr1.kind { + if are_exprs_equal(cx, expr1_negated, expr2) { + return Some((false, expr2)); + } + } + if let ExprKind::Unary(UnOp::UnNeg, expr2_negated) = &expr2.kind { + if are_exprs_equal(cx, expr1, expr2_negated) { + return Some((true, expr1)); + } + } + None +} + +fn check_custom_abs(cx: &LateContext<'_, '_>, expr: &Expr<'_>) { + if_chain! { + if let Some((cond, body, Some(else_body))) = higher::if_block(&expr); + if let ExprKind::Block(block, _) = body.kind; + if block.stmts.is_empty(); + if let Some(if_body_expr) = block.expr; + if let ExprKind::Block(else_block, _) = else_body.kind; + if else_block.stmts.is_empty(); + if let Some(else_body_expr) = else_block.expr; + if let Some((if_expr_positive, body)) = are_negated(cx, if_body_expr, else_body_expr); + then { + let positive_abs_sugg = ( + "manual implementation of `abs` method", + format!("{}.abs()", Sugg::hir(cx, body, "..")), + ); + let negative_abs_sugg = ( + "manual implementation of negation of `abs` method", + format!("-{}.abs()", Sugg::hir(cx, body, "..")), + ); + let sugg = if is_testing_positive(cx, cond, body) { + if if_expr_positive { + positive_abs_sugg + } else { + negative_abs_sugg + } + } else if is_testing_negative(cx, cond, body) { + if if_expr_positive { + negative_abs_sugg + } else { + positive_abs_sugg + } + } else { + return; + }; + span_lint_and_sugg( + cx, + SUBOPTIMAL_FLOPS, + expr.span, + sugg.0, + "try", + sugg.1, + Applicability::MachineApplicable, + ); + } + } +} + impl<'a, 'tcx> LateLintPass<'a, 'tcx> for FloatingPointArithmetic { fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_>) { if let ExprKind::MethodCall(ref path, _, args) = &expr.kind { @@ -375,6 +497,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for FloatingPointArithmetic { } else { check_expm1(cx, expr); check_mul_add(cx, expr); + check_custom_abs(cx, expr); } } } diff --git a/tests/ui/floating_point_abs.fixed b/tests/ui/floating_point_abs.fixed new file mode 100644 index 000000000000..b623e4988e7d --- /dev/null +++ b/tests/ui/floating_point_abs.fixed @@ -0,0 +1,98 @@ +// run-rustfix +#![warn(clippy::suboptimal_flops)] + +struct A { + a: f64, + b: f64, +} + +fn fake_abs1(num: f64) -> f64 { + num.abs() +} + +fn fake_abs2(num: f64) -> f64 { + num.abs() +} + +fn fake_abs3(a: A) -> f64 { + a.a.abs() +} + +fn fake_abs4(num: f64) -> f64 { + num.abs() +} + +fn fake_abs5(a: A) -> f64 { + a.a.abs() +} + +fn fake_nabs1(num: f64) -> f64 { + -num.abs() +} + +fn fake_nabs2(num: f64) -> f64 { + -num.abs() +} + +fn fake_nabs3(a: A) -> A { + A { + a: -a.a.abs(), + b: a.b, + } +} + +fn not_fake_abs1(num: f64) -> f64 { + if num > 0.0 { + num + } else { + -num - 1f64 + } +} + +fn not_fake_abs2(num: f64) -> f64 { + if num > 0.0 { + num + 1.0 + } else { + -(num + 1.0) + } +} + +fn not_fake_abs3(num1: f64, num2: f64) -> f64 { + if num1 > 0.0 { + num2 + } else { + -num2 + } +} + +fn not_fake_abs4(a: A) -> f64 { + if a.a > 0.0 { + a.b + } else { + -a.b + } +} + +fn not_fake_abs5(a: A) -> f64 { + if a.a > 0.0 { + a.a + } else { + -a.b + } +} + +fn main() { + fake_abs1(5.0); + fake_abs2(5.0); + fake_abs3(A { a: 5.0, b: 5.0 }); + fake_abs4(5.0); + fake_abs5(A { a: 5.0, b: 5.0 }); + fake_nabs1(5.0); + fake_nabs2(5.0); + fake_nabs3(A { a: 5.0, b: 5.0 }); + not_fake_abs1(5.0); + not_fake_abs2(5.0); + not_fake_abs3(5.0, 5.0); + not_fake_abs4(A { a: 5.0, b: 5.0 }); + not_fake_abs5(A { a: 5.0, b: 5.0 }); +} diff --git a/tests/ui/floating_point_abs.rs b/tests/ui/floating_point_abs.rs new file mode 100644 index 000000000000..cbf9c94e41e6 --- /dev/null +++ b/tests/ui/floating_point_abs.rs @@ -0,0 +1,126 @@ +// run-rustfix +#![warn(clippy::suboptimal_flops)] + +struct A { + a: f64, + b: f64, +} + +fn fake_abs1(num: f64) -> f64 { + if num >= 0.0 { + num + } else { + -num + } +} + +fn fake_abs2(num: f64) -> f64 { + if 0.0 < num { + num + } else { + -num + } +} + +fn fake_abs3(a: A) -> f64 { + if a.a > 0.0 { + a.a + } else { + -a.a + } +} + +fn fake_abs4(num: f64) -> f64 { + if 0.0 >= num { + -num + } else { + num + } +} + +fn fake_abs5(a: A) -> f64 { + if a.a < 0.0 { + -a.a + } else { + a.a + } +} + +fn fake_nabs1(num: f64) -> f64 { + if num < 0.0 { + num + } else { + -num + } +} + +fn fake_nabs2(num: f64) -> f64 { + if 0.0 >= num { + num + } else { + -num + } +} + +fn fake_nabs3(a: A) -> A { + A { + a: if a.a >= 0.0 { -a.a } else { a.a }, + b: a.b, + } +} + +fn not_fake_abs1(num: f64) -> f64 { + if num > 0.0 { + num + } else { + -num - 1f64 + } +} + +fn not_fake_abs2(num: f64) -> f64 { + if num > 0.0 { + num + 1.0 + } else { + -(num + 1.0) + } +} + +fn not_fake_abs3(num1: f64, num2: f64) -> f64 { + if num1 > 0.0 { + num2 + } else { + -num2 + } +} + +fn not_fake_abs4(a: A) -> f64 { + if a.a > 0.0 { + a.b + } else { + -a.b + } +} + +fn not_fake_abs5(a: A) -> f64 { + if a.a > 0.0 { + a.a + } else { + -a.b + } +} + +fn main() { + fake_abs1(5.0); + fake_abs2(5.0); + fake_abs3(A { a: 5.0, b: 5.0 }); + fake_abs4(5.0); + fake_abs5(A { a: 5.0, b: 5.0 }); + fake_nabs1(5.0); + fake_nabs2(5.0); + fake_nabs3(A { a: 5.0, b: 5.0 }); + not_fake_abs1(5.0); + not_fake_abs2(5.0); + not_fake_abs3(5.0, 5.0); + not_fake_abs4(A { a: 5.0, b: 5.0 }); + not_fake_abs5(A { a: 5.0, b: 5.0 }); +} diff --git a/tests/ui/floating_point_abs.stderr b/tests/ui/floating_point_abs.stderr new file mode 100644 index 000000000000..74a71f2ca7c5 --- /dev/null +++ b/tests/ui/floating_point_abs.stderr @@ -0,0 +1,80 @@ +error: manual implementation of `abs` method + --> $DIR/floating_point_abs.rs:10:5 + | +LL | / if num >= 0.0 { +LL | | num +LL | | } else { +LL | | -num +LL | | } + | |_____^ help: try: `num.abs()` + | + = note: `-D clippy::suboptimal-flops` implied by `-D warnings` + +error: manual implementation of `abs` method + --> $DIR/floating_point_abs.rs:18:5 + | +LL | / if 0.0 < num { +LL | | num +LL | | } else { +LL | | -num +LL | | } + | |_____^ help: try: `num.abs()` + +error: manual implementation of `abs` method + --> $DIR/floating_point_abs.rs:26:5 + | +LL | / if a.a > 0.0 { +LL | | a.a +LL | | } else { +LL | | -a.a +LL | | } + | |_____^ help: try: `a.a.abs()` + +error: manual implementation of `abs` method + --> $DIR/floating_point_abs.rs:34:5 + | +LL | / if 0.0 >= num { +LL | | -num +LL | | } else { +LL | | num +LL | | } + | |_____^ help: try: `num.abs()` + +error: manual implementation of `abs` method + --> $DIR/floating_point_abs.rs:42:5 + | +LL | / if a.a < 0.0 { +LL | | -a.a +LL | | } else { +LL | | a.a +LL | | } + | |_____^ help: try: `a.a.abs()` + +error: manual implementation of negation of `abs` method + --> $DIR/floating_point_abs.rs:50:5 + | +LL | / if num < 0.0 { +LL | | num +LL | | } else { +LL | | -num +LL | | } + | |_____^ help: try: `-num.abs()` + +error: manual implementation of negation of `abs` method + --> $DIR/floating_point_abs.rs:58:5 + | +LL | / if 0.0 >= num { +LL | | num +LL | | } else { +LL | | -num +LL | | } + | |_____^ help: try: `-num.abs()` + +error: manual implementation of negation of `abs` method + --> $DIR/floating_point_abs.rs:67:12 + | +LL | a: if a.a >= 0.0 { -a.a } else { a.a }, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `-a.a.abs()` + +error: aborting due to 8 previous errors +