From 9939549dae61ab8684cb99eb1f74ea769bf72043 Mon Sep 17 00:00:00 2001 From: AurelienFT Date: Wed, 5 Jun 2024 11:50:55 +0200 Subject: [PATCH] In string_patterns, use span instead of char to reduce allocation and complexity and some nits changes. --- clippy_lints/src/string_patterns.rs | 101 +++++++++--------- tests/ui/manual_pattern_char_comparison.fixed | 7 ++ tests/ui/manual_pattern_char_comparison.rs | 7 ++ .../ui/manual_pattern_char_comparison.stderr | 8 +- 4 files changed, 74 insertions(+), 49 deletions(-) diff --git a/clippy_lints/src/string_patterns.rs b/clippy_lints/src/string_patterns.rs index 5402d30bd834..b7d87a7efc13 100644 --- a/clippy_lints/src/string_patterns.rs +++ b/clippy_lints/src/string_patterns.rs @@ -1,18 +1,19 @@ use std::cmp::Ordering; use std::ops::ControlFlow; -use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then}; use clippy_utils::macros::matching_root_macro_call; use clippy_utils::path_to_local_id; -use clippy_utils::source::str_literal_to_char_literal; +use clippy_utils::source::{snippet_opt, str_literal_to_char_literal}; use clippy_utils::visitors::{for_each_expr, Descend}; +use itertools::Itertools; use rustc_ast::{BinOpKind, LitKind}; use rustc_errors::Applicability; use rustc_hir::{Expr, ExprKind, PatKind, QPath}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty; use rustc_session::declare_lint_pass; -use rustc_span::sym; +use rustc_span::{sym, Span}; declare_clippy_lint! { /// ### What it does @@ -110,17 +111,17 @@ fn check_single_char_pattern_lint(cx: &LateContext<'_>, arg: &Expr<'_>) { } } -fn get_char_value(expr: &Expr<'_>) -> Option { +fn get_char_span<'tcx>(cx: &'_ LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option { + if !cx.typeck_results().expr_ty_adjusted(expr).is_char() || expr.span.from_expansion() { + return None; + } match expr.kind { - ExprKind::Lit(lit) => match lit.node { - LitKind::Char(c) => Some(format!("'{}'", c.escape_default())), - _ => None, - }, + ExprKind::Lit(lit) if let LitKind::Char(_) = lit.node => Some(lit.span), ExprKind::Path(QPath::Resolved(_, path)) => { if path.segments.len() == 1 { let segment = &path.segments[0]; if segment.args.is_none() { - Some(segment.ident.name.to_string()) + Some(segment.ident.span) } else { None } @@ -138,7 +139,7 @@ fn check_manual_pattern_char_comparison(cx: &LateContext<'_>, method_arg: &Expr< && let body = cx.tcx.hir().body(closure.body) && let PatKind::Binding(_, binding, ..) = body.params[0].pat.kind { - let mut set_chars: Vec = Vec::new(); + let mut set_char_spans: Vec = Vec::new(); // We want to retrieve all the comparisons done. // They are ordered in a nested way and so we need to traverse the AST to collect them all. @@ -146,16 +147,14 @@ fn check_manual_pattern_char_comparison(cx: &LateContext<'_>, method_arg: &Expr< match sub_expr.kind { ExprKind::Binary(op, left, right) if op.node == BinOpKind::Eq => { if path_to_local_id(left, binding) - && cx.typeck_results().expr_ty_adjusted(right).kind() == &ty::Char - && let Some(c) = get_char_value(right) + && let Some(span) = get_char_span(cx, right) { - set_chars.push(c); + set_char_spans.push(span); ControlFlow::Continue(Descend::No) } else if path_to_local_id(right, binding) - && cx.typeck_results().expr_ty_adjusted(left).kind() == &ty::Char - && let Some(c) = get_char_value(left) + && let Some(span) = get_char_span(cx, left) { - set_chars.push(c); + set_char_spans.push(span); ControlFlow::Continue(Descend::No) } else { ControlFlow::Break(()) @@ -163,19 +162,16 @@ fn check_manual_pattern_char_comparison(cx: &LateContext<'_>, method_arg: &Expr< }, ExprKind::Binary(op, _, _) if op.node == BinOpKind::Or => ControlFlow::Continue(Descend::Yes), ExprKind::Match(match_value, [arm, _], _) => { - if matching_root_macro_call(cx, sub_expr.span, sym::matches_macro).is_none() { - return ControlFlow::Break(()); - } - if arm.guard.is_some() { - return ControlFlow::Break(()); - } - if !path_to_local_id(match_value, binding) { + if matching_root_macro_call(cx, sub_expr.span, sym::matches_macro).is_none() + || arm.guard.is_some() + || !path_to_local_id(match_value, binding) + { return ControlFlow::Break(()); } if arm.pat.walk_short(|pat| match pat.kind { PatKind::Lit(expr) if let ExprKind::Lit(lit) = expr.kind => { - if let LitKind::Char(c) = lit.node { - set_chars.push(format!("'{}'", c.escape_default())); + if let LitKind::Char(_) = lit.node { + set_char_spans.push(lit.span); } true }, @@ -194,27 +190,37 @@ fn check_manual_pattern_char_comparison(cx: &LateContext<'_>, method_arg: &Expr< { return; } - match set_chars.len().cmp(&1) { - Ordering::Equal => span_lint_and_sugg( - cx, - MANUAL_PATTERN_CHAR_COMPARISON, - method_arg.span, - "this manual char comparison can be written more succinctly", - "consider using a `char`", - set_chars[0].clone(), - applicability, - ), - Ordering::Greater => span_lint_and_sugg( - cx, - MANUAL_PATTERN_CHAR_COMPARISON, - method_arg.span, - "this manual char comparison can be written more succinctly", - "consider using an array of `char`", - format!("[{}]", set_chars.join(", ")), - applicability, - ), - Ordering::Less => {}, - } + span_lint_and_then( + cx, + MANUAL_PATTERN_CHAR_COMPARISON, + method_arg.span, + "this manual char comparison can be written more succinctly", + |diag| match set_char_spans.len().cmp(&1) { + Ordering::Equal => { + diag.span_suggestion( + method_arg.span, + "consider using a `char`", + snippet_opt(cx, set_char_spans[0]).unwrap(), + applicability, + ); + }, + Ordering::Greater => { + diag.span_suggestion( + method_arg.span, + "consider using an array of `char`", + format!( + "[{}]", + set_char_spans + .into_iter() + .map(|span| snippet_opt(cx, span).unwrap()) + .join(", ") + ), + applicability, + ); + }, + Ordering::Less => {}, + }, + ) } } @@ -228,9 +234,8 @@ impl<'tcx> LateLintPass<'tcx> for StringPatterns { && let Some(&(_, pos)) = PATTERN_METHODS .iter() .find(|(array_method_name, _)| *array_method_name == method_name) - && args.len() > pos + && let Some(arg) = args.get(pos) { - let arg = &args[pos]; check_single_char_pattern_lint(cx, arg); check_manual_pattern_char_comparison(cx, arg); diff --git a/tests/ui/manual_pattern_char_comparison.fixed b/tests/ui/manual_pattern_char_comparison.fixed index 3ebd5047c83f..588226b87e87 100644 --- a/tests/ui/manual_pattern_char_comparison.fixed +++ b/tests/ui/manual_pattern_char_comparison.fixed @@ -18,6 +18,7 @@ fn main() { sentence.split(['\n', 'X', 'Y']); sentence.splitn(3, 'X'); sentence.splitn(3, ['X', 'W']); + sentence.find('🎈'); let not_str = NotStr; not_str.find(|c: char| c == 'X'); @@ -39,4 +40,10 @@ fn main() { "".find(|c| matches!(c, 'a' | '1'..'4')); "".find(|c| c == 'a' || matches!(c, '1'..'4')); + macro_rules! m { + ($e:expr) => { + $e == '?' + }; + } + "".find(|c| m!(c)); } diff --git a/tests/ui/manual_pattern_char_comparison.rs b/tests/ui/manual_pattern_char_comparison.rs index f6dcec1705b2..5078f3ee27f3 100644 --- a/tests/ui/manual_pattern_char_comparison.rs +++ b/tests/ui/manual_pattern_char_comparison.rs @@ -18,6 +18,7 @@ fn main() { sentence.split(|c: char| matches!(c, '\n' | 'X' | 'Y')); sentence.splitn(3, |c: char| matches!(c, 'X')); sentence.splitn(3, |c: char| matches!(c, 'X' | 'W')); + sentence.find(|c| c == '🎈'); let not_str = NotStr; not_str.find(|c: char| c == 'X'); @@ -39,4 +40,10 @@ fn main() { "".find(|c| matches!(c, 'a' | '1'..'4')); "".find(|c| c == 'a' || matches!(c, '1'..'4')); + macro_rules! m { + ($e:expr) => { + $e == '?' + }; + } + "".find(|c| m!(c)); } diff --git a/tests/ui/manual_pattern_char_comparison.stderr b/tests/ui/manual_pattern_char_comparison.stderr index 4d08266bce53..b6b51794a11f 100644 --- a/tests/ui/manual_pattern_char_comparison.stderr +++ b/tests/ui/manual_pattern_char_comparison.stderr @@ -49,5 +49,11 @@ error: this manual char comparison can be written more succinctly LL | sentence.splitn(3, |c: char| matches!(c, 'X' | 'W')); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using an array of `char`: `['X', 'W']` -error: aborting due to 8 previous errors +error: this manual char comparison can be written more succinctly + --> tests/ui/manual_pattern_char_comparison.rs:21:19 + | +LL | sentence.find(|c| c == '🎈'); + | ^^^^^^^^^^^^^ help: consider using a `char`: `'🎈'` + +error: aborting due to 9 previous errors