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

[traviscross]

@bors2 try

[tmandry]

@rfcbot resolve is my understanding correct

I think I understand this now and am comfortable with moving forward conservatively, pending crater results.

@rfcbot reviewed
@rfcbot concern crater run results

[nikomatsakis]

As I said in the meeting:

@rfcbot reviewed

This seems like a sensible extension and intuitively matches what we do for returning a value of a private struct. Longer term, I would still like to reframe our privacy rules in terms of "capabilities associated with a trait" as I described earlier, and before we actually land this I do want to see the results of the crater run, but generally convinced this is the right next step.

[rust-bors]

☀️ Try build successful (CI)
Build commit: 81ab7f2 (81ab7f2139295590561adbe6d5b0aaa2feff765f, parent: 377656d3dd3f9c23a9c8713e163f4365a5261a84)

[petrochenkov]

@craterbot check

[craterbot]

👌 Experiment pr-146470-3 created and queued.
🤖 Automatically detected try build 81ab7f2
🔍 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-3 is now running

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

[craterbot]

🎉 Experiment pr-146470-3 is completed!
📊 3661 regressed and 3 fixed (757534 total)
📊 1978 spurious results on the retry-regressed-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-3/retry-regressed-list.txt ↩

[petrochenkov]

Most of the regressions are crates depending on image-0.25.(7,8,9).
Time to resurrect the private_in_public future compatibility warning, I guess, it was used for closing similar holes in the past.

[traviscross]

@craterbot check p=1 crates=https://crater-reports.s3.amazonaws.com/pr-146470-3/retry-regressed-list.txt

[craterbot]

👌 Experiment pr-146470-4 created and queued.
🤖 Automatically detected try build 81ab7f2
🔍 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

[nikomatsakis]

@petrochenkov

We discussed this in the lang-team meeting. We agree that it isn't viable to land this as an immediat breaking change and that, if we are to do this, future-compatibility warnings would be required. I think some of us were a bit on the fence about the mental model here and seeing a lot of regressions gave us some pause; others continued to support the change, just want to be sure we approach it carefully.

The main thing that is needed to finalize the decision is an analysis of the "root patterns" causing breakage. We would want to make sure that we have sensible equivalents to recommend to people.

For example, I looked into the code in embassy and found this pattern:

/// Implementation details for embassy macros.
/// Do not use. Used for macros and HALs only. Not covered by semver guarantees.
#[doc(hidden)]
#[cfg(not(feature = "nightly"))]
pub mod _export {
    //...
    trait TaskReturnValue {}
    impl TaskReturnValue for () {}
    impl TaskReturnValue for Never {}

    #[diagnostic::on_unimplemented(
        message = "task futures must resolve to `()` or `!`",
        note = "use `async fn` or change the return type to `impl Future<Output = ()>`"
    )]
    #[allow(private_bounds)]
    pub trait TaskFn<Args>: Copy {
        type Fut: Future<Output: TaskReturnValue> + 'static;
        //                       ^^^^^^^^^^^^^^^ the error is here
    }

@Nadrieril pointed out that this trait could as well be a sealed trait -- seems true, although more painful, but that's because sealed traits are painful.

[petrochenkov]

I'll link to this comment #146470 (comment) again.

Maybe predicates and bounds cannot actually leak anything, then they can be demoted to a lint, we just need to have some more or less convincing proof of that written.

Someone needs to try and break it and leak something through various parts (Generics, Predicates, Default type, Bounds) of an associated type. (To cause a linking error, or missing encoded MIR issues, for example.)
I can do it, but not this week (and maybe not next week).

[traviscross]

Someone needs to try and break it...

cc @theemathas

[craterbot]

🚧 Experiment pr-146470-4 is now running

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

[craterbot]

🎉 Experiment pr-146470-4 is completed!
📊 3316 regressed and 0 fixed (5518 total)
📊 264 spurious results on the retry-regressed-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-4/retry-regressed-list.txt ↩

[theemathas]

Someone needs to try and break it and leak something through various parts (Generics, Predicates, Default type, Bounds) of an associated type. (To cause a linking error, or missing encoded MIR issues, for example.)

@petrochenkov How does privacy affect linking or MIR encoding? I'm guessing that an ICE would occur if there were a way to directly call a trait method defined on a private Self type in a different crate, even if the trait was public?