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

New lint `no_effect_replace`

Closes #1595

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

Thank you for making Clippy better!

We're collecting our changelog from pull request descriptions.
If your PR only includes internal changes, you can just write
`changelog: none`. Otherwise, please write a short comment
explaining your change. Also, it's helpful for us that
the lint name is put into brackets `[]` and backticks `` ` ` ``,
e.g. ``[`lint_name`]``.

If your PR fixes an issue, you can add "fixes #issue_number" into this
PR description. This way the issue will be automatically closed when
your PR is merged.

If you added a new lint, here's a checklist for things that will be
checked during review or continuous integration.

- \[x] Followed [lint naming conventions][lint_naming]
- \[x] Added passing UI tests (including committed `.stderr` file)
- \[x] `cargo test` passes locally
- \[x] Executed `cargo dev update_lints`
- \[x] Added lint documentation
- \[x] Run `cargo dev fmt`

[lint_naming]: https://rust-lang.github.io/rfcs/0344-conventions-galore.html#lints

Note that you can skip the above if you are just opening a WIP PR in
order to get feedback.

Delete this line and everything above before opening your PR.

---

*Please write a short comment explaining your change (or "none" for internal only changes)*

changelog:
Add `no_effect_replace` lint.

Author: bors

CHANGELOG.md

@@ -3593,6 +3593,7 @@ Released 2018-09-13
 [`new_ret_no_self`]: https://rust-lang.github.io/rust-clippy/master/index.html#new_ret_no_self
 [`new_without_default`]: https://rust-lang.github.io/rust-clippy/master/index.html#new_without_default
 [`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

@@ -243,6 +243,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
     LintId::of(new_without_default::NEW_WITHOUT_DEFAULT),
     LintId::of(no_effect::NO_EFFECT),
     LintId::of(no_effect::UNNECESSARY_OPERATION),
+    LintId::of(no_effect_replace::NO_EFFECT_REPLACE),
     LintId::of(non_copy_const::BORROW_INTERIOR_MUTABLE_CONST),
     LintId::of(non_copy_const::DECLARE_INTERIOR_MUTABLE_CONST),
     LintId::of(non_expressive_names::JUST_UNDERSCORES_AND_DIGITS),

clippy_lints/src/lib.register_lints.rs

@@ -408,6 +408,7 @@ store.register_lints(&[
     no_effect::NO_EFFECT,
     no_effect::NO_EFFECT_UNDERSCORE_BINDING,
     no_effect::UNNECESSARY_OPERATION,
+    no_effect_replace::NO_EFFECT_REPLACE,
     non_copy_const::BORROW_INTERIOR_MUTABLE_CONST,
     non_copy_const::DECLARE_INTERIOR_MUTABLE_CONST,
     non_expressive_names::JUST_UNDERSCORES_AND_DIGITS,

clippy_lints/src/lib.register_suspicious.rs

@@ -25,6 +25,7 @@ store.register_group(true, "clippy::suspicious", Some("clippy_suspicious"), vec!
     LintId::of(loops::MUT_RANGE_BOUND),
     LintId::of(methods::SUSPICIOUS_MAP),
     LintId::of(mut_key::MUTABLE_KEY_TYPE),
+    LintId::of(no_effect_replace::NO_EFFECT_REPLACE),
     LintId::of(octal_escapes::OCTAL_ESCAPES),
     LintId::of(suspicious_trait_impl::SUSPICIOUS_ARITHMETIC_IMPL),
     LintId::of(suspicious_trait_impl::SUSPICIOUS_OP_ASSIGN_IMPL),

clippy_lints/src/lib.rs

@@ -321,6 +321,7 @@ mod neg_cmp_op_on_partial_ord;
 mod neg_multiply;
 mod new_without_default;
 mod no_effect;
+mod no_effect_replace;
 mod non_copy_const;
 mod non_expressive_names;
 mod non_octal_unix_permissions;
@@ -889,6 +890,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| Box::new(bytes_count_to_len::BytesCountToLen));
     let max_include_file_size = conf.max_include_file_size;
     store.register_late_pass(move || Box::new(large_include_file::LargeIncludeFile::new(max_include_file_size)));
+    store.register_late_pass(|| Box::new(no_effect_replace::NoEffectReplace));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }
 

clippy_lints/src/no_effect_replace.rs

@@ -0,0 +1,89 @@
+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::{Expr, ExprKind};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::sym;
+
+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"
+}
+declare_lint_pass!(NoEffectReplace => [NO_EFFECT_REPLACE]);
+
+impl LateLintPass<'_> for NoEffectReplace {
+    fn check_expr(&mut self, cx: &LateContext<'_>, expr: &'_ Expr<'_>) {
+        let (path, args) = match expr.kind {
+            ExprKind::MethodCall(path, args, _) => (path, args),
+            _ => return,
+        };
+
+        if !(args.len() == 3 || args.len() == 4)
+            || !(path.ident.name.as_str() == "replace" || path.ident.name.as_str() == "replacen")
+        {
+            return;
+        }
+
+        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) = &args[1].kind;
+            if let Some(param1) = lit_string_value(&spanned.node);
+
+            if let ExprKind::Lit(spanned) = &args[2].kind;
+            if let LitKind::Str(param2, _) = &spanned.node;
+            if param1 == param2.as_str();
+
+            then {
+                span_lint(
+                    cx,
+                    NO_EFFECT_REPLACE,
+                    expr.span,
+                    "you are using replace with same element",
+                );
+            }
+        }
+
+        if_chain! {
+            if let ExprKind::MethodCall(path, args, _) = expr.kind;
+            if (args.len() == 3 && path.ident.name.as_str() == "replace") || (args.len() == 4 && path.ident.name.as_str() == "replacen");
+            if SpanlessEq::new(cx).eq_expr(&args[1], &args[2]);
+
+            then {
+                span_lint(
+                    cx,
+                    NO_EFFECT_REPLACE,
+                    expr.span,
+                    "you are using replace with same element",
+                );
+            }
+        }
+    }
+}
+
+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: you are using replace with same element
+  --> $DIR/no_effect_replace.rs:4:13
+   |
+LL |     let _ = "12345".replace('1', "1");
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: `-D clippy::no-effect-replace` implied by `-D warnings`
+
+error: you are using replace with same element
+  --> $DIR/no_effect_replace.rs:5:13
+   |
+LL |     let _ = "12345".replace("12", "12");
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: you are using replace with same element
+  --> $DIR/no_effect_replace.rs:6:13
+   |
+LL |     let _ = String::new().replace("12", "12");
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: you are using replace with same element
+  --> $DIR/no_effect_replace.rs:8:13
+   |
+LL |     let _ = "12345".replacen('1', "1", 1);
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: you are using replace with same element
+  --> $DIR/no_effect_replace.rs:9:13
+   |
+LL |     let _ = "12345".replacen("12", "12", 1);
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: you are using replace with same element
+  --> $DIR/no_effect_replace.rs:10:13
+   |
+LL |     let _ = String::new().replacen("12", "12", 1);
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: you are using replace with same element
+  --> $DIR/no_effect_replace.rs:16:13
+   |
+LL |     let _ = "hello".replace(&x.f(), &x.f());
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: you are using replace with same element
+  --> $DIR/no_effect_replace.rs:19:13
+   |
+LL |     let _ = "hello".replace(&y(), &y());
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 8 previous errors
+