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

Manual map 7413 #7531

Merged
merged 6 commits into from
Aug 16, 2021
Merged

Manual map 7413 #7531

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4838c78
Improve `manual_map` and `map_entry`
Jarcho Aug 1, 2021
251dd30
Improve `manual_map`
Jarcho Aug 4, 2021
9500974
Fix tracking of which locals would need to be captured in a closure.
Jarcho Aug 9, 2021
5e4d8b4
Improve doc for `can_move_expr_to_closure_no_visit`
Jarcho Aug 9, 2021
10c0460
update stderr messages
Jarcho Aug 14, 2021
f0444d7
Use `each_binding_or_first` in `capture_local_usage`
Jarcho Aug 16, 2021
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
31 changes: 23 additions & 8 deletions clippy_lints/src/entry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,9 @@ use clippy_utils::{
};
use rustc_errors::Applicability;
use rustc_hir::{
hir_id::HirIdSet,
intravisit::{walk_expr, ErasedMap, NestedVisitorMap, Visitor},
Block, Expr, ExprKind, Guard, HirId, Local, Stmt, StmtKind, UnOp,
Block, Expr, ExprKind, Guard, HirId, Pat, Stmt, StmtKind, UnOp,
};
use rustc_lint::{LateContext, LateLintPass};
use rustc_session::{declare_lint_pass, declare_tool_lint};
Expand Down Expand Up @@ -336,6 +337,8 @@ struct InsertSearcher<'cx, 'tcx> {
edits: Vec<Edit<'tcx>>,
/// A stack of loops the visitor is currently in.
loops: Vec<HirId>,
/// Local variables created in the expression. These don't need to be captured.
locals: HirIdSet,
}
impl<'tcx> InsertSearcher<'_, 'tcx> {
/// Visit the expression as a branch in control flow. Multiple insert calls can be used, but
Expand Down Expand Up @@ -383,13 +386,16 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, 'tcx> {
}
},
StmtKind::Expr(e) => self.visit_expr(e),
StmtKind::Local(Local { init: Some(e), .. }) => {
self.allow_insert_closure &= !self.in_tail_pos;
self.in_tail_pos = false;
self.is_single_insert = false;
self.visit_expr(e);
StmtKind::Local(l) => {
self.visit_pat(l.pat);
if let Some(e) = l.init {
self.allow_insert_closure &= !self.in_tail_pos;
self.in_tail_pos = false;
self.is_single_insert = false;
self.visit_expr(e);
}
},
_ => {
StmtKind::Item(_) => {
self.allow_insert_closure &= !self.in_tail_pos;
self.is_single_insert = false;
},
Expand Down Expand Up @@ -471,6 +477,7 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, 'tcx> {
// Each branch may contain it's own insert expression.
let mut is_map_used = self.is_map_used;
for arm in arms {
self.visit_pat(arm.pat);
if let Some(Guard::If(guard) | Guard::IfLet(_, guard)) = arm.guard {
self.visit_non_tail_expr(guard);
}
Expand All @@ -496,7 +503,8 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, 'tcx> {
},
_ => {
self.allow_insert_closure &= !self.in_tail_pos;
self.allow_insert_closure &= can_move_expr_to_closure_no_visit(self.cx, expr, &self.loops);
self.allow_insert_closure &=
can_move_expr_to_closure_no_visit(self.cx, expr, &self.loops, &self.locals);
// Sub expressions are no longer in the tail position.
self.is_single_insert = false;
self.in_tail_pos = false;
Expand All @@ -505,6 +513,12 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, 'tcx> {
},
}
}

fn visit_pat(&mut self, p: &'tcx Pat<'tcx>) {
p.each_binding_or_first(&mut |_, id, _, _| {
self.locals.insert(id);
});
}
}

struct InsertSearchResults<'tcx> {
Expand Down Expand Up @@ -630,6 +644,7 @@ fn find_insert_calls(
in_tail_pos: true,
is_single_insert: true,
loops: Vec::new(),
locals: HirIdSet::default(),
};
s.visit_expr(expr);
let allow_insert_closure = s.allow_insert_closure;
Expand Down
36 changes: 30 additions & 6 deletions clippy_lints/src/manual_map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,14 @@ use clippy_utils::source::{snippet_with_applicability, snippet_with_context};
use clippy_utils::ty::{is_type_diagnostic_item, peel_mid_ty_refs_is_mutable};
use clippy_utils::{
can_move_expr_to_closure, in_constant, is_else_clause, is_lang_ctor, is_lint_allowed, path_to_local_id,
peel_hir_expr_refs,
peel_hir_expr_refs, peel_hir_expr_while, CaptureKind,
};
use rustc_ast::util::parser::PREC_POSTFIX;
use rustc_errors::Applicability;
use rustc_hir::LangItem::{OptionNone, OptionSome};
use rustc_hir::{Arm, BindingAnnotation, Block, Expr, ExprKind, HirId, MatchSource, Mutability, Pat, PatKind};
use rustc_hir::{
def::Res, Arm, BindingAnnotation, Block, Expr, ExprKind, HirId, MatchSource, Mutability, Pat, PatKind, Path, QPath,
};
use rustc_lint::{LateContext, LateLintPass, LintContext};
use rustc_middle::lint::in_external_macro;
use rustc_session::{declare_lint_pass, declare_tool_lint};
Expand Down Expand Up @@ -111,10 +113,6 @@ impl LateLintPass<'_> for ManualMap {
return;
}

if !can_move_expr_to_closure(cx, some_expr) {
return;
}

// Determine which binding mode to use.
let explicit_ref = some_pat.contains_explicit_ref_binding();
let binding_ref = explicit_ref.or_else(|| (ty_ref_count != pat_ref_count).then(|| ty_mutability));
Expand All @@ -125,6 +123,32 @@ impl LateLintPass<'_> for ManualMap {
None => "",
};

match can_move_expr_to_closure(cx, some_expr) {
Some(captures) => {
// Check if captures the closure will need conflict with borrows made in the scrutinee.
// TODO: check all the references made in the scrutinee expression. This will require interacting
// with the borrow checker. Currently only `<local>[.<field>]*` is checked for.
if let Some(binding_ref_mutability) = binding_ref {
let e = peel_hir_expr_while(scrutinee, |e| match e.kind {
ExprKind::Field(e, _) | ExprKind::AddrOf(_, _, e) => Some(e),
_ => None,
});
if let ExprKind::Path(QPath::Resolved(None, Path { res: Res::Local(l), .. })) = e.kind {
match captures.get(l) {
Some(CaptureKind::Value | CaptureKind::Ref(Mutability::Mut)) => return,
Some(CaptureKind::Ref(Mutability::Not))
if binding_ref_mutability == Mutability::Mut =>
{
return;
}
Some(CaptureKind::Ref(Mutability::Not)) | None => (),
}
}
}
},
None => return,
};

let mut app = Applicability::MachineApplicable;

// Remove address-of expressions from the scrutinee. Either `as_ref` will be called, or
Expand Down
Loading