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.

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

Fix needless_collect's tendency to suggest code requiring multiple mutable borrows of the same value. #7982

Merged
merged 3 commits into from
Nov 16, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
175 changes: 149 additions & 26 deletions clippy_lints/src/loops/needless_collect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,16 @@ use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_hir_and_then};
use clippy_utils::source::{snippet, snippet_with_applicability};
use clippy_utils::sugg::Sugg;
use clippy_utils::ty::is_type_diagnostic_item;
use clippy_utils::{is_trait_method, path_to_local_id};
use clippy_utils::{can_move_expr_to_closure, is_trait_method, path_to_local, path_to_local_id, CaptureKind};
use if_chain::if_chain;
use rustc_data_structures::fx::FxHashMap;
use rustc_errors::Applicability;
use rustc_hir::intravisit::{walk_block, walk_expr, NestedVisitorMap, Visitor};
use rustc_hir::{Block, Expr, ExprKind, HirId, PatKind, StmtKind};
use rustc_hir::{Block, Expr, ExprKind, HirId, HirIdSet, Local, Mutability, Node, PatKind, Stmt, StmtKind};
use rustc_lint::LateContext;
use rustc_middle::hir::map::Map;
use rustc_middle::ty::subst::GenericArgKind;
use rustc_middle::ty::{TyKind, TyS};
use rustc_span::sym;
use rustc_span::{MultiSpan, Span};

Expand Down Expand Up @@ -83,7 +86,8 @@ fn check_needless_collect_indirect_usage<'tcx>(expr: &'tcx Expr<'_>, cx: &LateCo
is_type_diagnostic_item(cx, ty, sym::VecDeque) ||
is_type_diagnostic_item(cx, ty, sym::BinaryHeap) ||
is_type_diagnostic_item(cx, ty, sym::LinkedList);
if let Some(iter_calls) = detect_iter_and_into_iters(block, id);
let iter_ty = cx.typeck_results().expr_ty(iter_source);
if let Some(iter_calls) = detect_iter_and_into_iters(block, id, cx, get_captured_ids(cx, iter_ty));
if let [iter_call] = &*iter_calls;
then {
let mut used_count_visitor = UsedCountVisitor {
Expand Down Expand Up @@ -167,37 +171,93 @@ enum IterFunctionKind {
Contains(Span),
}

struct IterFunctionVisitor {
uses: Vec<IterFunction>,
struct IterFunctionVisitor<'b, 'a> {
illegal_mutable_capture_ids: HirIdSet,
current_mutably_captured_ids: HirIdSet,
cx: &'a LateContext<'b>,
Blckbrry-Pi marked this conversation as resolved.
Show resolved Hide resolved
uses: Vec<Option<IterFunction>>,
hir_id_uses_map: FxHashMap<HirId, usize>,
current_statement_hir_id: Option<HirId>,
seen_other: bool,
target: HirId,
}
impl<'tcx> Visitor<'tcx> for IterFunctionVisitor {
impl<'tcx> Visitor<'tcx> for IterFunctionVisitor<'_, 'tcx> {
fn visit_block(&mut self, block: &'txc Block<'tcx>) {
Blckbrry-Pi marked this conversation as resolved.
Show resolved Hide resolved
for (expr, hir_id) in block
.stmts
.iter()
.filter_map(get_expr_and_hir_id_from_stmt)
.chain(block.expr.map(|expr| (expr, None)))
{
self.current_statement_hir_id = hir_id;
self.current_mutably_captured_ids = get_captured_ids(self.cx, self.cx.typeck_results().expr_ty(expr));
self.visit_expr(expr);
}
}
Blckbrry-Pi marked this conversation as resolved.
Show resolved Hide resolved

fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) {
// Check function calls on our collection
if let ExprKind::MethodCall(method_name, _, [recv, args @ ..], _) = &expr.kind {
if method_name.ident.name == sym!(collect) && is_trait_method(self.cx, expr, sym::Iterator) {
self.current_mutably_captured_ids = get_captured_ids(self.cx, self.cx.typeck_results().expr_ty(recv));
self.visit_expr(recv);
return;
}

if path_to_local_id(recv, self.target) {
match &*method_name.ident.name.as_str() {
"into_iter" => self.uses.push(IterFunction {
func: IterFunctionKind::IntoIter,
span: expr.span,
}),
"len" => self.uses.push(IterFunction {
func: IterFunctionKind::Len,
span: expr.span,
}),
"is_empty" => self.uses.push(IterFunction {
func: IterFunctionKind::IsEmpty,
span: expr.span,
}),
"contains" => self.uses.push(IterFunction {
func: IterFunctionKind::Contains(args[0].span),
span: expr.span,
}),
_ => self.seen_other = true,
if self
.illegal_mutable_capture_ids
.intersection(&self.current_mutably_captured_ids)
.next()
.is_none()
{
if let Some(hir_id) = self.current_statement_hir_id {
self.hir_id_uses_map.insert(hir_id, self.uses.len());
}
match &*method_name.ident.name.as_str() {
"into_iter" => self.uses.push(Some(IterFunction {
func: IterFunctionKind::IntoIter,
span: expr.span,
})),
"len" => self.uses.push(Some(IterFunction {
func: IterFunctionKind::Len,
span: expr.span,
})),
"is_empty" => self.uses.push(Some(IterFunction {
func: IterFunctionKind::IsEmpty,
span: expr.span,
})),
"contains" => self.uses.push(Some(IterFunction {
func: IterFunctionKind::Contains(args[0].span),
span: expr.span,
})),
_ => {
self.seen_other = true;
if let Some(hir_id) = self.current_statement_hir_id {
self.hir_id_uses_map.remove(&hir_id);
}
},
}
}
return;
}

if let Some(hir_id) = path_to_local(recv) {
if let Some(index) = self.hir_id_uses_map.remove(&hir_id) {
if self
.illegal_mutable_capture_ids
.intersection(&self.current_mutably_captured_ids)
.next()
.is_none()
{
if let Some(hir_id) = self.current_statement_hir_id {
self.hir_id_uses_map.insert(hir_id, index);
}
} else {
self.uses[index] = None;
}
}
}
}
// Check if the collection is used for anything else
if path_to_local_id(expr, self.target) {
Expand All @@ -213,6 +273,20 @@ impl<'tcx> Visitor<'tcx> for IterFunctionVisitor {
}
}

fn get_expr_and_hir_id_from_stmt<'v>(stmt: &'v Stmt<'v>) -> Option<(&'v Expr<'v>, Option<HirId>)> {
match stmt.kind {
StmtKind::Expr(expr) | StmtKind::Semi(expr) => Some((expr, None)),
StmtKind::Item(..) => None,
StmtKind::Local(Local { init, pat, .. }) => {
if let PatKind::Binding(_, hir_id, ..) = pat.kind {
init.map(|init_expr| (init_expr, Some(hir_id)))
} else {
init.map(|init_expr| (init_expr, None))
}
},
}
}

struct UsedCountVisitor<'a, 'tcx> {
cx: &'a LateContext<'tcx>,
id: HirId,
Expand All @@ -237,12 +311,61 @@ impl<'a, 'tcx> Visitor<'tcx> for UsedCountVisitor<'a, 'tcx> {

/// Detect the occurrences of calls to `iter` or `into_iter` for the
/// given identifier
fn detect_iter_and_into_iters<'tcx>(block: &'tcx Block<'tcx>, id: HirId) -> Option<Vec<IterFunction>> {
fn detect_iter_and_into_iters<'tcx: 'a, 'a>(
block: &'tcx Block<'tcx>,
id: HirId,
cx: &'a LateContext<'tcx>,
captured_ids: HirIdSet,
) -> Option<Vec<IterFunction>> {
let mut visitor = IterFunctionVisitor {
uses: Vec::new(),
target: id,
seen_other: false,
cx,
current_mutably_captured_ids: HirIdSet::default(),
illegal_mutable_capture_ids: captured_ids,
hir_id_uses_map: FxHashMap::default(),
current_statement_hir_id: None,
};
visitor.visit_block(block);
if visitor.seen_other { None } else { Some(visitor.uses) }
if visitor.seen_other {
None
} else {
Some(visitor.uses.into_iter().flatten().collect())
}
}

#[allow(rustc::usage_of_ty_tykind)]
Blckbrry-Pi marked this conversation as resolved.
Show resolved Hide resolved
fn get_captured_ids(cx: &LateContext<'tcx>, ty: &'_ TyS<'_>) -> HirIdSet {
fn get_captured_ids_recursive(cx: &LateContext<'tcx>, ty: &'_ TyS<'_>, set: &mut HirIdSet) {
match ty.kind() {
TyKind::Adt(_, generics) => {
for generic in *generics {
if let GenericArgKind::Type(ty) = generic.unpack() {
get_captured_ids_recursive(cx, ty, set);
}
}
},
TyKind::Closure(def_id, _) => {
let closure_hir_node = cx.tcx.hir().get_if_local(*def_id).unwrap();
if let Node::Expr(closure_expr) = closure_hir_node {
can_move_expr_to_closure(cx, closure_expr)
.unwrap()
.into_iter()
.for_each(|(hir_id, capture_kind)| {
if matches!(capture_kind, CaptureKind::Ref(Mutability::Mut)) {
set.insert(hir_id);
}
});
}
},
_ => (),
}
}

let mut set = HirIdSet::default();

get_captured_ids_recursive(cx, ty, &mut set);

set
}
31 changes: 31 additions & 0 deletions tests/ui/needless_collect_indirect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,37 @@ mod issue7110 {
}
}

mod issue7975 {
use super::*;

fn direct_mapping_with_used_mutable_reference() -> Vec<()> {
let test_vec: Vec<()> = vec![];
let mut vec_2: Vec<()> = vec![];
let mut_ref = &mut vec_2;
let collected_vec: Vec<_> = test_vec.into_iter().map(|_| mut_ref.push(())).collect();
collected_vec.into_iter().map(|_| mut_ref.push(())).collect()
}

fn indirectly_mapping_with_used_mutable_reference() -> Vec<()> {
let test_vec: Vec<()> = vec![];
let mut vec_2: Vec<()> = vec![];
let mut_ref = &mut vec_2;
let collected_vec: Vec<_> = test_vec.into_iter().map(|_| mut_ref.push(())).collect();
let iter = collected_vec.into_iter();
iter.map(|_| mut_ref.push(())).collect()
}

fn indirect_collect_after_indirect_mapping_with_used_mutable_reference() -> Vec<()> {
let test_vec: Vec<()> = vec![];
let mut vec_2: Vec<()> = vec![];
let mut_ref = &mut vec_2;
let collected_vec: Vec<_> = test_vec.into_iter().map(|_| mut_ref.push(())).collect();
let iter = collected_vec.into_iter();
let mapped_iter = iter.map(|_| mut_ref.push(()));
mapped_iter.collect()
}
}

fn allow_test() {
#[allow(clippy::needless_collect)]
let v = [1].iter().collect::<Vec<_>>();
Expand Down