Revert "Do not check privacy for RPITIT."

[mladedav]

The changes here were first merged in #143357 and later reverted in #144098 as it introduces new hard errors. There was a crater run tracked in #144139 to see how much projects would be broken (not that many, a few repositories on github are affected).

This reenables hard errors for privacy in RPITIT.

Fixes #143531
Closes #144139
Hopefully closes #71043

[rustbot]

compiler-errors is not on the review rotation at the moment.
They may take a while to respond.

[compiler-errors]

r? cjgillot

[mladedav]

Gentle ping @cjgillot

[joshtriplett]

This came up in today's @rust-lang/lang meeting. It's clear why this needed an FCP (as it's a breaking change), but we didn't feel like we had the context. Could we get a clear ask for what exactly the new hard error is that we're reviewing?

Does this just make it a hard error to write a public trait that has something like -> impl Trait using a private trait? Or is there more to it than that? The discussion in #144139 makes it sound like it's substantially more complex and subtle than that.

[mladedav]

It is a little bit more subtle in the current form, the main weirdness I remember is that creating a required method returning a private impl trait does not error out, only providing an implementation does, so

pub trait Foo {
    fn required_impl_trait() -> impl Private;
}

does not error while

pub trait Foo {
    fn required_impl_trait() -> impl Private;
}

impl Foo for S {
    fn required_impl_trait() -> impl Private { X }
}

and

pub trait Foo {
    fn provided_impl_trait() -> impl Private { X }
}

both error out.

The error is also reported when the Private trait is used in generic bounds of the method.

And then for AFIT it seems to work the same after desugaring, so async fn required_async_concrete() -> PrivateStruct; works the same way as it desugars to returning an impl trait which is considered private due to its private associated type. So declaring the method in the trait is not linted while implementing it is.

As I understand it, this is not as strict as it should be based on @petrochenkov's comment and even the first case of defining the trait should be rejected.

Here is a playground with more cases to see what does and does not produce errors (though the errors are just comments but compiling the code on this branch should provide the stated results).

---

To summarize, this adds errors when using a private trait in RPITIT but when the offending trait is not used in a trait bound and an implementation is not provided, there is a false negative an the error is not emitted even though it should be.

[traviscross]

@bors2 try

[traviscross]

@mladedav: I'm having trouble working out the reason why we'd give a hard error for the RPIT-in-trait-impl,

trait PrivTr {}
impl PrivTr for () {}
#[expect(private_bounds)]
pub trait PubTr {
    fn f1() -> impl PrivTr;
}
impl<T> PubTr for T {
    #[expect(private_interfaces)]
    fn f1() -> impl PrivTr {}
    //~^ error[E0446]: private trait `PrivTr` in public interface
    //~| help: can't leak private trait
}

given that we don't give an error for an RPIT-in-free-function,

trait PrivTr {}
impl PrivTr for () {}

#[expect(private_interfaces)]
pub fn f2() -> impl PrivTr {} //~ OK

and given that we allow the comparable associated type desugaring of the RPITIT:

trait PrivTr {}
impl PrivTr for () {}
pub trait PubTr {
    #[expect(private_bounds)]
    type F1: PrivTr; //~ OK
    fn f1() -> Self::F1;
}
impl<T> PubTr for T {
    type F1 = ();
    fn f1() -> Self::F1 {}
}

What's the rationale here?

cc @petrochenkov

---

I note that on nightly we give an error for this, when desugaring the RPIT-in-trait-impl to ATPIT:

#![feature(impl_trait_in_assoc_type)]
trait PrivTr {}
impl PrivTr for () {}
pub trait PubTr {
    #[expect(private_bounds)]
    type F1: PrivTr; //~ OK
    fn f1() -> Self::F1;
}
impl<T> PubTr for T {
    type F1 = impl PrivTr;
    //~^ error[E0446]: private trait `PrivTr` in public interface
    fn f1() -> Self::F1 {}
}

What's the rationale here? It makes sense why we can't leak a private type in this way -- we'd then be allowing a private type to be named. Why does this rise to the level of a hard error for a private trait in an impl trait bound?

---

Also, on the PR, I notice that placing the expect over the trait item doesn't work.

trait PrivTr {}
impl PrivTr for () {}
pub trait PubTr {
    #[expect(private_bounds)] //~ warning: this lint expectation is unfulfilled
    fn f1() -> impl PrivTr;
    //~^ warning: trait `PrivTr` is more private than the item `PubTr::f1::{anon_assoc#0}`
}

Should it?

[rust-bors]

☀️ Try build successful (CI)
Build commit: e117153 (e117153a45c546e883c1f91d82611775fcaeffe0, parent: c90bcb9571b7aab0d8beaa2ce8a998ffaf079d38)

[traviscross]

@craterbot check

[craterbot]

👌 Experiment pr-146470 created and queued.
🤖 Automatically detected try build e117153
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-146470 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[mladedav]

@mladedav: I'm having trouble working out the reason why we'd give a hard error for the RPIT-in-trait-impl, [...] What's the rationale here?

I have to say that I don't completely understand the code in question.

From my understanding of the history, @cjgillot first wrote these changes in #143357 to clean up the code and the addition of the error was accidental. But the error itself is desirable.

This is a follow up since the last crater run has finished and when I looked through it, while there was a little breakage in some github repositories, it did not seem like a major issue. So I wanted to drive this forward so that the errors are added rather earlier than later.

So I don't think there was anyone who explicitly decided that one of the forms should be rejected while the other should be allowed. I think we can all agree that all of them should be at least warnings, but according to this comment, they should all be errors. Therefore, I see this as an incremental step towards the end goal while I see that there are issues with this.

But it's completely fair if you think that the rules as they would be with this would be arbitrary or that it shouldn't lint twice on the same issue and that the errors should be improved to not mention things like PubTr::f1::{anon_assoc#0} and therefore we should not merge this and have someone build a more complete solution.

[petrochenkov]

What's the rationale here?

I'll recall the details and look at this this week.

[craterbot]

🎉 Experiment pr-146470 is completed!
📊 27 regressed and 2 fixed (731634 total)
📊 2113 spurious results on the retry-regessed-list.txt, consider a retry¹ if this is a significant amount.
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Footnotes

1. re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-146470/retry-regressed-list.txt ↩