Skip to content

Commit

Permalink
In string_patterns, use span instead of char to reduce allocation and…
Browse files Browse the repository at this point in the history 
… complexity and some nits changes.
  • Loading branch information
@AurelienFT
AurelienFT committed Jun 5, 2024
1 parent ec7e5d7 commit 9939549
Show file tree
Hide file tree
Showing 4 changed files with 74 additions and 49 deletions.
101 changes: 53 additions & 48 deletions clippy_lints/src/string_patterns.rs
View file
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -110,17 +111,17 @@ fn check_single_char_pattern_lint(cx: &LateContext<'_>, arg: &Expr<'_>) {
}
}

fn get_char_value(expr: &Expr<'_>) -> Option<String> {
fn get_char_span<'tcx>(cx: &'_ LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<Span> {
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
}
Expand All @@ -138,44 +139,39 @@ 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<String> = Vec::new();
let mut set_char_spans: Vec<Span> = 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.
if for_each_expr(body.value, |sub_expr| -> ControlFlow<(), Descend> {
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(())
}
},
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
},
Expand All @@ -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 => {},
},
)
}
}

Expand All @@ -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);
Expand Down
7 changes: 7 additions & 0 deletions tests/ui/manual_pattern_char_comparison.fixed
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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));
}
7 changes: 7 additions & 0 deletions tests/ui/manual_pattern_char_comparison.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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));
}
8 changes: 7 additions & 1 deletion tests/ui/manual_pattern_char_comparison.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

0 comments on commit 9939549

Please sign in to comment.