diff --git a/CHANGELOG.md b/CHANGELOG.md index db18bc296..ece111121 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5256,6 +5256,7 @@ Released 2018-09-13 [`string_extend_chars`]: https://rust-lang.github.io/rust-clippy/master/index.html#string_extend_chars [`string_from_utf8_as_bytes`]: https://rust-lang.github.io/rust-clippy/master/index.html#string_from_utf8_as_bytes [`string_lit_as_bytes`]: https://rust-lang.github.io/rust-clippy/master/index.html#string_lit_as_bytes +[`string_lit_chars_any`]: https://rust-lang.github.io/rust-clippy/master/index.html#string_lit_chars_any [`string_slice`]: https://rust-lang.github.io/rust-clippy/master/index.html#string_slice [`string_to_string`]: https://rust-lang.github.io/rust-clippy/master/index.html#string_to_string [`strlen_on_c_strings`]: https://rust-lang.github.io/rust-clippy/master/index.html#strlen_on_c_strings diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 51035a9d6..168dc9bf4 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -405,6 +405,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::methods::SKIP_WHILE_NEXT_INFO, crate::methods::STABLE_SORT_PRIMITIVE_INFO, crate::methods::STRING_EXTEND_CHARS_INFO, + crate::methods::STRING_LIT_CHARS_ANY_INFO, crate::methods::SUSPICIOUS_COMMAND_ARG_SPACE_INFO, crate::methods::SUSPICIOUS_MAP_INFO, crate::methods::SUSPICIOUS_SPLITN_INFO, diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 81f59f68e..6efeff920 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -86,6 +86,7 @@ mod skip_while_next; mod stable_sort_primitive; mod str_splitn; mod string_extend_chars; +mod string_lit_chars_any; mod suspicious_command_arg_space; mod suspicious_map; mod suspicious_splitn; @@ -115,7 +116,7 @@ use clippy_utils::consts::{constant, Constant}; use clippy_utils::diagnostics::{span_lint, span_lint_and_help}; use clippy_utils::msrvs::{self, Msrv}; use clippy_utils::ty::{contains_ty_adt_constructor_opaque, implements_trait, is_copy, is_type_diagnostic_item}; -use clippy_utils::{contains_return, is_bool, is_trait_method, iter_input_pats, return_ty}; +use clippy_utils::{contains_return, is_bool, is_trait_method, iter_input_pats, peel_blocks, return_ty}; use if_chain::if_chain; use rustc_hir as hir; use rustc_hir::{Expr, ExprKind, Node, Stmt, StmtKind, TraitItem, TraitItemKind}; @@ -3379,6 +3380,34 @@ declare_clippy_lint! { "calling `Stdin::read_line`, then trying to parse it without first trimming" } +declare_clippy_lint! { + /// ### What it does + /// Checks for `.chars().any(|i| i == c)`. + /// + /// ### Why is this bad? + /// It's significantly slower than using a pattern instead, like + /// `matches!(c, '\\' | '.' | '+')`. + /// + /// Despite this being faster, this is not `perf` as this is pretty common, and is a rather nice + /// way to check if a `char` is any in a set. In any case, this `restriction` lint is available + /// for situations where that additional performance is absolutely necessary. + /// + /// ### Example + /// ```rust + /// # let c = 'c'; + /// "\\.+*?()|[]{}^$#&-~".chars().any(|x| x == c); + /// ``` + /// Use instead: + /// ```rust + /// # let c = 'c'; + /// matches!(c, '\\' | '.' | '+' | '*' | '(' | ')' | '|' | '[' | ']' | '{' | '}' | '^' | '$' | '#' | '&' | '-' | '~'); + /// ``` + #[clippy::version = "1.72.0"] + pub STRING_LIT_CHARS_ANY, + restriction, + "checks for `.chars().any(|i| i == c)`" +} + declare_clippy_lint! { /// ### What it does /// Checks for usage of `.map(|_| format!(..)).collect::()`. @@ -3549,6 +3578,7 @@ impl_lint_pass!(Methods => [ DRAIN_COLLECT, MANUAL_TRY_FOLD, FORMAT_COLLECT, + STRING_LIT_CHARS_ANY, ]); /// Extracts a method call name, args, and `Span` of the method name. @@ -3923,6 +3953,13 @@ impl Methods { } } }, + ("any", [arg]) if let ExprKind::Closure(arg) = arg.kind + && let body = cx.tcx.hir().body(arg.body) + && let [param] = body.params + && let Some(("chars", recv, _, _, _)) = method_call(recv) => + { + string_lit_chars_any::check(cx, expr, recv, param, peel_blocks(body.value), &self.msrv); + } ("nth", [n_arg]) => match method_call(recv) { Some(("bytes", recv2, [], _, _)) => bytes_nth::check(cx, expr, recv2, n_arg), Some(("cloned", recv2, [], _, _)) => iter_overeager_cloned::check(cx, expr, recv, recv2, false, false), diff --git a/clippy_lints/src/methods/string_lit_chars_any.rs b/clippy_lints/src/methods/string_lit_chars_any.rs new file mode 100644 index 000000000..70da6ad58 --- /dev/null +++ b/clippy_lints/src/methods/string_lit_chars_any.rs @@ -0,0 +1,58 @@ +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::msrvs::{Msrv, MATCHES_MACRO}; +use clippy_utils::source::snippet_opt; +use clippy_utils::{is_from_proc_macro, is_trait_method, path_to_local}; +use itertools::Itertools; +use rustc_ast::LitKind; +use rustc_errors::Applicability; +use rustc_hir::{BinOpKind, Expr, ExprKind, Param, PatKind}; +use rustc_lint::LateContext; +use rustc_span::sym; + +use super::STRING_LIT_CHARS_ANY; + +pub(super) fn check<'tcx>( + cx: &LateContext<'tcx>, + expr: &'tcx Expr<'tcx>, + recv: &Expr<'_>, + param: &'tcx Param<'tcx>, + body: &Expr<'_>, + msrv: &Msrv, +) { + if msrv.meets(MATCHES_MACRO) + && is_trait_method(cx, expr, sym::Iterator) + && let PatKind::Binding(_, arg, _, _) = param.pat.kind + && let ExprKind::Lit(lit_kind) = recv.kind + && let LitKind::Str(val, _) = lit_kind.node + && let ExprKind::Binary(kind, lhs, rhs) = body.kind + && let BinOpKind::Eq = kind.node + && let Some(lhs_path) = path_to_local(lhs) + && let Some(rhs_path) = path_to_local(rhs) + && let scrutinee = match (lhs_path == arg, rhs_path == arg) { + (true, false) => rhs, + (false, true) => lhs, + _ => return, + } + && !is_from_proc_macro(cx, expr) + && let Some(scrutinee_snip) = snippet_opt(cx, scrutinee.span) + { + // Normalize the char using `map` so `join` doesn't use `Display`, if we don't then + // something like `r"\"` will become `'\'`, which is of course invalid + let pat_snip = val.as_str().chars().map(|c| format!("{c:?}")).join(" | "); + + span_lint_and_then( + cx, + STRING_LIT_CHARS_ANY, + expr.span, + "usage of `.chars().any(...)` to check if a char matches any from a string literal", + |diag| { + diag.span_suggestion_verbose( + expr.span, + "use `matches!(...)` instead", + format!("matches!({scrutinee_snip}, {pat_snip})"), + Applicability::MachineApplicable, + ); + } + ); + } +} diff --git a/tests/ui/string_lit_chars_any.fixed b/tests/ui/string_lit_chars_any.fixed new file mode 100644 index 000000000..d7ab9c339 --- /dev/null +++ b/tests/ui/string_lit_chars_any.fixed @@ -0,0 +1,50 @@ +//@run-rustfix +//@aux-build:proc_macros.rs:proc-macro +#![allow(clippy::eq_op, clippy::needless_raw_string_hashes, clippy::no_effect, unused)] +#![warn(clippy::string_lit_chars_any)] + +#[macro_use] +extern crate proc_macros; + +struct NotStringLit; + +impl NotStringLit { + fn chars(&self) -> impl Iterator { + "c".chars() + } +} + +fn main() { + let c = 'c'; + matches!(c, '\\' | '.' | '+' | '*' | '?' | '(' | ')' | '|' | '[' | ']' | '{' | '}' | '^' | '$' | '#' | '&' | '-' | '~'); + matches!(c, '\\' | '.' | '+' | '*' | '?' | '(' | ')' | '|' | '[' | ']' | '{' | '}' | '^' | '$' | '#' | '&' | '-' | '~'); + matches!(c, '\\' | '.' | '+' | '*' | '?' | '(' | ')' | '|' | '[' | ']' | '{' | '}' | '^' | '$' | '#' | '&' | '-' | '~'); + matches!(c, '\\' | '.' | '+' | '*' | '?' | '(' | ')' | '|' | '[' | ']' | '{' | '}' | '^' | '$' | '#' | '&' | '-' | '~'); + #[rustfmt::skip] + matches!(c, '\\' | '.' | '+' | '*' | '?' | '(' | ')' | '|' | '[' | ']' | '{' | '}' | '^' | '$' | '#' | '&' | '-' | '~'); + // Do not lint + NotStringLit.chars().any(|x| x == c); + "\\.+*?()|[]{}^$#&-~".chars().any(|x| { + let c = 'c'; + x == c + }); + "\\.+*?()|[]{}^$#&-~".chars().any(|x| { + 1; + x == c + }); + "\\.+*?()|[]{}^$#&-~".chars().any(|x| x == x); + "\\.+*?()|[]{}^$#&-~".chars().any(|_x| c == c); + matches!( + c, + '\\' | '.' | '+' | '*' | '(' | ')' | '|' | '[' | ']' | '{' | '}' | '^' | '$' | '#' | '&' | '-' | '~' + ); + external! { + let c = 'c'; + "\\.+*?()|[]{}^$#&-~".chars().any(|x| x == c); + } + with_span! { + span + let c = 'c'; + "\\.+*?()|[]{}^$#&-~".chars().any(|x| x == c); + } +} diff --git a/tests/ui/string_lit_chars_any.rs b/tests/ui/string_lit_chars_any.rs new file mode 100644 index 000000000..9408d7bb2 --- /dev/null +++ b/tests/ui/string_lit_chars_any.rs @@ -0,0 +1,50 @@ +//@run-rustfix +//@aux-build:proc_macros.rs:proc-macro +#![allow(clippy::eq_op, clippy::needless_raw_string_hashes, clippy::no_effect, unused)] +#![warn(clippy::string_lit_chars_any)] + +#[macro_use] +extern crate proc_macros; + +struct NotStringLit; + +impl NotStringLit { + fn chars(&self) -> impl Iterator { + "c".chars() + } +} + +fn main() { + let c = 'c'; + "\\.+*?()|[]{}^$#&-~".chars().any(|x| x == c); + r#"\.+*?()|[]{}^$#&-~"#.chars().any(|x| x == c); + "\\.+*?()|[]{}^$#&-~".chars().any(|x| c == x); + r#"\.+*?()|[]{}^$#&-~"#.chars().any(|x| c == x); + #[rustfmt::skip] + "\\.+*?()|[]{}^$#&-~".chars().any(|x| { x == c }); + // Do not lint + NotStringLit.chars().any(|x| x == c); + "\\.+*?()|[]{}^$#&-~".chars().any(|x| { + let c = 'c'; + x == c + }); + "\\.+*?()|[]{}^$#&-~".chars().any(|x| { + 1; + x == c + }); + "\\.+*?()|[]{}^$#&-~".chars().any(|x| x == x); + "\\.+*?()|[]{}^$#&-~".chars().any(|_x| c == c); + matches!( + c, + '\\' | '.' | '+' | '*' | '(' | ')' | '|' | '[' | ']' | '{' | '}' | '^' | '$' | '#' | '&' | '-' | '~' + ); + external! { + let c = 'c'; + "\\.+*?()|[]{}^$#&-~".chars().any(|x| x == c); + } + with_span! { + span + let c = 'c'; + "\\.+*?()|[]{}^$#&-~".chars().any(|x| x == c); + } +} diff --git a/tests/ui/string_lit_chars_any.stderr b/tests/ui/string_lit_chars_any.stderr new file mode 100644 index 000000000..ff951b73d --- /dev/null +++ b/tests/ui/string_lit_chars_any.stderr @@ -0,0 +1,58 @@ +error: usage of `.chars().any(...)` to check if a char matches any from a string literal + --> $DIR/string_lit_chars_any.rs:19:5 + | +LL | "//.+*?()|[]{}^$#&-~".chars().any(|x| x == c); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::string-lit-chars-any` implied by `-D warnings` +help: use `matches!(...)` instead + | +LL | matches!(c, '//' | '.' | '+' | '*' | '?' | '(' | ')' | '|' | '[' | ']' | '{' | '}' | '^' | '$' | '#' | '&' | '-' | '~'); + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +error: usage of `.chars().any(...)` to check if a char matches any from a string literal + --> $DIR/string_lit_chars_any.rs:20:5 + | +LL | r#"/.+*?()|[]{}^$#&-~"#.chars().any(|x| x == c); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: use `matches!(...)` instead + | +LL | matches!(c, '//' | '.' | '+' | '*' | '?' | '(' | ')' | '|' | '[' | ']' | '{' | '}' | '^' | '$' | '#' | '&' | '-' | '~'); + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +error: usage of `.chars().any(...)` to check if a char matches any from a string literal + --> $DIR/string_lit_chars_any.rs:21:5 + | +LL | "//.+*?()|[]{}^$#&-~".chars().any(|x| c == x); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: use `matches!(...)` instead + | +LL | matches!(c, '//' | '.' | '+' | '*' | '?' | '(' | ')' | '|' | '[' | ']' | '{' | '}' | '^' | '$' | '#' | '&' | '-' | '~'); + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +error: usage of `.chars().any(...)` to check if a char matches any from a string literal + --> $DIR/string_lit_chars_any.rs:22:5 + | +LL | r#"/.+*?()|[]{}^$#&-~"#.chars().any(|x| c == x); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: use `matches!(...)` instead + | +LL | matches!(c, '//' | '.' | '+' | '*' | '?' | '(' | ')' | '|' | '[' | ']' | '{' | '}' | '^' | '$' | '#' | '&' | '-' | '~'); + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +error: usage of `.chars().any(...)` to check if a char matches any from a string literal + --> $DIR/string_lit_chars_any.rs:24:5 + | +LL | "//.+*?()|[]{}^$#&-~".chars().any(|x| { x == c }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: use `matches!(...)` instead + | +LL | matches!(c, '//' | '.' | '+' | '*' | '?' | '(' | ')' | '|' | '[' | ']' | '{' | '}' | '^' | '$' | '#' | '&' | '-' | '~'); + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +error: aborting due to 5 previous errors +