conflicting impl since nightly-2024-05-01

[conradludgate]

Code

I tried this code:

https://github.com/conradludgate/measured/tree/b2978b32e2065cb6257244829b5fd6ae1dba4576

pub trait LabelGroupSet {
    type Group<'a>: LabelGroup;
}

impl<T: LabelSet> LabelGroupSet for T
where
    for<'a> T::Value<'a>: LabelGroup,
{
    type Group<'a> = T::Value<'a>;
}

pub trait LabelSet {
    type Value<'a>;
}

impl<T: LabelGroupSet + ?Sized> LabelGroupSet for &'static T {
    type Group<'a> = T::Group<'a>;
}

pub trait LabelGroup {}

I expected to see this happen: compiles successfully

Instead, this happened:

error[E0119]: conflicting implementations of trait `LabelGroupSet` for type `&'static _`
   --> core/src/label/value.rs:45:1
    |
45  | / impl<T: LabelSet> LabelGroupSet for T
46  | | where
47  | |     for<'a> T::Value<'a>: LabelGroup,
    | |_____________________________________^ conflicting implementation for `&'static _`
    |
   ::: core/src/label/group.rs:133:1
    |
133 |   impl<T: LabelGroupSet + ?Sized> LabelGroupSet for &'static T {
    |   ------------------------------------------------------------ first implementation here
    |
    = note: downstream crates may implement trait `label::value::LabelSet` for type `&'static _`
    = note: downstream crates may implement trait `label::group::LabelGroup` for type `<&'static _ as label::value::LabelSet>::Value<'a>`

Version it worked on

It most recently worked on: nightly-2024-04-30

Version with regression

according to cargo-bisect-rustc, the regression started with nightly-2024-05-01 in #117164

[Noratrieb]

the PR was FCPd, so I expect this to be expected breakage, but @lcnr @fmease in case it's not.

[lcnr]

Thank you for opening this issue!

further minimized

pub trait Overlap {}
impl<T: Overlap + ?Sized> Overlap for &'static T {}
impl<T: LabelSet> Overlap for T
where
    for<'a> T::Value<'a>: LabelGroup,
{}

pub trait LabelGroup {}
pub trait LabelSet {
    type Value<'a>;
}

This pattern is #114061. Coherence now considers for<'a> <&'static ?x as LabelSet>::Value<'a>: LabelGroup to be unknowable, as downstream crates could implement LabelSet for &'static LocalType where there's an impl of LabelGroup for LocalType. This should break. The FCP in #117164 (comment) did not mention that this part of the change will immediately be a hard error instead of a future-compat lint.

We use orphan_check_trait_ref in two places: for the orphan check itself, and when checking whether some trait goal is "knowable" We only added the future compat lint behavior to the orphan check, always using the correct approach in trait_ref_is_knowable. This is the first regression caused by that.

rust/compiler/rustc_trait_selection/src/traits/coherence.rs

Lines 944 to 948 in d43930d

	InCrate::Remote => {
	// The inference variable might be unified with a local
	// type in that remote crate.
	ControlFlow::Break(OrphanCheckEarlyExit::LocalTy(ty))
	}

---

Sorry for not mentioning this in the original FCP. I was aware of this immediate breakage but did not mention it.

Changing this to also be a future compat lint is annoying and I intentionally did not suggest this in #117164, as that is "inside of the trait solver" and has a smaller impact than the changes to the orphan check itself.

I would like to not revert this change as there have been some small follow-up PRs and I want to close these unsoundnesses asap. I propose accepting this breakage and to close this issue after adding it as a test. However, given that this has slipped through the FCP process, I am open to revert and re-land with another FCP in case there are any concerns by a @rust-lang/types member.

@rfcbot fcp close

[rfcbot]

Team member @lcnr has proposed to close this. The next step is review by the rest of the tagged team members:

• @BoxyUwU
• @aliemjay
• @compiler-errors
• @jackh726
• @lcnr
• @nikomatsakis
• @oli-obk
• @spastorino

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[fmease]

I'd like to note that this kind of code would've gotten rejected with the stabilization of -Znext-solver=coherence (#121848) even without #117164 which was explicitly mentioned in #121848 (Ctrl+F "#114061" or "Unknowable candidates for higher ranked trait goals"). So technically speaking this has already been accepted by FCP (#121848 (comment)).

You can verify this yourself by running rustc +nightly-2024-04-30 mcve.rs --crate-type=lib -Znext-solver=coherence.

[conradludgate]

I'm fine with the decision but it would have been nicer to have some warning with info

[fmease]

Yeah, that's understandable. Sorry about that!
#125763 (comment) was more addressed to T-types, not to you specifically.

[lcnr]

yes, we did no catch the breakage in your crate when running crater, unsure why that was the case 🤔

• https://crater-reports.s3.amazonaws.com/pr-117164-3/index.html
• https://crater-reports.s3.amazonaws.com/pr-121848-2/index.html

Generally we try to open fixes or at least issues to affected projects when landing breaking changes. This relies on us actually detecting these changes however 😅

[conradludgate]

yes, we did no catch the breakage in your crate when running crater, unsure why that was the case 🤔

Strange. This particular impl has been on crates.io since February 10 🤔 those crater runs were Feb-27 and Apr-27 respectively and measured is not in the dataset at all. Very strange.

https://docs.rs/measured/0.0.12/measured/label/group/trait.LabelGroupSet.html#impl-LabelGroupSet-for-T

[lqd]

yes, we did no catch the breakage in your crate when running crater, unsure why that was the case 🤔

yeah it's not the first time this happened, there's rust-lang/crater#727 and discussions on zulip IIRC

[nikomatsakis]

@rfcbot reviewed

…

On Thu, May 30, 2024, at 1:00 PM, Rémy Rakic wrote: > yes, we did no catch the breakage in your crate when running crater, unsure why that was the case 🤔 > yeah it's not the first time this happened, there's rust-lang/crater#727 <rust-lang/crater#727> and discussions on zulip IIRC — Reply to this email directly, view it on GitHub <#125763 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABF4ZWOU2HYIBT6YOVVZMDZE5LLFAVCNFSM6AAAAABIQPESU2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBQGI3TEOBWGM>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period, with a disposition to close, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.