conflicting impl since nightly-2024-05-01 about rust HOT 13 CLOSED

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.

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

from rust.

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

from rust.

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 😅

from rust.

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

from rust.

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.

from rust.

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.

from rust.

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

from rust.

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

from rust.

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

from rust.

from rust.

#125763 (comment)

@nikomatsakis Your vote didn't register for some reason. #125763 (comment)

edit by @lcnr: I manually checked nikos box now

from rust.

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

from rust.

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.

from rust.