[lang-team-minutes] private-in-public rules

[nikomatsakis]

The last point is relevant to private associated types in generic contexts as well, because they are so similar to impl Trait. If impls participating in Bounds for these associated types are “externalized” as if they participate in impl Bounds, then we are good and don’t need any additional interface checks for associated types.

I’m confused a bit by this @petrochenkov. Let me re-summarize what I think I understood, and you tell me if I’ve got it wrong:

• It would be nice if we could use privacy to allow us to elide symbols from binaries/rlibs/metadata, confident that downstream crates will not need to know about them.
• Unfortunately, if a type returns an impl Foo, then in fact it might be a private type masquerading under there, so in some cases private types can leak out (this does not apply to leaking via an object).
  • We can however identify such cases pretty easily, no? (Well, maybe it’s a bit tricky, but plausible.)
• Similar things would then apply if it is possible to synthesize a path to a private type in generic code (e.g., <T as Trait>::Foo where T winds up being instantiated to PubType but Foo is bound to a private type), so we would have to consider all types bound in an associated type as being potentially visible from the outside.

[petrochenkov]

Let me re-summarize what I think I understood, and you tell me if I’ve got it wrong

Everything is correct

It would be nice if we could use privacy to allow us to elide symbols

Privacy is already used for this, but not so well as possible for trait impls.

[ubsan]

I’ve just been bitten by these rules. I have a program which does something like this:

enum DebugPrint {
  Print,
  NoPrint,
}

mod mir {
  pub fn build_and_write(..., print_llir: ::DebugPrint) {
    ...
  }
}

with code similar to this, I get the following warning:

warning: private type `DebugPrint` in public interface (error E0446), #[warn(private_in_public)] on by default
    --> src/mir/mod.rs:1151:3
     |
1151 |   pub fn build_and_write(mut self, output: &str, print_llir: ::DebugPrint) {
     |   ^
     |
     = 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>

I can’t do anything about this, except make DebugPrint pub, which I really don’t want to do. Until pub(restricted) is actually implemented, warning about it seems really unfortunate and mean; it sucks to see this warning, and know that I have to make my code worse in order to get rid of a warning.

If it warned when you had something reachable, that would be different, but this is just a stupid, unfixable warning.

[petrochenkov]

@ubsan

enum DebugPrint {
  Print,
  NoPrint,
}

=>

mod detail {
    pub enum DebugPrint {
      Print,
      NoPrint,
    }
}

+ maybe some reexports for convenience.

This is the official™ fix and it’s described in the tracking issue.

[ubsan]

@petrochenkov

That is not a solution. That’s a hack.

[petrochenkov]

Hey, it can’t be a hack, it is recommended in the reference since at least 2013 as an example of a privacy system “surprisingly powerful for creating module hierarchies”

---

pub(crate) is the proper solution, unfortunately delayed due to some silly syntactic issue. After late checking is implemented, private-in-public check will hopefully became reachability-based after it’s lowered to a lint, solving this problem in another way. For now helper module is the solution.

[ubsan]

It being recommended does not make it not a hack. Currently, transmute is recommended for turning an object pointer into a function pointer, that doesn’t make it not a hack .

[ubsan]

I have a suggestion. I can’t imagine pub (anything) ever working as syntax. Anything we do will syntactically conflict with something:

pub(restricted)
  -> pub (tuple,)

pub<restricted>
  -> pub <T as Trait>::Type

pub {restricted}
  -> pub {anonymous: Struct}

pub::restricted
  -> pub ::Foo

and I argue that pub(restricted) is not useful. In my experience, there are three types of public that you actually want. Private, which is denoted by no keyword; public, which is denoted by the pub keyword; and the third, which is currently denoted by pub(crate), and which I recommend be denoted by crate, “crate visibility”.

This means that it’s possible to create a :vis matcher, makes it so that we can extend syntax without worrying, and is much easier to both read and write; there are no levels of pub, there’s public, crate-level, and private. When one sees pub, you know it’s public to the world.

[DanielKeep]

crate fn boom() { panic!("Oh noes!"); }

I think that reads really nicely. One compromise alternative might be to keep the pub to make it clear that it’s a visibility modifier (and just drop the parens):

pub crate fn boom() { ... }
crate pub fn boom() { ... }

The first feels more structurally sensible, but the second reads better. But, yeah, I like the idea. I’m just embarrassed it never occurred to me to drop the parens…

Edit: After mulling over it for a while, I think I like the crate pub variant the best. pub crate looks too much like it’s a public crate fn, which implies you can have a non-public crate fn. crate pub can be read either as crate pub being a two-word token, or as crate modifying pub to make it conditionally public.

[benh]

I’ve been waiting for pub to make sense to me again since 2014. We were so close with RFC 136 before it was inverted after being merged!

[petrochenkov]

I started working on the planned privacy changes, starting with checking types (inferred types in bodies and types in signatures) for privacy.

(I’d really like to move this forward faster, but unfortunately my job moved to another office this month, so I have to spend 2 extra hours a day to get there and back, and those are exactly the hours I spent on Rust, so I’m basically left with Saturdays at best.)

[petrochenkov]

@nikomatsakis You argued for keeping early private-in-public checks for associated types, but applying late checks to everything else. What do you think about usual type aliases? Should

pub type Pub = Priv;

be caught by an early check?

---

(The rest of the post is removed as entirely incorrect, early checking for type aliases doesn’t bring the described benefits.)

[nikomatsakis]

What do you think about usual type aliases?

Good question. By analogy, I think yes.

[nikomatsakis]

So I am ready to finally take some action here. My opinions have softened quite a bit now. I still think the core idea of <= privacy is a good one to maintain, but I really think we are not getting a lot of value out of the current rules, and certainly not an amount of value that is commensurate with the pain we cause.

I think I would like to see a system where we have some minimal amount of hard errors and then use lints for the rest. Specifically, I imagine lints for:

• marking something as pub that is not reachable from crate root (per the new modules RFC)
• marking something as pub that references things that are not pub (this is now a hard error)
  • e.g., functions that return private types, public trait impls that re-export private types

I think this is pretty close to what @petrochenkov and @eddyb proposed back in the day, but I have to re-read this thread in detail to be sure.

(I might even be willing to just abandon hard privacy guarantees, and simply lint against pub use foo::PrivateThing, but maybe we don’t have to go that far. I think making the second case into a lint, in particular, would satisfy a lot of use cases.)

@petrochenkov, what do you think about this, and would you be interested in writing up such an RFC? I feel like this is an ergonomics problem for Rust that we really ought to solve.

[aturon]

I think I would like to see a system where we have some minimal amount of hard errors and then use lints for the rest.

To flesh this out a bit more, the hard error portion should provide a core guarantee around privacy, like the following:

• Fields, inherent methods, and free functions can only be accessed according to their stated privacy.
• Trait implementations (including destructors), however, are always considered as public as the trait.

With this setup, it would still be a hard error to try to use a private field from a sibling module. But it would not be a hard error to re-export a type with greater visibility than what was originally stated for it.

[withoutboats]

Your breakdown didn’t address types; it seems like in this system types would not have a hard guarantee of privacy.

This seems fine to me. I only worry about how we will explain this distinction.

[petrochenkov]

I think I would like to see a system where we have some minimal amount of hard errors and then use lints for the rest. Specifically, I imagine lints for:

• marking something as pub that is not reachable from crate root (per the new modules RFC)
• marking something as pub that references things that are not pub (this is now a hard error)

e.g., functions that return private types, public trait impls that re-export private types

That’s pretty much the plan.
I wanted to implement the second item without an RFC, it’s on my work queue for months already (the queue is moving rarely these days).
I’ll prioritize this then and write an RFC, type privacy needs documentation anyway.

simply lint against pub use foo::PrivateThing

But it would not be a hard error to re-export a type with greater visibility than what was originally stated for it.

This can’t be done without undesirable consequences (either completely out-of-place hacks or breaking globs), privacy is pretty ingrained into the import resolution algorithm.
As I mentioned previously “private and cannot be reexported” is actually a “this pub use doesn’t reexport anything” error in disguise.

[nikomatsakis]

I’ll prioritize this then and write an RFC, type privacy needs documentation anyway.

Great! I feel like some sort of RFC would be advisable. I think we could probably move swiftly here. It’s not like the public-private rules are a “much loved” part of Rust anyway: I think anything that increases flexibility would not be that controversial. =)

[petrochenkov]

@aturon
One more detail about reexports with larger visibility.
It’s totally possible to permit this code without hacks and breaking anything:

// (1)
mod foo {
    pub(crate) struct Foo;
}

pub use foo::Foo;

, BUT the import visibility will be lowered to pub(crate)

// (1) desugars into
mod foo {
    pub(crate) struct Foo;
}

pub(crate) use foo::Foo;

, this may be pretty confusing and misleading.

Glob imports behave the same way currently, but it’s less confusing with globs:

mod foo {
    pub struct Public;
    struct Private;

    pub(crate) struct Foo;
}

pub use foo::*;
// Desugars into
pub use foo::Public; // Visibility stay the same
pub(crate) use foo::Foo; // Visibility is lowered to pub(crate)
/*         nothing       */ // foo::Private, visibility is lowered to pub(foo), filtered away as too private 

[nikomatsakis]

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.