squiid/rust
1
0
Fork 0
Code Issues Pull requests Projects Activity

Auto merge of #8479 - smoelius:unnecessary-filter-map, r=llogiq

Browse source 
Fix some `unnecessary_filter_map` false positives

This is a proposed fix for #4433.

It moves `clone_or_copy_needed` out of `unnecessary_iter_cloned.rs` and into `methods::utils`. It then adds a check of this function to `unnecessary_filter_map::check`.

Fixes #4433

changelog: none
This commit is contained in:
bors 2022-02-28 20:09:18 +00:00
parent 8d12cd4779 d123ffc6c7
commit e511476f24
5 changed files with 228 additions and 94 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

7
clippy_lints/src/methods/unnecessary_filter_map.rs
View file

@ -1,4 +1,6 @@
use super::utils::clone_or_copy_needed;
use clippy_utils::diagnostics::span_lint;
use clippy_utils::ty::is_copy;
use clippy_utils::usage::mutated_variables;
use clippy_utils::{is_lang_ctor, is_trait_method, path_to_local_id};
use rustc_hir as hir;
@ -20,6 +22,7 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, arg: &hir::Expr<
let arg_id = body.params[0].pat.hir_id;
let mutates_arg =
mutated_variables(&body.value, cx).map_or(true, |used_mutably| used_mutably.contains(&arg_id));
let (clone_or_copy_needed, _) = clone_or_copy_needed(cx, body.params[0].pat, &body.value);
let (mut found_mapping, mut found_filtering) = check_expression(cx, arg_id, &body.value);
@ -28,10 +31,10 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, arg: &hir::Expr<
found_mapping |= return_visitor.found_mapping;
found_filtering |= return_visitor.found_filtering;
let in_ty = cx.typeck_results().node_type(body.params[0].hir_id);
let sugg = if !found_filtering {
"map"
} else if !found_mapping && !mutates_arg {
let in_ty = cx.typeck_results().node_type(body.params[0].hir_id);
} else if !found_mapping && !mutates_arg && (!clone_or_copy_needed || is_copy(cx, in_ty)) {
match cx.typeck_results().expr_ty(&body.value).kind() {
ty::Adt(adt, subst) if cx.tcx.is_diagnostic_item(sym::Option, adt.did) && in_ty == subst.type_at(0) => {
"filter"

91
clippy_lints/src/methods/unnecessary_iter_cloned.rs
View file

@ -1,14 +1,12 @@
use super::utils::clone_or_copy_needed;
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::higher::ForLoop;
use clippy_utils::source::snippet_opt;
use clippy_utils::ty::{get_associated_type, get_iterator_item_ty, implements_trait};
use clippy_utils::{fn_def_id, get_parent_expr, path_to_local_id, usage};
use clippy_utils::{fn_def_id, get_parent_expr};
use rustc_errors::Applicability;
use rustc_hir::intravisit::{walk_expr, Visitor};
use rustc_hir::{def_id::DefId, BorrowKind, Expr, ExprKind, HirId, LangItem, Mutability, Pat};
use rustc_hir::{def_id::DefId, Expr, ExprKind, LangItem};
use rustc_lint::LateContext;
use rustc_middle::hir::nested_filter;
use rustc_middle::ty;
use rustc_span::{sym, Symbol};
use super::UNNECESSARY_TO_OWNED;
@ -100,89 +98,6 @@ pub fn check_for_loop_iter(
false
}
/// The core logic of `check_for_loop_iter` above, this function wraps a use of
/// `CloneOrCopyVisitor`.
fn clone_or_copy_needed<'tcx>(
cx: &LateContext<'tcx>,
pat: &Pat<'tcx>,
body: &'tcx Expr<'tcx>,
) -> (bool, Vec<&'tcx Expr<'tcx>>) {
let mut visitor = CloneOrCopyVisitor {
cx,
binding_hir_ids: pat_bindings(pat),
clone_or_copy_needed: false,
addr_of_exprs: Vec::new(),
};
visitor.visit_expr(body);
(visitor.clone_or_copy_needed, visitor.addr_of_exprs)
}
/// Returns a vector of all `HirId`s bound by the pattern.
fn pat_bindings(pat: &Pat<'_>) -> Vec<HirId> {
let mut collector = usage::ParamBindingIdCollector {
binding_hir_ids: Vec::new(),
};
collector.visit_pat(pat);
collector.binding_hir_ids
}
/// `clone_or_copy_needed` will be false when `CloneOrCopyVisitor` is done visiting if the only
/// operations performed on `binding_hir_ids` are:
/// * to take non-mutable references to them
/// * to use them as non-mutable `&self` in method calls
/// If any of `binding_hir_ids` is used in any other way, then `clone_or_copy_needed` will be true
/// when `CloneOrCopyVisitor` is done visiting.
struct CloneOrCopyVisitor<'cx, 'tcx> {
cx: &'cx LateContext<'tcx>,
binding_hir_ids: Vec<HirId>,
clone_or_copy_needed: bool,
addr_of_exprs: Vec<&'tcx Expr<'tcx>>,
}
impl<'cx, 'tcx> Visitor<'tcx> for CloneOrCopyVisitor<'cx, 'tcx> {
type NestedFilter = nested_filter::OnlyBodies;
fn nested_visit_map(&mut self) -> Self::Map {
self.cx.tcx.hir()
}
fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) {
walk_expr(self, expr);
if self.is_binding(expr) {
if let Some(parent) = get_parent_expr(self.cx, expr) {
match parent.kind {
ExprKind::AddrOf(BorrowKind::Ref, Mutability::Not, _) => {
self.addr_of_exprs.push(parent);
return;
},
ExprKind::MethodCall(_, args, _) => {
if_chain! {
if args.iter().skip(1).all(|arg| !self.is_binding(arg));
if let Some(method_def_id) = self.cx.typeck_results().type_dependent_def_id(parent.hir_id);
let method_ty = self.cx.tcx.type_of(method_def_id);
let self_ty = method_ty.fn_sig(self.cx.tcx).input(0).skip_binder();
if matches!(self_ty.kind(), ty::Ref(_, _, Mutability::Not));
then {
return;
}
}
},
_ => {},
}
}
self.clone_or_copy_needed = true;
}
}
}
impl<'cx, 'tcx> CloneOrCopyVisitor<'cx, 'tcx> {
fn is_binding(&self, expr: &Expr<'tcx>) -> bool {
self.binding_hir_ids
.iter()
.any(|hir_id| path_to_local_id(expr, *hir_id))
}
}
/// Returns true if the named method is `IntoIterator::into_iter`.
pub fn is_into_iter(cx: &LateContext<'_>, callee_def_id: DefId) -> bool {
cx.tcx.lang_items().require(LangItem::IntoIterIntoIter) == Ok(callee_def_id)

87
clippy_lints/src/methods/utils.rs
View file

@ -1,10 +1,14 @@
use clippy_utils::source::snippet_with_applicability;
use clippy_utils::ty::is_type_diagnostic_item;
use clippy_utils::{get_parent_expr, path_to_local_id, usage};
use if_chain::if_chain;
use rustc_ast::ast;
use rustc_errors::Applicability;
use rustc_hir as hir;
use rustc_hir::intravisit::{walk_expr, Visitor};
use rustc_hir::{BorrowKind, Expr, ExprKind, HirId, Mutability, Pat};
use rustc_lint::LateContext;
use rustc_middle::hir::nested_filter;
use rustc_middle::ty::{self, Ty};
use rustc_span::symbol::sym;
@ -79,3 +83,86 @@ pub(super) fn get_hint_if_single_char_arg(
}
}
}
/// The core logic of `check_for_loop_iter` in `unnecessary_iter_cloned.rs`, this function wraps a
/// use of `CloneOrCopyVisitor`.
pub(super) fn clone_or_copy_needed<'tcx>(
cx: &LateContext<'tcx>,
pat: &Pat<'tcx>,
body: &'tcx Expr<'tcx>,
) -> (bool, Vec<&'tcx Expr<'tcx>>) {
let mut visitor = CloneOrCopyVisitor {
cx,
binding_hir_ids: pat_bindings(pat),
clone_or_copy_needed: false,
addr_of_exprs: Vec::new(),
};
visitor.visit_expr(body);
(visitor.clone_or_copy_needed, visitor.addr_of_exprs)
}
/// Returns a vector of all `HirId`s bound by the pattern.
fn pat_bindings(pat: &Pat<'_>) -> Vec<HirId> {
let mut collector = usage::ParamBindingIdCollector {
binding_hir_ids: Vec::new(),
};
collector.visit_pat(pat);
collector.binding_hir_ids
}
/// `clone_or_copy_needed` will be false when `CloneOrCopyVisitor` is done visiting if the only
/// operations performed on `binding_hir_ids` are:
/// * to take non-mutable references to them
/// * to use them as non-mutable `&self` in method calls
/// If any of `binding_hir_ids` is used in any other way, then `clone_or_copy_needed` will be true
/// when `CloneOrCopyVisitor` is done visiting.
struct CloneOrCopyVisitor<'cx, 'tcx> {
cx: &'cx LateContext<'tcx>,
binding_hir_ids: Vec<HirId>,
clone_or_copy_needed: bool,
addr_of_exprs: Vec<&'tcx Expr<'tcx>>,
}
impl<'cx, 'tcx> Visitor<'tcx> for CloneOrCopyVisitor<'cx, 'tcx> {
type NestedFilter = nested_filter::OnlyBodies;
fn nested_visit_map(&mut self) -> Self::Map {
self.cx.tcx.hir()
}
fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) {
walk_expr(self, expr);
if self.is_binding(expr) {
if let Some(parent) = get_parent_expr(self.cx, expr) {
match parent.kind {
ExprKind::AddrOf(BorrowKind::Ref, Mutability::Not, _) => {
self.addr_of_exprs.push(parent);
return;
},
ExprKind::MethodCall(_, args, _) => {
if_chain! {
if args.iter().skip(1).all(|arg| !self.is_binding(arg));
if let Some(method_def_id) = self.cx.typeck_results().type_dependent_def_id(parent.hir_id);
let method_ty = self.cx.tcx.type_of(method_def_id);
let self_ty = method_ty.fn_sig(self.cx.tcx).input(0).skip_binder();
if matches!(self_ty.kind(), ty::Ref(_, _, Mutability::Not));
then {
return;
}
}
},
_ => {},
}
}
self.clone_or_copy_needed = true;
}
}
}
impl<'cx, 'tcx> CloneOrCopyVisitor<'cx, 'tcx> {
fn is_binding(&self, expr: &Expr<'tcx>) -> bool {
self.binding_hir_ids
.iter()
.any(|hir_id| path_to_local_id(expr, *hir_id))
}
}

129
tests/ui/unnecessary_filter_map.rs
View file

@ -1,3 +1,5 @@
#![allow(dead_code)]
fn main() {
let _ = (0..4).filter_map(|x| if x > 1 { Some(x) } else { None });
let _ = (0..4).filter_map(|x| {
@ -19,3 +21,130 @@ fn main() {
fn filter_map_none_changes_item_type() -> impl Iterator<Item = bool> {
"".chars().filter_map(|_| None)
}
// https://github.com/rust-lang/rust-clippy/issues/4433#issue-483920107
mod comment_483920107 {
enum Severity {
Warning,
Other,
}
struct ServerError;
impl ServerError {
fn severity(&self) -> Severity {
Severity::Warning
}
}
struct S {
warnings: Vec<ServerError>,
}
impl S {
fn foo(&mut self, server_errors: Vec<ServerError>) {
#[allow(unused_variables)]
let errors: Vec<ServerError> = server_errors
.into_iter()
.filter_map(|se| match se.severity() {
Severity::Warning => {
self.warnings.push(se);
None
},
_ => Some(se),
})
.collect();
}
}
}
// https://github.com/rust-lang/rust-clippy/issues/4433#issuecomment-611006622
mod comment_611006622 {
struct PendingRequest {
reply_to: u8,
token: u8,
expires: u8,
group_id: u8,
}
enum Value {
Null,
}
struct Node;
impl Node {
fn send_response(&self, _reply_to: u8, _token: u8, _value: Value) -> &Self {
self
}
fn on_error_warn(&self) -> &Self {
self
}
}
struct S {
pending_requests: Vec<PendingRequest>,
}
impl S {
fn foo(&mut self, node: Node, now: u8, group_id: u8) {
// "drain_filter"
self.pending_requests = self
.pending_requests
.drain(..)
.filter_map(|pending| {
if pending.expires <= now {
return None; // Expired, remove
}
if pending.group_id == group_id {
// Matched - reuse strings and remove
node.send_response(pending.reply_to, pending.token, Value::Null)
.on_error_warn();
None
} else {
// Keep waiting
Some(pending)
}
})
.collect();
}
}
}
// https://github.com/rust-lang/rust-clippy/issues/4433#issuecomment-621925270
// This extrapolation doesn't reproduce the false positive. Additional context seems necessary.
mod comment_621925270 {
struct Signature(u8);
fn foo(sig_packets: impl Iterator<Item = Result<Signature, ()>>) -> impl Iterator<Item = u8> {
sig_packets.filter_map(|res| match res {
Ok(Signature(sig_packet)) => Some(sig_packet),
_ => None,
})
}
}
// https://github.com/rust-lang/rust-clippy/issues/4433#issuecomment-1052978898
mod comment_1052978898 {
#![allow(clippy::redundant_closure)]
pub struct S(u8);
impl S {
pub fn consume(self) {
println!("yum");
}
}
pub fn filter_owned() -> impl Iterator<Item = S> {
(0..10).map(|i| S(i)).filter_map(|s| {
if s.0 & 1 == 0 {
s.consume();
None
} else {
Some(s)
}
})
}
}

8
tests/ui/unnecessary_filter_map.stderr
View file

@ -1,5 +1,5 @@
error: this `.filter_map` can be written more simply using `.filter`
--> $DIR/unnecessary_filter_map.rs:2:13
--> $DIR/unnecessary_filter_map.rs:4:13
|
LL | let _ = (0..4).filter_map(|x| if x > 1 { Some(x) } else { None });
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@ -7,7 +7,7 @@ LL | let _ = (0..4).filter_map(|x| if x > 1 { Some(x) } else { None });
= note: `-D clippy::unnecessary-filter-map` implied by `-D warnings`
error: this `.filter_map` can be written more simply using `.filter`
--> $DIR/unnecessary_filter_map.rs:3:13
--> $DIR/unnecessary_filter_map.rs:5:13
|
LL | let _ = (0..4).filter_map(|x| {
| _____________^
@ -19,7 +19,7 @@ LL | | });
| |______^
error: this `.filter_map` can be written more simply using `.filter`
--> $DIR/unnecessary_filter_map.rs:9:13
--> $DIR/unnecessary_filter_map.rs:11:13
|
LL | let _ = (0..4).filter_map(|x| match x {
| _____________^
@ -29,7 +29,7 @@ LL | | });
| |______^
error: this `.filter_map` can be written more simply using `.map`
--> $DIR/unnecessary_filter_map.rs:14:13
--> $DIR/unnecessary_filter_map.rs:16:13
|
LL | let _ = (0..4).filter_map(|x| Some(x + 1));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^