From 783108d1f128c07d135101a847643f91f270c71a Mon Sep 17 00:00:00 2001 From: Christoph Walcher Date: Fri, 14 Aug 2020 12:52:19 +0200 Subject: [PATCH] Merge lint with `single_char_pattern` --- clippy_lints/src/lib.rs | 8 +-- clippy_lints/src/methods/mod.rs | 89 ++++++++++++++++++++---- clippy_lints/src/single_char_push_str.rs | 62 ----------------- src/lintlist/mod.rs | 2 +- tests/ui/single_char_push_str.fixed | 5 ++ tests/ui/single_char_push_str.rs | 5 ++ tests/ui/single_char_push_str.stderr | 20 +++++- 7 files changed, 108 insertions(+), 83 deletions(-) delete mode 100644 clippy_lints/src/single_char_push_str.rs diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 20e55bc78d4a..c6a5ce289e7a 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -286,7 +286,6 @@ mod repeat_once; mod returns; mod serde_api; mod shadow; -mod single_char_push_str; mod single_component_path_imports; mod slow_vector_initialization; mod stable_sort_primitive; @@ -676,6 +675,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, @@ -773,7 +773,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &shadow::SHADOW_REUSE, &shadow::SHADOW_SAME, &shadow::SHADOW_UNRELATED, - &single_char_push_str::SINGLE_CHAR_PUSH_STR, &single_component_path_imports::SINGLE_COMPONENT_PATH_IMPORTS, &slow_vector_initialization::SLOW_VECTOR_INITIALIZATION, &stable_sort_primitive::STABLE_SORT_PRIMITIVE, @@ -930,7 +929,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(move || box escape::BoxedLocal{too_large_for_stack}); store.register_late_pass(|| box panic_unimplemented::PanicUnimplemented); store.register_late_pass(|| box strings::StringLitAsBytes); - store.register_late_pass(|| box single_char_push_str::SingleCharPushStrPass); store.register_late_pass(|| box derive::Derive); store.register_late_pass(|| box types::CharLitAsU8); store.register_late_pass(|| box vec::UselessVec); @@ -1346,6 +1344,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), @@ -1412,7 +1411,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&returns::NEEDLESS_RETURN), LintId::of(&returns::UNUSED_UNIT), LintId::of(&serde_api::SERDE_API_MISUSE), - LintId::of(&single_char_push_str::SINGLE_CHAR_PUSH_STR), LintId::of(&single_component_path_imports::SINGLE_COMPONENT_PATH_IMPORTS), LintId::of(&slow_vector_initialization::SLOW_VECTOR_INITIALIZATION), LintId::of(&stable_sort_primitive::STABLE_SORT_PRIMITIVE), @@ -1527,6 +1525,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), @@ -1551,7 +1550,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(®ex::TRIVIAL_REGEX), LintId::of(&returns::NEEDLESS_RETURN), LintId::of(&returns::UNUSED_UNIT), - LintId::of(&single_char_push_str::SINGLE_CHAR_PUSH_STR), LintId::of(&single_component_path_imports::SINGLE_COMPONENT_PATH_IMPORTS), LintId::of(&strings::STRING_LIT_AS_BYTES), LintId::of(&tabs_in_doc_comments::TABS_IN_DOC_COMMENTS), diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 570ae66d595e..f139e3e5b944 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<'tcx>( + cx: &LateContext<'tcx>, + arg: &'tcx 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<'tcx>(cx: &LateContext<'tcx>, _expr: &'tcx hir::Expr<'_>, arg: &'tcx 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<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, args: &'tcx [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/single_char_push_str.rs b/clippy_lints/src/single_char_push_str.rs deleted file mode 100644 index 68bbef7261a9..000000000000 --- a/clippy_lints/src/single_char_push_str.rs +++ /dev/null @@ -1,62 +0,0 @@ -use crate::utils::{match_def_path, paths, snippet_with_applicability, span_lint_and_sugg}; -use if_chain::if_chain; -use rustc_ast::ast::LitKind; -use rustc_errors::Applicability; -use rustc_hir::{Expr, ExprKind}; -use rustc_lint::{LateContext, LateLintPass}; -use rustc_session::{declare_lint_pass, declare_tool_lint}; - -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?** This is in all probability not the intended outcome. At - /// the least it hurts readability of the code. - /// - /// **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!(SingleCharPushStrPass => [SINGLE_CHAR_PUSH_STR]); - -impl<'tcx> LateLintPass<'tcx> for SingleCharPushStrPass { - fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - if_chain! { - if let ExprKind::MethodCall(_, _, ref args, _) = expr.kind; - if let [base_string, extension_string] = args; - 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); - if let ExprKind::Lit(ref lit) = extension_string.kind; - if let LitKind::Str(symbol,_) = lit.node; - let extension_string_val = symbol.as_str().to_string(); - if extension_string_val.len() == 1; - then { - let mut applicability = Applicability::MachineApplicable; - let base_string_snippet = snippet_with_applicability(cx, base_string.span, "_", &mut applicability); - let sugg = format!("{}.push({:?})", base_string_snippet, extension_string_val.chars().next().unwrap()); - 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 - ); - } - } - } -} diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 184273f48c32..085b05aaba45 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -2003,7 +2003,7 @@ pub static ref ALL_LINTS: Vec = vec![ group: "style", desc: "`push_str()` used with a single-character string literal as parameter", deprecation: None, - module: "single_char_push_str", + module: "methods", }, Lint { name: "single_component_path_imports", diff --git a/tests/ui/single_char_push_str.fixed b/tests/ui/single_char_push_str.fixed index 49607c49218a..0812c026a644 100644 --- a/tests/ui/single_char_push_str.fixed +++ b/tests/ui/single_char_push_str.fixed @@ -7,4 +7,9 @@ fn main() { 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 index bbeebd027b16..ab293bbe4eeb 100644 --- a/tests/ui/single_char_push_str.rs +++ b/tests/ui/single_char_push_str.rs @@ -7,4 +7,9 @@ fn main() { 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 index 453ec2d42f1f..0e9bdaa23e7e 100644 --- a/tests/ui/single_char_push_str.stderr +++ b/tests/ui/single_char_push_str.stderr @@ -12,5 +12,23 @@ error: calling `push_str()` using a single-character string literal LL | string.push_str("'"); | ^^^^^^^^^^^^^^^^^^^^ help: consider using `push` with a character literal: `string.push('/'')` -error: aborting due to 2 previous errors +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