Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Commit 12165fc

Browse files
Rollup merge of #139086 - meithecatte:expr-use-visitor-cleanup, r=compiler-errors
Various cleanup in ExprUseVisitor These are the non-behavior-changing commits from #138961.
2 parents b5ad69b + 908504e commit 12165fc

File tree

3 files changed

+62
-100
lines changed

3 files changed

+62
-100
lines changed

‎compiler/rustc_hir_typeck/src/expr_use_visitor.rs‎

Lines changed: 49 additions & 93 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
//! A different sort of visitor for walking fn bodies. Unlike the
22
//! normal visitor, which just walks the entire body in one shot, the
33
//! `ExprUseVisitor` determines how expressions are being used.
4+
//!
5+
//! In the compiler, this is only used for upvar inference, but there
6+
//! are many uses within clippy.
47
58
use std::cell::{Ref, RefCell};
69
use std::ops::Deref;
@@ -25,7 +28,7 @@ use rustc_middle::ty::{
2528
use rustc_middle::{bug, span_bug};
2629
use rustc_span::{ErrorGuaranteed, Span};
2730
use rustc_trait_selection::infer::InferCtxtExt;
28-
use tracing::{debug, trace};
31+
use tracing::{debug, instrument,trace};
2932

3033
use crate::fn_ctxt::FnCtxt;
3134

@@ -35,11 +38,8 @@ pub trait Delegate<'tcx> {
3538
/// The value found at `place` is moved, depending
3639
/// on `mode`. Where `diag_expr_id` is the id used for diagnostics for `place`.
3740
///
38-
/// Use of a `Copy` type in a ByValue context is considered a use
39-
/// by `ImmBorrow` and `borrow` is called instead. This is because
40-
/// a shared borrow is the "minimum access" that would be needed
41-
/// to perform a copy.
42-
///
41+
/// If the value is `Copy`, [`copy`][Self::copy] is called instead, which
42+
/// by default falls back to [`borrow`][Self::borrow].
4343
///
4444
/// The parameter `diag_expr_id` indicates the HIR id that ought to be used for
4545
/// diagnostics. Around pattern matching such as `let pat = expr`, the diagnostic
@@ -73,6 +73,10 @@ pub trait Delegate<'tcx> {
7373

7474
/// The value found at `place` is being copied.
7575
/// `diag_expr_id` is the id used for diagnostics (see `consume` for more details).
76+
///
77+
/// If an implementation is not provided, use of a `Copy` type in a ByValue context is instead
78+
/// considered a use by `ImmBorrow` and `borrow` is called instead. This is because a shared
79+
/// borrow is the "minimum access" that would be needed to perform a copy.
7680
fn copy(&mut self, place_with_id: &PlaceWithHirId<'tcx>, diag_expr_id: HirId) {
7781
// In most cases, copying data from `x` is equivalent to doing `*&x`, so by default
7882
// we treat a copy of `x` as a borrow of `x`.
@@ -141,6 +145,8 @@ impl<'tcx, D: Delegate<'tcx>> Delegate<'tcx> for &mut D {
141145
}
142146
}
143147

148+
/// This trait makes `ExprUseVisitor` usable with both [`FnCtxt`]
149+
/// and [`LateContext`], depending on where in the compiler it is used.
144150
pub trait TypeInformationCtxt<'tcx> {
145151
type TypeckResults<'a>: Deref<Target = ty::TypeckResults<'tcx>>
146152
where
@@ -154,7 +160,7 @@ pub trait TypeInformationCtxt<'tcx> {
154160

155161
fn try_structurally_resolve_type(&self, span: Span, ty: Ty<'tcx>) -> Ty<'tcx>;
156162

157-
fn report_error(&self, span: Span, msg: impl ToString) -> Self::Error;
163+
fn report_bug(&self, span: Span, msg: impl ToString) -> Self::Error;
158164

159165
fn error_reported_in_ty(&self, ty: Ty<'tcx>) -> Result<(), Self::Error>;
160166

@@ -189,7 +195,7 @@ impl<'tcx> TypeInformationCtxt<'tcx> for &FnCtxt<'_, 'tcx> {
189195
(**self).try_structurally_resolve_type(sp, ty)
190196
}
191197

192-
fn report_error(&self, span: Span, msg: impl ToString) -> Self::Error {
198+
fn report_bug(&self, span: Span, msg: impl ToString) -> Self::Error {
193199
self.dcx().span_delayed_bug(span, msg.to_string())
194200
}
195201

@@ -239,7 +245,7 @@ impl<'tcx> TypeInformationCtxt<'tcx> for (&LateContext<'tcx>, LocalDefId) {
239245
t
240246
}
241247

242-
fn report_error(&self, span: Span, msg: impl ToString) -> ! {
248+
fn report_bug(&self, span: Span, msg: impl ToString) -> ! {
243249
span_bug!(span, "{}", msg.to_string())
244250
}
245251

@@ -268,9 +274,9 @@ impl<'tcx> TypeInformationCtxt<'tcx> for (&LateContext<'tcx>, LocalDefId) {
268274
}
269275
}
270276

271-
/// The ExprUseVisitor type
277+
/// A visitor that reports how each expression is being used.
272278
///
273-
/// This is the code that actually walks the tree.
279+
/// See [module-level docs][self] and [`Delegate`] for details.
274280
pub struct ExprUseVisitor<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> {
275281
cx: Cx,
276282
/// We use a `RefCell` here so that delegates can mutate themselves, but we can
@@ -314,19 +320,17 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
314320
Ok(())
315321
}
316322

323+
#[instrument(skip(self), level = "debug")]
317324
fn consume_or_copy(&self, place_with_id: &PlaceWithHirId<'tcx>, diag_expr_id: HirId) {
318-
debug!("delegate_consume(place_with_id={:?})", place_with_id);
319-
320325
if self.cx.type_is_copy_modulo_regions(place_with_id.place.ty()) {
321326
self.delegate.borrow_mut().copy(place_with_id, diag_expr_id);
322327
} else {
323328
self.delegate.borrow_mut().consume(place_with_id, diag_expr_id);
324329
}
325330
}
326331

332+
#[instrument(skip(self), level = "debug")]
327333
pub fn consume_clone_or_copy(&self, place_with_id: &PlaceWithHirId<'tcx>, diag_expr_id: HirId) {
328-
debug!("delegate_consume_or_clone(place_with_id={:?})", place_with_id);
329-
330334
// `x.use` will do one of the following
331335
// * if it implements `Copy`, it will be a copy
332336
// * if it implements `UseCloned`, it will be a call to `clone`
@@ -351,18 +355,16 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
351355
}
352356

353357
// FIXME: It's suspicious that this is public; clippy should probably use `walk_expr`.
358+
#[instrument(skip(self), level = "debug")]
354359
pub fn consume_expr(&self, expr: &hir::Expr<'_>) -> Result<(), Cx::Error> {
355-
debug!("consume_expr(expr={:?})", expr);
356-
357360
let place_with_id = self.cat_expr(expr)?;
358361
self.consume_or_copy(&place_with_id, place_with_id.hir_id);
359362
self.walk_expr(expr)?;
360363
Ok(())
361364
}
362365

366+
#[instrument(skip(self), level = "debug")]
363367
pub fn consume_or_clone_expr(&self, expr: &hir::Expr<'_>) -> Result<(), Cx::Error> {
364-
debug!("consume_or_clone_expr(expr={:?})", expr);
365-
366368
let place_with_id = self.cat_expr(expr)?;
367369
self.consume_clone_or_copy(&place_with_id, place_with_id.hir_id);
368370
self.walk_expr(expr)?;
@@ -376,17 +378,15 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
376378
Ok(())
377379
}
378380

381+
#[instrument(skip(self), level = "debug")]
379382
fn borrow_expr(&self, expr: &hir::Expr<'_>, bk: ty::BorrowKind) -> Result<(), Cx::Error> {
380-
debug!("borrow_expr(expr={:?}, bk={:?})", expr, bk);
381-
382383
let place_with_id = self.cat_expr(expr)?;
383384
self.delegate.borrow_mut().borrow(&place_with_id, place_with_id.hir_id, bk);
384385
self.walk_expr(expr)
385386
}
386387

388+
#[instrument(skip(self), level = "debug")]
387389
pub fn walk_expr(&self, expr: &hir::Expr<'_>) -> Result<(), Cx::Error> {
388-
debug!("walk_expr(expr={:?})", expr);
389-
390390
self.walk_adjustment(expr)?;
391391

392392
match expr.kind {
@@ -733,9 +733,8 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
733733

734734
/// Indicates that the value of `blk` will be consumed, meaning either copied or moved
735735
/// depending on its type.
736+
#[instrument(skip(self), level = "debug")]
736737
fn walk_block(&self, blk: &hir::Block<'_>) -> Result<(), Cx::Error> {
737-
debug!("walk_block(blk.hir_id={})", blk.hir_id);
738-
739738
for stmt in blk.stmts {
740739
self.walk_stmt(stmt)?;
741740
}
@@ -861,7 +860,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
861860
}
862861

863862
/// Walks the autoref `autoref` applied to the autoderef'd
864-
/// `expr`. `base_place` is the mem-categorized form of `expr`
863+
/// `expr`. `base_place` is `expr` represented as a place,
865864
/// after all relevant autoderefs have occurred.
866865
fn walk_autoref(
867866
&self,
@@ -942,14 +941,13 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
942941
}
943942

944943
/// The core driver for walking a pattern
944+
#[instrument(skip(self), level = "debug")]
945945
fn walk_pat(
946946
&self,
947947
discr_place: &PlaceWithHirId<'tcx>,
948948
pat: &hir::Pat<'_>,
949949
has_guard: bool,
950950
) -> Result<(), Cx::Error> {
951-
debug!("walk_pat(discr_place={:?}, pat={:?}, has_guard={:?})", discr_place, pat, has_guard);
952-
953951
let tcx = self.cx.tcx();
954952
self.cat_pattern(discr_place.clone(), pat, &mut |place, pat| {
955953
match pat.kind {
@@ -1042,6 +1040,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
10421040
///
10431041
/// - When reporting the Place back to the Delegate, ensure that the UpvarId uses the enclosing
10441042
/// closure as the DefId.
1043+
#[instrument(skip(self), level = "debug")]
10451044
fn walk_captures(&self, closure_expr: &hir::Closure<'_>) -> Result<(), Cx::Error> {
10461045
fn upvar_is_local_variable(
10471046
upvars: Option<&FxIndexMap<HirId, hir::Upvar>>,
@@ -1051,8 +1050,6 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
10511050
upvars.map(|upvars| !upvars.contains_key(&upvar_id)).unwrap_or(body_owner_is_closure)
10521051
}
10531052

1054-
debug!("walk_captures({:?})", closure_expr);
1055-
10561053
let tcx = self.cx.tcx();
10571054
let closure_def_id = closure_expr.def_id;
10581055
// For purposes of this function, coroutine and closures are equivalent.
@@ -1164,55 +1161,17 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
11641161
}
11651162
}
11661163

1167-
/// The job of the categorization methods is to analyze an expression to
1168-
/// determine what kind of memory is used in evaluating it (for example,
1169-
/// where dereferences occur and what kind of pointer is dereferenced;
1170-
/// whether the memory is mutable, etc.).
1171-
///
1172-
/// Categorization effectively transforms all of our expressions into
1173-
/// expressions of the following forms (the actual enum has many more
1174-
/// possibilities, naturally, but they are all variants of these base
1175-
/// forms):
1176-
/// ```ignore (not-rust)
1177-
/// E = rvalue // some computed rvalue
1178-
/// | x // address of a local variable or argument
1179-
/// | *E // deref of a ptr
1180-
/// | E.comp // access to an interior component
1181-
/// ```
1182-
/// Imagine a routine ToAddr(Expr) that evaluates an expression and returns an
1183-
/// address where the result is to be found. If Expr is a place, then this
1184-
/// is the address of the place. If `Expr` is an rvalue, this is the address of
1185-
/// some temporary spot in memory where the result is stored.
1186-
///
1187-
/// Now, `cat_expr()` classifies the expression `Expr` and the address `A = ToAddr(Expr)`
1188-
/// as follows:
1189-
///
1190-
/// - `cat`: what kind of expression was this? This is a subset of the
1191-
/// full expression forms which only includes those that we care about
1192-
/// for the purpose of the analysis.
1193-
/// - `mutbl`: mutability of the address `A`.
1194-
/// - `ty`: the type of data found at the address `A`.
1164+
/// The job of the methods whose name starts with `cat_` is to analyze
1165+
/// expressions and construct the corresponding [`Place`]s. The `cat`
1166+
/// stands for "categorize", this is a leftover from long ago when
1167+
/// places were called "categorizations".
11951168
///
1196-
/// The resulting categorization tree differs somewhat from the expressions
1197-
/// themselves. For example, auto-derefs are explicit. Also, an index `a[b]` is
1198-
/// decomposed into two operations: a dereference to reach the array data and
1199-
/// then an index to jump forward to the relevant item.
1200-
///
1201-
/// ## By-reference upvars
1202-
///
1203-
/// One part of the codegen which may be non-obvious is that we translate
1204-
/// closure upvars into the dereference of a borrowed pointer; this more closely
1205-
/// resembles the runtime codegen. So, for example, if we had:
1206-
///
1207-
/// let mut x = 3;
1208-
/// let y = 5;
1209-
/// let inc = || x += y;
1210-
///
1211-
/// Then when we categorize `x` (*within* the closure) we would yield a
1212-
/// result of `*x'`, effectively, where `x'` is a `Categorization::Upvar` reference
1213-
/// tied to `x`. The type of `x'` will be a borrowed pointer.
1169+
/// Note that a [`Place`] differs somewhat from the expression itself. For
1170+
/// example, auto-derefs are explicit. Also, an index `a[b]` is decomposed into
1171+
/// two operations: a dereference to reach the array data and then an index to
1172+
/// jump forward to the relevant item.
12141173
impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx, Cx, D> {
1215-
fn resolve_type_vars_or_error(
1174+
fn resolve_type_vars_or_bug(
12161175
&self,
12171176
id: HirId,
12181177
ty: Option<Ty<'tcx>>,
@@ -1222,35 +1181,32 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
12221181
let ty = self.cx.resolve_vars_if_possible(ty);
12231182
self.cx.error_reported_in_ty(ty)?;
12241183
if ty.is_ty_var() {
1225-
debug!("resolve_type_vars_or_error: infer var from {:?}", ty);
1184+
debug!("resolve_type_vars_or_bug: infer var from {:?}", ty);
12261185
Err(self
12271186
.cx
1228-
.report_error(self.cx.tcx().hir().span(id), "encountered type variable"))
1187+
.report_bug(self.cx.tcx().hir().span(id), "encountered type variable"))
12291188
} else {
12301189
Ok(ty)
12311190
}
12321191
}
12331192
None => {
12341193
// FIXME: We shouldn't be relying on the infcx being tainted.
12351194
self.cx.tainted_by_errors()?;
1236-
bug!(
1237-
"no type for node {} in mem_categorization",
1238-
self.cx.tcx().hir_id_to_string(id)
1239-
);
1195+
bug!("no type for node {} in ExprUseVisitor", self.cx.tcx().hir_id_to_string(id));
12401196
}
12411197
}
12421198
}
12431199

12441200
fn node_ty(&self, hir_id: HirId) -> Result<Ty<'tcx>, Cx::Error> {
1245-
self.resolve_type_vars_or_error(hir_id, self.cx.typeck_results().node_type_opt(hir_id))
1201+
self.resolve_type_vars_or_bug(hir_id, self.cx.typeck_results().node_type_opt(hir_id))
12461202
}
12471203

12481204
fn expr_ty(&self, expr: &hir::Expr<'_>) -> Result<Ty<'tcx>, Cx::Error> {
1249-
self.resolve_type_vars_or_error(expr.hir_id, self.cx.typeck_results().expr_ty_opt(expr))
1205+
self.resolve_type_vars_or_bug(expr.hir_id, self.cx.typeck_results().expr_ty_opt(expr))
12501206
}
12511207

12521208
fn expr_ty_adjusted(&self, expr: &hir::Expr<'_>) -> Result<Ty<'tcx>, Cx::Error> {
1253-
self.resolve_type_vars_or_error(
1209+
self.resolve_type_vars_or_bug(
12541210
expr.hir_id,
12551211
self.cx.typeck_results().expr_ty_adjusted_opt(expr),
12561212
)
@@ -1285,7 +1241,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
12851241
self.pat_ty_unadjusted(pat)
12861242
}
12871243

1288-
/// Like `TypeckResults::pat_ty`, but ignores implicit `&` patterns.
1244+
/// Like [`Self::pat_ty_adjusted`], but ignores implicit `&` patterns.
12891245
fn pat_ty_unadjusted(&self, pat: &hir::Pat<'_>) -> Result<Ty<'tcx>, Cx::Error> {
12901246
let base_ty = self.node_ty(pat.hir_id)?;
12911247
trace!(?base_ty);
@@ -1315,7 +1271,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
13151271
debug!("By-ref binding of non-derefable type");
13161272
Err(self
13171273
.cx
1318-
.report_error(pat.span, "by-ref binding of non-derefable type"))
1274+
.report_bug(pat.span, "by-ref binding of non-derefable type"))
13191275
}
13201276
}
13211277
} else {
@@ -1511,7 +1467,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
15111467
}
15121468
}
15131469

1514-
def => span_bug!(span, "unexpected definition in memory categorization: {:?}", def),
1470+
def => span_bug!(span, "unexpected definition in ExprUseVisitor: {:?}", def),
15151471
}
15161472
}
15171473

@@ -1604,7 +1560,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
16041560
Some(ty) => ty,
16051561
None => {
16061562
debug!("explicit deref of non-derefable type: {:?}", base_curr_ty);
1607-
return Err(self.cx.report_error(
1563+
return Err(self.cx.report_bug(
16081564
self.cx.tcx().hir().span(node),
16091565
"explicit deref of non-derefable type",
16101566
));
@@ -1629,7 +1585,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
16291585
let ty::Adt(adt_def, _) = self.cx.try_structurally_resolve_type(span, ty).kind() else {
16301586
return Err(self
16311587
.cx
1632-
.report_error(span, "struct or tuple struct pattern not applied to an ADT"));
1588+
.report_bug(span, "struct or tuple struct pattern not applied to an ADT"));
16331589
};
16341590

16351591
match res {
@@ -1675,7 +1631,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
16751631
let ty = self.cx.typeck_results().node_type(pat_hir_id);
16761632
match self.cx.try_structurally_resolve_type(span, ty).kind() {
16771633
ty::Tuple(args) => Ok(args.len()),
1678-
_ => Err(self.cx.report_error(span, "tuple pattern not applied to a tuple")),
1634+
_ => Err(self.cx.report_bug(span, "tuple pattern not applied to a tuple")),
16791635
}
16801636
}
16811637

@@ -1854,7 +1810,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
18541810
debug!("explicit index of non-indexable type {:?}", place_with_id);
18551811
return Err(self
18561812
.cx
1857-
.report_error(pat.span, "explicit index of non-indexable type"));
1813+
.report_bug(pat.span, "explicit index of non-indexable type"));
18581814
};
18591815
let elt_place = self.cat_projection(
18601816
pat.hir_id,

‎compiler/rustc_hir_typeck/src/upvar.rs‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,12 +18,12 @@
1818
//! from there).
1919
//!
2020
//! The fact that we are inferring borrow kinds as we go results in a
21-
//! semi-hacky interaction with mem-categorization. In particular,
22-
//! mem-categorization will query the current borrow kind as it
23-
//! categorizes, and we'll return the *current* value, but this may get
21+
//! semi-hacky interaction with the way `ExprUseVisitor` is computing
22+
//! `Place`s. In particular, it will query the current borrow kind as it
23+
//! goes, and we'll return the *current* value, but this may get
2424
//! adjusted later. Therefore, in this module, we generally ignore the
25-
//! borrow kind (and derived mutabilities) that are returned from
26-
//! mem-categorization, since they may be inaccurate. (Another option
25+
//! borrow kind (and derived mutabilities) that `ExprUseVisitor` returns
26+
//! within `Place`s, since they may be inaccurate. (Another option
2727
//! would be to use a unification scheme, where instead of returning a
2828
//! concrete borrow kind like `ty::ImmBorrow`, we return a
2929
//! `ty::InferBorrow(upvar_id)` or something like that, but this would

0 commit comments

Comments
(0)

AltStyle によって変換されたページ (->オリジナル) /