Skip to content

Commit 031f7df

Browse files
committed
Trying to implement a "borrowck" pass
By overriding the `mir_borrowck` query
1 parent 7f8e2c8 commit 031f7df

File tree

10 files changed

+313
-410
lines changed

10 files changed

+313
-410
lines changed

Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ path = "src/driver.rs"
2222

2323
[dependencies]
2424
clippy_lints = { path = "clippy_lints" }
25+
clippy_utils = { path = "clippy_utils" }
2526
rustc_tools_util = "0.3.0"
2627
tempfile = { version = "3.2", optional = true }
2728
termize = "0.1"

clippy_lints/src/dereference.rs

Lines changed: 96 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
1+
use crate::{BorrowckContext, BorrowckLintPass};
12
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_hir_and_then};
2-
use clippy_utils::mir::{enclosing_mir, expr_local, local_assignments, used_exactly_once, PossibleBorrowerMap};
3+
use clippy_utils::mir::{expr_local, local_assignments, used_exactly_once, PossibleBorrowerMap};
34
use clippy_utils::msrvs::{self, Msrv};
45
use clippy_utils::source::{snippet_with_applicability, snippet_with_context};
56
use clippy_utils::sugg::has_enclosing_paren;
@@ -8,22 +9,23 @@ use clippy_utils::{
89
fn_def_id, get_parent_expr, get_parent_expr_for_hir, is_lint_allowed, path_to_local, walk_to_expr_usage,
910
};
1011

12+
use rustc_ast as ast;
1113
use rustc_ast::util::parser::{PREC_POSTFIX, PREC_PREFIX};
14+
use rustc_borrowck::consumers::BodyWithBorrowckFacts;
1215
use rustc_data_structures::fx::FxIndexMap;
1316
use rustc_data_structures::graph::iterate::{CycleDetector, TriColorDepthFirstSearch};
1417
use rustc_errors::Applicability;
18+
use rustc_hir::def_id::{DefId, LocalDefId};
1519
use rustc_hir::intravisit::{walk_ty, Visitor};
1620
use rustc_hir::{
17-
self as hir,
18-
def_id::{DefId, LocalDefId},
19-
BindingAnnotation, Body, BodyId, BorrowKind, Closure, Expr, ExprKind, FnRetTy, GenericArg, HirId, ImplItem,
21+
self as hir, BindingAnnotation, Body, BorrowKind, Closure, Expr, ExprKind, FnRetTy, GenericArg, HirId, ImplItem,
2022
ImplItemKind, Item, ItemKind, Local, MatchSource, Mutability, Node, Pat, PatKind, Path, QPath, TraitItem,
2123
TraitItemKind, TyKind, UnOp,
2224
};
2325
use rustc_index::bit_set::BitSet;
2426
use rustc_infer::infer::TyCtxtInferExt;
25-
use rustc_lint::{LateContext, LateLintPass};
26-
use rustc_middle::mir::{Rvalue, StatementKind};
27+
use rustc_lint::{late_lint_methods, LateContext, LateLintPass, LintPass};
28+
use rustc_middle::mir::{PlaceElem, Rvalue, StatementKind};
2729
use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow, AutoBorrowMutability};
2830
use rustc_middle::ty::{
2931
self, Binder, BoundVariableKind, Clause, EarlyBinder, FnSig, GenericArgKind, List, ParamEnv, ParamTy,
@@ -34,6 +36,7 @@ use rustc_span::{symbol::sym, Span, Symbol};
3436
use rustc_trait_selection::infer::InferCtxtExt as _;
3537
use rustc_trait_selection::traits::{query::evaluate_obligation::InferCtxtExt as _, Obligation, ObligationCause};
3638
use std::collections::VecDeque;
39+
use std::rc::Rc;
3740

3841
declare_clippy_lint! {
3942
/// ### What it does
@@ -148,15 +151,15 @@ declare_clippy_lint! {
148151
"dereferencing when the compiler would automatically dereference"
149152
}
150153

151-
impl_lint_pass!(Dereferencing<'_> => [
154+
impl_lint_pass!(Dereferencing<'_, '_> => [
152155
EXPLICIT_DEREF_METHODS,
153156
NEEDLESS_BORROW,
154157
REF_BINDING_TO_REFERENCE,
155158
EXPLICIT_AUTO_DEREF,
156159
]);
157160

158161
#[derive(Default)]
159-
pub struct Dereferencing<'tcx> {
162+
pub struct Dereferencing<'b, 'tcx> {
160163
state: Option<(State, StateData)>,
161164

162165
// While parsing a `deref` method call in ufcs form, the path to the function is itself an
@@ -167,7 +170,7 @@ pub struct Dereferencing<'tcx> {
167170
/// The body the first local was found in. Used to emit lints when the traversal of the body has
168171
/// been finished. Note we can't lint at the end of every body as they can be nested within each
169172
/// other.
170-
current_body: Option<BodyId>,
173+
current_body: Option<&'b Rc<BodyWithBorrowckFacts<'tcx>>>,
171174

172175
/// The list of locals currently being checked by the lint.
173176
/// If the value is `None`, then the binding has been seen as a ref pattern, but is not linted.
@@ -177,16 +180,15 @@ pub struct Dereferencing<'tcx> {
177180
/// e.g. `m!(x) | Foo::Bar(ref x)`
178181
ref_locals: FxIndexMap<HirId, Option<RefPat>>,
179182

180-
/// Stack of (body owner, `PossibleBorrowerMap`) pairs. Used by
181-
/// `needless_borrow_impl_arg_position` to determine when a borrowed expression can instead
182-
/// be moved.
183-
possible_borrowers: Vec<(LocalDefId, PossibleBorrowerMap<'tcx, 'tcx>)>,
183+
/// Used by `needless_borrow_impl_arg_position` to determine when a borrowed expression can
184+
/// instead be moved.
185+
possible_borrowers: Option<PossibleBorrowerMap<'b, 'tcx>>,
184186

185187
// `IntoIterator` for arrays requires Rust 1.53.
186188
msrv: Msrv,
187189
}
188190

189-
impl<'tcx> Dereferencing<'tcx> {
191+
impl<'b, 'tcx> Dereferencing<'b, 'tcx> {
190192
#[must_use]
191193
pub fn new(msrv: Msrv) -> Self {
192194
Self {
@@ -256,7 +258,7 @@ struct RefPat {
256258
hir_id: HirId,
257259
}
258260

259-
impl<'tcx> LateLintPass<'tcx> for Dereferencing<'tcx> {
261+
impl<'b, 'tcx> LateLintPass<'tcx> for Dereferencing<'b, 'tcx> {
260262
#[expect(clippy::too_many_lines)]
261263
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
262264
// Skip path expressions from deref calls. e.g. `Deref::deref(e)`
@@ -288,7 +290,13 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing<'tcx> {
288290
match (self.state.take(), kind) {
289291
(None, kind) => {
290292
let expr_ty = typeck.expr_ty(expr);
291-
let (position, adjustments) = walk_parents(cx, &mut self.possible_borrowers, expr, &self.msrv);
293+
let (position, adjustments) = walk_parents(
294+
cx,
295+
self.current_body.as_ref().unwrap(),
296+
&mut self.possible_borrowers,
297+
expr,
298+
&self.msrv,
299+
);
292300
match kind {
293301
RefOp::Deref => {
294302
let sub_ty = typeck.expr_ty(sub_expr);
@@ -548,7 +556,6 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing<'tcx> {
548556
then {
549557
let mut app = Applicability::MachineApplicable;
550558
let snip = snippet_with_context(cx, name.span, pat.span.ctxt(), "..", &mut app).0;
551-
self.current_body = self.current_body.or(cx.enclosing_body);
552559
self.ref_locals.insert(
553560
id,
554561
Some(RefPat {
@@ -564,40 +571,66 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing<'tcx> {
564571
}
565572
}
566573

567-
fn check_body_post(&mut self, cx: &LateContext<'tcx>, body: &'tcx Body<'_>) {
568-
if self.possible_borrowers.last().map_or(false, |&(local_def_id, _)| {
569-
local_def_id == cx.tcx.hir().body_owner_def_id(body.id())
570-
}) {
571-
self.possible_borrowers.pop();
572-
}
574+
fn check_body_post(&mut self, cx: &LateContext<'tcx>, _body: &'tcx Body<'_>) {
575+
self.possible_borrowers = None;
573576

574-
if Some(body.id()) == self.current_body {
575-
for pat in self.ref_locals.drain(..).filter_map(|(_, x)| x) {
576-
let replacements = pat.replacements;
577-
let app = pat.app;
578-
let lint = if pat.always_deref {
579-
NEEDLESS_BORROW
580-
} else {
581-
REF_BINDING_TO_REFERENCE
582-
};
583-
span_lint_hir_and_then(
584-
cx,
585-
lint,
586-
pat.hir_id,
587-
pat.spans,
588-
"this pattern creates a reference to a reference",
589-
|diag| {
590-
diag.multipart_suggestion("try this", replacements, app);
591-
},
592-
);
593-
}
594-
self.current_body = None;
577+
for pat in self.ref_locals.drain(..).filter_map(|(_, x)| x) {
578+
let replacements = pat.replacements;
579+
let app = pat.app;
580+
let lint = if pat.always_deref {
581+
NEEDLESS_BORROW
582+
} else {
583+
REF_BINDING_TO_REFERENCE
584+
};
585+
span_lint_hir_and_then(
586+
cx,
587+
lint,
588+
pat.hir_id,
589+
pat.spans,
590+
"this pattern creates a reference to a reference",
591+
|diag| {
592+
diag.multipart_suggestion("try this", replacements, app);
593+
},
594+
);
595595
}
596596
}
597597

598598
extract_msrv_attr!(LateContext);
599599
}
600600

601+
#[allow(rustc::lint_pass_impl_without_macro)]
602+
impl<'b, 'tcx> LintPass for &mut Dereferencing<'b, 'tcx> {
603+
fn name(&self) -> &'static str {
604+
panic!()
605+
}
606+
}
607+
608+
macro_rules! impl_late_lint_pass {
609+
([], [$($(#[$attr:meta])* fn $f:ident($($param:ident: $arg:ty),*);)*]) => {
610+
impl<'b, 'tcx> LateLintPass<'tcx> for &mut Dereferencing<'b, 'tcx> {
611+
$(fn $f(&mut self, context: &LateContext<'tcx>, $($param: $arg),*) {
612+
(*self).$f(context, $($param),*);
613+
})*
614+
}
615+
};
616+
}
617+
618+
late_lint_methods!(impl_late_lint_pass, []);
619+
620+
impl<'b, 'tcx> BorrowckLintPass<'b, 'tcx> for Dereferencing<'b, 'tcx> {
621+
fn check_body_with_facts(
622+
&mut self,
623+
cx: &BorrowckContext<'tcx>,
624+
body_with_facts: &'b Rc<BodyWithBorrowckFacts<'tcx>>,
625+
) {
626+
self.current_body = Some(body_with_facts);
627+
628+
let this = cx.visit_hir_body(self);
629+
630+
this.current_body = None;
631+
}
632+
}
633+
601634
fn try_parse_ref_op<'tcx>(
602635
tcx: TyCtxt<'tcx>,
603636
typeck: &'tcx TypeckResults<'_>,
@@ -701,9 +734,10 @@ impl Position {
701734
/// is, and which adjustments will be applied to it. Note this will not consider auto-borrow
702735
/// locations as those follow different rules.
703736
#[expect(clippy::too_many_lines)]
704-
fn walk_parents<'tcx>(
737+
fn walk_parents<'b, 'tcx>(
705738
cx: &LateContext<'tcx>,
706-
possible_borrowers: &mut Vec<(LocalDefId, PossibleBorrowerMap<'tcx, 'tcx>)>,
739+
body_with_facts: &'b Rc<BodyWithBorrowckFacts<'tcx>>,
740+
possible_borrowers: &mut Option<PossibleBorrowerMap<'b, 'tcx>>,
707741
e: &'tcx Expr<'_>,
708742
msrv: &Msrv,
709743
) -> (Position, &'tcx [Adjustment<'tcx>]) {
@@ -841,6 +875,7 @@ fn walk_parents<'tcx>(
841875
{
842876
needless_borrow_impl_arg_position(
843877
cx,
878+
body_with_facts,
844879
possible_borrowers,
845880
parent,
846881
i,
@@ -912,6 +947,7 @@ fn walk_parents<'tcx>(
912947
{
913948
needless_borrow_impl_arg_position(
914949
cx,
950+
body_with_facts,
915951
possible_borrowers,
916952
parent,
917953
i + 1,
@@ -1108,9 +1144,10 @@ fn call_is_qualified(expr: &Expr<'_>) -> bool {
11081144
// The "is copyable" condition is to avoid the case where removing the `&` means `e` would have to
11091145
// be moved, but it cannot be.
11101146
#[expect(clippy::too_many_arguments, clippy::too_many_lines)]
1111-
fn needless_borrow_impl_arg_position<'tcx>(
1147+
fn needless_borrow_impl_arg_position<'b, 'tcx>(
11121148
cx: &LateContext<'tcx>,
1113-
possible_borrowers: &mut Vec<(LocalDefId, PossibleBorrowerMap<'tcx, 'tcx>)>,
1149+
body_with_facts: &'b Rc<BodyWithBorrowckFacts<'tcx>>,
1150+
possible_borrowers: &mut Option<PossibleBorrowerMap<'b, 'tcx>>,
11141151
parent: &Expr<'tcx>,
11151152
arg_index: usize,
11161153
param_ty: ParamTy,
@@ -1188,7 +1225,7 @@ fn needless_borrow_impl_arg_position<'tcx>(
11881225

11891226
if !is_copy(cx, referent_ty)
11901227
&& (referent_ty.has_significant_drop(cx.tcx, cx.param_env)
1191-
|| !referent_used_exactly_once(cx, possible_borrowers, reference))
1228+
|| !referent_used_exactly_once(cx, body_with_facts, possible_borrowers, reference))
11921229
{
11931230
return false;
11941231
}
@@ -1289,28 +1326,25 @@ fn is_mixed_projection_predicate<'tcx>(
12891326
}
12901327
}
12911328

1292-
fn referent_used_exactly_once<'tcx>(
1329+
fn referent_used_exactly_once<'b, 'tcx>(
12931330
cx: &LateContext<'tcx>,
1294-
possible_borrowers: &mut Vec<(LocalDefId, PossibleBorrowerMap<'tcx, 'tcx>)>,
1331+
body_with_facts: &'b Rc<BodyWithBorrowckFacts<'tcx>>,
1332+
possible_borrowers: &mut Option<PossibleBorrowerMap<'b, 'tcx>>,
12951333
reference: &Expr<'tcx>,
12961334
) -> bool {
1297-
let mir = enclosing_mir(cx.tcx, reference.hir_id);
1298-
if let Some(local) = expr_local(cx.tcx, reference)
1335+
let mir = &body_with_facts.body;
1336+
if let Some(local) = expr_local(mir, reference)
12991337
&& let [location] = *local_assignments(mir, local).as_slice()
13001338
&& let Some(statement) = mir.basic_blocks[location.block].statements.get(location.statement_index)
13011339
&& let StatementKind::Assign(box (_, Rvalue::Ref(_, _, place))) = statement.kind
1302-
&& !place.has_deref()
1340+
&& !place.projection.contains(&PlaceElem::Deref)
13031341
// Ensure not in a loop (https://github.com/rust-lang/rust-clippy/issues/9710)
13041342
&& TriColorDepthFirstSearch::new(&mir.basic_blocks).run_from(location.block, &mut CycleDetector).is_none()
13051343
{
1306-
let body_owner_local_def_id = cx.tcx.hir().enclosing_body_owner(reference.hir_id);
1307-
if possible_borrowers
1308-
.last()
1309-
.map_or(true, |&(local_def_id, _)| local_def_id != body_owner_local_def_id)
1310-
{
1311-
possible_borrowers.push((body_owner_local_def_id, PossibleBorrowerMap::new(cx, body_owner_local_def_id)));
1344+
if possible_borrowers.is_none() {
1345+
*possible_borrowers = Some(PossibleBorrowerMap::new(cx.tcx, body_with_facts));
13121346
}
1313-
let possible_borrower = &mut possible_borrowers.last_mut().unwrap().1;
1347+
let possible_borrower = possible_borrowers.as_mut().unwrap();
13141348
// If `place.local` were not included here, the `copyable_iterator::warn` test would fail. The
13151349
// reason is that `PossibleBorrowerVisitor::visit_terminator` considers `place.local` a possible
13161350
// borrower of itself. See the comment in that method for an explanation as to why.
@@ -1625,7 +1659,7 @@ fn report<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, state: State, data
16251659
}
16261660
}
16271661

1628-
impl<'tcx> Dereferencing<'tcx> {
1662+
impl<'b, 'tcx> Dereferencing<'b, 'tcx> {
16291663
fn check_local_usage(&mut self, cx: &LateContext<'tcx>, e: &Expr<'tcx>, local: HirId) {
16301664
if let Some(outer_pat) = self.ref_locals.get_mut(&local) {
16311665
if let Some(pat) = outer_pat {

0 commit comments

Comments
 (0)