Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added a lint which corrects expressions like (a - b) < f32::EPSILON, according to #5819 #5952

Merged
merged 2 commits into from
Aug 25, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -1498,6 +1498,7 @@ Released 2018-09-13
[`float_arithmetic`]: https://rust-lang.github.io/rust-clippy/master/index.html#float_arithmetic
[`float_cmp`]: https://rust-lang.github.io/rust-clippy/master/index.html#float_cmp
[`float_cmp_const`]: https://rust-lang.github.io/rust-clippy/master/index.html#float_cmp_const
[`float_equality_without_abs`]: https://rust-lang.github.io/rust-clippy/master/index.html#float_equality_without_abs
[`fn_address_comparisons`]: https://rust-lang.github.io/rust-clippy/master/index.html#fn_address_comparisons
[`fn_params_excessive_bools`]: https://rust-lang.github.io/rust-clippy/master/index.html#fn_params_excessive_bools
[`fn_to_numeric_cast`]: https://rust-lang.github.io/rust-clippy/master/index.html#fn_to_numeric_cast
Expand Down
112 changes: 112 additions & 0 deletions clippy_lints/src/float_equality_without_abs.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
use crate::utils::{match_qpath, paths, snippet, span_lint_and_sugg};
use if_chain::if_chain;
use rustc_errors::Applicability;
use rustc_hir::{BinOpKind, Expr, ExprKind};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::ty;
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::source_map::Spanned;

declare_clippy_lint! {
/// **What it does:** Checks for statements of the form `(a - b) < f32::EPSILON` or
/// `(a - b) < f64::EPSILON`. Notes the missing `.abs()`.
///
/// **Why is this bad?** The code without `.abs()` is more likely to have a bug.
///
/// **Known problems:** If the user can ensure that b is larger than a, the `.abs()` is
/// technically unneccessary. However, it will make the code more robust and doesn't have any
/// large performance implications. If the abs call was deliberately left out for performance
/// reasons, it is probably better to state this explicitly in the code, which then can be done
/// with an allow.
///
/// **Example:**
///
/// ```rust
/// pub fn is_roughly_equal(a: f32, b: f32) -> bool {
/// (a - b) < f32::EPSILON
/// }
/// ```
/// Use instead:
/// ```rust
/// pub fn is_roughly_equal(a: f32, b: f32) -> bool {
/// (a - b).abs() < f32::EPSILON
/// }
/// ```
pub FLOAT_EQUALITY_WITHOUT_ABS,
correctness,
"float equality check without `.abs()`"
}

declare_lint_pass!(FloatEqualityWithoutAbs => [FLOAT_EQUALITY_WITHOUT_ABS]);

impl<'tcx> LateLintPass<'tcx> for FloatEqualityWithoutAbs {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
let lhs;
let rhs;

// check if expr is a binary expression with a lt or gt operator
if let ExprKind::Binary(op, ref left, ref right) = expr.kind {
match op.node {
BinOpKind::Lt => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be done as if let ExprKind::Binary(Lt | Rt, left, right) ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if I get this wrong, but I still need to differentiate between BinOpKind::Lt and BinOpKind::Gt. If I would use something like
if let ExprKind::Binary( Spanned { node: BinOpKind::Lt | BinOpKind::Gt, .. }, left, right, ) = ...
Then I would still need to sort out whether my Operator is < or >. Or am I wrong?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh sorry, you're absolutely right

lhs = left;
rhs = right;
},
BinOpKind::Gt => {
lhs = right;
rhs = left;
},
_ => return,
};
} else {
return;
}

if_chain! {

// left hand side is a substraction
if let ExprKind::Binary(
Spanned {
node: BinOpKind::Sub,
..
},
val_l,
val_r,
) = lhs.kind;

// right hand side matches either f32::EPSILON or f64::EPSILON
if let ExprKind::Path(ref epsilon_path) = rhs.kind;
if match_qpath(epsilon_path, &paths::F32_EPSILON) || match_qpath(epsilon_path, &paths::F64_EPSILON);

// values of the substractions on the left hand side are of the type float
let t_val_l = cx.typeck_results().expr_ty(val_l);
let t_val_r = cx.typeck_results().expr_ty(val_r);
if let ty::Float(_) = t_val_l.kind;
if let ty::Float(_) = t_val_r.kind;

then {
// get the snippet string
let lhs_string = snippet(
cx,
lhs.span,
"(...)",
);
// format the suggestion
let suggestion = if lhs_string.starts_with('(') {
format!("{}.abs()", lhs_string)
} else {
format!("({}).abs()", lhs_string)
};
Comment on lines +87 to +98
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@1c3t3a If you want to further improve this code, you can use utils::Sugg here, that will take care about this automatically.

// spans the lint
span_lint_and_sugg(
cx,
FLOAT_EQUALITY_WITHOUT_ABS,
expr.span,
"float equality check without `.abs()`",
"add `.abs()`",
suggestion,
Applicability::MaybeIncorrect,
);
}
}
}
}
5 changes: 5 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,7 @@ mod excessive_bools;
mod exit;
mod explicit_write;
mod fallible_impl_from;
mod float_equality_without_abs;
mod float_literal;
mod floating_point_arithmetic;
mod format;
Expand Down Expand Up @@ -549,6 +550,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
&exit::EXIT,
&explicit_write::EXPLICIT_WRITE,
&fallible_impl_from::FALLIBLE_IMPL_FROM,
&float_equality_without_abs::FLOAT_EQUALITY_WITHOUT_ABS,
&float_literal::EXCESSIVE_PRECISION,
&float_literal::LOSSY_FLOAT_LITERAL,
&floating_point_arithmetic::IMPRECISE_FLOPS,
Expand Down Expand Up @@ -1093,6 +1095,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
store.register_late_pass(|| box stable_sort_primitive::StableSortPrimitive);
store.register_late_pass(|| box repeat_once::RepeatOnce);
store.register_late_pass(|| box self_assignment::SelfAssignment);
store.register_late_pass(|| box float_equality_without_abs::FloatEqualityWithoutAbs);

store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![
LintId::of(&arithmetic::FLOAT_ARITHMETIC),
Expand Down Expand Up @@ -1268,6 +1271,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&eval_order_dependence::DIVERGING_SUB_EXPRESSION),
LintId::of(&eval_order_dependence::EVAL_ORDER_DEPENDENCE),
LintId::of(&explicit_write::EXPLICIT_WRITE),
LintId::of(&float_equality_without_abs::FLOAT_EQUALITY_WITHOUT_ABS),
LintId::of(&float_literal::EXCESSIVE_PRECISION),
LintId::of(&format::USELESS_FORMAT),
LintId::of(&formatting::POSSIBLE_MISSING_COMMA),
Expand Down Expand Up @@ -1686,6 +1690,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&enum_clike::ENUM_CLIKE_UNPORTABLE_VARIANT),
LintId::of(&eq_op::EQ_OP),
LintId::of(&erasing_op::ERASING_OP),
LintId::of(&float_equality_without_abs::FLOAT_EQUALITY_WITHOUT_ABS),
LintId::of(&formatting::POSSIBLE_MISSING_COMMA),
LintId::of(&functions::NOT_UNSAFE_PTR_ARG_DEREF),
LintId::of(&if_let_mutex::IF_LET_MUTEX),
Expand Down
2 changes: 2 additions & 0 deletions clippy_lints/src/utils/paths.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ pub const DROP_TRAIT: [&str; 4] = ["core", "ops", "drop", "Drop"];
pub const DURATION: [&str; 3] = ["core", "time", "Duration"];
pub const EARLY_CONTEXT: [&str; 4] = ["rustc", "lint", "context", "EarlyContext"];
pub const EXIT: [&str; 3] = ["std", "process", "exit"];
pub const F32_EPSILON: [&str; 2] = ["f32", "EPSILON"];
pub const F64_EPSILON: [&str; 2] = ["f64", "EPSILON"];
pub const FILE: [&str; 3] = ["std", "fs", "File"];
pub const FILE_TYPE: [&str; 3] = ["std", "fs", "FileType"];
pub const FMT_ARGUMENTS_NEW_V1: [&str; 4] = ["core", "fmt", "Arguments", "new_v1"];
Expand Down
7 changes: 7 additions & 0 deletions src/lintlist/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -661,6 +661,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
deprecation: None,
module: "misc",
},
Lint {
name: "float_equality_without_abs",
group: "correctness",
desc: "float equality check without `.abs()`",
deprecation: None,
module: "float_equality_without_abs",
},
Lint {
name: "fn_address_comparisons",
group: "correctness",
Expand Down
31 changes: 31 additions & 0 deletions tests/ui/float_equality_without_abs.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
#![warn(clippy::float_equality_without_abs)]

pub fn is_roughly_equal(a: f32, b: f32) -> bool {
(a - b) < f32::EPSILON
}

pub fn main() {
// all errors
is_roughly_equal(1.0, 2.0);
let a = 0.05;
let b = 0.0500001;

let _ = (a - b) < f32::EPSILON;
let _ = a - b < f32::EPSILON;
let _ = a - b.abs() < f32::EPSILON;
let _ = (a as f64 - b as f64) < f64::EPSILON;
let _ = 1.0 - 2.0 < f32::EPSILON;

let _ = f32::EPSILON > (a - b);
let _ = f32::EPSILON > a - b;
let _ = f32::EPSILON > a - b.abs();
let _ = f64::EPSILON > (a as f64 - b as f64);
let _ = f32::EPSILON > 1.0 - 2.0;

// those are correct
let _ = (a - b).abs() < f32::EPSILON;
let _ = (a as f64 - b as f64).abs() < f64::EPSILON;

let _ = f32::EPSILON > (a - b).abs();
let _ = f64::EPSILON > (a as f64 - b as f64).abs();
}
70 changes: 70 additions & 0 deletions tests/ui/float_equality_without_abs.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
error: float equality check without `.abs()`
--> $DIR/float_equality_without_abs.rs:4:5
|
LL | (a - b) < f32::EPSILON
| ^^^^^^^^^^^^^^^^^^^^^^ help: add `.abs()`: `(a - b).abs()`
|
= note: `-D clippy::float-equality-without-abs` implied by `-D warnings`

error: float equality check without `.abs()`
--> $DIR/float_equality_without_abs.rs:13:13
|
LL | let _ = (a - b) < f32::EPSILON;
| ^^^^^^^^^^^^^^^^^^^^^^ help: add `.abs()`: `(a - b).abs()`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This suggestion doesn't work. If applied, this will transform the code like this:

let _ = (a - b) < f32::EPSILON; // before
let _ = (a - b).abs(); // after

A better way to do this would be to use span_lint_and_then and then use the db.span_suggestion to only add the above suggestion to the span of (a - b).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh you're right! Thank you @flip1995 and sorry that I didn't see that earlier! I've corrected it on my own fork. Should I open a new PR for this fix? I've also started to use utils::Sugg. But I had some issues while parsing the expression, the simple way of

let suggestion = format!("{}.abs()", sugg::Sugg::hir(cx, &lhs, "..").maybe_par());

always brought up doubled parentheses. E.g. (a - b) -> ((a - b)). That's why I took a longer way and assembled the suggestion by myself. Maybe you could have a look on the code, it's all in this commit here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The linked commit looks good to me.

Yes please open a new PR about this!


error: float equality check without `.abs()`
--> $DIR/float_equality_without_abs.rs:14:13
|
LL | let _ = a - b < f32::EPSILON;
| ^^^^^^^^^^^^^^^^^^^^ help: add `.abs()`: `(a - b).abs()`

error: float equality check without `.abs()`
--> $DIR/float_equality_without_abs.rs:15:13
|
LL | let _ = a - b.abs() < f32::EPSILON;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: add `.abs()`: `(a - b.abs()).abs()`

error: float equality check without `.abs()`
--> $DIR/float_equality_without_abs.rs:16:13
|
LL | let _ = (a as f64 - b as f64) < f64::EPSILON;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: add `.abs()`: `(a as f64 - b as f64).abs()`

error: float equality check without `.abs()`
--> $DIR/float_equality_without_abs.rs:17:13
|
LL | let _ = 1.0 - 2.0 < f32::EPSILON;
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: add `.abs()`: `(1.0 - 2.0).abs()`

error: float equality check without `.abs()`
--> $DIR/float_equality_without_abs.rs:19:13
|
LL | let _ = f32::EPSILON > (a - b);
| ^^^^^^^^^^^^^^^^^^^^^^ help: add `.abs()`: `(a - b).abs()`

error: float equality check without `.abs()`
--> $DIR/float_equality_without_abs.rs:20:13
|
LL | let _ = f32::EPSILON > a - b;
| ^^^^^^^^^^^^^^^^^^^^ help: add `.abs()`: `(a - b).abs()`

error: float equality check without `.abs()`
--> $DIR/float_equality_without_abs.rs:21:13
|
LL | let _ = f32::EPSILON > a - b.abs();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: add `.abs()`: `(a - b.abs()).abs()`

error: float equality check without `.abs()`
--> $DIR/float_equality_without_abs.rs:22:13
|
LL | let _ = f64::EPSILON > (a as f64 - b as f64);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: add `.abs()`: `(a as f64 - b as f64).abs()`

error: float equality check without `.abs()`
--> $DIR/float_equality_without_abs.rs:23:13
|
LL | let _ = f32::EPSILON > 1.0 - 2.0;
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: add `.abs()`: `(1.0 - 2.0).abs()`

error: aborting due to 11 previous errors