Skip to content

Commit

Permalink
Auto merge of rust-lang#125413 - lcnr:ambig-drop-region-constraints, …
Browse files Browse the repository at this point in the history
…r=compiler-errors

drop region constraints for ambiguous goals

See the comment in `compute_external_query_constraints`. While the underlying issue is preexisting, this fixes a bug introduced by rust-lang#125343.

It slightly weakens the leak chec, even if we didn't have any test which was affected. I want to write such a test before merging this PR.

r? `@compiler-errors`
  • Loading branch information
bors committed May 27, 2024
2 parents f6e4703 + 24b5466 commit b0f8618
Show file tree
Hide file tree
Showing 6 changed files with 172 additions and 25 deletions.
64 changes: 39 additions & 25 deletions compiler/rustc_trait_selection/src/solve/eval_ctxt/canonical.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,13 @@ impl<'tcx> EvalCtxt<'_, InferCtxt<'tcx>> {
previous call to `try_evaluate_added_goals!`"
);

// We only check for leaks from universes which were entered inside
// of the query.
self.infcx.leak_check(self.max_input_universe, None).map_err(|e| {
trace!(?e, "failed the leak check");
NoSolution
})?;

// When normalizing, we've replaced the expected term with an unconstrained
// inference variable. This means that we dropped information which could
// have been important. We handle this by instead returning the nested goals
Expand All @@ -121,7 +128,7 @@ impl<'tcx> EvalCtxt<'_, InferCtxt<'tcx>> {
};

let external_constraints =
self.compute_external_query_constraints(normalization_nested_goals)?;
self.compute_external_query_constraints(certainty, normalization_nested_goals);
let (var_values, mut external_constraints) =
(self.var_values, external_constraints).fold_with(&mut EagerResolver::new(self.infcx));
// Remove any trivial region constraints once we've resolved regions
Expand Down Expand Up @@ -170,38 +177,45 @@ impl<'tcx> EvalCtxt<'_, InferCtxt<'tcx>> {
#[instrument(level = "trace", skip(self), ret)]
fn compute_external_query_constraints(
&self,
certainty: Certainty,
normalization_nested_goals: NestedNormalizationGoals<'tcx>,
) -> Result<ExternalConstraintsData<'tcx>, NoSolution> {
// We only check for leaks from universes which were entered inside
// of the query.
self.infcx.leak_check(self.max_input_universe, None).map_err(|e| {
trace!(?e, "failed the leak check");
NoSolution
})?;

// Cannot use `take_registered_region_obligations` as we may compute the response
// inside of a `probe` whenever we have multiple choices inside of the solver.
let region_obligations = self.infcx.inner.borrow().region_obligations().to_owned();
let mut region_constraints = self.infcx.with_region_constraints(|region_constraints| {
make_query_region_constraints(
self.tcx(),
region_obligations
.iter()
.map(|r_o| (r_o.sup_type, r_o.sub_region, r_o.origin.to_constraint_category())),
region_constraints,
)
});

let mut seen = FxHashSet::default();
region_constraints.outlives.retain(|outlives| seen.insert(*outlives));
) -> ExternalConstraintsData<'tcx> {
// We only return region constraints once the certainty is `Yes`. This
// is necessary as we may drop nested goals on ambiguity, which may result
// in unconstrained inference variables in the region constraints. It also
// prevents us from emitting duplicate region constraints, avoiding some
// unnecessary work. This slightly weakens the leak check in case it uses
// region constraints from an ambiguous nested goal. This is tested in both
// `tests/ui/higher-ranked/leak-check/leak-check-in-selection-5-ambig.rs` and
// `tests/ui/higher-ranked/leak-check/leak-check-in-selection-6-ambig-unify.rs`.
let region_constraints = if certainty == Certainty::Yes {
// Cannot use `take_registered_region_obligations` as we may compute the response
// inside of a `probe` whenever we have multiple choices inside of the solver.
let region_obligations = self.infcx.inner.borrow().region_obligations().to_owned();
let mut region_constraints = self.infcx.with_region_constraints(|region_constraints| {
make_query_region_constraints(
self.tcx(),
region_obligations.iter().map(|r_o| {
(r_o.sup_type, r_o.sub_region, r_o.origin.to_constraint_category())
}),
region_constraints,
)
});

let mut seen = FxHashSet::default();
region_constraints.outlives.retain(|outlives| seen.insert(*outlives));
region_constraints
} else {
Default::default()
};

let mut opaque_types = self.infcx.clone_opaque_types_for_query_response();
// Only return opaque type keys for newly-defined opaques
opaque_types.retain(|(a, _)| {
self.predefined_opaques_in_body.opaque_types.iter().all(|(pa, _)| pa != a)
});

Ok(ExternalConstraintsData { region_constraints, opaque_types, normalization_nested_goals })
ExternalConstraintsData { region_constraints, opaque_types, normalization_nested_goals }
}

/// After calling a canonical query, we apply the constraints returned
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
//@ revisions: old next
//@[next] compile-flags: -Znext-solver
//@ check-pass

// The new trait solver does not return region constraints if the goal
// is still ambiguous. This causes the following test to fail with ambiguity,
// even though `(): LeakCheckFailure<'!a, V>` would return `'!a: 'static`
// which would have caused a leak check failure.

trait Ambig {}
impl Ambig for u32 {}
impl Ambig for u16 {}

trait Id<T> {}
impl Id<u32> for u32 {}
impl Id<u16> for u16 {}


trait LeakCheckFailure<'a, V: ?Sized> {}
impl<V: ?Sized + Ambig> LeakCheckFailure<'static, V> for () {}

trait Trait<U, V> {}
impl<V> Trait<u32, V> for () where for<'a> (): LeakCheckFailure<'a, V> {}
impl<V> Trait<u16, V> for () {}
fn impls_trait<T: Trait<U, V>, U: Id<V>, V>() {}
fn main() {
impls_trait::<(), _, _>()
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
error[E0283]: type annotations needed
--> $DIR/leak-check-in-selection-6-ambig-unify.rs:30:5
|
LL | impls_trait::<(), _, _>()
| ^^^^^^^^^^^^^^^^^^^^^^^ cannot infer type of the type parameter `U` declared on the function `impls_trait`
|
note: multiple `impl`s satisfying `(): Trait<_, _>` found
--> $DIR/leak-check-in-selection-6-ambig-unify.rs:26:1
|
LL | impl<V> Trait<u32, V> for () where for<'b> (): LeakCheckFailure<'static, 'b, V> {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
LL | impl<V> Trait<u16, V> for () {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: required by a bound in `impls_trait`
--> $DIR/leak-check-in-selection-6-ambig-unify.rs:28:19
|
LL | fn impls_trait<T: Trait<U, V>, U: Id<V>, V>() {}
| ^^^^^^^^^^^ required by this bound in `impls_trait`

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0283`.
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
error[E0283]: type annotations needed
--> $DIR/leak-check-in-selection-6-ambig-unify.rs:30:5
|
LL | impls_trait::<(), _, _>()
| ^^^^^^^^^^^^^^^^^^^^^^^ cannot infer type of the type parameter `U` declared on the function `impls_trait`
|
note: multiple `impl`s satisfying `(): Trait<_, _>` found
--> $DIR/leak-check-in-selection-6-ambig-unify.rs:26:1
|
LL | impl<V> Trait<u32, V> for () where for<'b> (): LeakCheckFailure<'static, 'b, V> {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
LL | impl<V> Trait<u16, V> for () {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: required by a bound in `impls_trait`
--> $DIR/leak-check-in-selection-6-ambig-unify.rs:28:19
|
LL | fn impls_trait<T: Trait<U, V>, U: Id<V>, V>() {}
| ^^^^^^^^^^^ required by this bound in `impls_trait`

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0283`.
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
//@ revisions: old next
//@[next] compile-flags: -Znext-solver

// The new trait solver does not return region constraints if the goal
// is still ambiguous. This should cause the following test to fail with
// ambiguity as even if `(): LeakCheckFailure<'static, '!b, V>` unifies
// `'!b` with `'static`, we erase all region constraints.
//
// However, we do still unify the var_value for `'b` with `'static`,
// causing us to return this requirement via the `var_values` even if
// we don't return any region constraints. This is a bit inconsistent
// but isn't something we should really worry about imo.
trait Ambig {}
impl Ambig for u32 {}
impl Ambig for u16 {}

trait Id<T> {}
impl Id<u32> for u32 {}
impl Id<u16> for u16 {}


trait LeakCheckFailure<'a, 'b, V: ?Sized> {}
impl<'a, 'b: 'a, V: ?Sized + Ambig> LeakCheckFailure<'a, 'b, V> for () {}

trait Trait<U, V> {}
impl<V> Trait<u32, V> for () where for<'b> (): LeakCheckFailure<'static, 'b, V> {}
impl<V> Trait<u16, V> for () {}
fn impls_trait<T: Trait<U, V>, U: Id<V>, V>() {}
fn main() {
impls_trait::<(), _, _>()
//~^ ERROR type annotations needed
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
//@ check-pass
//@ compile-flags: -Znext-solver
//@ ignore-compare-mode-next-solver (explicitly enabled)

// Regression test for an ICE when trying to bootstrap rustc
// with #125343. An ambiguous goal returned a `TypeOutlives`
// constraint referencing an inference variable. This inference
// variable was created inside of the goal, causing it to be
// unconstrained in the caller. This then caused an ICE in MIR
// borrowck.

struct Foo<T>(T);
trait Extend<T> {
fn extend<I: IntoIterator<Item = T>>(iter: I);
}

impl<T> Extend<T> for Foo<T> {
fn extend<I: IntoIterator<Item = T>>(_: I) {
todo!()
}
}

impl<'a, T: 'a + Copy> Extend<&'a T> for Foo<T> {
fn extend<I: IntoIterator<Item = &'a T>>(iter: I) {
<Self as Extend<T>>::extend(iter.into_iter().copied())
}
}

fn main() {}

0 comments on commit b0f8618

Please sign in to comment.