Auto merge of rust-lang#5881 - wiomoc:feature/single-char-push_str, r=ebroto,flip1995

Lint `push_str` with a single-character string literal

Fixes rust-lang#5875
changelog:  `* [single_char_push_str]`

Author: bors

CHANGELOG.md

@@ -1699,6 +1699,7 @@ Released 2018-09-13
 [`should_implement_trait`]: https://rust-lang.github.io/rust-clippy/master/index.html#should_implement_trait
 [`similar_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#similar_names
 [`single_char_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_char_pattern
+[`single_char_push_str`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_char_push_str
 [`single_component_path_imports`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_component_path_imports
 [`single_match`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_match
 [`single_match_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_match_else

clippy_lints/src/lib.rs

@@ -678,6 +678,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &methods::SEARCH_IS_SOME,
         &methods::SHOULD_IMPLEMENT_TRAIT,
         &methods::SINGLE_CHAR_PATTERN,
+        &methods::SINGLE_CHAR_PUSH_STR,
         &methods::SKIP_WHILE_NEXT,
         &methods::STRING_EXTEND_CHARS,
         &methods::SUSPICIOUS_MAP,
@@ -1352,6 +1353,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&methods::SEARCH_IS_SOME),
         LintId::of(&methods::SHOULD_IMPLEMENT_TRAIT),
         LintId::of(&methods::SINGLE_CHAR_PATTERN),
+        LintId::of(&methods::SINGLE_CHAR_PUSH_STR),
         LintId::of(&methods::SKIP_WHILE_NEXT),
         LintId::of(&methods::STRING_EXTEND_CHARS),
         LintId::of(&methods::SUSPICIOUS_MAP),
@@ -1536,6 +1538,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&methods::OPTION_MAP_OR_NONE),
         LintId::of(&methods::RESULT_MAP_OR_INTO_OPTION),
         LintId::of(&methods::SHOULD_IMPLEMENT_TRAIT),
+        LintId::of(&methods::SINGLE_CHAR_PUSH_STR),
         LintId::of(&methods::STRING_EXTEND_CHARS),
         LintId::of(&methods::UNNECESSARY_FOLD),
         LintId::of(&methods::WRONG_SELF_CONVENTION),

clippy_lints/src/methods/mod.rs

@@ -1306,6 +1306,29 @@ declare_clippy_lint! {
     "using `.iter().next()` on a sliced array, which can be shortened to just `.get()`"
 }
 
+declare_clippy_lint! {
+    /// **What it does:** Warns when using push_str with a single-character string literal,
+    /// and push with a char would work fine.
+    ///
+    /// **Why is this bad?** It's less clear that we are pushing a single character
+    ///
+    /// **Known problems:** None
+    ///
+    /// **Example:**
+    /// ```
+    /// let mut string = String::new();
+    /// string.push_str("R");
+    /// ```
+    /// Could be written as
+    /// ```
+    /// let mut string = String::new();
+    /// string.push('R');
+    /// ```
+    pub SINGLE_CHAR_PUSH_STR,
+    style,
+    "`push_str()` used with a single-character string literal as parameter"
+}
+
 declare_lint_pass!(Methods => [
     UNWRAP_USED,
     EXPECT_USED,
@@ -1327,6 +1350,7 @@ declare_lint_pass!(Methods => [
     INEFFICIENT_TO_STRING,
     NEW_RET_NO_SELF,
     SINGLE_CHAR_PATTERN,
+    SINGLE_CHAR_PUSH_STR,
     SEARCH_IS_SOME,
     TEMPORARY_CSTRING_AS_PTR,
     FILTER_NEXT,
@@ -1441,6 +1465,12 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
                     inefficient_to_string::lint(cx, expr, &args[0], self_ty);
                 }
 
+                if let Some(fn_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) {
+                    if match_def_path(cx, fn_def_id, &paths::PUSH_STR) {
+                        lint_single_char_push_string(cx, expr, args);
+                    }
+                }
+
                 match self_ty.kind {
                     ty::Ref(_, ty, _) if ty.kind == ty::Str => {
                         for &(method, pos) in &PATTERN_METHODS {
@@ -3124,15 +3154,18 @@ fn lint_chars_last_cmp_with_unwrap<'tcx>(cx: &LateContext<'tcx>, info: &BinaryEx
     }
 }
 
-/// lint for length-1 `str`s for methods in `PATTERN_METHODS`
-fn lint_single_char_pattern<'tcx>(cx: &LateContext<'tcx>, _expr: &'tcx hir::Expr<'_>, arg: &'tcx hir::Expr<'_>) {
+fn get_hint_if_single_char_arg(
+    cx: &LateContext<'_>,
+    arg: &hir::Expr<'_>,
+    applicability: &mut Applicability,
+) -> Option<String> {
     if_chain! {
         if let hir::ExprKind::Lit(lit) = &arg.kind;
         if let ast::LitKind::Str(r, style) = lit.node;
-        if r.as_str().len() == 1;
+        let string = r.as_str();
+        if string.len() == 1;
         then {
-            let mut applicability = Applicability::MachineApplicable;
-            let snip = snippet_with_applicability(cx, arg.span, "..", &mut applicability);
+            let snip = snippet_with_applicability(cx, arg.span, &string, applicability);
             let ch = if let ast::StrStyle::Raw(nhash) = style {
                 let nhash = nhash as usize;
                 // for raw string: r##"a"##
@@ -3142,19 +3175,47 @@ fn lint_single_char_pattern<'tcx>(cx: &LateContext<'tcx>, _expr: &'tcx hir::Expr
                 &snip[1..(snip.len() - 1)]
             };
             let hint = format!("'{}'", if ch == "'" { "\\'" } else { ch });
-            span_lint_and_sugg(
-                cx,
-                SINGLE_CHAR_PATTERN,
-                arg.span,
-                "single-character string constant used as pattern",
-                "try using a `char` instead",
-                hint,
-                applicability,
-            );
+            Some(hint)
+        } else {
+            None
         }
     }
 }
 
+/// lint for length-1 `str`s for methods in `PATTERN_METHODS`
+fn lint_single_char_pattern(cx: &LateContext<'_>, _expr: &hir::Expr<'_>, arg: &hir::Expr<'_>) {
+    let mut applicability = Applicability::MachineApplicable;
+    if let Some(hint) = get_hint_if_single_char_arg(cx, arg, &mut applicability) {
+        span_lint_and_sugg(
+            cx,
+            SINGLE_CHAR_PATTERN,
+            arg.span,
+            "single-character string constant used as pattern",
+            "try using a `char` instead",
+            hint,
+            applicability,
+        );
+    }
+}
+
+/// lint for length-1 `str`s as argument for `push_str`
+fn lint_single_char_push_string(cx: &LateContext<'_>, expr: &hir::Expr<'_>, args: &[hir::Expr<'_>]) {
+    let mut applicability = Applicability::MachineApplicable;
+    if let Some(extension_string) = get_hint_if_single_char_arg(cx, &args[1], &mut applicability) {
+        let base_string_snippet = snippet_with_applicability(cx, args[0].span, "_", &mut applicability);
+        let sugg = format!("{}.push({})", base_string_snippet, extension_string);
+        span_lint_and_sugg(
+            cx,
+            SINGLE_CHAR_PUSH_STR,
+            expr.span,
+            "calling `push_str()` using a single-character string literal",
+            "consider using `push` with a character literal",
+            sugg,
+            applicability,
+        );
+    }
+}
+
 /// Checks for the `USELESS_ASREF` lint.
 fn lint_asref(cx: &LateContext<'_>, expr: &hir::Expr<'_>, call_name: &str, as_ref_args: &[hir::Expr<'_>]) {
     // when we get here, we've already checked that the call name is "as_ref" or "as_mut"

clippy_lints/src/utils/paths.rs

@@ -84,6 +84,7 @@ pub const POLL: [&str; 4] = ["core", "task", "poll", "Poll"];
 pub const PTR_EQ: [&str; 3] = ["core", "ptr", "eq"];
 pub const PTR_NULL: [&str; 2] = ["ptr", "null"];
 pub const PTR_NULL_MUT: [&str; 2] = ["ptr", "null_mut"];
+pub const PUSH_STR: [&str; 4] = ["alloc", "string", "String", "push_str"];
 pub const RANGE: [&str; 3] = ["core", "ops", "Range"];
 pub const RANGE_ARGUMENT_TRAIT: [&str; 3] = ["core", "ops", "RangeBounds"];
 pub const RANGE_FROM: [&str; 3] = ["core", "ops", "RangeFrom"];

src/lintlist/mod.rs

@@ -2012,6 +2012,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
         deprecation: None,
         module: "methods",
     },
+    Lint {
+        name: "single_char_push_str",
+        group: "style",
+        desc: "`push_str()` used with a single-character string literal as parameter",
+        deprecation: None,
+        module: "methods",
+    },
     Lint {
         name: "single_component_path_imports",
         group: "style",

tests/ui/single_char_push_str.fixed

@@ -0,0 +1,15 @@
+// run-rustfix
+#![warn(clippy::single_char_push_str)]
+
+fn main() {
+    let mut string = String::new();
+    string.push('R');
+    string.push('\'');
+
+    string.push('u');
+    string.push_str("st");
+    string.push_str("");
+    string.push('\x52');
+    string.push('\u{0052}');
+    string.push('a');
+}

tests/ui/single_char_push_str.rs

@@ -0,0 +1,15 @@
+// run-rustfix
+#![warn(clippy::single_char_push_str)]
+
+fn main() {
+    let mut string = String::new();
+    string.push_str("R");
+    string.push_str("'");
+
+    string.push('u');
+    string.push_str("st");
+    string.push_str("");
+    string.push_str("\x52");
+    string.push_str("\u{0052}");
+    string.push_str(r##"a"##);
+}

tests/ui/single_char_push_str.stderr

@@ -0,0 +1,34 @@
+error: calling `push_str()` using a single-character string literal
+  --> $DIR/single_char_push_str.rs:6:5
+   |
+LL |     string.push_str("R");
+   |     ^^^^^^^^^^^^^^^^^^^^ help: consider using `push` with a character literal: `string.push('R')`
+   |
+   = note: `-D clippy::single-char-push-str` implied by `-D warnings`
+
+error: calling `push_str()` using a single-character string literal
+  --> $DIR/single_char_push_str.rs:7:5
+   |
+LL |     string.push_str("'");
+   |     ^^^^^^^^^^^^^^^^^^^^ help: consider using `push` with a character literal: `string.push('/'')`
+
+error: calling `push_str()` using a single-character string literal
+  --> $DIR/single_char_push_str.rs:12:5
+   |
+LL |     string.push_str("/x52");
+   |     ^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `push` with a character literal: `string.push('/x52')`
+
+error: calling `push_str()` using a single-character string literal
+  --> $DIR/single_char_push_str.rs:13:5
+   |
+LL |     string.push_str("/u{0052}");
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `push` with a character literal: `string.push('/u{0052}')`
+
+error: calling `push_str()` using a single-character string literal
+  --> $DIR/single_char_push_str.rs:14:5
+   |
+LL |     string.push_str(r##"a"##);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `push` with a character literal: `string.push('a')`
+
+error: aborting due to 5 previous errors
+