Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: incorrect suggestions generated by manual_retain lint #12084

Merged
merged 2 commits into from
Jan 27, 2024
Merged

fix: incorrect suggestions generated by manual_retain lint #12084

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b5a2192
fix: incorrect suggestions generated by `manual_retain` lint
yuxqiu Jan 3, 2024
03b3a16
test: add more test cases
yuxqiu Jan 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
141 changes: 100 additions & 41 deletions clippy_lints/src/manual_retain.rs
View file
Open in desktop
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,25 @@ 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}| {})",
{
if let hir::PatKind::Ref(pat, _) = filter_params.pat.kind {
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,
);
// Be conservative now. Do nothing for the `Binding` case.
// TODO: Ideally, we can rewrite the lambda by stripping one level of reference
}
}

Expand Down Expand Up @@ -229,3 +271,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,
);
}
63 changes: 63 additions & 0 deletions tests/ui/manual_retain.fixed
View file
Open in desktop
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 @@ -171,6 +195,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 +275,37 @@ 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_11457() {
// Do not lint, as we need to modify the closure
let mut vals = vec![1, 2, 3, 4];
vals = vals.iter().filter(|v| **v != 1).cloned().collect();

// Do not lint, as we need to modify the closure
let mut s = String::from("foobar");
s = s.chars().filter(|c| *c != 'o').to_owned().collect();
}

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);
}