mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Auto merge of #122791 - compiler-errors:make-coinductive-always, r=lcnr

Browse Source 
Make inductive cycles always ambiguous

 This makes inductive cycles always result in ambiguity rather than be treated like a stack-dependent error.

This has some  interactions with specialization, and so breaks a few UI tests that I don't agree should've ever worked in the first place, and also breaks a handful of crates in a way that I don't believe is a problem.

On the bright side, it puts us in a better spot when it comes to eventually enabling coinduction everywhere.

## Results

This was cratered in https://github.com/rust-lang/rust/pull/116494#issuecomment-2008657494, which boils down to two regressions:
* `lu_packets` - This code should have never compiled in the first place. More below.
* **ALL** other regressions are due to `commit_verify@0.11.0-beta.1` (edit: and `commit_verify@0.10.x`) - This actually seems to be fixed in version `0.11.0-beta.5`, which is the *most* up to date version, but it's still prerelease on crates.io so I don't think cargo ends up picking `beta.5` when building dependent crates.

### `lu_packets`

Firstly, this crate uses specialization, so I think it's automatically worth breaking. However, I've minimized [the regression](https://crater-reports.s3.amazonaws.com/pr-116494-3/try%23d614ed876e31a5f3ad1d0fbf848fcdab3a29d1d8/gh/lcdr.lu_packets/log.txt) to:

```rust
// Upstream crate
pub trait Serialize {}
impl Serialize for &() {}
impl<S> Serialize for &[S] where for<'a> &'a S: Serialize {}

// ----------------------------------------------------------------------- //

// Downstream crate
#![feature(specialization)]
#![allow(incomplete_features, unused)]

use upstream::Serialize;

trait Replica {
    fn serialize();
}

impl<T> Replica for T {
    default fn serialize() {}
}

impl<T> Replica for Option<T>
where
    for<'a> &'a T: Serialize,
{
    fn serialize() {}
}
```

Specifically this fails when computing the specialization graph for the `downstream` crate.

The code ends up cycling on `&[?0]: Serialize` when we equate `&?0 = &[?1]` during impl matching, which ends up needing to prove `&[?1]: Serialize`, which since cycles are treated like ambiguity, ends up in a **fatal overflow**. For some reason this requires two crates, squashing them into one crate doesn't work.

Side-note: This code is subtly order dependent. When minimizing, I ended up having the code start failing on `nightly` very easily after removing and reordering impls. This seems to me all the more reason to remove this behavior altogether.

## Side-note: Item Bounds (edit: this was fixed independently in #121123)

Due to the changes in #120584 where we now consider an alias's item bounds *and* all the item bounds of the alias's nested self type aliases, I've had to add e6b64c61941120f734657106ae2479d05b463197 which is a hack to make sure we're not eagerly normalizing bounds that have nothing to do with the predicate we're trying to solve, and which result in.

This is fixed in a more principled way in #121123.

---

r? lcnr for an initial review
This commit is contained in:
bors 2024-04-03 00:09:44 +00:00
commit 40f743da23
12 changed files with 104 additions and 117 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

52
compiler/rustc_middle/src/traits/select.rs
View File

@ -193,7 +193,6 @@ pub enum SelectionCandidate<'tcx> {
/// The evaluation results are ordered:
/// - `EvaluatedToOk` implies `EvaluatedToOkModuloRegions`
/// implies `EvaluatedToAmbig` implies `EvaluatedToAmbigStackDependent`
/// - `EvaluatedToErr` implies `EvaluatedToErrStackDependent`
/// - the "union" of evaluation results is equal to their maximum -
/// all the "potential success" candidates can potentially succeed,
/// so they are noops when unioned with a definite error, and within
@ -219,52 +218,9 @@ pub enum EvaluationResult {
/// variables. We are somewhat imprecise there, so we don't actually
/// know the real result.
///
/// This can't be trivially cached for the same reason as `EvaluatedToErrStackDependent`.
/// This can't be trivially cached because the result depends on the
/// stack results.
EvaluatedToAmbigStackDependent,
/// Evaluation failed because we encountered an obligation we are already
/// trying to prove on this branch.
///
/// We know this branch can't be a part of a minimal proof-tree for
/// the "root" of our cycle, because then we could cut out the recursion
/// and maintain a valid proof tree. However, this does not mean
/// that all the obligations on this branch do not hold -- it's possible
/// that we entered this branch "speculatively", and that there
/// might be some other way to prove this obligation that does not
/// go through this cycle -- so we can't cache this as a failure.
///
/// For example, suppose we have this:
///
/// ```rust,ignore (pseudo-Rust)
/// pub trait Trait { fn xyz(); }
/// // This impl is "useless", but we can still have
/// // an `impl Trait for SomeUnsizedType` somewhere.
/// impl<T: Trait + Sized> Trait for T { fn xyz() {} }
///
/// pub fn foo<T: Trait + ?Sized>() {
/// <T as Trait>::xyz();
/// }
/// ```
///
/// When checking `foo`, we have to prove `T: Trait`. This basically
/// translates into this:
///
/// ```plain,ignore
/// (T: Trait + Sized →_\impl T: Trait), T: Trait ⊢ T: Trait
/// ```
///
/// When we try to prove it, we first go the first option, which
/// recurses. This shows us that the impl is "useless" -- it won't
/// tell us that `T: Trait` unless it already implemented `Trait`
/// by some other means. However, that does not prevent `T: Trait`
/// does not hold, because of the bound (which can indeed be satisfied
/// by `SomeUnsizedType` from another crate).
//
// FIXME: when an `EvaluatedToErrStackDependent` goes past its parent root, we
// ought to convert it to an `EvaluatedToErr`, because we know
// there definitely isn't a proof tree for that obligation. Not
// doing so is still sound -- there isn't any proof tree, so the
// branch still can't be a part of a minimal one -- but does not re-enable caching.
EvaluatedToErrStackDependent,
/// Evaluation failed.
EvaluatedToErr,
}
@ -290,13 +246,13 @@ pub fn may_apply(self) -> bool {
| EvaluatedToAmbig
| EvaluatedToAmbigStackDependent => true,
EvaluatedToErr | EvaluatedToErrStackDependent => false,
EvaluatedToErr => false,
}
}
pub fn is_stack_dependent(self) -> bool {
match self {
EvaluatedToAmbigStackDependent | EvaluatedToErrStackDependent => true,
EvaluatedToAmbigStackDependent => true,
EvaluatedToOkModuloOpaqueTypes
| EvaluatedToOk

3
compiler/rustc_trait_selection/src/infer.rs
View File

@ -49,8 +49,7 @@ fn type_is_sized_modulo_regions(&self, param_env: ty::ParamEnv<'tcx>, ty: Ty<'tc
/// - the parameter environment
///
/// Invokes `evaluate_obligation`, so in the event that evaluating
/// `Ty: Trait` causes overflow, EvaluatedToErrStackDependent
/// (or EvaluatedToAmbigStackDependent) will be returned.
/// `Ty: Trait` causes overflow, EvaluatedToAmbigStackDependent will be returned.
#[instrument(level = "debug", skip(self, params), ret)]
fn type_implements_trait(
&self,

2
compiler/rustc_trait_selection/src/traits/coherence.rs
View File

@ -210,7 +210,7 @@ fn overlap<'tcx>(
.intercrate(true)
.with_next_trait_solver(tcx.next_trait_solver_in_coherence())
.build();
let selcx = &mut SelectionContext::with_treat_inductive_cycle_as_ambig(&infcx);
let selcx = &mut SelectionContext::new(&infcx);
if track_ambiguity_causes.is_yes() {
selcx.enable_tracking_intercrate_ambiguity_causes();
}

42
compiler/rustc_trait_selection/src/traits/select/mod.rs
View File

@ -126,8 +126,6 @@ pub struct SelectionContext<'cx, 'tcx> {
/// policy. In essence, canonicalized queries need their errors propagated
/// rather than immediately reported because we do not have accurate spans.
query_mode: TraitQueryMode,
treat_inductive_cycle: TreatInductiveCycleAs,
}
// A stack that walks back up the stack frame.
@ -208,27 +206,6 @@ enum BuiltinImplConditions<'tcx> {
Ambiguous,
}
#[derive(Copy, Clone)]
pub enum TreatInductiveCycleAs {
/// This is the previous behavior, where `Recur` represents an inductive
/// cycle that is known not to hold. This is not forwards-compatible with
/// coinduction, and will be deprecated. This is the default behavior
/// of the old trait solver due to back-compat reasons.
Recur,
/// This is the behavior of the new trait solver, where inductive cycles
/// are treated as ambiguous and possibly holding.
Ambig,
}
impl From<TreatInductiveCycleAs> for EvaluationResult {
fn from(treat: TreatInductiveCycleAs) -> EvaluationResult {
match treat {
TreatInductiveCycleAs::Ambig => EvaluatedToAmbigStackDependent,
TreatInductiveCycleAs::Recur => EvaluatedToErrStackDependent,
}
}
}
impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
pub fn new(infcx: &'cx InferCtxt<'tcx>) -> SelectionContext<'cx, 'tcx> {
SelectionContext {
@ -236,19 +213,6 @@ pub fn new(infcx: &'cx InferCtxt<'tcx>) -> SelectionContext<'cx, 'tcx> {
freshener: infcx.freshener(),
intercrate_ambiguity_causes: None,
query_mode: TraitQueryMode::Standard,
treat_inductive_cycle: TreatInductiveCycleAs::Recur,
}
}
pub fn with_treat_inductive_cycle_as_ambig(
infcx: &'cx InferCtxt<'tcx>,
) -> SelectionContext<'cx, 'tcx> {
// Should be executed in a context where caching is disabled,
// otherwise the cache is poisoned with the temporary result.
assert!(infcx.intercrate);
SelectionContext {
treat_inductive_cycle: TreatInductiveCycleAs::Ambig,
..SelectionContext::new(infcx)
}
}
@ -756,7 +720,7 @@ fn evaluate_predicate_recursively<'o>(
stack.update_reached_depth(stack_arg.1);
return Ok(EvaluatedToOk);
} else {
return Ok(self.treat_inductive_cycle.into());
return Ok(EvaluatedToAmbigStackDependent);
}
}
return Ok(EvaluatedToOk);
@ -875,7 +839,7 @@ fn evaluate_predicate_recursively<'o>(
}
}
ProjectAndUnifyResult::FailedNormalization => Ok(EvaluatedToAmbig),
ProjectAndUnifyResult::Recursive => Ok(self.treat_inductive_cycle.into()),
ProjectAndUnifyResult::Recursive => Ok(EvaluatedToAmbigStackDependent),
ProjectAndUnifyResult::MismatchedProjectionTypes(_) => Ok(EvaluatedToErr),
}
}
@ -1180,7 +1144,7 @@ fn check_evaluation_cycle(
Some(EvaluatedToOk)
} else {
debug!("evaluate_stack --> recursive, inductive");
Some(self.treat_inductive_cycle.into())
Some(EvaluatedToAmbigStackDependent)
}
} else {
None

1
tests/ui/marker_trait_attr/unsound-overlap.rs
View File

@ -8,6 +8,7 @@ trait B {}
impl<T: A> B for T {}
impl<T: B> A for T {}
impl A for &str {}
//~^ ERROR type annotations needed: cannot satisfy `&str: A`
impl<T: A + B> A for (T,) {}
trait TraitWithAssoc {
type Assoc;

21
tests/ui/marker_trait_attr/unsound-overlap.stderr
View File

@ -1,5 +1,19 @@
error[E0283]: type annotations needed: cannot satisfy `&str: A`
--> $DIR/unsound-overlap.rs:10:12
|
LL | impl A for &str {}
| ^^^^
|
note: multiple `impl`s satisfying `&str: A` found
--> $DIR/unsound-overlap.rs:9:1
|
LL | impl<T: B> A for T {}
| ^^^^^^^^^^^^^^^^^^
LL | impl A for &str {}
| ^^^^^^^^^^^^^^^
error[E0119]: conflicting implementations of trait `TraitWithAssoc` for type `((&str,),)`
--> $DIR/unsound-overlap.rs:20:1
--> $DIR/unsound-overlap.rs:21:1
|
LL | impl<T: A> TraitWithAssoc for T {
| ------------------------------- first implementation here
@ -7,6 +21,7 @@ LL | impl<T: A> TraitWithAssoc for T {
LL | impl TraitWithAssoc for ((&str,),) {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `((&str,),)`
error: aborting due to 1 previous error
error: aborting due to 2 previous errors
For more information about this error, try `rustc --explain E0119`.
Some errors have detailed explanations: E0119, E0283.
For more information about an error, try `rustc --explain E0119`.

3
tests/ui/specialization/issue-39448.rs
View File

@ -22,6 +22,7 @@ trait FromA<T> {
}
impl<T: A, U: A + FromA<T>> FromA<T> for U {
//~^ ERROR cycle detected when computing whether impls specialize one another
default fn from(x: T) -> Self {
ToA::to(x)
}
@ -42,7 +43,7 @@ fn to(self) -> U {
#[allow(dead_code)]
fn foo<T: A, U: A>(x: T, y: U) -> U {
x.foo(y.to()).to() //~ ERROR overflow evaluating the requirement
x.foo(y.to()).to()
}
fn main() {

31
tests/ui/specialization/issue-39448.stderr
View File

@ -8,28 +8,21 @@ LL | #![feature(specialization)]
= help: consider using `min_specialization` instead, which is more stable and complete
= note: `#[warn(incomplete_features)]` on by default
error[E0275]: overflow evaluating the requirement `T: FromA<U>`
--> $DIR/issue-39448.rs:45:13
|
LL | x.foo(y.to()).to()
| ^^
|
note: required for `T` to implement `FromA<U>`
--> $DIR/issue-39448.rs:24:29
error[E0391]: cycle detected when computing whether impls specialize one another
--> $DIR/issue-39448.rs:24:1
|
LL | impl<T: A, U: A + FromA<T>> FromA<T> for U {
| -------- ^^^^^^^^ ^
| |
| unsatisfied trait bound introduced here
note: required for `U` to implement `ToA<T>`
--> $DIR/issue-39448.rs:34:12
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
LL | impl<T, U> ToA<U> for T
| ^^^^^^ ^
LL | where
LL | U: FromA<T>,
| -------- unsatisfied trait bound introduced here
= note: ...which requires evaluating trait selection obligation `u16: FromA<u8>`...
= note: ...which again requires computing whether impls specialize one another, completing the cycle
note: cycle used when building specialization graph of trait `FromA`
--> $DIR/issue-39448.rs:20:1
|
LL | trait FromA<T> {
| ^^^^^^^^^^^^^^
= note: see https://rustc-dev-guide.rust-lang.org/overview.html#queries and https://rustc-dev-guide.rust-lang.org/query.html for more information
error: aborting due to 1 previous error; 1 warning emitted
For more information about this error, try `rustc --explain E0275`.
For more information about this error, try `rustc --explain E0391`.

3
tests/ui/specialization/issue-39618.rs
View File

@ -2,8 +2,6 @@
// FIXME(JohnTitor): Centril pointed out this looks suspicions, we should revisit here.
// More context: https://github.com/rust-lang/rust/pull/69192#discussion_r379846796
//@ check-pass
#![feature(specialization)] //~ WARN the feature `specialization` is incomplete
trait Foo {
@ -19,6 +17,7 @@ fn bar(&self) {}
}
impl<T> Foo for T where T: Bar {
//~^ ERROR cycle detected when computing whether impls specialize one another
fn foo(&self) {}
}

20
tests/ui/specialization/issue-39618.stderr
View File

@ -1,5 +1,5 @@
warning: the feature `specialization` is incomplete and may not be safe to use and/or cause compiler crashes
--> $DIR/issue-39618.rs:7:12
--> $DIR/issue-39618.rs:5:12
|
LL | #![feature(specialization)]
| ^^^^^^^^^^^^^^
@ -8,5 +8,21 @@ LL | #![feature(specialization)]
= help: consider using `min_specialization` instead, which is more stable and complete
= note: `#[warn(incomplete_features)]` on by default
warning: 1 warning emitted
error[E0391]: cycle detected when computing whether impls specialize one another
--> $DIR/issue-39618.rs:19:1
|
LL | impl<T> Foo for T where T: Bar {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: ...which requires evaluating trait selection obligation `u64: Bar`...
= note: ...which again requires computing whether impls specialize one another, completing the cycle
note: cycle used when building specialization graph of trait `Foo`
--> $DIR/issue-39618.rs:7:1
|
LL | trait Foo {
| ^^^^^^^^^
= note: see https://rustc-dev-guide.rust-lang.org/overview.html#queries and https://rustc-dev-guide.rust-lang.org/query.html for more information
error: aborting due to 1 previous error; 1 warning emitted
For more information about this error, try `rustc --explain E0391`.

24
tests/ui/traits/stack-error-order-dependence-2.rs Normal file
View File

@ -0,0 +1,24 @@
//@ check-pass
// Regression test for <https://github.com/rust-lang/rust/issues/123303>.
// This time EXCEPT without `dyn` builtin bounds :^)
pub trait Trait: Supertrait {}
trait Impossible {}
impl<F: ?Sized + Impossible> Trait for F {}
pub trait Supertrait {}
impl<T: ?Sized + Trait + Impossible> Supertrait for T {}
fn needs_supertrait<T: ?Sized + Supertrait>() {}
fn needs_trait<T: ?Sized + Trait>() {}
struct A;
impl Trait for A where A: Supertrait {}
impl Supertrait for A {}
fn main() {
needs_supertrait::<A>();
needs_trait::<A>();
}

19
tests/ui/traits/stack-error-order-dependence.rs Normal file
View File

@ -0,0 +1,19 @@
//@ check-pass
// Regression test for <https://github.com/rust-lang/rust/issues/123303>.
pub trait Trait: Supertrait {}
trait Impossible {}
impl<F: ?Sized + Impossible> Trait for F {}
pub trait Supertrait {}
impl<T: ?Sized + Trait + Impossible> Supertrait for T {}
fn needs_supertrait<T: ?Sized + Supertrait>() {}
fn needs_trait<T: ?Sized + Trait>() {}
fn main() {
needs_supertrait::<dyn Trait>();
needs_trait::<dyn Trait>();
}