From fb298e80c3dcf9d5ed81c9b86dedd40256775c1d Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Thu, 9 May 2024 11:32:05 -0400 Subject: [PATCH] Apply nits --- .../rustc_hir_typeck/src/expr_use_visitor.rs | 150 ++++++++++++------ .../typeck/normalize-in-upvar-collection.rs | 4 + 2 files changed, 105 insertions(+), 49 deletions(-) diff --git a/compiler/rustc_hir_typeck/src/expr_use_visitor.rs b/compiler/rustc_hir_typeck/src/expr_use_visitor.rs index 702f559b2f3..05cedb19eb8 100644 --- a/compiler/rustc_hir_typeck/src/expr_use_visitor.rs +++ b/compiler/rustc_hir_typeck/src/expr_use_visitor.rs @@ -30,8 +30,6 @@ use ty::BorrowKind::ImmBorrow; use crate::fn_ctxt::FnCtxt; -type McResult = Result; - /// This trait defines the callbacks you can expect to receive when /// employing the ExprUseVisitor. pub trait Delegate<'tcx> { @@ -219,6 +217,8 @@ impl<'tcx> TypeInformationCtxt<'tcx> for (&LateContext<'tcx>, LocalDefId) { /// This is the code that actually walks the tree. 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 + /// still have calls to our own helper functions. delegate: RefCell, upvars: Option<&'tcx FxIndexMap>, } @@ -517,14 +517,14 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx discr: &Expr<'_>, discr_place: PlaceWithHirId<'tcx>, pats: impl Iterator>, - ) -> McResult<()> { + ) -> Result<(), ErrorGuaranteed> { // Matching should not always be considered a use of the place, hence // discr does not necessarily need to be borrowed. // We only want to borrow discr if the pattern contain something other // than wildcards. let mut needs_to_be_read = false; for pat in pats { - self.cat_pattern(discr_place.clone(), pat, |place, pat| { + self.cat_pattern(discr_place.clone(), pat, &mut |place, pat| { match &pat.kind { PatKind::Binding(.., opt_sub_pat) => { // If the opt_sub_pat is None, then the binding does not count as @@ -836,7 +836,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx debug!("walk_pat(discr_place={:?}, pat={:?}, has_guard={:?})", discr_place, pat, has_guard); let tcx = self.cx.tcx(); - return_if_err!(self.cat_pattern(discr_place.clone(), pat, |place, pat| { + return_if_err!(self.cat_pattern(discr_place.clone(), pat, &mut |place, pat| { if let PatKind::Binding(_, canonical_id, ..) = pat.kind { debug!("walk_pat: binding place={:?} pat={:?}", place, pat); if let Some(bm) = @@ -1021,8 +1021,61 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx } } } +} - fn resolve_type_vars_or_error(&self, id: HirId, ty: Option>) -> McResult> { +/// 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 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. +impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx, Cx, D> { + fn resolve_type_vars_or_error( + &self, + id: HirId, + ty: Option>, + ) -> Result, ErrorGuaranteed> { match ty { Some(ty) => { let ty = self.cx.resolve_vars_if_possible(ty); @@ -1051,15 +1104,15 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx } } - fn node_ty(&self, hir_id: HirId) -> McResult> { + fn node_ty(&self, hir_id: HirId) -> Result, ErrorGuaranteed> { self.resolve_type_vars_or_error(hir_id, self.cx.typeck_results().node_type_opt(hir_id)) } - fn expr_ty(&self, expr: &hir::Expr<'_>) -> McResult> { + fn expr_ty(&self, expr: &hir::Expr<'_>) -> Result, ErrorGuaranteed> { self.resolve_type_vars_or_error(expr.hir_id, self.cx.typeck_results().expr_ty_opt(expr)) } - fn expr_ty_adjusted(&self, expr: &hir::Expr<'_>) -> McResult> { + fn expr_ty_adjusted(&self, expr: &hir::Expr<'_>) -> Result, ErrorGuaranteed> { self.resolve_type_vars_or_error( expr.hir_id, self.cx.typeck_results().expr_ty_adjusted_opt(expr), @@ -1076,7 +1129,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx /// implicit deref patterns attached (e.g., it is really /// `&Some(x)`). In that case, we return the "outermost" type /// (e.g., `&Option`). - fn pat_ty_adjusted(&self, pat: &hir::Pat<'_>) -> McResult> { + fn pat_ty_adjusted(&self, pat: &hir::Pat<'_>) -> Result, ErrorGuaranteed> { // Check for implicit `&` types wrapping the pattern; note // that these are never attached to binding patterns, so // actually this is somewhat "disjoint" from the code below @@ -1091,8 +1144,8 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx self.pat_ty_unadjusted(pat) } - /// Like `pat_ty`, but ignores implicit `&` patterns. - fn pat_ty_unadjusted(&self, pat: &hir::Pat<'_>) -> McResult> { + /// Like `TypeckResults::pat_ty`, but ignores implicit `&` patterns. + fn pat_ty_unadjusted(&self, pat: &hir::Pat<'_>) -> Result, ErrorGuaranteed> { let base_ty = self.node_ty(pat.hir_id)?; trace!(?base_ty); @@ -1134,7 +1187,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx } } - fn cat_expr(&self, expr: &hir::Expr<'_>) -> McResult> { + fn cat_expr(&self, expr: &hir::Expr<'_>) -> Result, ErrorGuaranteed> { self.cat_expr_(expr, self.cx.typeck_results().expr_adjustments(expr)) } @@ -1144,7 +1197,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx &self, expr: &hir::Expr<'_>, adjustments: &[adjustment::Adjustment<'tcx>], - ) -> McResult> { + ) -> Result, ErrorGuaranteed> { match adjustments.split_last() { None => self.cat_expr_unadjusted(expr), Some((adjustment, previous)) => { @@ -1158,7 +1211,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx expr: &hir::Expr<'_>, previous: PlaceWithHirId<'tcx>, adjustment: &adjustment::Adjustment<'tcx>, - ) -> McResult> { + ) -> Result, ErrorGuaranteed> { self.cat_expr_adjusted_with(expr, || Ok(previous), adjustment) } @@ -1167,9 +1220,9 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx expr: &hir::Expr<'_>, previous: F, adjustment: &adjustment::Adjustment<'tcx>, - ) -> McResult> + ) -> Result, ErrorGuaranteed> where - F: FnOnce() -> McResult>, + F: FnOnce() -> Result, ErrorGuaranteed>, { let target = self.cx.resolve_vars_if_possible(adjustment.target); match adjustment.kind { @@ -1194,7 +1247,10 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx } } - fn cat_expr_unadjusted(&self, expr: &hir::Expr<'_>) -> McResult> { + fn cat_expr_unadjusted( + &self, + expr: &hir::Expr<'_>, + ) -> Result, ErrorGuaranteed> { let expr_ty = self.expr_ty(expr)?; match expr.kind { hir::ExprKind::Unary(hir::UnOp::Deref, e_base) => { @@ -1285,7 +1341,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx span: Span, expr_ty: Ty<'tcx>, res: Res, - ) -> McResult> { + ) -> Result, ErrorGuaranteed> { match res { Res::Def( DefKind::Ctor(..) @@ -1319,7 +1375,11 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx /// Note: the actual upvar access contains invisible derefs of closure /// environment and upvar reference as appropriate. Only regionck cares /// about these dereferences, so we let it compute them as needed. - fn cat_upvar(&self, hir_id: HirId, var_id: HirId) -> McResult> { + fn cat_upvar( + &self, + hir_id: HirId, + var_id: HirId, + ) -> Result, ErrorGuaranteed> { let closure_expr_def_id = self.cx.body_owner_def_id(); let upvar_id = ty::UpvarId { @@ -1368,7 +1428,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx &self, expr: &hir::Expr<'_>, base: &hir::Expr<'_>, - ) -> McResult> { + ) -> Result, ErrorGuaranteed> { // Reconstruct the output assuming it's a reference with the // same region and mutability as the receiver. This holds for // `Deref(Mut)::Deref(_mut)` and `Index(Mut)::index(_mut)`. @@ -1390,7 +1450,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx &self, node: HirId, base_place: PlaceWithHirId<'tcx>, - ) -> McResult> { + ) -> Result, ErrorGuaranteed> { let base_curr_ty = base_place.place.ty(); let deref_ty = match self .cx @@ -1415,18 +1475,6 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx Ok(PlaceWithHirId::new(node, base_place.place.base_ty, base_place.place.base, projections)) } - fn cat_pattern( - &self, - place: PlaceWithHirId<'tcx>, - pat: &hir::Pat<'_>, - mut op: F, - ) -> McResult<()> - where - F: FnMut(&PlaceWithHirId<'tcx>, &hir::Pat<'_>), - { - self.cat_pattern_(place, pat, &mut op) - } - /// Returns the variant index for an ADT used within a Struct or TupleStruct pattern /// Here `pat_hir_id` is the HirId of the pattern itself. fn variant_index_for_adt( @@ -1434,7 +1482,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx qpath: &hir::QPath<'_>, pat_hir_id: HirId, span: Span, - ) -> McResult { + ) -> Result { let res = self.cx.typeck_results().qpath_res(qpath, pat_hir_id); let ty = self.cx.typeck_results().node_type(pat_hir_id); let ty::Adt(adt_def, _) = self.cx.try_structurally_resolve_type(span, ty).kind() else { @@ -1469,7 +1517,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx pat_hir_id: HirId, variant_index: VariantIdx, span: Span, - ) -> McResult { + ) -> Result { let ty = self.cx.typeck_results().node_type(pat_hir_id); match self.cx.try_structurally_resolve_type(span, ty).kind() { ty::Adt(adt_def, _) => Ok(adt_def.variant(variant_index).fields.len()), @@ -1484,7 +1532,11 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx /// Returns the total number of fields in a tuple used within a Tuple pattern. /// Here `pat_hir_id` is the HirId of the pattern itself. - fn total_fields_in_tuple(&self, pat_hir_id: HirId, span: Span) -> McResult { + fn total_fields_in_tuple( + &self, + pat_hir_id: HirId, + span: Span, + ) -> Result { 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()), @@ -1502,12 +1554,12 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx /// In general, the way that this works is that we walk down the pattern, /// constructing a `PlaceWithHirId` that represents the path that will be taken /// to reach the value being matched. - fn cat_pattern_( + fn cat_pattern( &self, mut place_with_id: PlaceWithHirId<'tcx>, pat: &hir::Pat<'_>, op: &mut F, - ) -> McResult<()> + ) -> Result<(), ErrorGuaranteed> where F: FnMut(&PlaceWithHirId<'tcx>, &hir::Pat<'_>), { @@ -1578,7 +1630,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx subpat_ty, projection_kind, ); - self.cat_pattern_(sub_place, subpat, op)?; + self.cat_pattern(sub_place, subpat, op)?; } } @@ -1598,7 +1650,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx subpat_ty, projection_kind, ); - self.cat_pattern_(sub_place, subpat, op)?; + self.cat_pattern(sub_place, subpat, op)?; } } @@ -1623,18 +1675,18 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx field_ty, ProjectionKind::Field(field_index, variant_index), ); - self.cat_pattern_(field_place, fp.pat, op)?; + self.cat_pattern(field_place, fp.pat, op)?; } } PatKind::Or(pats) => { for pat in pats { - self.cat_pattern_(place_with_id.clone(), pat, op)?; + self.cat_pattern(place_with_id.clone(), pat, op)?; } } PatKind::Binding(.., Some(subpat)) => { - self.cat_pattern_(place_with_id, subpat, op)?; + self.cat_pattern(place_with_id, subpat, op)?; } PatKind::Box(subpat) | PatKind::Ref(subpat, _) => { @@ -1642,7 +1694,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx // PatKind::Ref since that information is already contained // in the type. let subplace = self.cat_deref(pat.hir_id, place_with_id)?; - self.cat_pattern_(subplace, subpat, op)?; + self.cat_pattern(subplace, subpat, op)?; } PatKind::Deref(subpat) => { let mutable = self.cx.typeck_results().pat_has_ref_mut_binding(subpat); @@ -1652,7 +1704,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx let ty = Ty::new_ref(self.cx.tcx(), re_erased, ty, mutability); // A deref pattern generates a temporary. let place = self.cat_rvalue(pat.hir_id, ty); - self.cat_pattern_(place, subpat, op)?; + self.cat_pattern(place, subpat, op)?; } PatKind::Slice(before, ref slice, after) => { @@ -1671,7 +1723,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx ProjectionKind::Index, ); for before_pat in before { - self.cat_pattern_(elt_place.clone(), before_pat, op)?; + self.cat_pattern(elt_place.clone(), before_pat, op)?; } if let Some(slice_pat) = *slice { let slice_pat_ty = self.pat_ty_adjusted(slice_pat)?; @@ -1681,10 +1733,10 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx slice_pat_ty, ProjectionKind::Subslice, ); - self.cat_pattern_(slice_place, slice_pat, op)?; + self.cat_pattern(slice_place, slice_pat, op)?; } for after_pat in after { - self.cat_pattern_(elt_place.clone(), after_pat, op)?; + self.cat_pattern(elt_place.clone(), after_pat, op)?; } } diff --git a/tests/ui/traits/next-solver/typeck/normalize-in-upvar-collection.rs b/tests/ui/traits/next-solver/typeck/normalize-in-upvar-collection.rs index 7e4e466fa4d..6567f275240 100644 --- a/tests/ui/traits/next-solver/typeck/normalize-in-upvar-collection.rs +++ b/tests/ui/traits/next-solver/typeck/normalize-in-upvar-collection.rs @@ -1,6 +1,10 @@ //@ compile-flags: -Znext-solver //@ check-pass +// Fixes a regression in icu_provider_adaptors where we weren't normalizing the +// return type of a function type before performing a `Ty::builtin_deref` call, +// leading to an ICE. + struct Struct { field: i32, }