Relax priv-in-pub lint on generic bounds and where clauses of trait impls.

[jswrenn]

The priv-in-pub lint is a legacy mechanism of the compiler, supplanted by a reachability-based type privacy analysis. This PR does not relax type privacy; it only relaxes the lint (as proposed by the type privacy RFC) in the case of trait impls.

Current Behavior

On public trait impls, it's currently an error to have a where bound constraining a private type with a trait:

pub trait Trait {}
pub struct Type {}

struct Priv {}
impl Trait for Priv {}

impl Trait for Type
where
    Priv: Trait // ERROR
{}

...and it's a warning to have have a public type constrained by a private trait:

pub trait Trait {}
pub struct Type {}

pub struct Pub {}
trait Priv {}
impl Priv for Pub {}

impl Trait for Type
where
    Pub: Priv // WARNING
{}

This lint applies to where clauses in other contexts, too; e.g. on free functions:

struct Priv<T>(T);
pub trait Pub {}
impl<T: Pub> Pub for Priv<T> {}

pub fn function<T>()
where
    Priv<T>: Pub // WARNING
{}

These constraints could be relaxed without issue.

New Behavior

This lint is relaxed for where clauses on trait impls, such that it's okay to have a where bound constraining a private type with a trait:

pub trait Trait {}
pub struct Type {}

struct Priv {}
impl Trait for Priv {}

impl Trait for Type
where
    Priv: Trait // OK
{}

...and it's okay to have a public type constrained by a private trait:

pub trait Trait {}
pub struct Type {}

pub struct Pub {}
trait Priv {}
impl Priv for Pub {}

impl Trait for Type
where
    Pub: Priv // OK
{}

Rationale

While the priv-in-pub lint is not essential for soundness, it can help programmers avoid pitfalls that would make their libraries difficult to use by others. For instance, such a lint is useful for free functions; e.g. if a downstream crate tries to call the function in the previous snippet in a generic context:

fn callsite<T>()
where
    Priv<T>: Pub // ERROR: omitting this bound is a compile error, but including it is too
{
    function::<T>()
}

...it cannot do so without repeating function's where bound, which we cannot do because Priv is out-of-scope. A lint for this case is arguably helpful.

However, this same reasoning doesn't hold for trait impls. To call an unconstrained method on a public trait impl with private bounds, you don't need to forward those private bounds, you can forward the public trait:

mod upstream {
    pub trait Trait {
        fn method(&self) {}
    }
    pub struct Type<T>(T);
    
    pub struct Pub<T>(T);
    trait Priv {}
    impl<T: Priv> Priv for Pub<T> {}
    
    impl<T> Trait for Type<T>
    where
        Pub<T>: Priv // WARNING
    {}
}

mod downstream {
    use super::upstream::*;
    
    fn function<T>(value: Type<T>)
    where
        Type<T>: Trait // <- no private deets!
    {
        value.method();
    }
}

This PR only eliminates the lint on trait impls. It leaves it intact for all other contexts, including trait definitions, inherent impls, and function definitions. It doesn't need to exist in those cases either, but I figured I'd first target a case where it's mostly pointless.

Other Notes

• See discussion on zulip.
• This PR effectively reverts Add error message for private fn #79291.

[rust-highfive]

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

[petrochenkov]

r? @petrochenkov

[petrochenkov]

I'm going to send this to the lang team for sanity checking.

This PR removes the private-in-public warnings from generic bounds and where clauses on impl items specifically.

• The behavior introduced by this PR matches the "Type privacy" RFC precisely if the private_bounds lint (https://github.com/rust-lang/rfcs/blob/master/text/2145-type-privacy.md#lint-2-private-traitstypes-in-secondary-interface-of-effectively-public-items) ends up being allow-by-default (which seems somewhat reasonable), or will similarly not be reported for impl items specifically.
• The behavior introduced by this PR will differ from the "Type privacy" RFC, if the private_bounds lint ends up being warn-by-default, in that case code allowed by this PR will start reporting warnings.

[nikomatsakis]

@petrochenkov I cannot quite tell from your comment what your take is here -- do you feel this is in the spirit of the original RFC? In what way does it differ? It seems surprising to me that you would say it conforms or not to the RFC based on whether the lint in the RFC is allow by default.

[petrochenkov]

do you feel this is in the spirit of the original RFC?

It's in the spirit of the RFC.
The RFC is just not entirely precise

This lint replaces part of private-in-public errors and can be reported as warn-by-default or allow-by-default.

Back then I wanted to start with warn-by-default to get some experience, but maybe be we already have enough experience to just allow it now.

[jswrenn]

This PR also draws a finer distinction than the proposed Lint #2 makes: this PR distinguishes between private items in the bounds of trait impls, and private items in other bounds. I describe the rationale for this distinction in the description of this PR, and this PR only impacts private items in the where bounds of trait impls.

---

@scottmcm suggested that all uses of private items in bounds should be allowed, but this PR doesn't go that far. I'm inclined to agree, though. The only danger of private items in bounds is that end-users can't 'forward' such bounds when they're writing generic code. But:

• not all bounds need to be forwarded
• you can already write un-forwardable bounds, just by adding a tad of indirection
• sometimes unforwardable bounds are desirable

So, the lint proposed proposed by the type privacy RFC would just a heuristic that sometimes helps avoid an API design decision that sometimes is a pitfall. There's enough "sometimes"s in there, that I'd argue such a lint would probably be more at home in clippy than rustc.

[scottmcm]

FWIW, I like the middle-ground proposed here. The OP did a good job convincing me that the "you can forward the public trait" is an interesting distinction. We can always remove more cases later.

[scottmcm]

We didn't have time to go into this in detail in today's lang team meeting, so let's try to make progress async:

@rfcbot fcp merge

The OP here has a good description of what this allows. People seem to want to do this enough that the workarounds are well-known; I think we should just allow it directly.

[rfcbot]

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

• @cramertj
• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm

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.

[nikomatsakis]

@rfcbot reviewed

[rfcbot]

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

[rfcbot]

The final comment period, with a disposition to merge, 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.

This will be merged soon.

[petrochenkov]

r=me after squashing commits and removing accidental changes from the resulting commit.

[petrochenkov]

@bors r+

[bors]

📌 Commit ebef8a8 has been approved by petrochenkov

[bors]

⌛ Testing commit ebef8a8 with merge 1c74c60a963806a73f5fcb12ebf3c7cf1e8b7b16...

[bors]

💔 Test failed - checks-actions

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain)

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

[ehuss]

@bors retry

network error