Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ambiguity on inductive cycles #20

Open
Tracked by #114862
lcnr opened this issue May 26, 2023 · 3 comments
Open
Tracked by #114862

ambiguity on inductive cycles #20

lcnr opened this issue May 26, 2023 · 3 comments
Labels
A-coherence Having to do with regressions in `-Ztrait-solver=next-coherence` S-breaking-change

Comments

@lcnr
Copy link
Contributor

lcnr commented May 26, 2023

We now always fail with ambiguity on inductive cycles, even in the old solver rust-lang/rust#118649


The old solver returns EvaluatedToRecur on inductive cycles which allows candidates with such cycles to be dropped, allowing the following example to compile:

trait Trait<T> {}

impl<T> Trait<u32> for T
where
    T: Trait<u32> {}
impl Trait<i32> for () {}

fn impls_trait<T: Trait<U>, U>() {}

fn main() {
    impls_trait::<(), _>();
}

more importantly, this affects coherence, e.g. interval-map:

use std::borrow::Borrow;
use std::cmp::Ordering;
use std::marker::PhantomData;

#[derive(PartialEq, Default)]
pub(crate) struct Interval<T>(PhantomData<T>);

// This impl overlaps with the `derive` unless we reject the nested
// `Interval<?1>: PartialOrd<Interval<?1>>` candidate which results
// in an inductive cycle rn.
impl<T, Q> PartialEq<Q> for Interval<T>
where
    T: Borrow<Q>,
    Q: ?Sized + PartialOrd,
{
    fn eq(&self, other: &Q) -> bool {
        true
    }
}

impl<T, Q> PartialOrd<Q> for Interval<T>
where
    T: Borrow<Q>,
    Q: ?Sized + PartialOrd,
{
    fn partial_cmp(&self, other: &Q) -> Option<Ordering> {
        None
    }
}

Returning NoSolution in the new solver would be unsound because the cycle may be with different inference variables due to canonicalization. For coinductive cycles we rerun the goal and continuously update the provisional result. Doing so for inductive cycles as well is non-trivial so I would like to ideally avoid it.

The new solver instead treats inductive cycles as overflow, meaning that the above test fails. This will end up breaking anyways once we change traits to be coinductive by default. It therefore seems desirable to prevent people from relying on that behavior as soon as possible.

@lcnr

This comment was marked as outdated.

@lcnr lcnr added the A-coherence Having to do with regressions in `-Ztrait-solver=next-coherence` label Jul 24, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 8, 2024
…=lcnr

Make inductive cycles in coherence ambiguous always

Logical conclusion of rust-lang#114040
One step after rust-lang#116493

cc rust-lang/trait-system-refactor-initiative#20

r? lcnr to kick off the FCP after review... maybe we should wait until 1.75 is landed? In that case, I'd still like to get the FCP boxes checked sooner since that'll be near the holidays which means everyone's away.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 9, 2024
…=lcnr

Make inductive cycles in coherence ambiguous always

Logical conclusion of rust-lang#114040
One step after rust-lang#116493

cc rust-lang/trait-system-refactor-initiative#20

r? lcnr to kick off the FCP after review... maybe we should wait until 1.75 is landed? In that case, I'd still like to get the FCP boxes checked sooner since that'll be near the holidays which means everyone's away.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jan 9, 2024
Rollup merge of rust-lang#118649 - compiler-errors:coherence-ambig, r=lcnr

Make inductive cycles in coherence ambiguous always

Logical conclusion of rust-lang#114040
One step after rust-lang#116493

cc rust-lang/trait-system-refactor-initiative#20

r? lcnr to kick off the FCP after review... maybe we should wait until 1.75 is landed? In that case, I'd still like to get the FCP boxes checked sooner since that'll be near the holidays which means everyone's away.
@lcnr
Copy link
Contributor Author

lcnr commented Feb 9, 2024

@compiler-errors tried to also change this outside of coherence, resulting in 21 crater regressions apparently rust-lang/rust#116494

we should minimize the affected crates and FCP merge this in the old solver, this should simplify the regression to the new solver

@lcnr
Copy link
Contributor Author

lcnr commented Mar 14, 2024

since rust-lang/rust#118649 this is now the default behavior in coherence, even with the old solver, still affects trait solving outside of it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-coherence Having to do with regressions in `-Ztrait-solver=next-coherence` S-breaking-change
Projects
None yet
Development

No branches or pull requests

1 participant