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

Tracking Issue for coinductive_overlap_in_coherence lint #114040

Closed
2 of 3 tasks
compiler-errors opened this issue Jul 25, 2023 · 1 comment
Closed
2 of 3 tasks

Tracking Issue for coinductive_overlap_in_coherence lint #114040

compiler-errors opened this issue Jul 25, 2023 · 1 comment
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC

Comments

@compiler-errors
Copy link
Member

compiler-errors commented Jul 25, 2023

This is the summary issue for the coinductive_overlap_in_coherence future-compatibility warning. The goal of this page is describe why this change was made and how you can fix code that is affected by it. It also provides a place to ask questions or register a complaint if you feel the change should not be made. For more information on the policy around future-compatibility warnings, see our breaking change policy guidelines.

What is a inductive cycle, why does it affect coherence?

Rust currently treats trait cycles as errors in the current trait solver. That is, if an impl requires some trait predicate to hold in order to prove that same trait predicate, then we know that it would require solving to depth infinity, and we can cut out the unnecessary work and conclude the predicate must never hold. This is called an inductive cycle.

For example:

trait A {}
impl<T> A for T where T: B {}

trait B {}
impl<T> B for T where T: A {}

fn needs_a<T: A>() {}

fn main() {
    needs_a::<i32>();
}

In order to prove that i32: A, we must prove that i32: B. However, to prove that i32: B, we must show that i32: A, completing the inductive cycle.

During coherence, when a where clause is known to error, we can use it to conclude that two impls do not overlap. We consider two impls to not overlap if unifying their impl headers results in a where-clause of one of the impls to not hold.

For example:

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

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

impl<T, Q> PartialEq<Q> for Interval<T>
where
    Q: PartialOrd,
{
    fn eq(&self, _: &Q) -> bool { todo!() }
}

impl<T, Q> PartialOrd<Q> for Interval<T>
where
    Q: PartialOrd,
{
    fn partial_cmp(&self, _: &Q) -> Option<Ordering> { todo!() }
}

When trying to show that the derive PartialEq and the manual impl don't overlap, we infer that Q = Interval<T>. Then we want to show that Interval<T>: PartialOrd, which is satisfied by the second impl. That then requires that Interval<T>: PartialOrd, leading to an inductive cycle, and we can conclude that the two impls may never overlap.

Why is this a problem?

In the new trait solver (#107374), we currently treat inductive cycles as ambiguity -- we may not assume they never hold, but we don't know that they certainly hold (due to the infinite depth required to prove it). This is to leave room for eventually migrating to coinductive solving, which is allowed to use the fact that we're proving a trait predicate to prove itself, breaking infinite predicate cycles. Unfortunately, this does mean that during coherence, we may no longer be able to prove that two impls overlap.

Tracking

@compiler-errors compiler-errors added the C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC label Jul 25, 2023
@compiler-errors compiler-errors changed the title Tracking Issue for inductive_overlap_in_coherence lint Tracking Issue for coinductive_overlap_in_coherence lint Jul 25, 2023
bors added a commit to rust-lang-ci/rust that referenced this issue Aug 15, 2023
…nt, r=lcnr

Warn on inductive cycle in coherence leading to impls being considered not overlapping

This PR implements a `coinductive_overlap_in_coherence` lint (rust-lang#114040), which warns users against cases where two impls are considered **not** to overlap during coherence due to an inductive cycle disproving one of the predicates after unifying the two impls.

Cases where this lint fires will become an overlap error if we ever move to coinduction, so I'd like to make this a warning to avoid having more crates take advantage of this behavior in the mean time. Also, since the new trait solver treats inductive cycles as ambiguity, not an error, this is a blocker for landing the new trait solver in coherence.
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

lcnr commented Feb 9, 2024

has been changed to a hard error in #118649

@lcnr lcnr closed this as completed Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC
Projects
None yet
Development

No branches or pull requests

2 participants