From 80826c3944df4233fd532ee8cf465851dfe1b0fc Mon Sep 17 00:00:00 2001 From: Guilherme-Vasconcelos <49197151+Guilherme-Vasconcelos@users.noreply.github.com> Date: Sun, 17 Jul 2022 21:20:51 -0300 Subject: [PATCH] Implement clippy::manual_empty_string_creations lint --- CHANGELOG.md | 1 + clippy_lints/src/lib.register_all.rs | 1 + clippy_lints/src/lib.register_lints.rs | 1 + clippy_lints/src/lib.register_style.rs | 1 + clippy_lints/src/lib.rs | 2 + .../src/manual_empty_string_creations.rs | 141 ++++++++++++++++++ clippy_utils/src/msrvs.rs | 2 +- tests/ui/manual_empty_string_creations.fixed | 63 ++++++++ tests/ui/manual_empty_string_creations.rs | 63 ++++++++ tests/ui/manual_empty_string_creations.stderr | 58 +++++++ 10 files changed, 332 insertions(+), 1 deletion(-) create mode 100644 clippy_lints/src/manual_empty_string_creations.rs create mode 100644 tests/ui/manual_empty_string_creations.fixed create mode 100644 tests/ui/manual_empty_string_creations.rs create mode 100644 tests/ui/manual_empty_string_creations.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index d74ec71a0e8..eedbe10320d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3828,6 +3828,7 @@ Released 2018-09-13 [`manual_assert`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_assert [`manual_async_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_async_fn [`manual_bits`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_bits +[`manual_empty_string_creations`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_empty_string_creations [`manual_filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_filter_map [`manual_find`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_find [`manual_find_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_find_map diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index 01082cc8eeb..e253f656b0d 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -124,6 +124,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(main_recursion::MAIN_RECURSION), LintId::of(manual_async_fn::MANUAL_ASYNC_FN), LintId::of(manual_bits::MANUAL_BITS), + LintId::of(manual_empty_string_creations::MANUAL_EMPTY_STRING_CREATIONS), LintId::of(manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE), LintId::of(manual_rem_euclid::MANUAL_REM_EUCLID), LintId::of(manual_retain::MANUAL_RETAIN), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index c540573b802..29d923a0d8d 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -244,6 +244,7 @@ store.register_lints(&[ manual_assert::MANUAL_ASSERT, manual_async_fn::MANUAL_ASYNC_FN, manual_bits::MANUAL_BITS, + manual_empty_string_creations::MANUAL_EMPTY_STRING_CREATIONS, manual_instant_elapsed::MANUAL_INSTANT_ELAPSED, manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE, manual_ok_or::MANUAL_OK_OR, diff --git a/clippy_lints/src/lib.register_style.rs b/clippy_lints/src/lib.register_style.rs index bfa654238f1..6972c75597a 100644 --- a/clippy_lints/src/lib.register_style.rs +++ b/clippy_lints/src/lib.register_style.rs @@ -44,6 +44,7 @@ store.register_group(true, "clippy::style", Some("clippy_style"), vec![ LintId::of(main_recursion::MAIN_RECURSION), LintId::of(manual_async_fn::MANUAL_ASYNC_FN), LintId::of(manual_bits::MANUAL_BITS), + LintId::of(manual_empty_string_creations::MANUAL_EMPTY_STRING_CREATIONS), LintId::of(manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE), LintId::of(map_clone::MAP_CLONE), LintId::of(match_result_ok::MATCH_RESULT_OK), diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index e6a405f8170..4201cd9c0bf 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -273,6 +273,7 @@ mod main_recursion; mod manual_assert; mod manual_async_fn; mod manual_bits; +mod manual_empty_string_creations; mod manual_instant_elapsed; mod manual_non_exhaustive; mod manual_ok_or; @@ -933,6 +934,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| Box::new(std_instead_of_core::StdReexports::default())); store.register_late_pass(|| Box::new(manual_instant_elapsed::ManualInstantElapsed)); store.register_late_pass(|| Box::new(partialeq_to_none::PartialeqToNone)); + store.register_late_pass(|| Box::new(manual_empty_string_creations::ManualEmptyStringCreations)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/manual_empty_string_creations.rs b/clippy_lints/src/manual_empty_string_creations.rs new file mode 100644 index 00000000000..dd602a89b4f --- /dev/null +++ b/clippy_lints/src/manual_empty_string_creations.rs @@ -0,0 +1,141 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use rustc_ast::LitKind; +use rustc_errors::Applicability::MachineApplicable; +use rustc_hir::{Expr, ExprKind, PathSegment, QPath, TyKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::{sym, symbol, Span}; + +declare_clippy_lint! { + /// ### What it does + /// + /// Checks for usage of `""` to create a `String`, such as `"".to_string()`, `"".to_owned()`, + /// `String::from("")` and others. + /// + /// ### Why is this bad? + /// + /// Different ways of creating an empty string makes your code less standardized, which can + /// be confusing. + /// + /// ### Example + /// ```rust + /// let a = "".to_string(); + /// let b: String = "".into(); + /// ``` + /// Use instead: + /// ```rust + /// let a = String::new(); + /// let b = String::new(); + /// ``` + #[clippy::version = "1.65.0"] + pub MANUAL_EMPTY_STRING_CREATIONS, + style, + "empty String is being created manually" +} +declare_lint_pass!(ManualEmptyStringCreations => [MANUAL_EMPTY_STRING_CREATIONS]); + +impl LateLintPass<'_> for ManualEmptyStringCreations { + fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) { + if expr.span.from_expansion() { + return; + } + + let ty = cx.typeck_results().expr_ty(expr); + match ty.kind() { + ty::Adt(adt_def, _) if adt_def.is_struct() => { + if !cx.tcx.is_diagnostic_item(sym::String, adt_def.did()) { + return; + } + }, + _ => return, + } + + match expr.kind { + ExprKind::Call(func, args) => { + parse_call(cx, expr.span, func, args); + }, + ExprKind::MethodCall(path_segment, args, _) => { + parse_method_call(cx, expr.span, path_segment, args); + }, + _ => (), + } + } +} + +/// Checks if an expression's kind corresponds to an empty &str. +fn is_expr_kind_empty_str(expr_kind: &ExprKind<'_>) -> bool { + if let ExprKind::Lit(lit) = expr_kind && + let LitKind::Str(value, _) = lit.node && + value == symbol::kw::Empty + { + return true; + } + + false +} + +/// Emits the `MANUAL_EMPTY_STRING_CREATION` warning and suggests the appropriate fix. +fn warn_then_suggest(cx: &LateContext<'_>, span: Span) { + span_lint_and_sugg( + cx, + MANUAL_EMPTY_STRING_CREATIONS, + span, + "empty String is being created manually", + "consider using", + "String::new()".into(), + MachineApplicable, + ); +} + +/// Tries to parse an expression as a method call, emiting the warning if necessary. +fn parse_method_call(cx: &LateContext<'_>, span: Span, path_segment: &PathSegment<'_>, args: &[Expr<'_>]) { + if args.is_empty() { + // When parsing TryFrom::try_from(...).expect(...), we will have more than 1 arg. + return; + } + + let ident = path_segment.ident.as_str(); + let method_arg_kind = &args[0].kind; + if ["to_string", "to_owned", "into"].contains(&ident) && is_expr_kind_empty_str(method_arg_kind) { + warn_then_suggest(cx, span); + } else if let ExprKind::Call(func, args) = method_arg_kind { + // If our first argument is a function call itself, it could be an `unwrap`-like function. + // E.g. String::try_from("hello").unwrap(), TryFrom::try_from("").expect("hello"), etc. + parse_call(cx, span, func, args); + } +} + +/// Tries to parse an expression as a function call, emiting the warning if necessary. +fn parse_call(cx: &LateContext<'_>, span: Span, func: &Expr<'_>, args: &[Expr<'_>]) { + if args.len() != 1 { + return; + } + + let arg_kind = &args[0].kind; + if let ExprKind::Path(qpath) = &func.kind { + if let QPath::TypeRelative(_, _) = qpath { + // String::from(...) or String::try_from(...) + if let QPath::TypeRelative(ty, path_seg) = qpath && + [sym::from, sym::try_from].contains(&path_seg.ident.name) && + let TyKind::Path(qpath) = &ty.kind && + let QPath::Resolved(_, path) = qpath && + let [path_seg] = path.segments && + path_seg.ident.name == sym::String && + is_expr_kind_empty_str(arg_kind) + { + warn_then_suggest(cx, span); + } + } else if let QPath::Resolved(_, path) = qpath { + // From::from(...) or TryFrom::try_from(...) + if let [path_seg1, path_seg2] = path.segments && + is_expr_kind_empty_str(arg_kind) && ( + (path_seg1.ident.name == sym::From && path_seg2.ident.name == sym::from) || + (path_seg1.ident.name == sym::TryFrom && path_seg2.ident.name == sym::try_from) + ) + { + warn_then_suggest(cx, span); + } + } + } +} diff --git a/clippy_utils/src/msrvs.rs b/clippy_utils/src/msrvs.rs index 9e238c6f1ac..d539d11a39f 100644 --- a/clippy_utils/src/msrvs.rs +++ b/clippy_utils/src/msrvs.rs @@ -32,8 +32,8 @@ msrv_aliases! { 1,30,0 { ITERATOR_FIND_MAP, TOOL_ATTRIBUTES } 1,28,0 { FROM_BOOL } 1,26,0 { RANGE_INCLUSIVE, STRING_RETAIN } + 1,24,0 { IS_ASCII_DIGIT } 1,18,0 { HASH_MAP_RETAIN, HASH_SET_RETAIN } 1,17,0 { FIELD_INIT_SHORTHAND, STATIC_IN_CONST, EXPECT_ERR } 1,16,0 { STR_REPEAT } - 1,24,0 { IS_ASCII_DIGIT } } diff --git a/tests/ui/manual_empty_string_creations.fixed b/tests/ui/manual_empty_string_creations.fixed new file mode 100644 index 00000000000..3516312ee0c --- /dev/null +++ b/tests/ui/manual_empty_string_creations.fixed @@ -0,0 +1,63 @@ +// run-rustfix + +#![warn(clippy::manual_empty_string_creations)] + +macro_rules! create_strings_from_macro { + // When inside a macro, nothing should warn to prevent false positives. + ($some_str:expr) => { + let _: String = $some_str.into(); + let _ = $some_str.to_string(); + } +} + +fn main() { + // Method calls + let _ = String::new(); + let _ = "no warning".to_string(); + + let _ = String::new(); + let _ = "no warning".to_owned(); + + let _: String = String::new(); + let _: String = "no warning".into(); + + let _: SomeOtherStruct = "no warning".into(); + let _: SomeOtherStruct = "".into(); // No warning too. We are not converting into String. + + // Calls + let _ = String::new(); + let _ = String::new(); + let _ = String::from("no warning"); + let _ = SomeOtherStruct::from("no warning"); + let _ = SomeOtherStruct::from(""); // Again: no warning. + + let _ = String::new(); + let _ = String::try_from("no warning").unwrap(); + let _ = String::try_from("no warning").expect("this should not warn"); + let _ = SomeOtherStruct::try_from("no warning").unwrap(); + let _ = SomeOtherStruct::try_from("").unwrap(); // Again: no warning. + + let _: String = String::new(); + let _: String = From::from("no warning"); + let _: SomeOtherStruct = From::from("no warning"); + let _: SomeOtherStruct = From::from(""); // Again: no warning. + + let _: String = String::new(); + let _: String = TryFrom::try_from("no warning").unwrap(); + let _: String = TryFrom::try_from("no warning").expect("this should not warn"); + let _: String = String::new(); + let _: SomeOtherStruct = TryFrom::try_from("no_warning").unwrap(); + let _: SomeOtherStruct = TryFrom::try_from("").unwrap(); // Again: no warning. + + // Macros (never warn) + create_strings_from_macro!(""); + create_strings_from_macro!("Hey"); +} + +struct SomeOtherStruct {} + +impl From<&str> for SomeOtherStruct { + fn from(_value: &str) -> Self { + Self {} + } +} diff --git a/tests/ui/manual_empty_string_creations.rs b/tests/ui/manual_empty_string_creations.rs new file mode 100644 index 00000000000..ed39a05ed5c --- /dev/null +++ b/tests/ui/manual_empty_string_creations.rs @@ -0,0 +1,63 @@ +// run-rustfix + +#![warn(clippy::manual_empty_string_creations)] + +macro_rules! create_strings_from_macro { + // When inside a macro, nothing should warn to prevent false positives. + ($some_str:expr) => { + let _: String = $some_str.into(); + let _ = $some_str.to_string(); + }; +} + +fn main() { + // Method calls + let _ = "".to_string(); + let _ = "no warning".to_string(); + + let _ = "".to_owned(); + let _ = "no warning".to_owned(); + + let _: String = "".into(); + let _: String = "no warning".into(); + + let _: SomeOtherStruct = "no warning".into(); + let _: SomeOtherStruct = "".into(); // No warning too. We are not converting into String. + + // Calls + let _ = String::from(""); + let _ = ::from(""); + let _ = String::from("no warning"); + let _ = SomeOtherStruct::from("no warning"); + let _ = SomeOtherStruct::from(""); // Again: no warning. + + let _ = String::try_from("").unwrap(); + let _ = String::try_from("no warning").unwrap(); + let _ = String::try_from("no warning").expect("this should not warn"); + let _ = SomeOtherStruct::try_from("no warning").unwrap(); + let _ = SomeOtherStruct::try_from("").unwrap(); // Again: no warning. + + let _: String = From::from(""); + let _: String = From::from("no warning"); + let _: SomeOtherStruct = From::from("no warning"); + let _: SomeOtherStruct = From::from(""); // Again: no warning. + + let _: String = TryFrom::try_from("").unwrap(); + let _: String = TryFrom::try_from("no warning").unwrap(); + let _: String = TryFrom::try_from("no warning").expect("this should not warn"); + let _: String = TryFrom::try_from("").expect("this should warn"); + let _: SomeOtherStruct = TryFrom::try_from("no_warning").unwrap(); + let _: SomeOtherStruct = TryFrom::try_from("").unwrap(); // Again: no warning. + + // Macros (never warn) + create_strings_from_macro!(""); + create_strings_from_macro!("Hey"); +} + +struct SomeOtherStruct {} + +impl From<&str> for SomeOtherStruct { + fn from(_value: &str) -> Self { + Self {} + } +} diff --git a/tests/ui/manual_empty_string_creations.stderr b/tests/ui/manual_empty_string_creations.stderr new file mode 100644 index 00000000000..f38ba02a508 --- /dev/null +++ b/tests/ui/manual_empty_string_creations.stderr @@ -0,0 +1,58 @@ +error: empty String is being created manually + --> $DIR/manual_empty_string_creations.rs:15:13 + | +LL | let _ = "".to_string(); + | ^^^^^^^^^^^^^^ help: consider using: `String::new()` + | + = note: `-D clippy::manual-empty-string-creations` implied by `-D warnings` + +error: empty String is being created manually + --> $DIR/manual_empty_string_creations.rs:18:13 + | +LL | let _ = "".to_owned(); + | ^^^^^^^^^^^^^ help: consider using: `String::new()` + +error: empty String is being created manually + --> $DIR/manual_empty_string_creations.rs:21:21 + | +LL | let _: String = "".into(); + | ^^^^^^^^^ help: consider using: `String::new()` + +error: empty String is being created manually + --> $DIR/manual_empty_string_creations.rs:28:13 + | +LL | let _ = String::from(""); + | ^^^^^^^^^^^^^^^^ help: consider using: `String::new()` + +error: empty String is being created manually + --> $DIR/manual_empty_string_creations.rs:29:13 + | +LL | let _ = ::from(""); + | ^^^^^^^^^^^^^^^^^^ help: consider using: `String::new()` + +error: empty String is being created manually + --> $DIR/manual_empty_string_creations.rs:34:13 + | +LL | let _ = String::try_from("").unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `String::new()` + +error: empty String is being created manually + --> $DIR/manual_empty_string_creations.rs:40:21 + | +LL | let _: String = From::from(""); + | ^^^^^^^^^^^^^^ help: consider using: `String::new()` + +error: empty String is being created manually + --> $DIR/manual_empty_string_creations.rs:45:21 + | +LL | let _: String = TryFrom::try_from("").unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `String::new()` + +error: empty String is being created manually + --> $DIR/manual_empty_string_creations.rs:48:21 + | +LL | let _: String = TryFrom::try_from("").expect("this should warn"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `String::new()` + +error: aborting due to 9 previous errors +