Skip to content

ambiguity on inductive cycles #20

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

Closed
Tracked by #114862
lcnr opened this issue May 26, 2023 · 4 comments
Closed
Tracked by #114862

ambiguity on inductive cycles #20

lcnr opened this issue May 26, 2023 · 4 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 rust-lang/rust#122791


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

This comment has been minimized.

@lcnr

This comment has been minimized.

@lcnr
Copy link
Contributor Author

lcnr commented Feb 27, 2025

as this now matches the behavior of the old solver, closing in favor of #114

@lcnr lcnr closed this as completed Feb 27, 2025
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