mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Auto merge of #7982 - Blckbrry-Pi:master, r=llogiq

Browse Source 
Fix `needless_collect`'s tendency to suggest code requiring multiple mutable borrows of the same value.

Fixes error specified in #7975.

changelog: [`needless_collect`] no longer suggests removal of `collect` when removal would create code requiring mutably borrowing a value multiple times.
This commit is contained in:
bors 2021-11-16 23:01:02 +00:00
commit 83ad51108b
2 changed files with 183 additions and 26 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

178
clippy_lints/src/loops/needless_collect.rs
View File

@ -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::{self, TyS};
use rustc_span::sym;
use rustc_span::{MultiSpan, Span};
@ -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 {
@ -167,37 +171,89 @@ enum IterFunctionKind {
Contains(Span),
}
struct IterFunctionVisitor {
uses: Vec<IterFunction>,
struct IterFunctionVisitor<'a, 'tcx> {
illegal_mutable_capture_ids: HirIdSet,
current_mutably_captured_ids: HirIdSet,
cx: &'a LateContext<'tcx>,
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: &'tcx Block<'tcx>) {
for (expr, hir_id) in block.stmts.iter().filter_map(get_expr_and_hir_id_from_stmt) {
self.visit_block_expr(expr, hir_id);
}
if let Some(expr) = block.expr {
self.visit_block_expr(expr, None);
}
}
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) {
@ -213,6 +269,28 @@ impl<'tcx> Visitor<'tcx> for IterFunctionVisitor {
}
}
impl<'tcx> IterFunctionVisitor<'_, 'tcx> {
fn visit_block_expr(&mut self, expr: &'tcx Expr<'tcx>, hir_id: Option<HirId>) {
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);
}
}
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,
@ -237,12 +315,60 @@ 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())
}
}
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() {
ty::Adt(_, generics) => {
for generic in *generics {
if let GenericArgKind::Type(ty) = generic.unpack() {
get_captured_ids_recursive(cx, ty, set);
}
}
},
ty::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
tests/ui/needless_collect_indirect.rs
View File

@ -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<_>>();