diff --git a/CHANGELOG.md b/CHANGELOG.md index 37d46d349667..db8590b44aee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6106,6 +6106,7 @@ Released 2018-09-13 [`almost_complete_letter_range`]: https://rust-lang.github.io/rust-clippy/master/index.html#almost_complete_letter_range [`almost_complete_range`]: https://rust-lang.github.io/rust-clippy/master/index.html#almost_complete_range [`almost_swapped`]: https://rust-lang.github.io/rust-clippy/master/index.html#almost_swapped +[`always_true_conditions`]: https://rust-lang.github.io/rust-clippy/master/index.html#always_true_conditions [`approx_constant`]: https://rust-lang.github.io/rust-clippy/master/index.html#approx_constant [`arbitrary_source_item_ordering`]: https://rust-lang.github.io/rust-clippy/master/index.html#arbitrary_source_item_ordering [`arc_with_non_send_sync`]: https://rust-lang.github.io/rust-clippy/master/index.html#arc_with_non_send_sync diff --git a/clippy_lints/src/always_true_conditions.rs b/clippy_lints/src/always_true_conditions.rs new file mode 100644 index 000000000000..9aa6aa45be05 --- /dev/null +++ b/clippy_lints/src/always_true_conditions.rs @@ -0,0 +1,84 @@ +use clippy_utils::diagnostics::span_lint; +use rustc_hir::def::Res; +use rustc_hir::{BinOpKind, Expr, ExprKind, QPath}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::declare_lint_pass; + +declare_clippy_lint! { + /// ### What it does + /// + /// Flags a relativly common error where users comparing a varible to a primative use || instal of && in conjunction + /// with !=. This lint was originally meant for simple n != 1 || n != 2 type statements, but the lint will detect + /// the primitive and varible in any order for any length, as long as the variable stays the same, and the condition + /// is always 1 primitive and 1 varible. + /// + /// ### Why is this bad? + /// + ///This is bad because the code will always result in true. If this is intentional a constant can be used in the + ///case of a boolean varibale assignment, or code in an if block should just be moved outside with comments + ///explaining why. + /// + /// ### Example + /// ```no_run + /// let foo = "anything"; + /// if foo != "thing1" || foo != "thing2" { + /// println!("always executes"); + /// } + /// ``` + /// Use instead: + /// ```no_run + /// let foo = "anything"; + /// if foo != "thing1" && foo != "thing2" { + /// println!("sometimes executes"); + /// } + /// ``` + #[clippy::version = "1.87.0"] + pub ALWAYS_TRUE_CONDITIONS, + nursery, + "checks for if statement conditions which are always true" +} + +declare_lint_pass!(AlwaysTrueConditions => [ALWAYS_TRUE_CONDITIONS]); + +fn context_applicable(expr: &Expr<'_>) -> Option { + if let ExprKind::Binary(new_op, new_f, new_l) = expr.kind { + if new_op.node == BinOpKind::Or { + let f = context_applicable(new_f); + let l = context_applicable(new_l); + if l == f { l } else { None } + } else if new_op.node == BinOpKind::Ne { + normalize_expression(new_f, new_l) + } else { + None + } + } else { + None + } +} + +fn normalize_expression<'a>(f: &'a Expr<'a>, l: &'a Expr<'a>) -> Option { + if let (ExprKind::Path(QPath::Resolved(_, path)), ExprKind::Lit(_)) = (f.kind, l.kind) { + Some(path.res) + } else if let (ExprKind::Lit(_), ExprKind::Path(QPath::Resolved(_, path))) = (f.kind, l.kind) { + Some(path.res) + } else { + None + } +} + +impl LateLintPass<'_> for AlwaysTrueConditions { + fn check_expr(&mut self, cx: &LateContext<'_>, e: &Expr<'_>) { + if let ExprKind::Binary(f_op_kind, f_cond, l_cond) = e.kind + && let BinOpKind::Or = f_op_kind.node + { + let msg = "expression will always be true, did you mean to use &&?"; + + let f_res = context_applicable(f_cond); + let l_res = context_applicable(l_cond); + + if f_res.is_some() && (l_res == f_res) { + span_lint(cx, ALWAYS_TRUE_CONDITIONS, e.span, msg); + } + } + } +} diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 375d179681da..25b86d8589a6 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -5,6 +5,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::absolute_paths::ABSOLUTE_PATHS_INFO, crate::almost_complete_range::ALMOST_COMPLETE_RANGE_INFO, + crate::always_true_conditions::ALWAYS_TRUE_CONDITIONS_INFO, crate::approx_const::APPROX_CONSTANT_INFO, crate::arbitrary_source_item_ordering::ARBITRARY_SOURCE_ITEM_ORDERING_INFO, crate::arc_with_non_send_sync::ARC_WITH_NON_SEND_SYNC_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index a4ad9424b3eb..ef8419d2e4c9 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -65,6 +65,7 @@ pub mod deprecated_lints; // begin lints modules, do not remove this comment, it's used in `update_lints` mod absolute_paths; mod almost_complete_range; +mod always_true_conditions; mod approx_const; mod arbitrary_source_item_ordering; mod arc_with_non_send_sync; @@ -829,5 +830,6 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co store.register_late_pass(|_| Box::new(toplevel_ref_arg::ToplevelRefArg)); store.register_late_pass(|_| Box::new(volatile_composites::VolatileComposites)); store.register_late_pass(|_| Box::new(replace_box::ReplaceBox)); + store.register_late_pass(move |_| Box::new(always_true_conditions::AlwaysTrueConditions)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/tests/ui/always_true_conditions.rs b/tests/ui/always_true_conditions.rs new file mode 100644 index 000000000000..a422d426283a --- /dev/null +++ b/tests/ui/always_true_conditions.rs @@ -0,0 +1,66 @@ +#![warn(clippy::always_true_conditions)] + +fn foo_m(name: &str) { + if name != "Min" && name != "Max" { + todo!() + } else { + todo!() + } +} + +fn foo_s(name: &str) { + if name != "Min" || name != "Max" { + //~^ always_true_conditions + todo!() + } + if name != "Min" && name != "Max" { + todo!() + } +} + +fn catch_or_failure(input: &str) { + let b = true; + if b || input != "foo" { + todo!() + } +} + +fn catch_scope_or_failures(input: &str) { + let b = true; + { if b || input != "foo" {} } +} + +fn catch_eq_failures() { + let res = "test"; + if res == "foo" || res == "bar" { + todo!() + } +} + +fn catch_diff_var_failure(input: &str) { + let b = "value"; + if b != "bar" || input != "foo" { + todo!() + } +} + +fn catch_yoda_notation(input: &str) { + let b = 2; + if 3 != b || 5 != b { + //~^ always_true_conditions + todo!() + } + + if b != 3 || 5 != b { + //~^ always_true_conditions + todo!() + } +} + +fn non_if_funcitonality_tests(input: &str) { + let x: bool = ("a" != input || "b" != input); + //~^ always_true_conditions + let y: bool = ("a" == input || "b" != input); + let z: bool = (input != "2" || input != "4"); + //~^ always_true_conditions +} diff --git a/tests/ui/always_true_conditions.stderr b/tests/ui/always_true_conditions.stderr new file mode 100644 index 000000000000..b50fd3451912 --- /dev/null +++ b/tests/ui/always_true_conditions.stderr @@ -0,0 +1,35 @@ +error: expression will always be true, did you mean to use &&? + --> tests/ui/always_true_conditions.rs:12:8 + | +LL | if name != "Min" || name != "Max" { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::always-true-conditions` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::always_true_conditions)]` + +error: expression will always be true, did you mean to use &&? + --> tests/ui/always_true_conditions.rs:49:8 + | +LL | if 3 != b || 5 != b { + | ^^^^^^^^^^^^^^^^ + +error: expression will always be true, did you mean to use &&? + --> tests/ui/always_true_conditions.rs:54:8 + | +LL | if b != 3 || 5 != b { + | ^^^^^^^^^^^^^^^^ + +error: expression will always be true, did you mean to use &&? + --> tests/ui/always_true_conditions.rs:61:19 + | +LL | let x: bool = ("a" != input || "b" != input); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: expression will always be true, did you mean to use &&? + --> tests/ui/always_true_conditions.rs:64:19 + | +LL | let z: bool = (input != "2" || input != "4"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 5 previous errors +