Rust: Tracking issue for `private_in_public` compatibility lint.

[petrochenkov]

What is this lint about

RFC 136 describes rules for using private types and traits in interfaces of public items. The main idea is that an entity restricted to some module cannot be used outside of that module. Private items are considered restricted to the module they are defined in, pub items are considered unrestricted and being accessible for all the universe of crates, RFC 1422 describes some more fine-grained restrictions. Note that the restrictions are determined entirely by item's declarations and don't require any reexport traversal and reachability analysis. "Used" means different things for different entities, for example private modules simply can't be named outside of their module, private types can't be named and additionally values of their type cannot be obtained outside of their module, etc. To enforce these rules more visible entities cannot contain less visible entities in their interfaces. Consider, for example, this function:

mod m {
    struct S;
    pub fn f() -> S { S } // ERROR
}

If this were allowed, then values of private type S could leave its module.

Despite the RFC being accepted these rules were not completely enforced until https://github.com/rust-lang/rust/pull/29973 fixed most of the missing cases. Some more missing cases were covered later by PRs https://github.com/rust-lang/rust/pull/32674 and https://github.com/rust-lang/rust/pull/31362. For backward compatibility the new errors were reported as warnings (lints). Now it's time to retire these warnings and turn them into hard errors, which they are supposed to be.

This issue also tracks another very similar lint, pub_use_of_private_extern_crate, checking for a specific "private-in-public" situation.

How to fix this warning/error

If you _really_ want to use some private unnameable type or trait in a public interface, for example to emulate sealed traits, then there's a universal workaround - make the item public, but put it into a private inner module.

mod m {
    mod detail {
        pub trait Float {}
        impl Float for f32 {}
        impl Float for f64 {}
    }
    pub fn multiply_by_2<T: detail::Float>(arg: T) { .... } // OK
}

The trait Float is public from the private-in-public checker's point of view, because it uses only local reasoning, however Float is unnameable from outside of m and effectively private, because it isn't actually reexported from m despite being potentially reexportable.
You'll also have to manually document what kind of mystery set of arguments your public function multiply_by_2 accepts, because unnameable traits are effectively private for rustdoc.

Current status

• [x] https://github.com/rust-lang/rust/pull/29973 introduces the private_in_public lint as warn-by-default
• [x] https://github.com/rust-lang/rust/pull/34206 makes the private_in_public lint deny-by-default
• [x] https://github.com/rust-lang/rust/pull/36270 makes the private_in_public lint warn-by-default again due to relatively large amount of breakage
• [ ] PR ? makes the private_in_public lint deny-by-default
• [ ] PR ? makes the private_in_public lint a hard error

[petrochenkov]

Making private_in_public an error also fixes https://github.com/rust-lang/rust/issues/28514

[petrochenkov]

Current status of crates from the regression report:

:full_moon: encoding - fixed, published
:first_quarter_moon: rotor - fixed, unpublished
:new_moon: ioctl - pr sent, not merged
:full_moon: genmesh - fixed, published
:full_moon: daggy - fixed, published
:first_quarter_moon: rust-locale - fixed, unpublished
:first_quarter_moon: iota-editor - fixed, unpublished
:new_moon: json_rpc - pr sent, not merged
:new_moon_with_face: couchdb - can't reproduce, other errors
:full_moon: libxdo - fixed, published
:new_moon: cuticula - pr sent, not merged
:full_moon: peano - fixed, published
:first_quarter_moon: plumbum - fixed, unpublished
:full_moon: postgis - fixed, published
:first_quarter_moon: probor - fixed, unpublished
:full_moon: endianrw - fixed, published
:full_moon: rodio - fixed, published
:full_moon: rose_tree - fixed, published
:first_quarter_moon: rustpather - fixed, unpublished
:new_moon_with_face: fbx_direct - can't reproduce, other errors
:full_moon: shuffled-iter - fixed, published
:full_moon: flexmesh - fixed, published
:full_moon: skeletal_animation - fixed, published
:first_quarter_moon: stdx - fixed, unpublished
:first_quarter_moon: tick - fixed, unpublished
:full_moon: glitter - fixed, published

[eddyb]

@petrochenkov @nikomatsakis Any chance of flipping the switch to hard error this year?
I'm asking because I'm having to do some node ID gymnastics to support the old error/new lint system.
OTOH, I should do a crater run, since I'm moving it to semantic types, so I can try just not checking bounds.

[petrochenkov]

@eddyb

Any chance of flipping the switch to hard error this year?

Not without implementing https://github.com/rust-lang/rfcs/pull/1671 (or rather https://internals.rust-lang.org/t/rfc-reduce-false-positives-for-private-type-in-public-interface/3678) first, I suppose.
After that the lint can hopefully be turned to an error, or at least the old checker can be dropped.

[nikomatsakis]

Gah, I've been meaning to start a thread to discuss this topic in more depth! I had completely forgotten.

[eddyb]

@nikomatsakis You remembered just in time! :laughing:
I've just opened #37676 and will do a crater run on it and on a version that errors and ignores bounds.

[eddyb]

Crater report shows 32 root regressions (out of which log-panics and sdl2_gfx are false positives).
@petrochenkov Even if it's ignoring bounds, the fallout looks pretty bad. If you have time, could you look over the ones which didn't show up on previous runs? When the error is in a dependency, that's usually due to outdated dependencies + --cap-lints allow, but there could be new cases I triggered.

Some of them are old versions of xml-rs (0.1.* and 0.2.*) and nix (0.4.*) - couldn't their authors release new patch versions pub-ifying all relevant types? cc @netvl @carllerche

[petrochenkov]

I'll look at the fallout today or tomorrow.
The problem is that people sometimes use allow(private_in_public) deliberately and the previous run was done after making private_in_public deny-by-default, so it didn't caught those cases.

[eddyb]

:disappointed: Would it then make sense to use forbid as a default, at least for crater runs?

[petrochenkov]

Using forbid for crater runs looks like a good idea.

I've looked through about half of regressions, all looks legitimate and many are indeed due to nix and xml-rs.
As a side note, error spans are pretty bad - they point to a whole item, it's not clear what exactly is private, especially if it's on some other line.

[petrochenkov]

There may be another solution to all this private-in-public problem.

Implement https://github.com/rust-lang/rust/issues/30476 and make reporting of this kind of privacy errors late, as opposed to early/preventive as it is today. I expect less fallout from this, because things like this

mod m {
    struct S;
    mod n {
        pub fn f() -> super::S { super::S }
    }

    fn g() {
        les s = n::f();
    }
}

won't report errors.

All private-in-public errors in rustc_privacy (but not in resolve!) could be lowered to lints then, because early reporting is still useful for catching silly human errors.

[eddyb]

Yeah, that's a bit of a pickle. Not sure what the optimal solution is there, nothing obvious works.
One magical way would be to associate "constructors" to each syntactical type node, a type parametrized by the syntactical sub-nodes, so that leaves can be distinguished from types coming "from elsewhere".

But that doesn't scale to resolutions of associated types through where clauses, which go through the trait machinery, and at that point you'd have to negate all the benefits of uniform semantical types to get accurate location information for everything.

[eddyb]

Oh, #30476 looks interesting. We can do it uniformly in inference writeback, is intra-function enough?

[petrochenkov]

We can do it uniformly in inference writeback, is intra-function enough?

A privacy pass simply needs to go through all HIR nodes and check their types^* (somehow avoiding duplicated errors, but that's details).
Some nodes are intra-function, some are in item interfaces. Not sure what are consequences exactly, but looks like it can't always be done through function contexts.

^* At least adjusted types, not sure about non-adjusted.

[eddyb]

Ah, right, if you do it on the nodes after inference you can do it at any point after typeck.

The reason why I was thinking of doing something during typeck is because I imagined one could avoid duplicated errors by being able to tell exactly when the private type "entered" the function.
OTOH, something like associated type projection normalization would muddle the water _anyway_.

[eddyb]

I'm going to do a crater run with either the privacy pass or writeback, checking the types (whichever is easier to hack together, although the privacy pass should have everything already in place).

[eddyb]

Crater report for expressions' & patterns' type privacy checking shows 7 regressions:

• [ ] [locale](https://tools.taskcluster.net/task-inspector/#E5gtIeL4RqCJYfSd4yW3OQ) (error log)
• [ ] [couchdb](https://tools.taskcluster.net/task-inspector/#my5cvu4YR3qLVKYxPRsmwA) (error log)
• [ ] [mpd](https://tools.taskcluster.net/task-inspector/#zmf6eXK6TDmrDMM_MIBoIw) (error log)
• [ ] [rgmk](https://tools.taskcluster.net/task-inspector/#nw0vQp57T0SnFpByivHyew) (error log)
• [ ] [rules](https://tools.taskcluster.net/task-inspector/#6oVytuWCSPaIo78yofu7Aw) (error log)
• [ ] [rustpather](https://tools.taskcluster.net/task-inspector/#q-9LhCpqR4uybGLg-Tut_w) (error log)
• [ ] [carboxyl](https://tools.taskcluster.net/task-inspector/#2VakvfEFSnuxB9L4W5njEw) (error log)

The last one is actually carboxyl_time depending on the outdated carboxyl version 0.1.2.
This is far less than 32 and appears actually manageable if we want to enforce it.

For the record, the entire implementation (minus test fixes) can be found at https://github.com/eddyb/rust/commit/3c914a6825be5db77a40f92bb881b65554ff2afd.
You can cherry-pick that on top of #37676 to play with it yourself (I hope I didn't miss anything in it).

EDIT: I forgot that for projections to be public iff the trait and Self/parameters are all public, all associated types also have to be enforced (and right now they're still under the lint).
This might have an effect, although I'm not sure on what scale or what the right way to detect it is.

[petrochenkov]

Excellent!
All regressions look like consequences of private-in-public not always being a hard error, not weird cases described in https://github.com/rust-lang/rust/issues/30476 and https://github.com/rust-lang/rust/issues/30476 and uncatchable by private-in-public.
Many of them also look familiar because they involve crates from this list for which authors were not willing to merge fixes or publish updated versions.

[nikomatsakis]

We discussed in @rust-lang/lang meeting. Consensus was that we should write-up the plan to do "minimal" enforcement for errors:

• check at use-site that you don't use private types
• check that you don't leak private types in "public" impls
  
  
  
  • Question: is that adequate? Can you not use private types in any impls?
  
  

We would still want lints that can try to help you find coverage gaps in your definitions at the definition site.

[petrochenkov]

@nikomatsakis

check at use-site that you don't use private types

Formally, this + reexport checking in resolve would be enough to enforce the basic guarantee "private things can't leave their module".
Everything else can be turned into a lint (probably deny-by-default) that shouldn't necessarily be completely precise and can use reachability and maybe even heuristics.

check that you don't leak private types in "public" impls

I don't understand what you mean.
"types" as opposed to traits? why only impls? what "public" in quotes means?

I wanted to write an RFC describing this scheme and replacing https://github.com/rust-lang/rfcs/pull/1671 + write and implementation, but I barely have time for anything beyond answering the mail currently :(

[aturon]

Note: there are some recent thoughts in a new internals thread.

[nikomatsakis]

@petrochenkov

Maybe best to carry this on the internals thread, but what did you mean by "reexport checking in resolve" when you wrote:

Formally, this + reexport checking in resolve would be enough to enforce the basic guarantee "private things can't leave their module".

(I think that (as I explained in the internals thread) we do need some checking for associated types in impls, at least.)

[vberger]

I don't know how much this is expected/already known, but the lint triggers in cases like this one:

pub mod foo {
    struct Private;

    mod internal {
        pub type Bar = Vec<super::Private>;
    }
}

with

warning: private type `foo::Private` in public interface (error E0446)
 --> src/lib.rs:7:9
  |
7 |         pub type Bar = Vec<super::Private>;
  |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: #[warn(private_in_public)] on by default
  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
  = note: for more information, see issue #34537 <https://github.com/rust-lang/rust/issues/34537>

Though here, relative to module foo, Private and internal::Bar are both private. As such, I find this surprising that the lints triggers on this.

[petrochenkov]

@vberger
This is expected behavior at the moment (Bar is pub, Private is pub(in foo), pub > pub(in foo) => private-in-public error), but it will change soon and this specific case won't be reported.

[nox]

How the hell is pub_use_of_private_extern_crate a "private-in-public" situation? How does one reexport a crate now? Why are we deprecating that without an epoch? That's a breaking change that will break existing code.

[nox]

I am aware one can now use pub extern crate, but that can mean having to bump the minimum Rust version in these crates for absolutely no real reason.

[petrochenkov]

@nox

How the hell is pub_use_of_private_extern_crate a "private-in-public" situation?

Private items cannot be publicly reexported, that's a general rule that wasn't properly enforced for crate items in early versions of rustc. The rule is pretty fundamental for the import resolution algorithm.

How does one reexport a crate now?

Using pub extern crate.

Why are we deprecating that without an epoch? That's a breaking change that will break existing code.

Plenty of bugfixes breaking small numbers of crates were gradually landed in the previous years without epochs, using deprecation periods. The official policy is https://github.com/rust-lang/rfcs/blob/master/text/1122-language-semver.md

Regarding pub_use_of_private_extern_crate specificlly, in June crater run found 7 crates on crates.io exploiting this bug and I sent PRs with fixes to all of them. So the ecosystem in general is not affected.
If it affects something not on crates.io, e.g. Servo, there's still plenty of time to fix it, pub_use_of_private_extern_crate is still a lint and won't break dependencies of affected crates, I didn't plan making it a hard error until the next June or so, it could be delayed even further given sufficient demand.

[SimonSapin]

Not all Rust code in the world is in crates.io, and we don’t know how representative crates.io is. Even with several months of warning, making breaking changes that are not soundness fixes without an epoch discredits our stability promise. This one feels particularly frivolous.

[roblabla]

The lint is currently warning on the following code : https://play.rust-lang.org/?gist=398cc0302ea20d2781d62db6deb53d97&version=nightly&mode=debug

Is this a bug ? If not, is the reasoning explained anywhere ? I feel like implementing a public trait on private traits shouldn't cause the warning ?

[petrochenkov]

@roblabla
This is expected behavior in the currently implemented rules, because the rules are pretty dumb - "something private anywhere in the interface -> report an error/warning".

In revised (but not yet implemented) rules this is a lint that's probably going to be allow-by-default, because this is still kind of a documentation issue (how do you explain to the library users what trait Public is implemented for?) that people may want to catch.

[bkchr]

Hey, I got the following code: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=454c3de6b22b250e5824d8efd9a2d049

What would be the correct way to solve this problem? And yes I want to have Bar<T>: Clone in the where clause :D

[petrochenkov]

@bkchr

mod m {
    mod implementation_details {
        #[derive(Clone)]
        pub struct Bar<T> {
            data: T,
        }   
    }
    use implementation_details::*;

    pub struct Foo<T> {
        data: Bar<T>,
    }

    impl<T> Clone for Foo<T> where Bar<T>: Clone {
        fn clone(&self) -> Self {
            Self {
                data: self.data.clone()
            }
        }
    }
}

However, the original code (private type in a where clause) will "just work" once https://github.com/rust-lang/rust/issues/48054 is implemented (hopefully this year).

[bkchr]

@petrochenkov yeah I was aware that this probably works. This example was just for illustration, the second derive is actually also generated and will annoy people to get this warning. I probably need to disable the warning for this then.

[derekdreery]

I think I'm getting a false positive in my code.

The only thing I export from a library is a proc macro, but the lint is firing. It is coming from something like

struct Private;

mod module {
    pub fn my_fn() -> Result<(), super::Private> {
        //...
    }
}

pub fn another_fn() {
    // use module::my_fn & Private here
}

so the "public" type Private is actually not public at all.

If you need the actual code this is in I can stick it on github.

[petrochenkov]

@derekdreery
This works as expected currently - the rules are based on nominal visibility rather than reachability, so pub in my_fn prevents non-pub entities from appearing in its interface.
There's an accepted but unimplemented RFC that is supposed to fix cases like this.

[derekdreery]

Thanks for replying!

I've worked round it for now using pub(crate). Hopefully this message will reach anyone with the same issue.

[Matthias247]

I just got this error today, and I am not sure whether this is as intended or a bug. The error shows up in this example

enum Xyz {
    A,
    B,
}

mod sub {
    mod sub_a {
        use crate::Xyz;
        pub fn x() -> Xyz { Xyz::A }
    }

    pub use sub_a::x;
}

playground

It complains about Xyz being leaked, even though the place where it is used (module sub) isn't exported to the public.

If I do the following change and place Xyz in a submodule and import it from there it doesn't complain anymore, even though it seems like neither the visibility of Xyz or sub changed:

mod xyz_module {
    pub enum Xyz {
        A,
        B,
    }
}
use xyz_module::Xyz;

mod sub {
    mod sub_a {
        use crate::Xyz;
        pub fn x() -> Xyz { Xyz::A }
    }

    pub use sub_a::x;
}

playground

[CWood1]

Is there a recommended workaround/method for inspecting private fields of structs in tests? I have, loosely speaking, the following:

struct A {
    // a bunch of stuff
}

pub struct B {
    // a bunch of stuff
    the_a: A,
}

impl B {
    // a bunch of stuff
    #[cfg(test)]
    pub fn test_the_a(&self) -> &A {
        self.the_a.as_ref()
    }
}

A depends on a whole tree of private types only in the B package (so returning member vars is also pretty impractical), and I'm trying to access A for inspection in the unit tests in the A module. Is there a simple way, to avoid this error, before it becomes a breaking issue?

[Kixunil]

@CWood1 I just slap pub(crate) on those fields, unless unsafe is involved. It's not ideal, but it works.