Skip to content

Commit

Permalink
Simplify lint logic and address code review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@nahuakang
nahuakang committed Aug 19, 2022
1 parent fb30b64 commit 36cba51
Show file tree
Hide file tree
Showing 5 changed files with 201 additions and 330 deletions.
301 changes: 61 additions & 240 deletions clippy_lints/src/methods/collapsible_str_replace.rs
View file
Original file line number Diff line number Diff line change
@@ -1,275 +1,96 @@
// run-rustfix

use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::get_parent_expr;
use clippy_utils::source::snippet;
use clippy_utils::visitors::for_each_expr;
use clippy_utils::{eq_expr_value, get_parent_expr};
use core::ops::ControlFlow;
use if_chain::if_chain;
use rustc_ast::ast::LitKind;
use rustc_data_structures::fx::FxHashSet;
use rustc_errors::Applicability;
use rustc_hir as hir;
use rustc_hir::{ExprKind, Path, QPath};
use rustc_lint::LateContext;
use rustc_middle::ty;
use rustc_span::source_map::Spanned;
use rustc_span::Span;
use std::collections::VecDeque;

use super::method_call;
use super::COLLAPSIBLE_STR_REPLACE;

pub(super) fn check<'tcx>(
cx: &LateContext<'tcx>,
expr: &'tcx hir::Expr<'tcx>,
name: &str,
recv: &'tcx hir::Expr<'tcx>,
from: &'tcx hir::Expr<'tcx>,
to: &'tcx hir::Expr<'tcx>,
) {
if name == "replace" {
// The receiver of the method call must be `str` type to lint `collapsible_str_replace`
let original_recv = find_original_recv(recv);
let original_recv_ty_kind = cx.typeck_results().expr_ty(original_recv).peel_refs().kind();
let original_recv_is_str_kind = matches!(original_recv_ty_kind, ty::Str);

if_chain! {
if original_recv_is_str_kind;
if let Some(parent) = get_parent_expr(cx, expr);
if let Some((name, ..)) = method_call(parent);
if name == "replace";

then {
// If the parent node is a `str::replace` call, we've already handled the lint, don't lint again
return;
}
let replace_methods = collect_replace_calls(cx, expr, to);
if replace_methods.methods.len() > 1 {
let from_kind = cx.typeck_results().expr_ty(from).peel_refs().kind();
// If the parent node's `to` argument is the same as the `to` argument
// of the last replace call in the current chain, don't lint as it was already linted
if let Some(parent) = get_parent_expr(cx, expr)
&& let Some(("replace", [_, current_from, current_to], _)) = method_call(parent)
&& eq_expr_value(cx, to, current_to)
&& from_kind == cx.typeck_results().expr_ty(current_from).peel_refs().kind()
{
return;
}

if let Some(("replace", ..)) = method_call(recv) {
// Check if there's an earlier `str::replace` call
if original_recv_is_str_kind {
check_consecutive_replace_calls(cx, expr);
}
}
check_consecutive_replace_calls(cx, expr, &replace_methods, to);
}
}

/// Check a chain of `str::replace` calls for `collapsible_str_replace` lint.
fn check_consecutive_replace_calls<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'tcx>) {
if_chain! {
if let Some(from_args) = get_replace_call_from_args_if_all_char_ty(cx, expr);
if let Some(to_arg) = get_replace_call_unique_to_arg_repr(expr);
then {
let earliest_replace_call_span = get_earliest_replace_call_span(expr);

if replace_call_from_args_are_only_lit_chars(&from_args) {
let from_arg_reprs: Vec<String> = from_args.iter().map(|from_arg| {
get_replace_call_char_arg_repr(from_arg).unwrap()
}).collect();
let app = Applicability::MachineApplicable;

span_lint_and_sugg(
cx,
COLLAPSIBLE_STR_REPLACE,
expr.span.with_lo(earliest_replace_call_span.lo()),
"used consecutive `str::replace` call",
"replace with",
format!(
"replace(|c| matches!(c, {}), {})",
from_arg_reprs.join(" | "),
to_arg,
),
app,
);
} else {
// Use fallback lint
let from_arg_reprs: Vec<String> = from_args.iter().map(|from_arg| {
get_replace_call_char_arg_repr(from_arg).unwrap()
}).collect();
let app = Applicability::MachineApplicable;

span_lint_and_sugg(
cx,
COLLAPSIBLE_STR_REPLACE,
expr.span.with_lo(earliest_replace_call_span.lo()),
"used consecutive `str::replace` call",
"replace with",
format!(
"replace(&[{}], {})",
from_arg_reprs.join(" , "),
to_arg,
),
app,
);
}
}
}
struct ReplaceMethods<'tcx> {
methods: VecDeque<&'tcx hir::Expr<'tcx>>,
from_args: VecDeque<&'tcx hir::Expr<'tcx>>,
}

/// Check if all the `from` arguments of a chain of consecutive calls to `str::replace`
/// are all of `ExprKind::Lit` types. If any is not, return false.
fn replace_call_from_args_are_only_lit_chars<'tcx>(from_args: &[&'tcx hir::Expr<'tcx>]) -> bool {
let mut only_lit_chars = true;

for from_arg in from_args.iter() {
match from_arg.kind {
ExprKind::Lit(..) => {},
_ => only_lit_chars = false,
}
}

only_lit_chars
}

/// Collect and return all of the `from` arguments of a chain of consecutive `str::replace` calls
/// if these `from` arguments's expressions are of the `ty::Char` kind. Otherwise return `None`.
fn get_replace_call_from_args_if_all_char_ty<'tcx>(
fn collect_replace_calls<'tcx>(
cx: &LateContext<'tcx>,
expr: &'tcx hir::Expr<'tcx>,
) -> Option<Vec<&'tcx hir::Expr<'tcx>>> {
let mut all_from_args_are_chars = true;
let mut from_args = Vec::new();
to_arg: &'tcx hir::Expr<'tcx>,
) -> ReplaceMethods<'tcx> {
let mut methods = VecDeque::new();
let mut from_args = VecDeque::new();

let _: Option<()> = for_each_expr(expr, |e| {
if let Some((name, [_, args @ ..], _)) = method_call(e) {
match (name, args) {
("replace", [from, _]) => {
let from_ty_kind = cx.typeck_results().expr_ty(from).peel_refs().kind();
if matches!(from_ty_kind, ty::Char) {
from_args.push(from);
} else {
all_from_args_are_chars = false;
}
ControlFlow::Continue(())
},
_ => ControlFlow::BREAK,
}
} else {
ControlFlow::Continue(())
}
});

if all_from_args_are_chars {
return Some(from_args);
}

None
}

/// Return a unique String representation of the `to` argument used in a chain of `str::replace`
/// calls if each `str::replace` call's `to` argument is identical to the other `to` arguments in
/// the chain. Otherwise, return `None`.
fn get_replace_call_unique_to_arg_repr<'tcx>(expr: &'tcx hir::Expr<'tcx>) -> Option<String> {
let mut to_args = Vec::new();

let _: Option<()> = for_each_expr(expr, |e| {
if let Some((name, [_, args @ ..], _)) = method_call(e) {
match (name, args) {
("replace", [_, to]) => {
to_args.push(to);
ControlFlow::Continue(())
},
_ => ControlFlow::BREAK,
if let Some(("replace", [_, from, to], _)) = method_call(e) {
if eq_expr_value(cx, to_arg, to) && cx.typeck_results().expr_ty(from).peel_refs().is_char() {
methods.push_front(e);
from_args.push_front(from);
ControlFlow::Continue(())
} else {
ControlFlow::BREAK
}
} else {
ControlFlow::Continue(())
}
});

// let mut to_arg_repr_set = FxHashSet::default();
let mut to_arg_reprs = Vec::new();
for &to_arg in &to_args {
if let Some(to_arg_repr) = get_replace_call_char_arg_repr(to_arg) {
to_arg_reprs.push(to_arg_repr);
}
}

let to_arg_repr_set = to_arg_reprs.iter().cloned().collect::<FxHashSet<_>>();
// Check if the set of `to` argument representations has more than one unique value
if to_arg_repr_set.len() != 1 {
return None;
}

// Return the single representation value
to_arg_reprs.pop()
ReplaceMethods { methods, from_args }
}

/// Get the representation of an argument of a `str::replace` call either of the literal char value
/// or variable name, i.e. the resolved path segments `ident`.
/// Return:
/// - the str literal with double quotes, e.g. "\"l\""
/// - the char literal with single quotes, e.g. "'l'"
/// - the variable as a String, e.g. "l"
fn get_replace_call_char_arg_repr<'tcx>(arg: &'tcx hir::Expr<'tcx>) -> Option<String> {
match arg.kind {
ExprKind::Lit(Spanned {
node: LitKind::Str(to_arg_val, _),
..
}) => {
let repr = to_arg_val.as_str();
let double_quote = "\"";
Some(double_quote.to_owned() + repr + double_quote)
},
ExprKind::Lit(Spanned {
node: LitKind::Char(to_arg_val),
..
}) => {
let repr = to_arg_val.to_string();
let double_quote = "\'";
Some(double_quote.to_owned() + &repr + double_quote)
},
ExprKind::Path(QPath::Resolved(
_,
Path {
segments: path_segments,
..
},
)) => {
// join the path_segments values by "::"
let path_segment_ident_names: Vec<&str> = path_segments
.iter()
.map(|path_seg| path_seg.ident.name.as_str())
.collect();
Some(path_segment_ident_names.join("::"))
},
_ => None,
/// Check a chain of `str::replace` calls for `collapsible_str_replace` lint.
fn check_consecutive_replace_calls<'tcx>(
cx: &LateContext<'tcx>,
expr: &'tcx hir::Expr<'tcx>,
replace_methods: &ReplaceMethods<'tcx>,
to_arg: &'tcx hir::Expr<'tcx>,
) {
let from_args = &replace_methods.from_args;
let from_arg_reprs: Vec<String> = from_args
.iter()
.map(|from_arg| snippet(cx, from_arg.span, "..").to_string())
.collect();
let app = Applicability::MachineApplicable;
let earliest_replace_call = replace_methods.methods.front().unwrap();
if let Some((_, [..], span_lo)) = method_call(earliest_replace_call) {
span_lint_and_sugg(
cx,
COLLAPSIBLE_STR_REPLACE,
expr.span.with_lo(span_lo.lo()),
"used consecutive `str::replace` call",
"replace with",
format!(
"replace([{}], {})",
from_arg_reprs.join(", "),
snippet(cx, to_arg.span, ".."),
),
app,
);
}
}

fn get_earliest_replace_call_span<'tcx>(expr: &'tcx hir::Expr<'tcx>) -> Span {
let mut earliest_replace_call_span = expr.span;

let _: Option<()> = for_each_expr(expr, |e| {
if let Some((name, [_, args @ ..], span)) = method_call(e) {
match (name, args) {
("replace", [_, _]) => {
earliest_replace_call_span = span;
ControlFlow::Continue(())
},
_ => ControlFlow::BREAK,
}
} else {
ControlFlow::Continue(())
}
});

earliest_replace_call_span
}

/// Find the original receiver of a chain of `str::replace` method calls.
fn find_original_recv<'tcx>(recv: &'tcx hir::Expr<'tcx>) -> &'tcx hir::Expr<'tcx> {
let mut original_recv = recv;

let _: Option<()> = for_each_expr(recv, |e| {
if let Some((name, [prev_recv, args @ ..], _)) = method_call(e) {
match (name, args) {
("replace", [_, _]) => {
original_recv = prev_recv;
ControlFlow::Continue(())
},
_ => ControlFlow::BREAK,
}
} else {
ControlFlow::Continue(())
}
});

original_recv
}
14 changes: 9 additions & 5 deletions clippy_lints/src/methods/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ declare_clippy_lint! {
/// ```
/// Use instead:
/// ```rust
/// let hello = "hesuo worpd".replace(|c| matches!(c, 's' | 'u' | 'p'), "l");
/// let hello = "hesuo worpd".replace(&['s', 'u', 'p'], "l");
/// ```
#[clippy::version = "1.64.0"]
pub COLLAPSIBLE_STR_REPLACE,
Expand Down Expand Up @@ -3507,6 +3507,14 @@ impl Methods {
("repeat", [arg]) => {
repeat_once::check(cx, expr, recv, arg);
},
(name @ ("replace" | "replacen"), [arg1, arg2] | [arg1, arg2, _]) => {
no_effect_replace::check(cx, expr, arg1, arg2);

// Check for repeated `str::replace` calls to perform `collapsible_str_replace` lint
if name == "replace" && let Some(("replace", ..)) = method_call(recv) {
collapsible_str_replace::check(cx, expr, arg1, arg2);
}
},
("resize", [count_arg, default_arg]) => {
vec_resize_to_zero::check(cx, expr, count_arg, default_arg, span);
},
Expand All @@ -3519,10 +3527,6 @@ impl Methods {
("sort_unstable_by", [arg]) => {
unnecessary_sort_by::check(cx, expr, recv, arg, true);
},
("replace" | "replacen", [arg1, arg2] | [arg1, arg2, _]) => {
no_effect_replace::check(cx, expr, arg1, arg2);
collapsible_str_replace::check(cx, expr, name, recv);
},
("splitn" | "rsplitn", [count_arg, pat_arg]) => {
if let Some((Constant::Int(count), _)) = constant(cx, cx.typeck_results(), count_arg) {
suspicious_splitn::check(cx, name, expr, recv, count);
Expand Down
Loading

0 comments on commit 36cba51

Please sign in to comment.