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

Various cleanup in ExprUseVisitor #139086

Merged
merged 5 commits into from
Mar 29, 2025
+62 −100
Merged

Various cleanup in ExprUseVisitor #139086

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a86e0da
doc(hir::Place): clarify that places aren't always place expressions
meithecatte Feb 26, 2025
376c88e
ExprUseVisitor: add clarifying doc comments
meithecatte Feb 26, 2025
aab1293
ExprUseVisitor: error -> bug in helper names
meithecatte Feb 26, 2025
f161953
ExprUseVisitor: remove leftover mentions of mem-categorization
meithecatte Feb 26, 2025
908504e
ExprUseVisitor: use tracing::instrument as appropriate
meithecatte Mar 26, 2025
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
142 changes: 49 additions & 93 deletions compiler/rustc_hir_typeck/src/expr_use_visitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
//! A different sort of visitor for walking fn bodies. Unlike the
//! normal visitor, which just walks the entire body in one shot, the
//! `ExprUseVisitor` determines how expressions are being used.
//!
//! In the compiler, this is only used for upvar inference, but there
//! are many uses within clippy.

use std::cell::{Ref, RefCell};
use std::ops::Deref;
Expand All @@ -25,7 +28,7 @@ use rustc_middle::ty::{
use rustc_middle::{bug, span_bug};
use rustc_span::{ErrorGuaranteed, Span};
use rustc_trait_selection::infer::InferCtxtExt;
use tracing::{debug, trace};
use tracing::{debug, instrument, trace};

use crate::fn_ctxt::FnCtxt;

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

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

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

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

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

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

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

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

Expand Down Expand Up @@ -239,7 +245,7 @@ impl<'tcx> TypeInformationCtxt<'tcx> for (&LateContext<'tcx>, LocalDefId) {
t
}

fn report_error(&self, span: Span, msg: impl ToString) -> ! {
fn report_bug(&self, span: Span, msg: impl ToString) -> ! {
span_bug!(span, "{}", msg.to_string())
}

Expand Down Expand Up @@ -268,9 +274,9 @@ impl<'tcx> TypeInformationCtxt<'tcx> for (&LateContext<'tcx>, LocalDefId) {
}
}

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

#[instrument(skip(self), level = "debug")]
fn consume_or_copy(&self, place_with_id: &PlaceWithHirId<'tcx>, diag_expr_id: HirId) {
debug!("delegate_consume(place_with_id={:?})", place_with_id);

if self.cx.type_is_copy_modulo_regions(place_with_id.place.ty()) {
self.delegate.borrow_mut().copy(place_with_id, diag_expr_id);
} else {
self.delegate.borrow_mut().consume(place_with_id, diag_expr_id);
}
}

#[instrument(skip(self), level = "debug")]
pub fn consume_clone_or_copy(&self, place_with_id: &PlaceWithHirId<'tcx>, diag_expr_id: HirId) {
debug!("delegate_consume_or_clone(place_with_id={:?})", place_with_id);

// `x.use` will do one of the following
// * if it implements `Copy`, it will be a copy
// * if it implements `UseCloned`, it will be a call to `clone`
Expand All @@ -351,18 +355,16 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
}

// FIXME: It's suspicious that this is public; clippy should probably use `walk_expr`.
#[instrument(skip(self), level = "debug")]
pub fn consume_expr(&self, expr: &hir::Expr<'_>) -> Result<(), Cx::Error> {
debug!("consume_expr(expr={:?})", expr);

let place_with_id = self.cat_expr(expr)?;
self.consume_or_copy(&place_with_id, place_with_id.hir_id);
self.walk_expr(expr)?;
Ok(())
}

#[instrument(skip(self), level = "debug")]
pub fn consume_or_clone_expr(&self, expr: &hir::Expr<'_>) -> Result<(), Cx::Error> {
debug!("consume_or_clone_expr(expr={:?})", expr);

let place_with_id = self.cat_expr(expr)?;
self.consume_clone_or_copy(&place_with_id, place_with_id.hir_id);
self.walk_expr(expr)?;
Expand All @@ -376,17 +378,15 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
Ok(())
}

#[instrument(skip(self), level = "debug")]
fn borrow_expr(&self, expr: &hir::Expr<'_>, bk: ty::BorrowKind) -> Result<(), Cx::Error> {
debug!("borrow_expr(expr={:?}, bk={:?})", expr, bk);

let place_with_id = self.cat_expr(expr)?;
self.delegate.borrow_mut().borrow(&place_with_id, place_with_id.hir_id, bk);
self.walk_expr(expr)
}

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

self.walk_adjustment(expr)?;

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

/// Indicates that the value of `blk` will be consumed, meaning either copied or moved
/// depending on its type.
#[instrument(skip(self), level = "debug")]
fn walk_block(&self, blk: &hir::Block<'_>) -> Result<(), Cx::Error> {
debug!("walk_block(blk.hir_id={})", blk.hir_id);

for stmt in blk.stmts {
self.walk_stmt(stmt)?;
}
Expand Down Expand Up @@ -861,7 +860,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
}

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

/// The core driver for walking a pattern
#[instrument(skip(self), level = "debug")]
fn walk_pat(
&self,
discr_place: &PlaceWithHirId<'tcx>,
pat: &hir::Pat<'_>,
has_guard: bool,
) -> Result<(), Cx::Error> {
debug!("walk_pat(discr_place={:?}, pat={:?}, has_guard={:?})", discr_place, pat, has_guard);

let tcx = self.cx.tcx();
self.cat_pattern(discr_place.clone(), pat, &mut |place, pat| {
match pat.kind {
Expand Down Expand Up @@ -1042,6 +1040,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
///
/// - When reporting the Place back to the Delegate, ensure that the UpvarId uses the enclosing
/// closure as the DefId.
#[instrument(skip(self), level = "debug")]
fn walk_captures(&self, closure_expr: &hir::Closure<'_>) -> Result<(), Cx::Error> {
fn upvar_is_local_variable(
upvars: Option<&FxIndexMap<HirId, hir::Upvar>>,
Expand All @@ -1051,8 +1050,6 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
upvars.map(|upvars| !upvars.contains_key(&upvar_id)).unwrap_or(body_owner_is_closure)
}

debug!("walk_captures({:?})", closure_expr);

let tcx = self.cx.tcx();
let closure_def_id = closure_expr.def_id;
// For purposes of this function, coroutine and closures are equivalent.
Expand Down Expand Up @@ -1164,55 +1161,17 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
}
}

/// The job of the categorization methods is to analyze an expression to
/// determine what kind of memory is used in evaluating it (for example,
/// where dereferences occur and what kind of pointer is dereferenced;
/// whether the memory is mutable, etc.).
///
/// Categorization effectively transforms all of our expressions into
/// expressions of the following forms (the actual enum has many more
/// possibilities, naturally, but they are all variants of these base
/// forms):
/// ```ignore (not-rust)
/// E = rvalue // some computed rvalue
/// | x // address of a local variable or argument
/// | *E // deref of a ptr
/// | E.comp // access to an interior component
/// ```
/// Imagine a routine ToAddr(Expr) that evaluates an expression and returns an
/// address where the result is to be found. If Expr is a place, then this
/// is the address of the place. If `Expr` is an rvalue, this is the address of
/// some temporary spot in memory where the result is stored.
///
/// Now, `cat_expr()` classifies the expression `Expr` and the address `A = ToAddr(Expr)`
/// as follows:
///
/// - `cat`: what kind of expression was this? This is a subset of the
/// full expression forms which only includes those that we care about
/// for the purpose of the analysis.
/// - `mutbl`: mutability of the address `A`.
/// - `ty`: the type of data found at the address `A`.
/// The job of the methods whose name starts with `cat_` is to analyze
/// expressions and construct the corresponding [`Place`]s. The `cat`
/// stands for "categorize", this is a leftover from long ago when
/// places were called "categorizations".
///
/// The resulting categorization tree differs somewhat from the expressions
/// themselves. For example, auto-derefs are explicit. Also, an index `a[b]` is
/// decomposed into two operations: a dereference to reach the array data and
/// then an index to jump forward to the relevant item.
///
/// ## By-reference upvars
///
/// One part of the codegen which may be non-obvious is that we translate
/// closure upvars into the dereference of a borrowed pointer; this more closely
/// resembles the runtime codegen. So, for example, if we had:
///
/// let mut x = 3;
/// let y = 5;
/// let inc = || x += y;
///
/// Then when we categorize `x` (*within* the closure) we would yield a
/// result of `*x'`, effectively, where `x'` is a `Categorization::Upvar` reference
/// tied to `x`. The type of `x'` will be a borrowed pointer.
/// Note that a [`Place`] differs somewhat from the expression itself. For
/// example, auto-derefs are explicit. Also, an index `a[b]` is decomposed into
/// two operations: a dereference to reach the array data and then an index to
/// jump forward to the relevant item.
impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx, Cx, D> {
fn resolve_type_vars_or_error(
fn resolve_type_vars_or_bug(
&self,
id: HirId,
ty: Option<Ty<'tcx>>,
Expand All @@ -1222,35 +1181,32 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
let ty = self.cx.resolve_vars_if_possible(ty);
self.cx.error_reported_in_ty(ty)?;
if ty.is_ty_var() {
debug!("resolve_type_vars_or_error: infer var from {:?}", ty);
debug!("resolve_type_vars_or_bug: infer var from {:?}", ty);
Err(self
.cx
.report_error(self.cx.tcx().hir().span(id), "encountered type variable"))
.report_bug(self.cx.tcx().hir().span(id), "encountered type variable"))
} else {
Ok(ty)
}
}
None => {
// FIXME: We shouldn't be relying on the infcx being tainted.
self.cx.tainted_by_errors()?;
bug!(
"no type for node {} in mem_categorization",
self.cx.tcx().hir_id_to_string(id)
);
bug!("no type for node {} in ExprUseVisitor", self.cx.tcx().hir_id_to_string(id));
}
}
}

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

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

fn expr_ty_adjusted(&self, expr: &hir::Expr<'_>) -> Result<Ty<'tcx>, Cx::Error> {
self.resolve_type_vars_or_error(
self.resolve_type_vars_or_bug(
expr.hir_id,
self.cx.typeck_results().expr_ty_adjusted_opt(expr),
)
Expand Down Expand Up @@ -1285,7 +1241,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
self.pat_ty_unadjusted(pat)
}

/// Like `TypeckResults::pat_ty`, but ignores implicit `&` patterns.
/// Like [`Self::pat_ty_adjusted`], but ignores implicit `&` patterns.
fn pat_ty_unadjusted(&self, pat: &hir::Pat<'_>) -> Result<Ty<'tcx>, Cx::Error> {
let base_ty = self.node_ty(pat.hir_id)?;
trace!(?base_ty);
Expand Down Expand Up @@ -1315,7 +1271,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
debug!("By-ref binding of non-derefable type");
Err(self
.cx
.report_error(pat.span, "by-ref binding of non-derefable type"))
.report_bug(pat.span, "by-ref binding of non-derefable type"))
}
}
} else {
Expand Down Expand Up @@ -1511,7 +1467,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
}
}

def => span_bug!(span, "unexpected definition in memory categorization: {:?}", def),
def => span_bug!(span, "unexpected definition in ExprUseVisitor: {:?}", def),
}
}

Expand Down Expand Up @@ -1604,7 +1560,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
Some(ty) => ty,
None => {
debug!("explicit deref of non-derefable type: {:?}", base_curr_ty);
return Err(self.cx.report_error(
return Err(self.cx.report_bug(
self.cx.tcx().hir().span(node),
"explicit deref of non-derefable type",
));
Expand All @@ -1629,7 +1585,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
let ty::Adt(adt_def, _) = self.cx.try_structurally_resolve_type(span, ty).kind() else {
return Err(self
.cx
.report_error(span, "struct or tuple struct pattern not applied to an ADT"));
.report_bug(span, "struct or tuple struct pattern not applied to an ADT"));
};

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

Expand Down Expand Up @@ -1854,7 +1810,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
debug!("explicit index of non-indexable type {:?}", place_with_id);
return Err(self
.cx
.report_error(pat.span, "explicit index of non-indexable type"));
.report_bug(pat.span, "explicit index of non-indexable type"));
};
let elt_place = self.cat_projection(
pat.hir_id,
Expand Down
10 changes: 5 additions & 5 deletions compiler/rustc_hir_typeck/src/upvar.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,12 @@
//! from there).
//!
//! The fact that we are inferring borrow kinds as we go results in a
//! semi-hacky interaction with mem-categorization. In particular,
//! mem-categorization will query the current borrow kind as it
//! categorizes, and we'll return the *current* value, but this may get
//! semi-hacky interaction with the way `ExprUseVisitor` is computing
//! `Place`s. In particular, it will query the current borrow kind as it
//! goes, and we'll return the *current* value, but this may get
//! adjusted later. Therefore, in this module, we generally ignore the
//! borrow kind (and derived mutabilities) that are returned from
//! mem-categorization, since they may be inaccurate. (Another option
//! borrow kind (and derived mutabilities) that `ExprUseVisitor` returns
//! within `Place`s, since they may be inaccurate. (Another option
//! would be to use a unification scheme, where instead of returning a
//! concrete borrow kind like `ty::ImmBorrow`, we return a
//! `ty::InferBorrow(upvar_id)` or something like that, but this would
Expand Down
Loading
Loading