Skip to content

Commit

Permalink
Auto merge of #101680 - jackh726:implied-cleanup, r=lcnr
Browse files Browse the repository at this point in the history 
Fix implied outlives bounds logic for projections

The logic here is subtly wrong. I put a bit of an explanation in a767d7b5165cea8ee5cbe494a4a636c50ef67c9c.

TL;DR: we register outlives predicates to be proved, because wf code normalizes projections (from the unnormalized types) to type variables. This causes us to register those as constraints instead of implied. This was "fine", because we later added that implied bound in the normalized type, and delayed registering constraints. When I went to cleanup `free_region_relations` to *not* delay adding constraints, this bug was uncovered.

cc. `@aliemjay` because this caused your test failure in #99832 (I only realized as I was writing this)

r? `@nikomatsakis`
  • Loading branch information
@bors
bors committed Feb 10, 2023
2 parents a12d31d + 0637b6b commit a697573
Show file tree
Hide file tree
Showing 7 changed files with 144 additions and 106 deletions.
1 change: 1 addition & 0 deletions compiler/rustc_borrowck/src/region_infer/values.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@ pub(crate) struct PlaceholderIndices {
}

impl PlaceholderIndices {
/// Returns the `PlaceholderIndex` for the inserted `PlaceholderRegion`
pub(crate) fn insert(&mut self, placeholder: ty::PlaceholderRegion) -> PlaceholderIndex {
let (index, _) = self.indices.insert_full(placeholder);
index.into()
Expand Down
155 changes: 87 additions & 68 deletions compiler/rustc_borrowck/src/type_check/free_region_relations.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use rustc_infer::infer::InferCtxt;
use rustc_middle::mir::ConstraintCategory;
use rustc_middle::traits::query::OutlivesBound;
use rustc_middle::ty::{self, RegionVid, Ty};
use rustc_span::Span;
use rustc_trait_selection::traits::query::type_op::{self, TypeOp};
use std::rc::Rc;
use type_op::TypeOpOutput;
Expand Down Expand Up @@ -217,8 +218,27 @@ impl<'tcx> UniversalRegionRelationsBuilder<'_, 'tcx> {
self.inverse_outlives.add(fr_b, fr_a);
}

#[instrument(level = "debug", skip(self))]
pub(crate) fn create(mut self) -> CreateResult<'tcx> {
let span = self.infcx.tcx.def_span(self.universal_regions.defining_ty.def_id());

// Insert the facts we know from the predicates. Why? Why not.
let param_env = self.param_env;
self.add_outlives_bounds(outlives::explicit_outlives_bounds(param_env));

// - outlives is reflexive, so `'r: 'r` for every region `'r`
// - `'static: 'r` for every region `'r`
// - `'r: 'fn_body` for every (other) universally quantified
// region `'r`, all of which are provided by our caller
let fr_static = self.universal_regions.fr_static;
let fr_fn_body = self.universal_regions.fr_fn_body;
for fr in self.universal_regions.universal_regions() {
debug!("build: relating free region {:?} to itself and to 'static", fr);
self.relate_universal_regions(fr, fr);
self.relate_universal_regions(fr_static, fr);
self.relate_universal_regions(fr, fr_fn_body);
}

let unnormalized_input_output_tys = self
.universal_regions
.unnormalized_input_tys
Expand All @@ -236,78 +256,58 @@ impl<'tcx> UniversalRegionRelationsBuilder<'_, 'tcx> {
// the `relations` is built.
let mut normalized_inputs_and_output =
Vec::with_capacity(self.universal_regions.unnormalized_input_tys.len() + 1);
let constraint_sets: Vec<_> = unnormalized_input_output_tys
.flat_map(|ty| {
debug!("build: input_or_output={:?}", ty);
// We add implied bounds from both the unnormalized and normalized ty.
// See issue #87748
let constraints_implied1 = self.add_implied_bounds(ty);
let TypeOpOutput { output: norm_ty, constraints: constraints1, .. } = self
.param_env
.and(type_op::normalize::Normalize::new(ty))
.fully_perform(self.infcx)
.unwrap_or_else(|_| {
let reported = self
.infcx
.tcx
.sess
.delay_span_bug(span, &format!("failed to normalize {:?}", ty));
TypeOpOutput {
output: self.infcx.tcx.ty_error_with_guaranteed(reported),
constraints: None,
error_info: None,
}
});
// Note: we need this in examples like
// ```
// trait Foo {
// type Bar;
// fn foo(&self) -> &Self::Bar;
// }
// impl Foo for () {
// type Bar = ();
// fn foo(&self) -> &() {}
// }
// ```
// Both &Self::Bar and &() are WF
let constraints_implied2 =
if ty != norm_ty { self.add_implied_bounds(norm_ty) } else { None };
normalized_inputs_and_output.push(norm_ty);
constraints1.into_iter().chain(constraints_implied1).chain(constraints_implied2)
})
.collect();
let mut constraints = vec![];
for ty in unnormalized_input_output_tys {
debug!("build: input_or_output={:?}", ty);
// We add implied bounds from both the unnormalized and normalized ty.
// See issue #87748
let constraints_unnorm = self.add_implied_bounds(ty);
if let Some(c) = constraints_unnorm {
constraints.push(c)
}
let TypeOpOutput { output: norm_ty, constraints: constraints_normalize, .. } = self
.param_env
.and(type_op::normalize::Normalize::new(ty))
.fully_perform(self.infcx)
.unwrap_or_else(|_| {
self.infcx
.tcx
.sess
.delay_span_bug(span, &format!("failed to normalize {:?}", ty));
TypeOpOutput {
output: self.infcx.tcx.ty_error(),
constraints: None,
error_info: None,
}
});
if let Some(c) = constraints_normalize {
constraints.push(c)
}

// Insert the facts we know from the predicates. Why? Why not.
let param_env = self.param_env;
self.add_outlives_bounds(outlives::explicit_outlives_bounds(param_env));
// Note: we need this in examples like
// ```
// trait Foo {
// type Bar;
// fn foo(&self) -> &Self::Bar;
// }
// impl Foo for () {
// type Bar = ();
// fn foo(&self) ->&() {}
// }
// ```
// Both &Self::Bar and &() are WF
if ty != norm_ty {
let constraints_norm = self.add_implied_bounds(norm_ty);
if let Some(c) = constraints_norm {
constraints.push(c)
}
}

// Finally:
// - outlives is reflexive, so `'r: 'r` for every region `'r`
// - `'static: 'r` for every region `'r`
// - `'r: 'fn_body` for every (other) universally quantified
// region `'r`, all of which are provided by our caller
let fr_static = self.universal_regions.fr_static;
let fr_fn_body = self.universal_regions.fr_fn_body;
for fr in self.universal_regions.universal_regions() {
debug!("build: relating free region {:?} to itself and to 'static", fr);
self.relate_universal_regions(fr, fr);
self.relate_universal_regions(fr_static, fr);
self.relate_universal_regions(fr, fr_fn_body);
normalized_inputs_and_output.push(norm_ty);
}

for data in &constraint_sets {
constraint_conversion::ConstraintConversion::new(
self.infcx,
&self.universal_regions,
&self.region_bound_pairs,
self.implicit_region_bound,
self.param_env,
Locations::All(span),
span,
ConstraintCategory::Internal,
&mut self.constraints,
)
.convert_all(data);
for c in constraints {
self.push_region_constraints(c, span);
}

CreateResult {
Expand All @@ -321,6 +321,24 @@ impl<'tcx> UniversalRegionRelationsBuilder<'_, 'tcx> {
}
}

#[instrument(skip(self, data), level = "debug")]
fn push_region_constraints(&mut self, data: &QueryRegionConstraints<'tcx>, span: Span) {
debug!("constraints generated: {:#?}", data);

constraint_conversion::ConstraintConversion::new(
self.infcx,
&self.universal_regions,
&self.region_bound_pairs,
self.implicit_region_bound,
self.param_env,
Locations::All(span),
span,
ConstraintCategory::Internal,
&mut self.constraints,
)
.convert_all(data);
}

/// Update the type of a single local, which should represent
/// either the return type of the MIR or one of its arguments. At
/// the same time, compute and add any implied bounds that come
Expand All @@ -332,6 +350,7 @@ impl<'tcx> UniversalRegionRelationsBuilder<'_, 'tcx> {
.and(type_op::implied_outlives_bounds::ImpliedOutlivesBounds { ty })
.fully_perform(self.infcx)
.unwrap_or_else(|_| bug!("failed to compute implied bounds {:?}", ty));
debug!(?bounds, ?constraints);
self.add_outlives_bounds(bounds);
constraints
}
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_borrowck/src/type_check/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -910,6 +910,8 @@ pub(crate) struct MirTypeckRegionConstraints<'tcx> {
}

impl<'tcx> MirTypeckRegionConstraints<'tcx> {
/// Creates a `Region` for a given `PlaceholderRegion`, or returns the
/// region that corresponds to a previously created one.
fn placeholder_region(
&mut self,
infcx: &InferCtxt<'tcx>,
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_infer/src/infer/outlives/verify.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,7 @@ impl<'cx, 'tcx> VerifyBoundCx<'cx, 'tcx> {
///
/// In some cases, such as when `erased_ty` represents a `ty::Param`, however,
/// the result is precise.
#[instrument(level = "debug", skip(self))]
fn declared_generic_bounds_from_env_for_erased_ty(
&self,
erased_ty: Ty<'tcx>,
Expand Down
86 changes: 50 additions & 36 deletions compiler/rustc_traits/src/implied_outlives_bounds.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,47 +70,61 @@ fn compute_implied_outlives_bounds<'tcx>(
let obligations = wf::obligations(ocx.infcx, param_env, CRATE_DEF_ID, 0, arg, DUMMY_SP)
.unwrap_or_default();

// While these predicates should all be implied by other parts of
// the program, they are still relevant as they may constrain
// inference variables, which is necessary to add the correct
// implied bounds in some cases, mostly when dealing with projections.
ocx.register_obligations(
obligations.iter().filter(|o| o.predicate.has_non_region_infer()).cloned(),
);

// From the full set of obligations, just filter down to the
// region relationships.
outlives_bounds.extend(obligations.into_iter().filter_map(|obligation| {
for obligation in obligations {
debug!(?obligation);
assert!(!obligation.has_escaping_bound_vars());
match obligation.predicate.kind().no_bound_vars() {
None => None,
Some(pred) => match pred {
ty::PredicateKind::Clause(ty::Clause::Trait(..))
| ty::PredicateKind::Subtype(..)
| ty::PredicateKind::Coerce(..)
| ty::PredicateKind::Clause(ty::Clause::Projection(..))
| ty::PredicateKind::ClosureKind(..)
| ty::PredicateKind::ObjectSafe(..)
| ty::PredicateKind::ConstEvaluatable(..)
| ty::PredicateKind::ConstEquate(..)
| ty::PredicateKind::Ambiguous
| ty::PredicateKind::TypeWellFormedFromEnv(..) => None,
ty::PredicateKind::WellFormed(arg) => {
wf_args.push(arg);
None

// While these predicates should all be implied by other parts of
// the program, they are still relevant as they may constrain
// inference variables, which is necessary to add the correct
// implied bounds in some cases, mostly when dealing with projections.
//
// Another important point here: we only register `Projection`
// predicates, since otherwise we might register outlives
// predicates containing inference variables, and we don't
// learn anything new from those.
if obligation.predicate.has_non_region_infer() {
match obligation.predicate.kind().skip_binder() {
ty::PredicateKind::Clause(ty::Clause::Projection(..)) => {
ocx.register_obligation(obligation.clone());
}
_ => {}
}
}

ty::PredicateKind::Clause(ty::Clause::RegionOutlives(
ty::OutlivesPredicate(r_a, r_b),
)) => Some(ty::OutlivesPredicate(r_a.into(), r_b)),
let pred = match obligation.predicate.kind().no_bound_vars() {
None => continue,
Some(pred) => pred,
};
match pred {
ty::PredicateKind::Clause(ty::Clause::Trait(..))
| ty::PredicateKind::Subtype(..)
| ty::PredicateKind::Coerce(..)
| ty::PredicateKind::Clause(ty::Clause::Projection(..))
| ty::PredicateKind::ClosureKind(..)
| ty::PredicateKind::ObjectSafe(..)
| ty::PredicateKind::ConstEvaluatable(..)
| ty::PredicateKind::ConstEquate(..)
| ty::PredicateKind::Ambiguous
| ty::PredicateKind::TypeWellFormedFromEnv(..) => {}

// We need to search through *all* WellFormed predicates
ty::PredicateKind::WellFormed(arg) => {
wf_args.push(arg);
}

// We need to register region relationships
ty::PredicateKind::Clause(ty::Clause::RegionOutlives(ty::OutlivesPredicate(
r_a,
r_b,
))) => outlives_bounds.push(ty::OutlivesPredicate(r_a.into(), r_b)),

ty::PredicateKind::Clause(ty::Clause::TypeOutlives(ty::OutlivesPredicate(
ty_a,
r_b,
))) => Some(ty::OutlivesPredicate(ty_a.into(), r_b)),
},
ty::PredicateKind::Clause(ty::Clause::TypeOutlives(ty::OutlivesPredicate(
ty_a,
r_b,
))) => outlives_bounds.push(ty::OutlivesPredicate(ty_a.into(), r_b)),
}
}));
}
}

// This call to `select_all_or_error` is necessary to constrain inference variables, which we
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_traits/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#![deny(rustc::untranslatable_diagnostic)]
#![deny(rustc::diagnostic_outside_of_impl)]
#![feature(let_chains)]
#![feature(drain_filter)]
#![recursion_limit = "256"]

#[macro_use]
Expand Down
4 changes: 2 additions & 2 deletions tests/ui/nll/issue-52057.rs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// Regression test for #52057. There is an implied bound
// that `I: 'a` where `'a` is the lifetime of `self` in `parse_first`;
// but to observe that, one must normalize first.
// that `I: 'x` where `'x` is the lifetime of the reference `&mut Self::Input`
// in `parse_first`; but to observe that, one must normalize first.
//
// run-pass

Expand Down

0 comments on commit a697573

Please sign in to comment.