Skip to content

Commit

Permalink
Auto merge of #124336 - compiler-errors:super-outlives, r=lcnr
Browse files Browse the repository at this point in the history
Enforce supertrait outlives obligations hold when confirming impl

**TL;DR:** We elaborate super-predicates and apply any outlives obligations when proving an impl holds to fix a mismatch between implied bounds.

Bugs in implied bounds (and implied well-formedness) occur whenever there is a mismatch between the assumptions that some code can assume to hold, and the obligations that a caller/user of that code must prove. If the former is stronger than the latter, then unsoundness occurs.

Take a look at the example unsoundness:

```rust
use std::fmt::Display;

trait Static: 'static {}
impl<T> Static for &'static T {}
fn foo<S: Display>(x: S) -> Box<dyn Display>
where
    &'static S: Static,
{
    Box::new(x)
}

fn main() {
    let s = foo(&String::from("blah blah blah"));
    println!("{}", s);
}
```

This specific example occurs because we elaborate obligations in `fn foo`:

* `&'static S: Static`
    * `&'static S: 'static` <- super predicate
        * `S: 'static` <- elaborating outlives bounds

However, when calling `foo`, we only need to prove the direct set of where clauses. So at the call site for some substitution `S = &'not_static str`, that means only proving `&'static &'not_static str: Static`. To prove this, we apply the impl, which itself holds trivially since it has no where clauses.

This is the mismatch -- `foo` is allowed to assume that `S: 'static` via elaborating supertraits, but callers of `foo` never need to prove that `S: 'static`.

There are several approaches to fixing this, all of which have problems due to current limitations in our type system:
1. proving the elaborated set of predicates always - This leads to issues since we don't have coinductive trait semantics, so we easily hit new cycles.
    * This would fix our issue, since callers of `foo` would have to both prove `&'static &'not_static str: Static` and its elaborated bounds, which would surface the problematic `'not_static: 'static` outlives obligation.
    * However, proving supertraits when proving impls leads to inductive cycles which can't be fixed until we get coinductive trait semantics.
2. Proving that an impl header is WF when applying that impl:
    * This would fix our issue, since when we try to prove `&'static &'not_static str: Static`, we'd need to prove `WF(&'static &'not_static str)`, which would surface the problematic `'not_static: 'static` outlives obligation.
    * However, this leads to issues since we don't have higher-ranked implied bounds. This breaks things when trying to apply impls to higher-ranked trait goals.

To get around these limitations, we apply a subset of (1.), which is to elaborate the supertrait obligations of the impl but filter only the (region/type) outlives out of that set, since those can never participate in an inductive cycle. This is likely not sufficient to fix a pathological example of this issue, but it does clearly fill in a major gap that we're currently overlooking.

This can also result in 'unintended' errors due to missing implied-bounds on binders. We did not encounter this in the crater run and don't expect people to rely on this code in practice:
```rust
trait Outlives<'b>: 'b {}
impl<'b, T> Outlives<'b> for &'b T {}
fn foo<'b>()
where
    // This bound will break due to this PR as we end up proving
    // `&'b &'!a (): 'b` without the implied `'!a: 'b`
    // bound.
    for<'a> &'b &'a (): Outlives<'b>,
{}
```

Fixes #98117

---

Crater: #124336 (comment)
Triaged: #124336 (comment)

All of the fallout is due to generic const exprs, and can be ignored.
  • Loading branch information
bors committed Aug 5, 2024
2 parents 83e9b93 + fa9ae7b commit 2b78d92
Show file tree
Hide file tree
Showing 11 changed files with 132 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use rustc_type_ir::data_structures::HashMap;
use rustc_type_ir::fold::{TypeFoldable, TypeFolder, TypeSuperFoldable};
use rustc_type_ir::inherent::*;
use rustc_type_ir::lang_items::TraitSolverLangItem;
use rustc_type_ir::{self as ty, Interner, Upcast as _};
use rustc_type_ir::{self as ty, elaborate, Interner, Upcast as _};
use rustc_type_ir_macros::{TypeFoldable_Generic, TypeVisitable_Generic};
use tracing::instrument;

Expand Down Expand Up @@ -671,11 +671,19 @@ where
{
let cx = ecx.cx();
let mut requirements = vec![];
requirements.extend(
// Elaborating all supertrait outlives obligations here is not soundness critical,
// since if we just used the unelaborated set, then the transitive supertraits would
// be reachable when proving the former. However, since we elaborate all supertrait
// outlives obligations when confirming impls, we would end up with a different set
// of outlives obligations here if we didn't do the same, leading to ambiguity.
// FIXME(-Znext-solver=coinductive): Adding supertraits here can be removed once we
// make impls coinductive always, since they'll always need to prove their supertraits.
requirements.extend(elaborate::elaborate(
cx,
cx.explicit_super_predicates_of(trait_ref.def_id)
.iter_instantiated(cx, trait_ref.args)
.map(|(pred, _)| pred),
);
));

// FIXME(associated_const_equality): Also add associated consts to
// the requirements here.
Expand Down
13 changes: 13 additions & 0 deletions compiler/rustc_next_trait_solver/src/solve/trait_goals.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,19 @@ where
.map(|pred| goal.with(cx, pred));
ecx.add_goals(GoalSource::ImplWhereBound, where_clause_bounds);

// We currently elaborate all supertrait outlives obligations from impls.
// This can be removed when we actually do coinduction correctly, and prove
// all supertrait obligations unconditionally.
let goal_clause: I::Clause = goal.predicate.upcast(cx);
for clause in elaborate::elaborate(cx, [goal_clause]) {
if matches!(
clause.kind().skip_binder(),
ty::ClauseKind::TypeOutlives(..) | ty::ClauseKind::RegionOutlives(..)
) {
ecx.add_goal(GoalSource::Misc, goal.with(cx, clause));
}
}

ecx.evaluate_added_goals_and_make_canonical_response(maximal_certainty)
})
}
Expand Down
35 changes: 33 additions & 2 deletions compiler/rustc_trait_selection/src/traits/select/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,17 @@ use std::fmt::{self, Display};
use std::ops::ControlFlow;
use std::{cmp, iter};

use hir::def::DefKind;
use rustc_data_structures::fx::{FxHashSet, FxIndexMap, FxIndexSet};
use rustc_data_structures::stack::ensure_sufficient_stack;
use rustc_errors::{Diag, EmissionGuarantee};
use rustc_hir as hir;
use rustc_hir::def_id::DefId;
use rustc_hir::LangItem;
use rustc_infer::infer::relate::TypeRelation;
use rustc_infer::infer::BoundRegionConversionTime::HigherRankedType;
use rustc_infer::infer::{BoundRegionConversionTime, DefineOpaqueTypes};
use rustc_infer::infer::BoundRegionConversionTime::{self, HigherRankedType};
use rustc_infer::infer::DefineOpaqueTypes;
use rustc_infer::traits::util::elaborate;
use rustc_infer::traits::TraitObligation;
use rustc_middle::bug;
use rustc_middle::dep_graph::{dep_kinds, DepNodeIndex};
Expand Down Expand Up @@ -2798,6 +2800,35 @@ impl<'tcx> SelectionContext<'_, 'tcx> {
});
}

// Register any outlives obligations from the trait here, cc #124336.
if matches!(self.tcx().def_kind(def_id), DefKind::Impl { of_trait: true })
&& let Some(header) = self.tcx().impl_trait_header(def_id)
{
let trait_clause: ty::Clause<'tcx> =
header.trait_ref.instantiate(self.tcx(), args).upcast(self.tcx());
for clause in elaborate(self.tcx(), [trait_clause]) {
if matches!(
clause.kind().skip_binder(),
ty::ClauseKind::TypeOutlives(..) | ty::ClauseKind::RegionOutlives(..)
) {
let clause = normalize_with_depth_to(
self,
param_env,
cause.clone(),
recursion_depth,
clause,
&mut obligations,
);
obligations.push(Obligation {
cause: cause.clone(),
recursion_depth,
param_env,
predicate: clause.as_predicate(),
});
}
}
}

obligations
}
}
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_type_ir/src/inherent.rs
Original file line number Diff line number Diff line change
Expand Up @@ -451,6 +451,8 @@ pub trait Clause<I: Interner<Clause = Self>>:
+ UpcastFrom<I, ty::Binder<I, ty::ClauseKind<I>>>
+ UpcastFrom<I, ty::TraitRef<I>>
+ UpcastFrom<I, ty::Binder<I, ty::TraitRef<I>>>
+ UpcastFrom<I, ty::TraitPredicate<I>>
+ UpcastFrom<I, ty::Binder<I, ty::TraitPredicate<I>>>
+ UpcastFrom<I, ty::ProjectionPredicate<I>>
+ UpcastFrom<I, ty::Binder<I, ty::ProjectionPredicate<I>>>
+ IntoKind<Kind = ty::Binder<I, ty::ClauseKind<I>>>
Expand Down
1 change: 1 addition & 0 deletions tests/ui/fn/implied-bounds-unnorm-associated-type-5.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ trait Trait<'a>: 'a {
// if the `T: 'a` bound gets implied we would probably get ub here again
impl<'a, T> Trait<'a> for T {
//~^ ERROR the parameter type `T` may not live long enough
//~| ERROR the parameter type `T` may not live long enough
type Type = ();
}

Expand Down
17 changes: 15 additions & 2 deletions tests/ui/fn/implied-bounds-unnorm-associated-type-5.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,21 @@ help: consider adding an explicit lifetime bound
LL | impl<'a, T: 'a> Trait<'a> for T {
| ++++

error[E0309]: the parameter type `T` may not live long enough
--> $DIR/implied-bounds-unnorm-associated-type-5.rs:6:27
|
LL | impl<'a, T> Trait<'a> for T {
| -- ^ ...so that the type `T` will meet its required lifetime bounds
| |
| the parameter type `T` must be valid for the lifetime `'a` as defined here...
|
help: consider adding an explicit lifetime bound
|
LL | impl<'a, T: 'a> Trait<'a> for T {
| ++++

error[E0505]: cannot move out of `x` because it is borrowed
--> $DIR/implied-bounds-unnorm-associated-type-5.rs:21:10
--> $DIR/implied-bounds-unnorm-associated-type-5.rs:22:10
|
LL | let x = String::from("Hello World!");
| - binding `x` declared here
Expand All @@ -33,7 +46,7 @@ help: consider cloning the value if the performance cost is acceptable
LL | let y = f(&x.clone(), ());
| ++++++++

error: aborting due to 2 previous errors
error: aborting due to 3 previous errors

Some errors have detailed explanations: E0309, E0505.
For more information about an error, try `rustc --explain E0309`.
1 change: 1 addition & 0 deletions tests/ui/static/static-lifetime.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
pub trait Arbitrary: Sized + 'static {}

impl<'a, A: Clone> Arbitrary for ::std::borrow::Cow<'a, A> {} //~ ERROR lifetime bound
//~^ ERROR cannot infer an appropriate lifetime for lifetime parameter `'a`

fn main() {
}
30 changes: 28 additions & 2 deletions tests/ui/static/static-lifetime.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,32 @@ LL | impl<'a, A: Clone> Arbitrary for ::std::borrow::Cow<'a, A> {}
| ^^
= note: but lifetime parameter must outlive the static lifetime

error: aborting due to 1 previous error
error[E0495]: cannot infer an appropriate lifetime for lifetime parameter `'a` due to conflicting requirements
--> $DIR/static-lifetime.rs:3:34
|
LL | impl<'a, A: Clone> Arbitrary for ::std::borrow::Cow<'a, A> {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
note: first, the lifetime cannot outlive the lifetime `'a` as defined here...
--> $DIR/static-lifetime.rs:3:6
|
LL | impl<'a, A: Clone> Arbitrary for ::std::borrow::Cow<'a, A> {}
| ^^
note: ...so that the types are compatible
--> $DIR/static-lifetime.rs:3:34
|
LL | impl<'a, A: Clone> Arbitrary for ::std::borrow::Cow<'a, A> {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^
= note: expected `<Cow<'a, A> as Arbitrary>`
found `<Cow<'_, A> as Arbitrary>`
= note: but, the lifetime must be valid for the static lifetime...
note: ...so that the declared lifetime parameter bounds are satisfied
--> $DIR/static-lifetime.rs:3:34
|
LL | impl<'a, A: Clone> Arbitrary for ::std::borrow::Cow<'a, A> {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0478`.
Some errors have detailed explanations: E0478, E0495.
For more information about an error, try `rustc --explain E0478`.
12 changes: 12 additions & 0 deletions tests/ui/wf/wf-in-where-clause-static.current.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
error[E0716]: temporary value dropped while borrowed
--> $DIR/wf-in-where-clause-static.rs:18:18
|
LL | let s = foo(&String::from("blah blah blah"));
| -----^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-- temporary value is freed at the end of this statement
| | |
| | creates a temporary value which is freed while still in use
| argument requires that borrow lasts for `'static`

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0716`.
12 changes: 12 additions & 0 deletions tests/ui/wf/wf-in-where-clause-static.next.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
error[E0716]: temporary value dropped while borrowed
--> $DIR/wf-in-where-clause-static.rs:18:18
|
LL | let s = foo(&String::from("blah blah blah"));
| -----^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-- temporary value is freed at the end of this statement
| | |
| | creates a temporary value which is freed while still in use
| argument requires that borrow lasts for `'static`

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0716`.
10 changes: 4 additions & 6 deletions tests/ui/wf/wf-in-where-clause-static.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
//@ check-pass
//@ known-bug: #98117

// Should fail. Functions are responsible for checking the well-formedness of
// their own where clauses, so this should fail and require an explicit bound
// `T: 'static`.
//@ revisions: current next
//@ ignore-compare-mode-next-solver (explicit revisions)
//@[next] compile-flags: -Znext-solver

use std::fmt::Display;

Expand All @@ -19,5 +16,6 @@ where

fn main() {
let s = foo(&String::from("blah blah blah"));
//~^ ERROR temporary value dropped while borrowed
println!("{}", s);
}

0 comments on commit 2b78d92

Please sign in to comment.