Skip to content

Commit

Permalink
fix: incorrect suggestions generated by manual_retain lint
Browse files Browse the repository at this point in the history
  • Loading branch information
@yuxqiu
yuxqiu committed Jan 3, 2024
1 parent 9eb2b22 commit 8c7f950
Show file tree
Hide file tree
Showing 4 changed files with 334 additions and 64 deletions.
144 changes: 103 additions & 41 deletions clippy_lints/src/manual_retain.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use rustc_lint::{LateContext, LateLintPass};
use rustc_semver::RustcVersion;
use rustc_session::impl_lint_pass;
use rustc_span::symbol::sym;
use rustc_span::Span;

const ACCEPTABLE_METHODS: [&[&str]; 5] = [
&paths::BINARYHEAP_ITER,
Expand All @@ -28,6 +29,7 @@ const ACCEPTABLE_TYPES: [(rustc_span::Symbol, Option<RustcVersion>); 7] = [
(sym::Vec, None),
(sym::VecDeque, None),
];
const MAP_TYPES: [rustc_span::Symbol; 2] = [sym::BTreeMap, sym::HashMap];

declare_clippy_lint! {
/// ### What it does
Expand All @@ -44,6 +46,7 @@ declare_clippy_lint! {
/// ```no_run
/// let mut vec = vec![0, 1, 2];
/// vec.retain(|x| x % 2 == 0);
/// vec.retain(|x| x % 2 == 0);
/// ```
#[clippy::version = "1.64.0"]
pub MANUAL_RETAIN,
Expand Down Expand Up @@ -74,9 +77,9 @@ impl<'tcx> LateLintPass<'tcx> for ManualRetain {
&& let Some(collect_def_id) = cx.typeck_results().type_dependent_def_id(collect_expr.hir_id)
&& cx.tcx.is_diagnostic_item(sym::iterator_collect_fn, collect_def_id)
{
check_into_iter(cx, parent_expr, left_expr, target_expr, &self.msrv);
check_iter(cx, parent_expr, left_expr, target_expr, &self.msrv);
check_to_owned(cx, parent_expr, left_expr, target_expr, &self.msrv);
check_into_iter(cx, left_expr, target_expr, parent_expr.span, &self.msrv);
check_iter(cx, left_expr, target_expr, parent_expr.span, &self.msrv);
check_to_owned(cx, left_expr, target_expr, parent_expr.span, &self.msrv);
}
}

Expand All @@ -85,9 +88,9 @@ impl<'tcx> LateLintPass<'tcx> for ManualRetain {

fn check_into_iter(
cx: &LateContext<'_>,
parent_expr: &hir::Expr<'_>,
left_expr: &hir::Expr<'_>,
target_expr: &hir::Expr<'_>,
parent_expr_span: Span,
msrv: &Msrv,
) {
if let hir::ExprKind::MethodCall(_, into_iter_expr, [_], _) = &target_expr.kind
Expand All @@ -98,16 +101,39 @@ fn check_into_iter(
&& Some(into_iter_def_id) == cx.tcx.lang_items().into_iter_fn()
&& match_acceptable_type(cx, left_expr, msrv)
&& SpanlessEq::new(cx).eq_expr(left_expr, struct_expr)
&& let hir::ExprKind::MethodCall(_, _, [closure_expr], _) = target_expr.kind
&& let hir::ExprKind::Closure(closure) = closure_expr.kind
&& let filter_body = cx.tcx.hir().body(closure.body)
&& let [filter_params] = filter_body.params
{
suggest(cx, parent_expr, left_expr, target_expr);
if match_map_type(cx, left_expr) {
if let hir::PatKind::Tuple([key_pat, value_pat], _) = filter_params.pat.kind {
if let Some(sugg) = make_sugg(cx, key_pat, value_pat, left_expr, filter_body) {
make_span_lint_and_sugg(cx, parent_expr_span, sugg);
}
}
// Cannot lint other cases because `retain` requires two parameters
} else {
// Can always move because `retain` and `filter` have the same bound on the predicate
// for other types
make_span_lint_and_sugg(
cx,
parent_expr_span,
format!(
"{}.retain({})",
snippet(cx, left_expr.span, ".."),
snippet(cx, closure_expr.span, "..")
),
);
}
}
}

fn check_iter(
cx: &LateContext<'_>,
parent_expr: &hir::Expr<'_>,
left_expr: &hir::Expr<'_>,
target_expr: &hir::Expr<'_>,
parent_expr_span: Span,
msrv: &Msrv,
) {
if let hir::ExprKind::MethodCall(_, filter_expr, [], _) = &target_expr.kind
Expand All @@ -122,16 +148,50 @@ fn check_iter(
&& match_acceptable_def_path(cx, iter_expr_def_id)
&& match_acceptable_type(cx, left_expr, msrv)
&& SpanlessEq::new(cx).eq_expr(left_expr, struct_expr)
&& let hir::ExprKind::MethodCall(_, _, [closure_expr], _) = filter_expr.kind
&& let hir::ExprKind::Closure(closure) = closure_expr.kind
&& let filter_body = cx.tcx.hir().body(closure.body)
&& let [filter_params] = filter_body.params
{
suggest(cx, parent_expr, left_expr, filter_expr);
match filter_params.pat.kind {
// hir::PatKind::Binding(_, _, _, None) => {
// // Be conservative now. Do nothing here.
// // TODO: Ideally, we can rewrite the lambda by stripping one level of reference
// },
hir::PatKind::Tuple([_, _], _) => {
// the `&&` reference for the `filter` method will be auto derefed to `ref`
// so, we can directly use the lambda
// https://doc.rust-lang.org/reference/patterns.html#binding-modes
make_span_lint_and_sugg(
cx,
parent_expr_span,
format!(
"{}.retain({})",
snippet(cx, left_expr.span, ".."),
snippet(cx, closure_expr.span, "..")
),
);
},
hir::PatKind::Ref(pat, _) => make_span_lint_and_sugg(
cx,
parent_expr_span,
format!(
"{}.retain(|{}| {})",
snippet(cx, left_expr.span, ".."),
snippet(cx, pat.span, ".."),
snippet(cx, filter_body.value.span, "..")
),
),
_ => {},
}
}
}

fn check_to_owned(
cx: &LateContext<'_>,
parent_expr: &hir::Expr<'_>,
left_expr: &hir::Expr<'_>,
target_expr: &hir::Expr<'_>,
parent_expr_span: Span,
msrv: &Msrv,
) {
if msrv.meets(msrvs::STRING_RETAIN)
Expand All @@ -147,43 +207,28 @@ fn check_to_owned(
&& let ty = cx.typeck_results().expr_ty(str_expr).peel_refs()
&& is_type_lang_item(cx, ty, hir::LangItem::String)
&& SpanlessEq::new(cx).eq_expr(left_expr, str_expr)
{
suggest(cx, parent_expr, left_expr, filter_expr);
}
}

fn suggest(cx: &LateContext<'_>, parent_expr: &hir::Expr<'_>, left_expr: &hir::Expr<'_>, filter_expr: &hir::Expr<'_>) {
if let hir::ExprKind::MethodCall(_, _, [closure], _) = filter_expr.kind
&& let hir::ExprKind::Closure(&hir::Closure { body, .. }) = closure.kind
&& let filter_body = cx.tcx.hir().body(body)
&& let hir::ExprKind::MethodCall(_, _, [closure_expr], _) = filter_expr.kind
&& let hir::ExprKind::Closure(closure) = closure_expr.kind
&& let filter_body = cx.tcx.hir().body(closure.body)
&& let [filter_params] = filter_body.params
&& let Some(sugg) = match filter_params.pat.kind {
hir::PatKind::Binding(_, _, filter_param_ident, None) => Some(format!(
"{}.retain(|{filter_param_ident}| {})",
snippet(cx, left_expr.span, ".."),
snippet(cx, filter_body.value.span, "..")
)),
hir::PatKind::Tuple([key_pat, value_pat], _) => make_sugg(cx, key_pat, value_pat, left_expr, filter_body),
hir::PatKind::Ref(pat, _) => match pat.kind {
hir::PatKind::Binding(_, _, filter_param_ident, None) => Some(format!(
"{}.retain(|{filter_param_ident}| {})",
{
match filter_params.pat.kind {
// hir::PatKind::Binding(_, _, _, None) => {
// // Be conservative now. Do nothing here.
// // TODO: Ideally, we can rewrite the lambda by stripping one level of reference
// },
hir::PatKind::Ref(pat, _) => make_span_lint_and_sugg(
cx,
parent_expr_span,
format!(
"{}.retain(|{}| {})",
snippet(cx, left_expr.span, ".."),
snippet(cx, pat.span, ".."),
snippet(cx, filter_body.value.span, "..")
)),
_ => None,
},
_ => None,
),
),
_ => {},
}
{
span_lint_and_sugg(
cx,
MANUAL_RETAIN,
parent_expr.span,
"this expression can be written more simply using `.retain()`",
"consider calling `.retain()` instead",
sugg,
Applicability::MachineApplicable,
);
}
}

Expand Down Expand Up @@ -229,3 +274,20 @@ fn match_acceptable_type(cx: &LateContext<'_>, expr: &hir::Expr<'_>, msrv: &Msrv
&& acceptable_msrv.map_or(true, |acceptable_msrv| msrv.meets(acceptable_msrv))
})
}

fn match_map_type(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> bool {
let expr_ty = cx.typeck_results().expr_ty(expr).peel_refs();
MAP_TYPES.iter().any(|ty| is_type_diagnostic_item(cx, expr_ty, *ty))
}

fn make_span_lint_and_sugg(cx: &LateContext<'_>, span: Span, sugg: String) {
span_lint_and_sugg(
cx,
MANUAL_RETAIN,
span,
"this expression can be written more simply using `.retain()`",
"consider calling `.retain()` instead",
sugg,
Applicability::MachineApplicable,
);
}
56 changes: 56 additions & 0 deletions tests/ui/manual_retain.fixed
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ fn main() {
_msrv_153();
_msrv_126();
_msrv_118();

issue_10393();
issue_12081();
}

fn binary_heap_retain() {
Expand All @@ -23,6 +26,11 @@ fn binary_heap_retain() {
binary_heap.retain(|x| x % 2 == 0);
binary_heap.retain(|x| x % 2 == 0);

// Do lint, because we use pattern matching
let mut tuples = BinaryHeap::from([(0, 1), (1, 2), (2, 3)]);
tuples.retain(|(ref x, ref y)| *x == 0);
tuples.retain(|(x, y)| *x == 0);

// Do not lint, because type conversion is performed
binary_heap = binary_heap
.into_iter()
Expand Down Expand Up @@ -55,6 +63,9 @@ fn btree_map_retain() {
btree_map.retain(|_, &mut v| v % 2 == 0);
btree_map.retain(|k, &mut v| (k % 2 == 0) && (v % 2 == 0));

// Do not lint, because the parameters are not matched in tuple pattern
btree_map = btree_map.into_iter().filter(|t| t.0 % 2 == 0).collect();

// Do not lint.
btree_map = btree_map
.into_iter()
Expand All @@ -76,6 +87,11 @@ fn btree_set_retain() {
btree_set.retain(|x| x % 2 == 0);
btree_set.retain(|x| x % 2 == 0);

// Do lint, because we use pattern matching
let mut tuples = BTreeSet::from([(0, 1), (1, 2), (2, 3)]);
tuples.retain(|(ref x, ref y)| *x == 0);
tuples.retain(|(x, y)| *x == 0);

// Do not lint, because type conversion is performed
btree_set = btree_set
.iter()
Expand Down Expand Up @@ -108,6 +124,9 @@ fn hash_map_retain() {
hash_map.retain(|_, &mut v| v % 2 == 0);
hash_map.retain(|k, &mut v| (k % 2 == 0) && (v % 2 == 0));

// Do not lint, because the parameters are not matched in tuple pattern
hash_map = hash_map.into_iter().filter(|t| t.0 % 2 == 0).collect();

// Do not lint.
hash_map = hash_map
.into_iter()
Expand All @@ -128,6 +147,11 @@ fn hash_set_retain() {
hash_set.retain(|x| x % 2 == 0);
hash_set.retain(|x| x % 2 == 0);

// Do lint, because we use pattern matching
let mut tuples = HashSet::from([(0, 1), (1, 2), (2, 3)]);
tuples.retain(|(ref x, ref y)| *x == 0);
tuples.retain(|(x, y)| *x == 0);

// Do not lint, because type conversion is performed
hash_set = hash_set.into_iter().filter(|x| x % 2 == 0).collect::<HashSet<i8>>();
hash_set = hash_set
Expand Down Expand Up @@ -157,6 +181,9 @@ fn string_retain() {
// Do lint.
s.retain(|c| c != 'o');

// Do not lint, because we need to rewrite the lambda
s = s.chars().filter(|c| *c != 'o').to_owned().collect();

// Do not lint, because this expression is not assign.
let mut bar: String = s.chars().filter(|&c| c != 'o').to_owned().collect();

Expand All @@ -171,6 +198,11 @@ fn vec_retain() {
vec.retain(|x| x % 2 == 0);
vec.retain(|x| x % 2 == 0);

// Do lint, because we use pattern matching
let mut tuples = vec![(0, 1), (1, 2), (2, 3)];
tuples.retain(|(ref x, ref y)| *x == 0);
tuples.retain(|(x, y)| *x == 0);

// Do not lint, because type conversion is performed
vec = vec.into_iter().filter(|x| x % 2 == 0).collect::<Vec<i8>>();
vec = vec.iter().filter(|&x| x % 2 == 0).copied().collect::<Vec<i8>>();
Expand Down Expand Up @@ -246,3 +278,27 @@ fn _msrv_118() {
let mut hash_map: HashMap<i8, i8> = (0..8).map(|x| (x, x * 10)).collect();
hash_map = hash_map.into_iter().filter(|(k, _)| k % 2 == 0).collect();
}

fn issue_10393() {
// Do lint
let mut vec = vec![(0, 1), (1, 2), (2, 3)];
vec.retain(|(x, y)| *x == 0);

// Do lint
let mut tuples = vec![(true, -2), (false, 3)];
tuples.retain(|(_, n)| *n > 0);
}

fn issue_12081() {
let mut vec = vec![0, 1, 2];

// Do lint
vec.retain(|&x| x == 0);
vec.retain(|&x| x == 0);
vec.retain(|&x| x == 0);

// Do lint
vec.retain(|x| *x == 0);
vec.retain(|x| *x == 0);
vec.retain(|x| *x == 0);
}

0 comments on commit 8c7f950

Please sign in to comment.