Skip to content

Commit

Permalink
Auto merge of #125468 - BoxyUwU:remove_defid_from_regionparam, r=comp…
Browse files Browse the repository at this point in the history 
…iler-errors

Remove `DefId` from `EarlyParamRegion`

Currently we represent usages of `Region` parameters via the `ReEarlyParam` or `ReLateParam` variants. The `ReEarlyParam` is effectively equivalent to `TyKind::Param` and `ConstKind::Param` (i.e. it stores a `Symbol` and a `u32` index) however it also stores a `DefId` for the definition of the lifetime parameter.

This was used in roughly two places:
- Borrowck diagnostics instead of threading the appropriate `body_id` down to relevant locations. Interestingly there were already some places that had to pass down a `DefId` manually.
- Some opaque type checking logic was using the `DefId` field to track captured lifetimes

I've split this PR up into a commit for generate rote changes to diagnostics code to pass around a `DefId` manually everywhere, and another commit for the opaque type related changes which likely require more careful review as they might change the semantics of lints/errors.

Instead of manually passing the `DefId` around everywhere I previously tried to bundle it in with `TypeErrCtxt` but ran into issues with some call sites of `infcx.err_ctxt` being unable to provide a `DefId`, particularly places involved with trait solving and normalization. It might be worth investigating adding some new wrapper type to pass this around everywhere but I think this might be acceptable for now.

This pr also has the effect of reducing the size of `EarlyParamRegion` from 16 bytes -> 8 bytes. I wouldn't expect this to have any direct performance improvement however, other variants of `RegionKind` over `8` bytes are all because they contain a `BoundRegionKind` which is, as far as I know, mostly there for diagnostics. If we're ever able to remove this it would shrink the `RegionKind` type from `24` bytes to `12` (and with clever bit packing we might be able to get it to `8` bytes). I am curious what the performance impact would be of removing interning of `Region`'s if we ever manage to shrink `RegionKind` that much.

Sidenote: by removing the `DefId` the `Debug` output for `Region` has gotten significantly nicer. As an example see this opaque type debug print before vs after this PR:
`Opaque(DefId(0:13 ~ impl_trait_captures[aeb9]::foo::{opaque#0}), [DefId(0:9 ~ impl_trait_captures[aeb9]::foo::'a)_'a/#0, T, DefId(0:9 ~ impl_trait_captures[aeb9]::foo::'a)_'a/#0])`
`Opaque(DefId(0:13 ~ impl_trait_captures[aeb9]::foo::{opaque#0}), ['a/#0, T, 'a/#0])`

r? `@compiler-errors` (I would like someone who understands the opaque type setup to atleast review the type system commit, but the rest is likely reviewable by anyone)
  • Loading branch information
@bors
bors committed May 27, 2024
2 parents cdc509f + ed8e436 commit fec98b3
Show file tree
Hide file tree
Showing 31 changed files with 320 additions and 212 deletions.
36 changes: 22 additions & 14 deletions compiler/rustc_borrowck/src/diagnostics/bound_region_errors.rs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use rustc_errors::Diag;
use rustc_hir::def_id::LocalDefId;
use rustc_infer::infer::canonical::Canonical;
use rustc_infer::infer::error_reporting::nice_region_error::NiceRegionError;
use rustc_infer::infer::region_constraints::Constraint;
Expand Down Expand Up @@ -241,7 +242,7 @@ impl<'tcx> TypeOpInfo<'tcx> for PredicateQuery<'tcx> {
mbcx.infcx.tcx.infer_ctxt().build_with_canonical(cause.span, &self.canonical_query);
let ocx = ObligationCtxt::new(&infcx);
type_op_prove_predicate_with_cause(&ocx, key, cause);
try_extract_error_from_fulfill_cx(&ocx, placeholder_region, error_region)
try_extract_error_from_fulfill_cx(&ocx, mbcx.mir_def_id(), placeholder_region, error_region)
}
}

Expand Down Expand Up @@ -287,7 +288,7 @@ where
let (param_env, value) = key.into_parts();
let _ = ocx.normalize(&cause, param_env, value.value);

try_extract_error_from_fulfill_cx(&ocx, placeholder_region, error_region)
try_extract_error_from_fulfill_cx(&ocx, mbcx.mir_def_id(), placeholder_region, error_region)
}
}

Expand Down Expand Up @@ -318,7 +319,7 @@ impl<'tcx> TypeOpInfo<'tcx> for AscribeUserTypeQuery<'tcx> {
mbcx.infcx.tcx.infer_ctxt().build_with_canonical(cause.span, &self.canonical_query);
let ocx = ObligationCtxt::new(&infcx);
type_op_ascribe_user_type_with_span(&ocx, key, Some(cause.span)).ok()?;
try_extract_error_from_fulfill_cx(&ocx, placeholder_region, error_region)
try_extract_error_from_fulfill_cx(&ocx, mbcx.mir_def_id(), placeholder_region, error_region)
}
}

Expand All @@ -342,6 +343,7 @@ impl<'tcx> TypeOpInfo<'tcx> for crate::type_check::InstantiateOpaqueType<'tcx> {
) -> Option<Diag<'tcx>> {
try_extract_error_from_region_constraints(
mbcx.infcx,
mbcx.mir_def_id(),
placeholder_region,
error_region,
self.region_constraints.as_ref().unwrap(),
Expand All @@ -358,6 +360,7 @@ impl<'tcx> TypeOpInfo<'tcx> for crate::type_check::InstantiateOpaqueType<'tcx> {
#[instrument(skip(ocx), level = "debug")]
fn try_extract_error_from_fulfill_cx<'tcx>(
ocx: &ObligationCtxt<'_, 'tcx>,
generic_param_scope: LocalDefId,
placeholder_region: ty::Region<'tcx>,
error_region: Option<ty::Region<'tcx>>,
) -> Option<Diag<'tcx>> {
Expand All @@ -368,6 +371,7 @@ fn try_extract_error_from_fulfill_cx<'tcx>(
let region_constraints = ocx.infcx.with_region_constraints(|r| r.clone());
try_extract_error_from_region_constraints(
ocx.infcx,
generic_param_scope,
placeholder_region,
error_region,
&region_constraints,
Expand All @@ -379,6 +383,7 @@ fn try_extract_error_from_fulfill_cx<'tcx>(
#[instrument(level = "debug", skip(infcx, region_var_origin, universe_of_region))]
fn try_extract_error_from_region_constraints<'tcx>(
infcx: &InferCtxt<'tcx>,
generic_param_scope: LocalDefId,
placeholder_region: ty::Region<'tcx>,
error_region: Option<ty::Region<'tcx>>,
region_constraints: &RegionConstraintData<'tcx>,
Expand Down Expand Up @@ -452,15 +457,18 @@ fn try_extract_error_from_region_constraints<'tcx>(
RegionResolutionError::ConcreteFailure(cause.clone(), sub_region, placeholder_region)
}
};
NiceRegionError::new(&infcx.err_ctxt(), error).try_report_from_nll().or_else(|| {
if let SubregionOrigin::Subtype(trace) = cause {
Some(
infcx
.err_ctxt()
.report_and_explain_type_error(*trace, TypeError::RegionsPlaceholderMismatch),
)
} else {
None
}
})
NiceRegionError::new(&infcx.err_ctxt(), generic_param_scope, error)
.try_report_from_nll()
.or_else(|| {
if let SubregionOrigin::Subtype(trace) = cause {
Some(
infcx.err_ctxt().report_and_explain_type_error(
*trace,
TypeError::RegionsPlaceholderMismatch,
),
)
} else {
None
}
})
}
39 changes: 24 additions & 15 deletions compiler/rustc_borrowck/src/diagnostics/region_errors.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
let named_region = self.regioncx.name_regions(self.infcx.tcx, member_region);
let diag = unexpected_hidden_region_diagnostic(
self.infcx.tcx,
self.mir_def_id(),
span,
named_ty,
named_region,
Expand Down Expand Up @@ -453,7 +454,8 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
// Check if we can use one of the "nice region errors".
if let (Some(f), Some(o)) = (self.to_error_region(fr), self.to_error_region(outlived_fr)) {
let infer_err = self.infcx.err_ctxt();
let nice = NiceRegionError::new_from_span(&infer_err, cause.span, o, f);
let nice =
NiceRegionError::new_from_span(&infer_err, self.mir_def_id(), cause.span, o, f);
if let Some(diag) = nice.try_report_from_nll() {
self.buffer_error(diag);
return;
Expand Down Expand Up @@ -843,14 +845,16 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
if *outlived_f != ty::ReStatic {
return;
}
let suitable_region = self.infcx.tcx.is_suitable_region(f);
let suitable_region = self.infcx.tcx.is_suitable_region(self.mir_def_id(), f);
let Some(suitable_region) = suitable_region else {
return;
};

let fn_returns = self.infcx.tcx.return_type_impl_or_dyn_traits(suitable_region.def_id);

let param = if let Some(param) = find_param_with_region(self.infcx.tcx, f, outlived_f) {
let param = if let Some(param) =
find_param_with_region(self.infcx.tcx, self.mir_def_id(), f, outlived_f)
{
param
} else {
return;
Expand Down Expand Up @@ -959,7 +963,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
return;
};

let param = match find_param_with_region(tcx, f, o) {
let param = match find_param_with_region(tcx, self.mir_def_id(), f, o) {
Some(param) => param,
None => return,
};
Expand Down Expand Up @@ -1022,25 +1026,30 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
return;
};

let Some((ty_sub, _)) = self
.infcx
.tcx
.is_suitable_region(sub)
.and_then(|anon_reg| find_anon_type(self.infcx.tcx, sub, &anon_reg.bound_region))
let Some((ty_sub, _)) =
self.infcx.tcx.is_suitable_region(self.mir_def_id(), sub).and_then(|anon_reg| {
find_anon_type(self.infcx.tcx, self.mir_def_id(), sub, &anon_reg.bound_region)
})
else {
return;
};

let Some((ty_sup, _)) = self
.infcx
.tcx
.is_suitable_region(sup)
.and_then(|anon_reg| find_anon_type(self.infcx.tcx, sup, &anon_reg.bound_region))
let Some((ty_sup, _)) =
self.infcx.tcx.is_suitable_region(self.mir_def_id(), sup).and_then(|anon_reg| {
find_anon_type(self.infcx.tcx, self.mir_def_id(), sup, &anon_reg.bound_region)
})
else {
return;
};

suggest_adding_lifetime_params(self.infcx.tcx, sub, ty_sup, ty_sub, diag);
suggest_adding_lifetime_params(
self.infcx.tcx,
diag,
self.mir_def_id(),
sub,
ty_sup,
ty_sub,
);
}

#[allow(rustc::diagnostic_outside_of_impl)]
Expand Down
8 changes: 5 additions & 3 deletions compiler/rustc_borrowck/src/diagnostics/region_name.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,8 @@ impl<'tcx> MirBorrowckCtxt<'_, 'tcx> {
debug!("give_region_a_name: error_region = {:?}", error_region);
match *error_region {
ty::ReEarlyParam(ebr) => ebr.has_name().then(|| {
let span = tcx.hir().span_if_local(ebr.def_id).unwrap_or(DUMMY_SP);
let def_id = tcx.generics_of(self.mir_def_id()).region_param(ebr, tcx).def_id;
let span = tcx.hir().span_if_local(def_id).unwrap_or(DUMMY_SP);
RegionName { name: ebr.name, source: RegionNameSource::NamedEarlyParamRegion(span) }
}),

Expand Down Expand Up @@ -912,7 +913,8 @@ impl<'tcx> MirBorrowckCtxt<'_, 'tcx> {
};

let tcx = self.infcx.tcx;
let region_parent = tcx.parent(region.def_id);
let region_def = tcx.generics_of(self.mir_def_id()).region_param(region, tcx).def_id;
let region_parent = tcx.parent(region_def);
let DefKind::Impl { .. } = tcx.def_kind(region_parent) else {
return None;
};
Expand All @@ -925,7 +927,7 @@ impl<'tcx> MirBorrowckCtxt<'_, 'tcx> {
Some(RegionName {
name: self.synthesize_region_name(),
source: RegionNameSource::AnonRegionFromImplSignature(
tcx.def_span(region.def_id),
tcx.def_span(region_def),
// FIXME(compiler-errors): Does this ever actually show up
// anywhere other than the self type? I couldn't create an
// example of a `'_` in the impl's trait being referenceable.
Expand Down
15 changes: 5 additions & 10 deletions compiler/rustc_hir_analysis/src/check/check.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -538,11 +538,9 @@ fn check_opaque_precise_captures<'tcx>(tcx: TyCtxt<'tcx>, opaque_def_id: LocalDe
// the cases that were stabilized with the `impl_trait_projection`
// feature -- see <https://github.com/rust-lang/rust/pull/115659>.
if let DefKind::LifetimeParam = tcx.def_kind(def_id)
&& let ty::ReEarlyParam(ty::EarlyParamRegion { def_id, .. })
| ty::ReLateParam(ty::LateParamRegion {
bound_region: ty::BoundRegionKind::BrNamed(def_id, _),
..
}) = *tcx.map_opaque_lifetime_to_parent_lifetime(def_id.expect_local())
&& let Some(def_id) = tcx
.map_opaque_lifetime_to_parent_lifetime(def_id.expect_local())
.opt_param_def_id(tcx, tcx.parent(opaque_def_id.to_def_id()))
{
shadowed_captures.insert(def_id);
}
Expand Down Expand Up @@ -585,12 +583,9 @@ fn check_opaque_precise_captures<'tcx>(tcx: TyCtxt<'tcx>, opaque_def_id: LocalDe
// Check if the lifetime param was captured but isn't named in the precise captures list.
if variances[param.index as usize] == ty::Invariant {
if let DefKind::OpaqueTy = tcx.def_kind(tcx.parent(param.def_id))
&& let ty::ReEarlyParam(ty::EarlyParamRegion { def_id, .. })
| ty::ReLateParam(ty::LateParamRegion {
bound_region: ty::BoundRegionKind::BrNamed(def_id, _),
..
}) = *tcx
&& let Some(def_id) = tcx
.map_opaque_lifetime_to_parent_lifetime(param.def_id.expect_local())
.opt_param_def_id(tcx, tcx.parent(opaque_def_id.to_def_id()))
{
tcx.dcx().emit_err(errors::LifetimeNotCaptured {
opaque_span,
Expand Down
14 changes: 5 additions & 9 deletions compiler/rustc_hir_analysis/src/check/compare_impl_item.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -876,7 +876,8 @@ impl<'tcx> ty::FallibleTypeFolder<TyCtxt<'tcx>> for RemapHiddenTyRegions<'tcx> {
ty::ReLateParam(_) => {}
// Remap early-bound regions as long as they don't come from the `impl` itself,
// in which case we don't really need to renumber them.
ty::ReEarlyParam(ebr) if self.tcx.parent(ebr.def_id) != self.impl_def_id => {}
ty::ReEarlyParam(ebr)
if ebr.index >= self.tcx.generics_of(self.impl_def_id).count() as u32 => {}
_ => return Ok(region),
}

Expand All @@ -889,12 +890,8 @@ impl<'tcx> ty::FallibleTypeFolder<TyCtxt<'tcx>> for RemapHiddenTyRegions<'tcx> {
);
}
} else {
let guar = match region.kind() {
ty::ReEarlyParam(ty::EarlyParamRegion { def_id, .. })
| ty::ReLateParam(ty::LateParamRegion {
bound_region: ty::BoundRegionKind::BrNamed(def_id, _),
..
}) => {
let guar = match region.opt_param_def_id(self.tcx, self.tcx.parent(self.def_id)) {
Some(def_id) => {
let return_span = if let ty::Alias(ty::Opaque, opaque_ty) = self.ty.kind() {
self.tcx.def_span(opaque_ty.def_id)
} else {
Expand All @@ -914,7 +911,7 @@ impl<'tcx> ty::FallibleTypeFolder<TyCtxt<'tcx>> for RemapHiddenTyRegions<'tcx> {
.with_note(format!("hidden type inferred to be `{}`", self.ty))
.emit()
}
_ => {
None => {
// This code path is not reached in any tests, but may be
// reachable. If this is triggered, it should be converted
// to `delayed_bug` and the triggering case turned into a
Expand All @@ -928,7 +925,6 @@ impl<'tcx> ty::FallibleTypeFolder<TyCtxt<'tcx>> for RemapHiddenTyRegions<'tcx> {
Ok(ty::Region::new_early_param(
self.tcx,
ty::EarlyParamRegion {
def_id: e.def_id,
name: e.name,
index: (e.index as usize - self.num_trait_args + self.num_impl_args) as u32,
},
Expand Down
34 changes: 10 additions & 24 deletions compiler/rustc_hir_analysis/src/check/wfcheck.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -675,11 +675,7 @@ fn gather_gat_bounds<'tcx, T: TypeFoldable<TyCtxt<'tcx>>>(
let region_param = gat_generics.param_at(*region_a_idx, tcx);
let region_param = ty::Region::new_early_param(
tcx,
ty::EarlyParamRegion {
def_id: region_param.def_id,
index: region_param.index,
name: region_param.name,
},
ty::EarlyParamRegion { index: region_param.index, name: region_param.name },
);
// The predicate we expect to see. (In our example,
// `Self: 'me`.)
Expand Down Expand Up @@ -708,21 +704,13 @@ fn gather_gat_bounds<'tcx, T: TypeFoldable<TyCtxt<'tcx>>>(
let region_a_param = gat_generics.param_at(*region_a_idx, tcx);
let region_a_param = ty::Region::new_early_param(
tcx,
ty::EarlyParamRegion {
def_id: region_a_param.def_id,
index: region_a_param.index,
name: region_a_param.name,
},
ty::EarlyParamRegion { index: region_a_param.index, name: region_a_param.name },
);
// Same for the region.
let region_b_param = gat_generics.param_at(*region_b_idx, tcx);
let region_b_param = ty::Region::new_early_param(
tcx,
ty::EarlyParamRegion {
def_id: region_b_param.def_id,
index: region_b_param.index,
name: region_b_param.name,
},
ty::EarlyParamRegion { index: region_b_param.index, name: region_b_param.name },
);
// The predicate we expect to see.
bounds.insert(
Expand Down Expand Up @@ -2101,16 +2089,14 @@ fn lint_redundant_lifetimes<'tcx>(
}

for &victim in &lifetimes[(idx + 1)..] {
// We should only have late-bound lifetimes of the `BrNamed` variety,
// since we get these signatures straight from `hir_lowering`. And any
// other regions (ReError/ReStatic/etc.) shouldn't matter, since we
// All region parameters should have a `DefId` available as:
// - Late-bound parameters should be of the`BrNamed` variety,
// since we get these signatures straight from `hir_lowering`.
// - Early-bound parameters unconditionally have a `DefId` available.
//
// Any other regions (ReError/ReStatic/etc.) shouldn't matter, since we
// can't really suggest to remove them.
let (ty::ReEarlyParam(ty::EarlyParamRegion { def_id, .. })
| ty::ReLateParam(ty::LateParamRegion {
bound_region: ty::BoundRegionKind::BrNamed(def_id, _),
..
})) = victim.kind()
else {
let Some(def_id) = victim.opt_param_def_id(tcx, owner_id.to_def_id()) else {
continue;
};

Expand Down
1 change: 0 additions & 1 deletion compiler/rustc_hir_analysis/src/collect.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -453,7 +453,6 @@ impl<'tcx> HirTyLowerer<'tcx> for ItemCtxt<'tcx> {
poly_trait_ref,
|_| {
ty::Region::new_early_param(self.tcx, ty::EarlyParamRegion {
def_id: item_def_id,
index: 0,
name: Symbol::intern(&lt_name),
})
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_hir_analysis/src/collect/predicates_of.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,7 @@ fn compute_bidirectional_outlives_predicates<'tcx>(
if let ty::ReEarlyParam(..) = *orig_lifetime {
let dup_lifetime = ty::Region::new_early_param(
tcx,
ty::EarlyParamRegion { def_id: param.def_id, index: param.index, name: param.name },
ty::EarlyParamRegion { index: param.index, name: param.name },
);
let span = tcx.def_span(param.def_id);
predicates.push((
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_hir_analysis/src/hir_ty_lowering/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
let item_def_id = tcx.hir().ty_param_owner(def_id.expect_local());
let generics = tcx.generics_of(item_def_id);
let index = generics.param_def_id_to_index[&def_id];
ty::Region::new_early_param(tcx, ty::EarlyParamRegion { def_id, index, name })
ty::Region::new_early_param(tcx, ty::EarlyParamRegion { index, name })
}

Some(rbv::ResolvedArg::Free(scope, id)) => {
Expand Down
5 changes: 4 additions & 1 deletion compiler/rustc_infer/src/errors/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use rustc_errors::{
MultiSpan, SubdiagMessageOp, Subdiagnostic,
};
use rustc_hir as hir;
use rustc_hir::def_id::LocalDefId;
use rustc_hir::intravisit::{walk_ty, Visitor};
use rustc_hir::FnRetTy;
use rustc_macros::{Diagnostic, Subdiagnostic};
Expand Down Expand Up @@ -344,6 +345,7 @@ impl Subdiagnostic for LifetimeMismatchLabels {

pub struct AddLifetimeParamsSuggestion<'a> {
pub tcx: TyCtxt<'a>,
pub generic_param_scope: LocalDefId,
pub sub: Region<'a>,
pub ty_sup: &'a hir::Ty<'a>,
pub ty_sub: &'a hir::Ty<'a>,
Expand All @@ -357,7 +359,8 @@ impl Subdiagnostic for AddLifetimeParamsSuggestion<'_> {
_f: &F,
) {
let mut mk_suggestion = || {
let Some(anon_reg) = self.tcx.is_suitable_region(self.sub) else {
let Some(anon_reg) = self.tcx.is_suitable_region(self.generic_param_scope, self.sub)
else {
return false;
};

Expand Down
Loading

0 comments on commit fec98b3

Please sign in to comment.