Revert "Do not check privacy for RPITIT." by mladedav · Pull Request #146470 · rust-lang/rust (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation59 Commits2 Checks12 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

@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

@mladedav

@rustbot rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

labels

Sep 12, 2025

@rustbot

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

@compiler-errors

@mladedav

@traviscross traviscross added needs-fcp

This change is insta-stable, or significant enough to need a team FCP to proceed.

P-lang-drag-1

Lang team prioritization drag level 1. https://rust-lang.zulipchat.com/#narrow/channel/410516-t-lang

T-lang

Relevant to the language team

I-lang-radar

Items that are on lang's radar and will need eventual work or consideration.

labels

Nov 5, 2025

@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

@rust-bors

This comment has been minimized.

rust-bors bot added a commit that referenced this pull request

Nov 6, 2025

@rust-bors

…-errors, r=

Revert "Do not check privacy for RPITIT."

@traviscross

This comment was marked as resolved.

@traviscross

This comment was marked as resolved.

@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 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 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 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

@craterbot

🚧 Experiment pr-146470 is now running

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

@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

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...

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 twofold:

  1. Bring the way we treat trait privacy errors in RPITITs into line with RPITs.
  2. Bring the way we treat associated type bounds into line with the type privacy RFC.

Privacy errors

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.

Associated type bounds

The type privacy RFC states that due to technical limitations in the compiler, we should treat all occurrences of a private trait in the bounds of a public trait's associated type as an error.

RPITIT is documented in its RFC to behave like an associated type is created for the return type of the method, and it is also implemented in this way in the compiler. This implies that any trait method of a public trait of the form fn method(...) -> impl Private where Private is a private trait should be an error. It currently isn't, and this change corrects that.

Note that we do not currently implement this correctly for the bounds of user-written associated types. @petrochenkov wants to include c5221eb in this PR, which would mean fixing the bug both for RPITITs and explicit associated types.

@tmandry

@petrochenkov Do you agree with the analysis I wrote above?

Also, regarding the equivalence to associated types, in the lang meeting we were surprised to see that this compiled:

mod m { pub trait Outer { #[expect(private_bounds)] type Inner: Private; fn get(&self) -> Self::Inner; } trait Private {} }

fn foo<T: m::Outer>(inp: &T) { let x: T::Inner = inp.get(); }

@petrochenkov

@petrochenkov Do you agree with the analysis I wrote above?

I agree.

Also, regarding the equivalence to associated types, in the lang meeting we were surprised to see that this compiled:

Discussed above in #146470 (comment).
Upd: I think c5221eb should be bundled into this PR and everything should be merged together.

@tmandry

Thanks for confirming @petrochenkov. So if I understand correctly, another framing we can use is that there was a bug in how we handled associated type bounds (it's a bug in that it diverged from the original pub-in-priv RFC). Together with c5221eb, this PR would be fixing that bug both in direct associated types and in RPITITs (which desugar to associated types).

@tmandry

@tmandry

@petrochenkov Has a crater run ever been done for c5221eb?

If not, could one of you include it in this PR and kick off a crater run please?

@petrochenkov

Has a crater run ever been done for c5221eb?

No

@petrochenkov @mladedav

@rustbot

⚠️ Warning ⚠️

@mladedav

I've added 14fea185a93 which seems to be the same commit, but I couldn't find c5221eb anywhere in the history or petrochenkov's fork.

@craterbot check

@craterbot

🔒 Error: you're not allowed to interact with this bot.

🔑 If you are a member of a Rust team and need access, please update rust-lang/team to grant your team or yourself access to the crater permission.
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@rust-log-analyzer

The job x86_64-gnu-gcc failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot) 


---- [pretty] tests/pretty/qpath-associated-type-bound.rs stdout ----
------rustc stdout------------------------------

------rustc stderr------------------------------
error[E0446]: private trait `Tu` in public interface
##[error]  --> <anon>:6:9
   |
 6 |         type Ts: super::Tu;
   |         ^^^^^^^^^^^^^^^^^^ can't leak private trait
...
10 | trait Tu {
   | -------- `Tu` declared as private

warning: trait `Tu` is never used
##[warning]  --> <anon>:10:7
   |
10 | trait Tu {
   |       ^^
   |
   = note: `#[warn(dead_code)]` (part of `#[warn(unused)]`) on by default

warning: function `foo` is never used
##[warning]  --> <anon>:14:4
   |
14 | fn foo<T: m::Tr>() { <T as m::Tr>::Ts::dummy(); }
   |    ^^^

warning: trait `Tr` is never used
##[warning] --> <anon>:5:15
  |
---
------------------------------------------

error: pretty-printed source does not typecheck
status: exit status: 1
command: env -u RUSTC_LOG_COLOR RUSTC_ICE="0" RUST_BACKTRACE="short" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "-" "-Zno-codegen" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/pretty/qpath-associated-type-bound/qpath-associated-type-bound.pretty-out" "--target=x86_64-unknown-linux-gnu" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/pretty" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/pretty/qpath-associated-type-bound/auxiliary.pretty" "-A" "internal_features" "--check-cfg" "cfg(test,FALSE)" "-Crpath" "-Cdebuginfo=0"
stdout: none
--- stderr -------------------------------
error[E0446]: private trait `Tu` in public interface
##[error]  --> <anon>:6:9
   |
 6 |         type Ts: super::Tu;
   |         ^^^^^^^^^^^^^^^^^^ can't leak private trait
...
10 | trait Tu {
   | -------- `Tu` declared as private

warning: trait `Tu` is never used
##[warning]  --> <anon>:10:7
   |
10 | trait Tu {
   |       ^^
   |
   = note: `#[warn(dead_code)]` (part of `#[warn(unused)]`) on by default

warning: function `foo` is never used
##[warning]  --> <anon>:14:4
   |
14 | fn foo<T: m::Tr>() { <T as m::Tr>::Ts::dummy(); }
   |    ^^^

warning: trait `Tr` is never used
##[warning] --> <anon>:5:15
  |

For more information how to resolve CI failures of this job, visit this link.

@traviscross

@rust-bors

This comment has been minimized.

rust-bors bot added a commit that referenced this pull request

Dec 10, 2025

@rust-bors

…-errors, r=

Revert "Do not check privacy for RPITIT."

@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

Labels

disposition-merge

This issue / PR is in PFCP or FCP with a disposition to merge it.

I-lang-nominated

Nominated for discussion during a lang team meeting.

I-lang-radar

Items that are on lang's radar and will need eventual work or consideration.

needs-fcp

This change is insta-stable, or significant enough to need a team FCP to proceed.

P-lang-drag-1

Lang team prioritization drag level 1. https://rust-lang.zulipchat.com/#narrow/channel/410516-t-lang

proposed-final-comment-period

Proposed to merge/close by relevant subteam, see T- label. Will enter FCP once signed off.

S-waiting-on-crater

Status: Waiting on a crater run to be completed.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

T-lang

Relevant to the language team