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

[craterbot]

🚧 Experiment pr-146470-2 is now running

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

[craterbot]

🎉 Experiment pr-146470-2 is completed!
📊 18 regressed and 0 fixed (142 total)
📊 39 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-2/retry-regressed-list.txt ↩

[petrochenkov]

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

That's a bug in the implementation of rule #144139 (comment).
Fix - c5221eb.

[petrochenkov]

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

The associated type item newly added for the RPITIT is used as a lint node, which is a different node from fn f1 to which the expect attribute is attached.

[petrochenkov]

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?

So, an associated type's interface can contain

• Generics
• Predicates
• Default type
• Bounds

type Type<T: PrivTrait /*predicates*/ = PrivTy /*generics*/>: PrivTrait /*bounds*/
    where Something: PrivTrait /*predicates*/
    = PrivTy /*default type*/;

And we just don't differentiate and check all of them when reporting the hard error version of the priv-in-pub check.
Originally that was simply a conservative choice allowing to avoid thinking too much about which of those can actually leak something private.
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.

Why this is a hard error in the first place (rule #144139 (comment))?
During post-factum type privacy checks when we see <Type as Trait>::Assoc we cannot see and check its underlying type (that's the technical limitation mentioned in the RFC), we can only check that Type and Trait are not private.
So at definition site we must ensure (not just lint) that impl Trait for Type cannot define an Assoc that is more private than any of them.

[traviscross]

Thanks, that makes sense and is helpful. If you could, help me distinguish these cases according to that, or let me know if you think any of these behaviors are incorrect under this model. In particular, I'm suspicious of the hard error in the first case given the other two.

A private trait in the bound of an impl Trait type that's used as the definition of an associated type in an impl raises an error (the associated type, in the trait, uses a public supertrait as the bound):

#![feature(impl_trait_in_assoc_type)]
pub trait Super {}
trait PrivTr: Super {}
impl Super for () {}
impl PrivTr for () {}
pub trait PubTr {
    type F1: Super;
    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 {}
}

Similar to the above but with the impl Trait, with the bound that includes the private trait, pulled out to a public type alias.

#![feature(impl_trait_in_assoc_type, type_alias_impl_trait)]
pub trait Super {}
trait PrivTr: Super {}
impl Super for () {}
impl PrivTr for () {}
pub trait PubTr {
    type F1: Super;
    fn f1() -> Self::F1;
}
#[expect(private_interfaces)]
pub type Alias = impl PrivTr;
#[define_opaque(Alias)]
fn defines() -> Alias {}
impl<T> PubTr for T {
    type F1 = Alias; //~ OK
    fn f1() -> Self::F1 { defines() }
}

RPIT-in-free-function:

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

[petrochenkov]

Here

type F1 = Alias; //~ OK

the compiler cannot normalize properly, and then see and check the underlying type of Alias.
Otherwise it would produce an error.

So either the normalization should be implemented properly (#126076), or type_alias_impl_traits (like pub type Alias = impl PrivTr;) should follow the same rule as associated types and produce hard errors instead of lints at definition site.
Thankfully, such free type aliases with normalization issues are not yet stable.

The "RPIT-in-free-function" is ok, it's not an associated type context, so it's a lint and not an error.

[traviscross]

Makes sense. Thanks for helping me see the model more clearly.

[traviscross]

We talked about this in the lang call today. Let's propose to do this.

@rfcbot fcp merge lang

[rust-rfcbot]

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

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

Concerns:

• is my understanding correct (Revert "Do not check privacy for RPITIT." #146470 (comment))

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!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[traviscross]

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}`
}

We talked about this in the lang call as well. @petrochenkov had explained the reason for this here:

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

The associated type item newly added for the RPITIT is used as a lint node, which is a different node from fn f1 to which the expect attribute is attached.

That explains this mechanically, but it still felt like a bug to us. We'd expect that the attribute should take effect when placed above the function item.

[nikomatsakis]

We talked about this in the @rust-lang/lang call. I was trying to put this decision on clearer footing. I think the right way to describe our model is that we have

• capabilities indicated by the level of privacy declared at the declaration

and then we have the notion that nothing in the language should permit other crates to exceed those "capabilities". Useful questions I have found in this space is "if I were am unsafe code, what can I rely on" (e.g., these methods cannot ever be called) and "what kind of changes can I make without affecting consumers of my crate" (e.g., SemVer hazards).

I would like a clear statement of "if my module M has visibility onto a given item I, what does that allow me to do, and what can I assume that others cannot do?"

One obvious capability is directly naming the item.

For a struct, I think we include "accessing the (visible) members of the struct". In other words, a pub method on a private struct still ought not to be callable from outside the crate.

The question then is what are the "capabilities" associated with a trait? Off the top of my head...

• determining whether the trait is implemented for a type or indeed implemented at all;
• accessing members of the trait when applied to some type;
• knowing that it "exists".

Some examples that come to mind. Given this:

// Crate A
trait PrivateToMyCrate;

pub trait Get {
    type Output;
    
    fn get_output() -> Self::Output;
}

impl Get for () {
    type Output = impl PrivateToMyCrate;
  
    fn get_output() -> Self::Output { () }
}

then Crate A can add something like this:

// also in Crate A:
pub fn foo(t: impl PrivateToMyCrate) { }

and then Crate B would be able to do this, which implies that they kinda "know the trait exists" and also "know that it is implemented", albeit only for this opaque type:

// in Crate B:
crate_a::foo(<() as Get>::get_output())

It's not entirely clear if this is an issue.

The idea is that this framework might give us a clearer way to decide issues like this in the future.

[nikomatsakis]

@tmandry and I talked about the possibility of trying to draft something like that, either as an update to the RFC or as an issue to FCP, something.

[petrochenkov]

The associated type item newly added for the RPITIT is used as a lint node, which is a different node from fn f1 to which the expect attribute is attached.

That explains this mechanically, but it still felt like a bug to us. We'd expect that the attribute should take effect when placed above the function item.

Yeah, that should certainly be fixable, just a technical issue.

[tmandry]

and then Crate B would be able to do this, which implies that they kinda "know the trait exists" and also "know that it is implemented", albeit only for this opaque type:

You can already do something stronger than that today:

trait Private {}

impl Private for () {}

#[expect(private_bounds)]
pub fn do_something(_x: impl Private) {}

We only lint on this case, and it is easily observable that (): Private.

Furthermore, we allow you to write (unstably, with private_bounds lints):

trait Private {}

pub type Foo = impl Private;

I don't see why associated types should be treated any differently than type aliases. I see that @traviscross asked about this before and I think the response from @petrochenkov was:

During post-factum type privacy checks when we see ::Assoc we cannot see and check its underlying type (that's the technical limitation mentioned in the RFC), we can only check that Type and Trait are not private.
So at definition site we must ensure (not just lint) that impl Trait for Type cannot define an Assoc that is more private than any of them.

I don't understand what this is trying to prevent. Put another way, what code does this allow to compile that definitely shouldn't? How is it different from allowing pub type Foo = impl Private?

[traviscross]

We could argue, I think, that such code -- the first example -- is OK because we haven't leaked the identity of Private -- we could replace Private in do_something with any other trait that implements () without breaking downstreams (and it's not immediately coming to mind what an unsafe code author could be reasonably relying on here that this would break).

Perhaps that generalizes. Maybe the relevant question isn't what we've exposed about the private trait but whether we could locally replace that private trait with any other private trait that still offers no less than what we have committed ourselves to with its use in the public item.

[traviscross]

Worth noting too is that while we don't give a def-site error for this,

trait Priv {}
impl Priv for () {}
#[expect(private_interfaces)]
pub fn f() -> impl Priv {}

we do give a use-site error if you try to make that call:

mod m1 {
    trait Priv {}
    impl Priv for () {}
    #[expect(private_interfaces)]
    pub fn f() -> impl Priv {}
}

fn main() {
    let _x = crate::m1::f(); //~ error: trait `Priv` is private
}

Playground link

[traviscross]

In fact, we even give a use-site error just for naming that public item, even if not called:

mod m1 {
    trait Priv {}
    impl Priv for () {}
    #[expect(private_interfaces)]
    pub fn f() -> impl Priv {}
}

fn main() {
    let _x = crate::m1::f; //~ error: trait `Priv` is private
}

Playground link

[tmandry]

Ok, I have spent too much time working out the intent of this change but I think I understand it now. Please let me know if my understanding is correct. I'm filing a concern to check this and also to check that it matches the understanding of lang members who checked their boxes.

@rfcbot concern is my understanding correct

Lang report

The intent of this breaking change is to bring the way we treat trait privacy errors in RPITITs into line with RPITs. Specifically, with RPIT it is not possible to call, or even name, a function that returns an RPIT with a private trait from a context that cannot name the trait: (Thanks to @traviscross for the example)

mod m1 {
    trait Priv {}
    impl Priv for () {}
    #[expect(private_interfaces)]
    pub fn f() -> impl Priv {}
}

fn main() {
    let _x = crate::m1::f; //~ error: trait `Priv` is private
}

We cannot reproduce this exact behavior with trait methods due to a technical limitation in the compiler. Therefore we pick a more conservative option of erroring at the definition site instead of the use site. This preserves the property that a function cannot be used to produce a value of type impl Trait in a context where Trait is private.

If we later want to relax this property we can, but for now this fixes a bug where RPITIT was unintentionally more permissive than other aspects of Rust's privacy system. The breakage is minimal, and isolated to a single crate with a handful of dependencies.