diff --git a/CHANGELOG.md b/CHANGELOG.md index 714337732545..50fe7612909b 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 7ae185103a38..518e6905eff6 100644 --- a/clippy_lints/src/lib.rs +++ b/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), diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index b6266ef2ba19..614773a7e26b 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/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 { 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" diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index 923b319d7778..ffab0395120a 100644 --- a/clippy_lints/src/utils/paths.rs +++ b/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"]; diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 46827084a609..f76a215007c4 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -2012,6 +2012,13 @@ pub static ref ALL_LINTS: Vec = 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", diff --git a/tests/ui/single_char_push_str.fixed b/tests/ui/single_char_push_str.fixed new file mode 100644 index 000000000000..0812c026a644 --- /dev/null +++ b/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'); +} diff --git a/tests/ui/single_char_push_str.rs b/tests/ui/single_char_push_str.rs new file mode 100644 index 000000000000..ab293bbe4eeb --- /dev/null +++ b/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"##); +} diff --git a/tests/ui/single_char_push_str.stderr b/tests/ui/single_char_push_str.stderr new file mode 100644 index 000000000000..0e9bdaa23e7e --- /dev/null +++ b/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 +