diff --git a/examples/README.md b/examples/README.md index 8e1a78444..9ee50fec5 100644 --- a/examples/README.md +++ b/examples/README.md @@ -18,6 +18,7 @@ The example libraries are separated into the following three categories: | [`incorrect_matches_operation`](./general/incorrect_matches_operation) | Incorrect operators used with matches! macros | | [`non_local_effect_before_error_return`](./general/non_local_effect_before_error_return) | Non-local effects before return of an error | | [`non_thread_safe_call_in_test`](./general/non_thread_safe_call_in_test) | Non-thread-safe function calls in tests | +| [`suspicious_saturating_op`](./general/suspicious_saturating_op) | Consecutive saturating operations | | [`wrong_serialize_struct_arg`](./general/wrong_serialize_struct_arg) | Calls to `serialize_struct` with incorrect `len` arguments | ## Supplementary diff --git a/examples/general/Cargo.lock b/examples/general/Cargo.lock index 3bfe5f72e..a3e31c0be 100644 --- a/examples/general/Cargo.lock +++ b/examples/general/Cargo.lock @@ -436,6 +436,7 @@ dependencies = [ "incorrect_matches_operation", "non_local_effect_before_error_return", "non_thread_safe_call_in_test", + "suspicious_saturating_op", "wrong_serialize_struct_arg", ] @@ -1024,6 +1025,16 @@ dependencies = [ "digest", ] +[[package]] +name = "suspicious_saturating_op" +version = "2.6.0" +dependencies = [ + "clippy_utils", + "dylint_linting", + "dylint_testing", + "if_chain", +] + [[package]] name = "syn" version = "2.0.48" diff --git a/examples/general/Cargo.toml b/examples/general/Cargo.toml index 59861a8d2..3ff1067d5 100644 --- a/examples/general/Cargo.toml +++ b/examples/general/Cargo.toml @@ -24,6 +24,9 @@ non_local_effect_before_error_return = { path = "non_local_effect_before_error_r non_thread_safe_call_in_test = { path = "non_thread_safe_call_in_test", features = [ "rlib", ] } +suspicious_saturating_op = { path = "suspicious_saturating_op", features = [ + "rlib", +] } wrong_serialize_struct_arg = { path = "wrong_serialize_struct_arg", features = [ "rlib", ] } diff --git a/examples/general/suspicious_saturating_op/Cargo.toml b/examples/general/suspicious_saturating_op/Cargo.toml new file mode 100644 index 000000000..3c81b8348 --- /dev/null +++ b/examples/general/suspicious_saturating_op/Cargo.toml @@ -0,0 +1,25 @@ +[package] +name = "suspicious_saturating_op" +version = "2.6.0" +authors = ["Samuel E. Moelius III "] +description = "A lint to check for consecutive saturating operations" +edition = "2021" +publish = false + +[lib] +crate-type = ["cdylib", "rlib"] + +[dependencies] +clippy_utils = { workspace = true } +if_chain = "1.0" + +dylint_linting = { path = "../../../utils/linting" } + +[dev-dependencies] +dylint_testing = { path = "../../../utils/testing" } + +[features] +rlib = ["dylint_linting/constituent"] + +[package.metadata.rust-analyzer] +rustc_private = true diff --git a/examples/general/suspicious_saturating_op/README.md b/examples/general/suspicious_saturating_op/README.md new file mode 100644 index 000000000..b717a9bc4 --- /dev/null +++ b/examples/general/suspicious_saturating_op/README.md @@ -0,0 +1,17 @@ +# suspicious_saturating_op + +### What it does +Checks for consecutive saturating operations. + +### Why is this bad? +If the first operation saturates, the second operation may produce an incorrect result. + +### Example +```rust +x = x.saturating_add(y).saturating_sub(z); +``` +Use instead: +```rust +x = x.checked_add(y)?; +x = x.checked_sub(z)?; +``` diff --git a/examples/general/suspicious_saturating_op/src/lib.rs b/examples/general/suspicious_saturating_op/src/lib.rs new file mode 100644 index 000000000..8290d29ea --- /dev/null +++ b/examples/general/suspicious_saturating_op/src/lib.rs @@ -0,0 +1,119 @@ +#![feature(rustc_private)] +#![recursion_limit = "256"] +#![warn(unused_extern_crates)] + +extern crate rustc_hir; +extern crate rustc_middle; +extern crate rustc_span; + +use clippy_utils::diagnostics::span_lint; +use if_chain::if_chain; +use rustc_hir::{Expr, ExprKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty; +use rustc_span::symbol::Ident; + +dylint_linting::declare_late_lint! { + /// ### What it does + /// Checks for consecutive saturating operations. + /// + /// ### Why is this bad? + /// If the first operation saturates, the second operation may produce an incorrect result. + /// + /// ### Example + /// ```rust + /// # fn foo() -> Option { + /// # let mut x: u64 = 1; + /// # let y: u64 = 1; + /// # let z: u64 = 1; + /// x = x.saturating_add(y).saturating_sub(z); + /// # None + /// # } + /// ``` + /// Use instead: + /// ```rust + /// # fn foo() -> Option { + /// # let mut x: u64 = 1; + /// # let y: u64 = 1; + /// # let z: u64 = 1; + /// x = x.checked_add(y)?; + /// x = x.checked_sub(z)?; + /// # None + /// # } + /// ``` + pub SUSPICIOUS_SATURATING_OP, + Warn, + "consecutive saturating operations" +} + +const SIGNED_OR_UNSIGNED: &[[&'static str; 2]] = &[ + ["div", "mul"], + ["div", "pow"], + ["mul", "div"], + ["pow", "div"], +]; + +const UNSIGNED_ONLY: &[[&'static str; 2]] = &[ + ["add", "div"], + ["add", "sub"], + ["sub", "add"], + ["sub", "mul"], + ["sub", "pow"], +]; + +impl<'tcx> LateLintPass<'tcx> for SuspiciousSaturatingOp { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &Expr<'_>) { + if_chain! { + if let ExprKind::MethodCall(second, receiver_1, _, _) = expr.kind; + if second.args.is_none(); + if let ExprKind::MethodCall(first, receiver_0, _, _) = receiver_1.kind; + if first.args.is_none(); + then { + for pattern in SIGNED_OR_UNSIGNED { + if check(cx, expr, &first.ident, &second.ident, pattern) { + return; + } + } + let ty = cx.typeck_results().expr_ty(receiver_0); + if matches!(ty.kind(), ty::Uint(_)) { + for pattern in UNSIGNED_ONLY { + if check(cx, expr, &first.ident, &second.ident, pattern) { + return; + } + } + } + } + } + } +} + +fn check( + cx: &LateContext<'_>, + expr: &Expr<'_>, + first: &Ident, + second: &Ident, + pattern: &[&'static str; 2], +) -> bool { + if first.as_str() == String::from("saturating_") + pattern[0] + && (second.as_str() == pattern[1] + || second.as_str().ends_with(&(String::from("_") + pattern[1]))) + { + span_lint( + cx, + SUSPICIOUS_SATURATING_OP, + expr.span, + &format!("suspicious use of `{}` followed by `{}`", first, second), + ); + true + } else { + false + } +} + +#[test] +fn ui() { + dylint_testing::ui_test( + env!("CARGO_PKG_NAME"), + &std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("ui"), + ); +} diff --git a/examples/general/suspicious_saturating_op/ui/main.rs b/examples/general/suspicious_saturating_op/ui/main.rs new file mode 100644 index 000000000..9e12ae785 --- /dev/null +++ b/examples/general/suspicious_saturating_op/ui/main.rs @@ -0,0 +1,7 @@ +fn main() { + let mut x: u64 = 1; + let y: u64 = 1; + let z: u64 = 1; + x = x.saturating_add(y).saturating_sub(z); + println!("{}", x); +} diff --git a/examples/general/suspicious_saturating_op/ui/main.stderr b/examples/general/suspicious_saturating_op/ui/main.stderr new file mode 100644 index 000000000..79aa4ebd7 --- /dev/null +++ b/examples/general/suspicious_saturating_op/ui/main.stderr @@ -0,0 +1,11 @@ +error: suspicious use of `saturating_add` followed by `saturating_sub` + --> $DIR/main.rs:5:9 + | +LL | x = x.saturating_add(y).saturating_sub(z); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D suspicious-saturating-op` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(suspicious_saturating_op)]` + +error: aborting due to 1 previous error +