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

Trait coherency regression in 1.78 #124664

Closed
w4 opened this issue May 3, 2024 · 3 comments
Closed

Trait coherency regression in 1.78 #124664

w4 opened this issue May 3, 2024 · 3 comments
Labels
A-coherence Area: Coherence C-bug Category: This is a bug. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-types Relevant to the types team, which will review and decide on the PR/issue.

Comments

@w4
Copy link

w4 commented May 3, 2024

Code

I tried this code:

trait Vendor {
    type Processor<'a>: VendorProcessor;
}

trait VendorProcessor {
    type EventGroup: Group;
}

trait Group {}

trait EventProcessor<T> {}

struct MyEventProcessor<T: Vendor> {
    t: T,
}

struct MyEvent {}

impl Group for MyEvent {}

impl<T: Vendor> EventProcessor<MyEvent> for MyEventProcessor<T> {}

impl<T, Group> EventProcessor<Group> for MyEventProcessor<T>
where
    T: Vendor,
    for<'a> T::Processor<'a>: VendorProcessor<EventGroup = Group>,
{}

I expected a working build.

Instead, this happened:

error[E0119]: conflicting implementations of trait `EventProcessor<MyEvent>` for type `MyEventProcessor<_>`
  --> minimal-repro/src/lib.rs:23:1
   |
21 |   impl<T: Vendor> EventProcessor<MyEvent> for MyEventProcessor<T> {}
   |   --------------------------------------------------------------- first implementation here
22 |
23 | / impl<T, Group> EventProcessor<Group> for MyEventProcessor<T>
24 | | where
25 | |     T: Vendor,
26 | |     for<'a> T::Processor<'a>: VendorProcessor<EventGroup = Group>,
   | |__________________________________________________________________^ conflicting implementation for `MyEventProcessor<_>`

If this was a bugfix for trait coherency, it doesn't seem to have been documented in the changelog. The two implementations could technically be irrefutably non-conflicting though if there is no impl VendorProcessor<EventGroup = MyEvent> on private types.

Version it worked on

It most recently worked on: 1.77.2

Version with regression

rustc --version --verbose:

rustc 1.78.0 (9b00956e5 2024-04-29)
binary: rustc
commit-hash: 9b00956e56009bab2aa15d7bff10916599e3d6d6
commit-date: 2024-04-29
host: aarch64-apple-darwin
release: 1.78.0
LLVM version: 18.1.2

@rustbot modify labels: +regression-from-stable-to-stable -regression-untriaged

@w4 w4 added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels May 3, 2024
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-untriaged Untriaged performance or correctness regression. labels May 3, 2024
@w4 w4 changed the title Trait conflicting implementation detection regression in 1.78 Trait coherency regression in 1.78 May 3, 2024
@lqd
Copy link
Member

lqd commented May 3, 2024

This is from #120584, I think it was expected because of existing brokenness but I may be wrong and see no FCP, so will cc its author @compiler-errors.

@compiler-errors
Copy link
Member

compiler-errors commented May 3, 2024

Yeah, this is a legitimate bug in the code sample that was shared. Downstream crates can implement Vendor with a Processor type that implements VendorProcessor<EventGroup = MyEvent>. Coherence doesn't take into account the privacy of the items, so privacy here doesn't really matter.

For the record, this also would have begun to fail when we land the new trait solver for coherence (#121848) which will ideally happen soon.

@w4
Copy link
Author

w4 commented May 4, 2024

Thanks guys. It is clear now that this code sample only compiled because of the HRTB, but it was nice because the trait solver did actually work correctly here for our particular use case, as it was able to disambiguate between our usages of VendorProcessor<MyEvent> and VendorProcessor<Group> but I understand other 99% of use cases where this is broken. Closing.

@w4 w4 closed this as completed May 4, 2024
@apiraino apiraino removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label May 4, 2024
@saethlin saethlin added T-types Relevant to the types team, which will review and decide on the PR/issue. A-coherence Area: Coherence and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels May 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-coherence Area: Coherence C-bug Category: This is a bug. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants