Auto merge of #8754 - guerinoni:no_effect_replace, r=llogiq

New lint `no_effect_replace`

Closes #1595

Signed-off-by: Federico Guerinoni <[email protected]>

changelog: Add [`no_effect_replace`] lint.

Author: bors

CHANGELOG.md

@@ -3613,6 +3613,7 @@ Released 2018-09-13
 [`new_without_default`]: https://rust-lang.github.io/rust-clippy/master/index.html#new_without_default
 [`new_without_default_derive`]: https://rust-lang.github.io/rust-clippy/master/index.html#new_without_default_derive
 [`no_effect`]: https://rust-lang.github.io/rust-clippy/master/index.html#no_effect
+[`no_effect_replace`]: https://rust-lang.github.io/rust-clippy/master/index.html#no_effect_replace
 [`no_effect_underscore_binding`]: https://rust-lang.github.io/rust-clippy/master/index.html#no_effect_underscore_binding
 [`non_ascii_literal`]: https://rust-lang.github.io/rust-clippy/master/index.html#non_ascii_literal
 [`non_octal_unix_permissions`]: https://rust-lang.github.io/rust-clippy/master/index.html#non_octal_unix_permissions

clippy_lints/src/lib.register_all.rs

@@ -189,6 +189,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
     LintId::of(methods::NEEDLESS_OPTION_TAKE),
     LintId::of(methods::NEEDLESS_SPLITN),
     LintId::of(methods::NEW_RET_NO_SELF),
+    LintId::of(methods::NO_EFFECT_REPLACE),
     LintId::of(methods::OK_EXPECT),
     LintId::of(methods::OPTION_AS_REF_DEREF),
     LintId::of(methods::OPTION_FILTER_MAP),

clippy_lints/src/lib.register_lints.rs

@@ -330,6 +330,7 @@ store.register_lints(&[
     methods::NEEDLESS_OPTION_TAKE,
     methods::NEEDLESS_SPLITN,
     methods::NEW_RET_NO_SELF,
+    methods::NO_EFFECT_REPLACE,
     methods::OK_EXPECT,
     methods::OPTION_AS_REF_DEREF,
     methods::OPTION_FILTER_MAP,

clippy_lints/src/lib.register_suspicious.rs

@@ -23,6 +23,7 @@ store.register_group(true, "clippy::suspicious", Some("clippy_suspicious"), vec!
     LintId::of(loops::EMPTY_LOOP),
     LintId::of(loops::FOR_LOOPS_OVER_FALLIBLES),
     LintId::of(loops::MUT_RANGE_BOUND),
+    LintId::of(methods::NO_EFFECT_REPLACE),
     LintId::of(methods::SUSPICIOUS_MAP),
     LintId::of(mut_key::MUTABLE_KEY_TYPE),
     LintId::of(octal_escapes::OCTAL_ESCAPES),

clippy_lints/src/methods/mod.rs

@@ -44,6 +44,7 @@ mod map_identity;
 mod map_unwrap_or;
 mod needless_option_as_deref;
 mod needless_option_take;
+mod no_effect_replace;
 mod ok_expect;
 mod option_as_ref_deref;
 mod option_map_or_none;
@@ -2195,6 +2196,24 @@ declare_clippy_lint! {
     "using `.as_ref().take()` on a temporary value"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for `replace` statements which have no effect.
+    ///
+    /// ### Why is this bad?
+    /// It's either a mistake or confusing.
+    ///
+    /// ### Example
+    /// ```rust
+    /// "1234".replace("12", "12");
+    /// "1234".replacen("12", "12", 1);
+    /// ```
+    #[clippy::version = "1.62.0"]
+    pub NO_EFFECT_REPLACE,
+    suspicious,
+    "replace with no effect"
+}
+
 pub struct Methods {
     avoid_breaking_exported_api: bool,
     msrv: Option<RustcVersion>,
@@ -2294,6 +2313,7 @@ impl_lint_pass!(Methods => [
     NEEDLESS_OPTION_AS_DEREF,
     IS_DIGIT_ASCII_RADIX,
     NEEDLESS_OPTION_TAKE,
+    NO_EFFECT_REPLACE,
 ]);
 
 /// Extracts a method call name, args, and `Span` of the method name.
@@ -2705,6 +2725,9 @@ impl Methods {
                         unnecessary_lazy_eval::check(cx, expr, recv, u_arg, "unwrap_or");
                     },
                 },
+                ("replace" | "replacen", [arg1, arg2] | [arg1, arg2, _]) => {
+                    no_effect_replace::check(cx, expr, arg1, arg2);
+                },
                 _ => {},
             }
         }

clippy_lints/src/methods/no_effect_replace.rs

@@ -0,0 +1,47 @@
+use clippy_utils::diagnostics::span_lint;
+use clippy_utils::ty::is_type_diagnostic_item;
+use clippy_utils::SpanlessEq;
+use if_chain::if_chain;
+use rustc_ast::LitKind;
+use rustc_hir::ExprKind;
+use rustc_lint::LateContext;
+use rustc_span::sym;
+
+use super::NO_EFFECT_REPLACE;
+
+pub(super) fn check<'tcx>(
+    cx: &LateContext<'tcx>,
+    expr: &'tcx rustc_hir::Expr<'_>,
+    arg1: &'tcx rustc_hir::Expr<'_>,
+    arg2: &'tcx rustc_hir::Expr<'_>,
+) {
+    let ty = cx.typeck_results().expr_ty(expr).peel_refs();
+    if !(ty.is_str() || is_type_diagnostic_item(cx, ty, sym::String)) {
+        return;
+    }
+
+    if_chain! {
+        if let ExprKind::Lit(spanned) = &arg1.kind;
+        if let Some(param1) = lit_string_value(&spanned.node);
+
+        if let ExprKind::Lit(spanned) = &arg2.kind;
+        if let LitKind::Str(param2, _) = &spanned.node;
+        if param1 == param2.as_str();
+
+        then {
+            span_lint(cx, NO_EFFECT_REPLACE, expr.span, "replacing text with itself");
+        }
+    }
+
+    if SpanlessEq::new(cx).eq_expr(arg1, arg2) {
+        span_lint(cx, NO_EFFECT_REPLACE, expr.span, "replacing text with itself");
+    }
+}
+
+fn lit_string_value(node: &LitKind) -> Option<String> {
+    match node {
+        LitKind::Char(value) => Some(value.to_string()),
+        LitKind::Str(value, _) => Some(value.as_str().to_owned()),
+        _ => None,
+    }
+}

tests/ui/no_effect_replace.rs

@@ -0,0 +1,51 @@
+#![warn(clippy::no_effect_replace)]
+
+fn main() {
+    let _ = "12345".replace('1', "1");
+    let _ = "12345".replace("12", "12");
+    let _ = String::new().replace("12", "12");
+
+    let _ = "12345".replacen('1', "1", 1);
+    let _ = "12345".replacen("12", "12", 1);
+    let _ = String::new().replacen("12", "12", 1);
+
+    let _ = "12345".replace("12", "22");
+    let _ = "12345".replacen("12", "22", 1);
+
+    let mut x = X::default();
+    let _ = "hello".replace(&x.f(), &x.f());
+    let _ = "hello".replace(&x.f(), &x.ff());
+
+    let _ = "hello".replace(&y(), &y());
+    let _ = "hello".replace(&y(), &z());
+
+    let _ = Replaceme.replace("a", "a");
+}
+
+#[derive(Default)]
+struct X {}
+
+impl X {
+    fn f(&mut self) -> String {
+        "he".to_string()
+    }
+
+    fn ff(&mut self) -> String {
+        "hh".to_string()
+    }
+}
+
+fn y() -> String {
+    "he".to_string()
+}
+
+fn z() -> String {
+    "hh".to_string()
+}
+
+struct Replaceme;
+impl Replaceme {
+    pub fn replace(&mut self, a: &str, b: &str) -> Self {
+        Self
+    }
+}

tests/ui/no_effect_replace.stderr

@@ -0,0 +1,52 @@
+error: replacing text with itself
+  --> $DIR/no_effect_replace.rs:4:13
+   |
+LL |     let _ = "12345".replace('1', "1");
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: `-D clippy::no-effect-replace` implied by `-D warnings`
+
+error: replacing text with itself
+  --> $DIR/no_effect_replace.rs:5:13
+   |
+LL |     let _ = "12345".replace("12", "12");
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: replacing text with itself
+  --> $DIR/no_effect_replace.rs:6:13
+   |
+LL |     let _ = String::new().replace("12", "12");
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: replacing text with itself
+  --> $DIR/no_effect_replace.rs:8:13
+   |
+LL |     let _ = "12345".replacen('1', "1", 1);
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: replacing text with itself
+  --> $DIR/no_effect_replace.rs:9:13
+   |
+LL |     let _ = "12345".replacen("12", "12", 1);
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: replacing text with itself
+  --> $DIR/no_effect_replace.rs:10:13
+   |
+LL |     let _ = String::new().replacen("12", "12", 1);
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: replacing text with itself
+  --> $DIR/no_effect_replace.rs:16:13
+   |
+LL |     let _ = "hello".replace(&x.f(), &x.f());
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: replacing text with itself
+  --> $DIR/no_effect_replace.rs:19:13
+   |
+LL |     let _ = "hello".replace(&y(), &y());
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 8 previous errors
+