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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type alias can be used to bypass privacy check #28450

Closed
seanmonstar opened this Issue Sep 16, 2015 · 32 comments

Comments

Projects
None yet
@seanmonstar
Contributor

seanmonstar commented Sep 16, 2015

struct Priv;

pub use self::private::public;

mod private {
    pub type Priv = super::Priv;

    pub fn public(_x: Priv) {
    }
}

The function public is exported, accepting the type Priv, even though Priv is not exported.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Sep 17, 2015

Member

triage: I-nominated

cc @nrc, @rust-lang/lang

Member

alexcrichton commented Sep 17, 2015

triage: I-nominated

cc @nrc, @rust-lang/lang

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Sep 17, 2015

Member

The workings around this were always a little subtle to me, so I'm not 100% sure if this is working as intended or just a bug where we didn't follow a type where we should.

Note that a fix for this (if we want one) is likely to have a lot of fallout so we'd definitely want to run it through crater to select a mitigation strategy.

Member

alexcrichton commented Sep 17, 2015

The workings around this were always a little subtle to me, so I'm not 100% sure if this is working as intended or just a bug where we didn't follow a type where we should.

Note that a fix for this (if we want one) is likely to have a lot of fallout so we'd definitely want to run it through crater to select a mitigation strategy.

@sfackler

This comment has been minimized.

Show comment
Hide comment
@sfackler

sfackler Sep 17, 2015

Member

core-foundation-rs uses this to define opaque pointer type definitions for FFI bindings: https://github.com/servo/core-foundation-rs/blob/master/src/array.rs#L42-L44, so there will be some fallout to take care of if the logic is updated.

Member

sfackler commented Sep 17, 2015

core-foundation-rs uses this to define opaque pointer type definitions for FFI bindings: https://github.com/servo/core-foundation-rs/blob/master/src/array.rs#L42-L44, so there will be some fallout to take care of if the logic is updated.

@nrc

This comment has been minimized.

Show comment
Hide comment
@nrc

nrc Sep 17, 2015

Member

I think this (that a type alias can increase visibility) is a feature, not a bug. The fact that we can re-export functions without re-exporting all the types they refer to is unfortunate, but kind of intentional. The last time this kind of thing was discussed, we decided that we would not try to be complete in the checking of private types in public signatures, since it is way too complex and we'd only try to enforce a syntactic distinction, rather than taking reachability, etc. into account.

Member

nrc commented Sep 17, 2015

I think this (that a type alias can increase visibility) is a feature, not a bug. The fact that we can re-export functions without re-exporting all the types they refer to is unfortunate, but kind of intentional. The last time this kind of thing was discussed, we decided that we would not try to be complete in the checking of private types in public signatures, since it is way too complex and we'd only try to enforce a syntactic distinction, rather than taking reachability, etc. into account.

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Sep 17, 2015

Contributor

@sfackler

This does not, in fact, offer any real protection:

trait Pointer { type Pointee; }
impl<T:?Sized> Pointer for *const T { type Pointee = T; }
type __CFArray = <CFArrayRef as Pointer>::Pointee;
Contributor

arielb1 commented Sep 17, 2015

@sfackler

This does not, in fact, offer any real protection:

trait Pointer { type Pointee; }
impl<T:?Sized> Pointer for *const T { type Pointee = T; }
type __CFArray = <CFArrayRef as Pointer>::Pointee;
@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Sep 17, 2015

Member

@nrc yeah that was my vague recollection as well, but I thought the rule we were going for was "if this isn't pub you know it's not reachable", which isn't true in this example.

Member

alexcrichton commented Sep 17, 2015

@nrc yeah that was my vague recollection as well, but I thought the rule we were going for was "if this isn't pub you know it's not reachable", which isn't true in this example.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Sep 17, 2015

Contributor

I would expect a pub type Foo = Bar to enforce the same rules as pub use Bar or pub fn f(b: Bar).

Contributor

nikomatsakis commented Sep 17, 2015

I would expect a pub type Foo = Bar to enforce the same rules as pub use Bar or pub fn f(b: Bar).

@eefriedman

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Sep 17, 2015

Contributor

thematically related to #28325, at least

Contributor

nikomatsakis commented Sep 17, 2015

thematically related to #28325, at least

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Sep 17, 2015

Contributor

triage: P-high -- backwards compat

Contributor

nikomatsakis commented Sep 17, 2015

triage: P-high -- backwards compat

@eefriedman

This comment has been minimized.

Show comment
Hide comment
@eefriedman

eefriedman Sep 17, 2015

Contributor

This is trivial to fix in some sense... but the usage of a type alias isn't really integral to the original testcase. Consider:

#![crate_type="lib"]
mod b {
    mod a {
        pub struct S1;
        pub struct S2(pub S1);
    }
    pub use self::a::S2;
}

pub fn g(s : b::S2) {
    let s = s.0; // Oops, I can't name the type of the field.
}
Contributor

eefriedman commented Sep 17, 2015

This is trivial to fix in some sense... but the usage of a type alias isn't really integral to the original testcase. Consider:

#![crate_type="lib"]
mod b {
    mod a {
        pub struct S1;
        pub struct S2(pub S1);
    }
    pub use self::a::S2;
}

pub fn g(s : b::S2) {
    let s = s.0; // Oops, I can't name the type of the field.
}
@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Sep 18, 2015

Contributor

@eefriedman that is ok, because the struct was declared pub. We do not
try to guarantee that you can name the types, but we do try to guarantee
that if something is not declared pub, it will not be accessible from
outside the current module.

On Thu, Sep 17, 2015 at 7:51 PM, eefriedman notifications@github.com
wrote:

This is trivial to fix in some sense... but the usage of a type alias
isn't really integral to the original testcase. Consider:

#![crate_type="lib"]mod b {
mod a {
pub struct S1;
pub struct S2(pub S1);
}
pub use self:🅰️:S2;
}
pub fn g(s : b::S2) {
let s = s.0; // Oops, I can't name the type of the field.
}


Reply to this email directly or view it on GitHub
#28450 (comment).

Contributor

nikomatsakis commented Sep 18, 2015

@eefriedman that is ok, because the struct was declared pub. We do not
try to guarantee that you can name the types, but we do try to guarantee
that if something is not declared pub, it will not be accessible from
outside the current module.

On Thu, Sep 17, 2015 at 7:51 PM, eefriedman notifications@github.com
wrote:

This is trivial to fix in some sense... but the usage of a type alias
isn't really integral to the original testcase. Consider:

#![crate_type="lib"]mod b {
mod a {
pub struct S1;
pub struct S2(pub S1);
}
pub use self:🅰️:S2;
}
pub fn g(s : b::S2) {
let s = s.0; // Oops, I can't name the type of the field.
}


Reply to this email directly or view it on GitHub
#28450 (comment).

@aturon aturon assigned aturon and unassigned nrc Oct 8, 2015

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Nov 5, 2015

Member

I'm curious whether we expect the following to be allowed:

type Foo = u8;
pub fn foo(f: Foo) {}

That is, if you use a private type alias to reference a public type in a signature -- is that OK? Currently the snippet fails to compile. Fixing the original issue turns up some cases within std where private aliases to public types are being used.

Member

aturon commented Nov 5, 2015

I'm curious whether we expect the following to be allowed:

type Foo = u8;
pub fn foo(f: Foo) {}

That is, if you use a private type alias to reference a public type in a signature -- is that OK? Currently the snippet fails to compile. Fixing the original issue turns up some cases within std where private aliases to public types are being used.

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Nov 5, 2015

Contributor

@aturon
type is supposed to be equivalent to use in its abilities (at least this is how EmbargoVisitor works now)

struct Private;
pub use Private as Public; // Makes `Private` exported, now it can be used in external interfaces
pub type Public = Private; // Makes `Private` exported, now it can be used in external interfaces

However, use have one artificial restriction - it can reexport only "shallowly public" types, i.e. marked with pub, but possibly residing in private modules. type doesn't have this restriction. This restriction should either be imposed on both use and type or on none of them.
Second, unlike for uses, VisiblePrivateTypesVisitor don't go down the chain of type aliases to check the exported-ness of the original type, but it should. VisiblePrivateTypesVisitor is not well aligned with other parts of privacy in general.

(I'm currently rewriting all this, I've finished EmbargoVisitor and going to proceed to VisiblePrivateTypesVisitor soon)

Contributor

petrochenkov commented Nov 5, 2015

@aturon
type is supposed to be equivalent to use in its abilities (at least this is how EmbargoVisitor works now)

struct Private;
pub use Private as Public; // Makes `Private` exported, now it can be used in external interfaces
pub type Public = Private; // Makes `Private` exported, now it can be used in external interfaces

However, use have one artificial restriction - it can reexport only "shallowly public" types, i.e. marked with pub, but possibly residing in private modules. type doesn't have this restriction. This restriction should either be imposed on both use and type or on none of them.
Second, unlike for uses, VisiblePrivateTypesVisitor don't go down the chain of type aliases to check the exported-ness of the original type, but it should. VisiblePrivateTypesVisitor is not well aligned with other parts of privacy in general.

(I'm currently rewriting all this, I've finished EmbargoVisitor and going to proceed to VisiblePrivateTypesVisitor soon)

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Nov 5, 2015

Contributor

Another very simpe hole in VisiblePrivateTypesVisitor is

mod private {
    pub struct ShallowlyPublic;
}

// Compiles!
// Value of non-exported type is available to other crates -> hello link time errors.
pub fn f() -> private::ShallowlyPublic {
    private::ShallowlyPublic
}
Contributor

petrochenkov commented Nov 5, 2015

Another very simpe hole in VisiblePrivateTypesVisitor is

mod private {
    pub struct ShallowlyPublic;
}

// Compiles!
// Value of non-exported type is available to other crates -> hello link time errors.
pub fn f() -> private::ShallowlyPublic {
    private::ShallowlyPublic
}
@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Nov 5, 2015

Member

@petrochenkov OK, thanks for the heads up that you're doing a rewrite. I was going to look at a couple issues in this vicinity as well, but maybe you can factor them in:

The current goal is to get all of this checking in line with the relevant RFC. We may want to loosen some of the rules, eventually, but for now we should hew toward the conservative spec. The fundamental guarantee in the spec is that a type definition may only escape a module if it is marked pub. It's a bit less clear about aliases.

I definitely agree that use and type should have the same privacy restrictions.

Member

aturon commented Nov 5, 2015

@petrochenkov OK, thanks for the heads up that you're doing a rewrite. I was going to look at a couple issues in this vicinity as well, but maybe you can factor them in:

The current goal is to get all of this checking in line with the relevant RFC. We may want to loosen some of the rules, eventually, but for now we should hew toward the conservative spec. The fundamental guarantee in the spec is that a type definition may only escape a module if it is marked pub. It's a bit less clear about aliases.

I definitely agree that use and type should have the same privacy restrictions.

@seanmonstar

This comment has been minimized.

Show comment
Hide comment
@seanmonstar

seanmonstar Nov 6, 2015

Contributor

I definitely agree that use and type should have the same privacy restrictions.

I'm not so sure. Here's a few examples:

type Foo = Arc<Mutex<io::Read + Send + 'static>>;
pub fn foo(x: Foo) {}

In this example, I've found myself wanting to use a type alias, but I haven't wanted to export the type (and thus write docs for it).

pub use self::inner::Foo;
pub type Bar = self::inner::Bar;
mod inner {
    struct Foo;
    struct Bar;
}

In this example, I feel that the re-export should be valid, but the pub type Bar should error, because it's trying to expose something private. If you wanted to re-export Bar, you could have done pub use.


I would expect pub type Foo = Bar to basically be 'unwrapped' during the privacy check, and be treated as pub struct Foo(pub Bar);. If Bar is not public, that type should be a privacy error.

Any desire to use type as a re-exporting mechanism should use pub use instead. Have only 1 way to do things.

Contributor

seanmonstar commented Nov 6, 2015

I definitely agree that use and type should have the same privacy restrictions.

I'm not so sure. Here's a few examples:

type Foo = Arc<Mutex<io::Read + Send + 'static>>;
pub fn foo(x: Foo) {}

In this example, I've found myself wanting to use a type alias, but I haven't wanted to export the type (and thus write docs for it).

pub use self::inner::Foo;
pub type Bar = self::inner::Bar;
mod inner {
    struct Foo;
    struct Bar;
}

In this example, I feel that the re-export should be valid, but the pub type Bar should error, because it's trying to expose something private. If you wanted to re-export Bar, you could have done pub use.


I would expect pub type Foo = Bar to basically be 'unwrapped' during the privacy check, and be treated as pub struct Foo(pub Bar);. If Bar is not public, that type should be a privacy error.

Any desire to use type as a re-exporting mechanism should use pub use instead. Have only 1 way to do things.

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Nov 6, 2015

Contributor

@seanmonstar
Removing reexporting through type is certainly a variant, even the RFC says "If it is a type declaration, items referred to in its definition must be public.", but it's not the current behavior, and some people in the discussion above consider it a feature, so the breakage will have to be estimated and the people convinced.

Contributor

petrochenkov commented Nov 6, 2015

@seanmonstar
Removing reexporting through type is certainly a variant, even the RFC says "If it is a type declaration, items referred to in its definition must be public.", but it's not the current behavior, and some people in the discussion above consider it a feature, so the breakage will have to be estimated and the people convinced.

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Nov 6, 2015

Contributor

@seanmonstar
I'd personally keep the reexporting through type because it's more powerful than use due to generic parameters, for example you can do things like

// Stolen from http://en.cppreference.com/w/cpp/numeric/random, we don't have integer generic parameters yet
pub type mt19937 = mersenne_twister_engine<u32, 32, 624, 397, 31, 
                             0x9908b0df, 11, 
                             0xffffffff, 7, 
                             0x9d2c5680, 15, 
                             0xefc60000, 18, 1812433253>

without allowing users to change the parameters by themselves.

Contributor

petrochenkov commented Nov 6, 2015

@seanmonstar
I'd personally keep the reexporting through type because it's more powerful than use due to generic parameters, for example you can do things like

// Stolen from http://en.cppreference.com/w/cpp/numeric/random, we don't have integer generic parameters yet
pub type mt19937 = mersenne_twister_engine<u32, 32, 624, 397, 31, 
                             0x9908b0df, 11, 
                             0xffffffff, 7, 
                             0x9d2c5680, 15, 
                             0xefc60000, 18, 1812433253>

without allowing users to change the parameters by themselves.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Nov 6, 2015

Contributor

@petrochenkov

Another very simpe hole in VisiblePrivateTypesVisitor is ...

I don't think this is a hole. Rather, I think it is legal to pub use a public type from a private module. As I understand the rules, they are very simple: if an item is declared as pub, then it is considered public to the entire outside world and can thus be re-exported by anyone who can reach it.

(I happen to think we should extend this system somewhat, probably by allowing you to limit pub declarations to some enclosing modules, but this is the system we have today.)

However, use have one artificial restriction - it can reexport only "shallowly public" types, i.e. marked with pub, but possibly residing in private modules. type doesn't have this restriction. This restriction should either be imposed on both use and type or on none of them.

I don't understand this restriction, can you clarify what it is? I think probably both pub use and pub type should be limited to items declared as pub, and it doesn't really matter whether the modules they are declared in are private or public, so long as they are accessible to the one who declared the pub use and pub type.

Contributor

nikomatsakis commented Nov 6, 2015

@petrochenkov

Another very simpe hole in VisiblePrivateTypesVisitor is ...

I don't think this is a hole. Rather, I think it is legal to pub use a public type from a private module. As I understand the rules, they are very simple: if an item is declared as pub, then it is considered public to the entire outside world and can thus be re-exported by anyone who can reach it.

(I happen to think we should extend this system somewhat, probably by allowing you to limit pub declarations to some enclosing modules, but this is the system we have today.)

However, use have one artificial restriction - it can reexport only "shallowly public" types, i.e. marked with pub, but possibly residing in private modules. type doesn't have this restriction. This restriction should either be imposed on both use and type or on none of them.

I don't understand this restriction, can you clarify what it is? I think probably both pub use and pub type should be limited to items declared as pub, and it doesn't really matter whether the modules they are declared in are private or public, so long as they are accessible to the one who declared the pub use and pub type.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Nov 6, 2015

Contributor

@petrochenkov

Another very simpe hole in VisiblePrivateTypesVisitor is

(Unless by hole you just meant that we get link time errors, in which case I agree there is some sort of bug with the linker's code that identifies reachable symbols.)

Contributor

nikomatsakis commented Nov 6, 2015

@petrochenkov

Another very simpe hole in VisiblePrivateTypesVisitor is

(Unless by hole you just meant that we get link time errors, in which case I agree there is some sort of bug with the linker's code that identifies reachable symbols.)

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Nov 6, 2015

Contributor

@seanmonstar

In this example that you gave:

pub use self::inner::Foo;
pub type Bar = self::inner::Bar;
mod inner {
    struct Foo;
    struct Bar;
}

The current rules are that both are in error. This is because Foo and Bar are declared as private. The goal of the current rules is that if a struct is declared as private, I never have to look outside the current module to find references to it. (But if a struct is declared as public, I have to go analyze things to figure out how far it escapes beyond the current module.)

Contributor

nikomatsakis commented Nov 6, 2015

@seanmonstar

In this example that you gave:

pub use self::inner::Foo;
pub type Bar = self::inner::Bar;
mod inner {
    struct Foo;
    struct Bar;
}

The current rules are that both are in error. This is because Foo and Bar are declared as private. The goal of the current rules is that if a struct is declared as private, I never have to look outside the current module to find references to it. (But if a struct is declared as public, I have to go analyze things to figure out how far it escapes beyond the current module.)

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Nov 6, 2015

Contributor

@aturon

That is, if you use a private type alias to reference a public type in a signature -- is that OK?

I expect this to be in an error, as it is today. I find this mildly annoying sometimes, but it makes sense, since type aliases appear in rustdoc and so forth.

Contributor

nikomatsakis commented Nov 6, 2015

@aturon

That is, if you use a private type alias to reference a public type in a signature -- is that OK?

I expect this to be in an error, as it is today. I find this mildly annoying sometimes, but it makes sense, since type aliases appear in rustdoc and so forth.

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Nov 6, 2015

Contributor

@nikomatsakis

I don't think this is a hole. Rather, I think it is legal to pub use a public type from a private module. As I understand the rules, they are very simple: if an item is declared as pub, then it is considered public to the entire outside world and can thus be re-exported by anyone who can reach it.
(Unless by hole you just meant that we get link time errors, in which case I agree there is some sort of bug with the linker's code that identifies reachable symbols.)

In this case EmbargoVisitor (supplying symbols for the linker, among the other things) and VisiblePrivateTypesVisitor are in serious contradiction. Supposedly, EmbargoVisitor follows the older version of the privacy RFC, because it exports only things that are actually reexported from private inner modules to outside with pub use or pub type and not just can be reexported because they are marked with pub.
Example:

mod private { pub struct PubMarked; }
pub fn f() -> private::PubMarked { ... } // Not exported by EmbargoVisitor, linker errors are possible
pub use private::PubMarked as Pub;
mod private { pub struct PubMarked; }
pub fn f() -> private::PubMarked { ... } // Exported by EmbargoVisitor, because of `pub use`

If EmbargoVisitor starts exporting everything pub in whatever inner modules, then rustdoc, stability pass, reachability pass etc. will be flooded by pub-but-not-actually-public items, because they also use the exported set provided by EmbargoVisitor :D

Contributor

petrochenkov commented Nov 6, 2015

@nikomatsakis

I don't think this is a hole. Rather, I think it is legal to pub use a public type from a private module. As I understand the rules, they are very simple: if an item is declared as pub, then it is considered public to the entire outside world and can thus be re-exported by anyone who can reach it.
(Unless by hole you just meant that we get link time errors, in which case I agree there is some sort of bug with the linker's code that identifies reachable symbols.)

In this case EmbargoVisitor (supplying symbols for the linker, among the other things) and VisiblePrivateTypesVisitor are in serious contradiction. Supposedly, EmbargoVisitor follows the older version of the privacy RFC, because it exports only things that are actually reexported from private inner modules to outside with pub use or pub type and not just can be reexported because they are marked with pub.
Example:

mod private { pub struct PubMarked; }
pub fn f() -> private::PubMarked { ... } // Not exported by EmbargoVisitor, linker errors are possible
pub use private::PubMarked as Pub;
mod private { pub struct PubMarked; }
pub fn f() -> private::PubMarked { ... } // Exported by EmbargoVisitor, because of `pub use`

If EmbargoVisitor starts exporting everything pub in whatever inner modules, then rustdoc, stability pass, reachability pass etc. will be flooded by pub-but-not-actually-public items, because they also use the exported set provided by EmbargoVisitor :D

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Nov 6, 2015

Contributor

Now I'm reading the older edition of https://github.com/rust-lang/rfcs/blob/master/text/0136-no-privates-in-public.md written by @glaebhoerl and I like it better than the current version.

Basically,
old edition - only pub use and pub type can export things
new edition - everything can export pub things
my personal favorite edition - only pub use and pub type can export pub things

Edit: with "new edition" it will be pretty hard to reduce the surface area of produced object files.

Contributor

petrochenkov commented Nov 6, 2015

Now I'm reading the older edition of https://github.com/rust-lang/rfcs/blob/master/text/0136-no-privates-in-public.md written by @glaebhoerl and I like it better than the current version.

Basically,
old edition - only pub use and pub type can export things
new edition - everything can export pub things
my personal favorite edition - only pub use and pub type can export pub things

Edit: with "new edition" it will be pretty hard to reduce the surface area of produced object files.

@glaebhoerl

This comment has been minimized.

Show comment
Hide comment
@glaebhoerl

glaebhoerl Nov 6, 2015

Contributor

(FWIW I like my original idea better too ;) the problem was basically that I didn't manage to figure out the right way to think about and specify the semantics in time (the one I wrote in the RFC was pretty confused, as far as I remember) and no one else seemed interested in trying, so we ended up with this less-ambitious formulation.)

Contributor

glaebhoerl commented Nov 6, 2015

(FWIW I like my original idea better too ;) the problem was basically that I didn't manage to figure out the right way to think about and specify the semantics in time (the one I wrote in the RFC was pretty confused, as far as I remember) and no one else seemed interested in trying, so we ended up with this less-ambitious formulation.)

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Nov 6, 2015

Contributor

To some extent, this is water under the bridge, at least until another RFC is written. However, the problem with the initial formulation, from my POV, is that, under that formulation, just looking at a declaration (say, of a struct) did not give you the full story of its scoping. The story is simpler today: if you see pub, you know that it is public to the world (or, at least, that is all the language is guaranteeing you). The way this played out at the time was that we literally could not tell whether the code was correctly implemented or not, because it wasn't enough to look at the definitions involved, one had to go over the code to find all pub uses and so forth.

One flaw, though, is that the story of pub is now too simple to do the things you want: there are types that should not be public to the whole world, but only to some subset of the module tree (or perhaps just to the current crate), but we have no way to declare this. I'd like to extend the pub syntax to let you declare this. But I don't want to infer the access level based on what is exported: I think things are often exported in error, and it's better to declare at the site of the item how far you intend for it to be exported, and then check that the exports obey that declaration. This lets you reason about a module in isolation, rather than looking at how others choose to pub use its contents.

All that said, I think that evaluating the overall reachability of items (versus checking whether they obey the "no private things in public APIs" rule) seems to just be two distinct concepts, and we have to deal with that. (That is, I'm not sure that collapsing both into one visitor is a good direction, though clearly they share some common code -- probably the bit that computes the successors to a given node in the reachability graph.) Evaluating the reachable set seems, however, to be a fairly straightforward process: it's just a DFS over the reachability graph, stopping at private edges, right?

Contributor

nikomatsakis commented Nov 6, 2015

To some extent, this is water under the bridge, at least until another RFC is written. However, the problem with the initial formulation, from my POV, is that, under that formulation, just looking at a declaration (say, of a struct) did not give you the full story of its scoping. The story is simpler today: if you see pub, you know that it is public to the world (or, at least, that is all the language is guaranteeing you). The way this played out at the time was that we literally could not tell whether the code was correctly implemented or not, because it wasn't enough to look at the definitions involved, one had to go over the code to find all pub uses and so forth.

One flaw, though, is that the story of pub is now too simple to do the things you want: there are types that should not be public to the whole world, but only to some subset of the module tree (or perhaps just to the current crate), but we have no way to declare this. I'd like to extend the pub syntax to let you declare this. But I don't want to infer the access level based on what is exported: I think things are often exported in error, and it's better to declare at the site of the item how far you intend for it to be exported, and then check that the exports obey that declaration. This lets you reason about a module in isolation, rather than looking at how others choose to pub use its contents.

All that said, I think that evaluating the overall reachability of items (versus checking whether they obey the "no private things in public APIs" rule) seems to just be two distinct concepts, and we have to deal with that. (That is, I'm not sure that collapsing both into one visitor is a good direction, though clearly they share some common code -- probably the bit that computes the successors to a given node in the reachability graph.) Evaluating the reachable set seems, however, to be a fairly straightforward process: it's just a DFS over the reachability graph, stopping at private edges, right?

@seanmonstar

This comment has been minimized.

Show comment
Hide comment
@seanmonstar

seanmonstar Nov 6, 2015

Contributor

@nikomatsakis woops, I forgot to make the 2 internal structs public.

// these do the same thing, and compile today
pub use self::internal::Foo;
pub type Bar = self::internal::Bar;
mod internal {
    pub struct Foo;
    pub struct Bar;
}

I was suggesting that the privacy visitor should assume the type alias is a wrapper struct with a public type: pub struct Bar(pub self::internal::Bar).


@petrochenkov Yes, being able to do that is excellent, and I didn't mean to take that away. However, I'd want it to be illegal if any of those slots used a private item.

pub struct Foo;
struct Bar;

pub type MaybeFoo = Option<Foo>; // <-- cool
pub type MaybeBar = Option<Bar>; // <-- compiles today, but i think it shouldnt
Contributor

seanmonstar commented Nov 6, 2015

@nikomatsakis woops, I forgot to make the 2 internal structs public.

// these do the same thing, and compile today
pub use self::internal::Foo;
pub type Bar = self::internal::Bar;
mod internal {
    pub struct Foo;
    pub struct Bar;
}

I was suggesting that the privacy visitor should assume the type alias is a wrapper struct with a public type: pub struct Bar(pub self::internal::Bar).


@petrochenkov Yes, being able to do that is excellent, and I didn't mean to take that away. However, I'd want it to be illegal if any of those slots used a private item.

pub struct Foo;
struct Bar;

pub type MaybeFoo = Option<Foo>; // <-- cool
pub type MaybeBar = Option<Bar>; // <-- compiles today, but i think it shouldnt
@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Nov 6, 2015

Contributor

@nikomatsakis

One flaw, though, is that the story of pub is now too simple to do the things you want: there are types that should not be public to the whole world, but only to some subset of the module tree (or perhaps just to the current crate), but we have no way to declare this. I'd like to extend the pub syntax to let you declare this. But I don't want to infer the access level based on what is exported: I think things are often exported in error, and it's better to declare at the site of the item how far you intend for it to be exported, and then check that the exports obey that declaration. This lets you reason about a module in isolation, rather than looking at how others choose to pub use its contents.

I see, this is a good motivation, EmbargoVisitor should be updated then, even if it makes it do sufficiently more work.
I'll probably write a post on internals about what data is needed from EmbargoVisitor by later stages of compilation, because now things can be directly accessible, accessible with help of use aliases, acessible with help of type aliases, not accessible by name but obtainable, and all of them have to be documented, linted, stabilized, marked reachable differently.

@seanmonstar
I'm of the same opinion then, pub type = T not requiring pub on T is a minor bug and should be fixed if the crater results are good.

Contributor

petrochenkov commented Nov 6, 2015

@nikomatsakis

One flaw, though, is that the story of pub is now too simple to do the things you want: there are types that should not be public to the whole world, but only to some subset of the module tree (or perhaps just to the current crate), but we have no way to declare this. I'd like to extend the pub syntax to let you declare this. But I don't want to infer the access level based on what is exported: I think things are often exported in error, and it's better to declare at the site of the item how far you intend for it to be exported, and then check that the exports obey that declaration. This lets you reason about a module in isolation, rather than looking at how others choose to pub use its contents.

I see, this is a good motivation, EmbargoVisitor should be updated then, even if it makes it do sufficiently more work.
I'll probably write a post on internals about what data is needed from EmbargoVisitor by later stages of compilation, because now things can be directly accessible, accessible with help of use aliases, acessible with help of type aliases, not accessible by name but obtainable, and all of them have to be documented, linted, stabilized, marked reachable differently.

@seanmonstar
I'm of the same opinion then, pub type = T not requiring pub on T is a minor bug and should be fixed if the crater results are good.

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Nov 6, 2015

Member

Thanks @petrochenkov. Do you mind if I re-assign this and the other privacy bugs to you, since I've halted work on them while you work on your rewrite?

Member

aturon commented Nov 6, 2015

Thanks @petrochenkov. Do you mind if I re-assign this and the other privacy bugs to you, since I've halted work on them while you work on your rewrite?

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Nov 6, 2015

Contributor

@aturon
Sure, I have plenty of time in the next few weeks, I think I'll be able to finish the rewrite.

Contributor

petrochenkov commented Nov 6, 2015

@aturon
Sure, I have plenty of time in the next few weeks, I think I'll be able to finish the rewrite.

@aturon aturon self-assigned this Nov 9, 2015

@petrochenkov petrochenkov referenced this issue Nov 14, 2015

Closed

Linker error with non-reachable type #16734

0 of 3 tasks complete
@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Nov 20, 2015

Contributor

Prohibiting non-pub types on the right side of pub type breaks some noticeable amount of code in rustc. Fixing #28325 and other issues has some fallout too, but less than pub type. It will require at least a couple of crater runs, periods of warnings for the most significant breakage points and everything like this. (And I haven't even considered associated types yet.)

Also, strictly speaking, almost everything involving public traits should be pub due to type inference being too smart, but I'm going close my eyes on this so far (or maybe fix it somehow).

Contributor

petrochenkov commented Nov 20, 2015

Prohibiting non-pub types on the right side of pub type breaks some noticeable amount of code in rustc. Fixing #28325 and other issues has some fallout too, but less than pub type. It will require at least a couple of crater runs, periods of warnings for the most significant breakage points and everything like this. (And I haven't even considered associated types yet.)

Also, strictly speaking, almost everything involving public traits should be pub due to type inference being too smart, but I'm going close my eyes on this so far (or maybe fix it somehow).

bors added a commit that referenced this issue Dec 18, 2015

Auto merge of #29973 - petrochenkov:privinpub, r=nikomatsakis
Some notes:
This patch enforces the rules from [RFC 136](https://github.com/rust-lang/rfcs/blob/master/text/0136-no-privates-in-public.md) and makes "private in public" a module-level concept and not crate-level. Only `pub` annotations are used by the new algorithm, crate-level exported node set produced by `EmbargoVisitor` is not used. The error messages are tweaked accordingly and don't use the word "exported" to avoid confusing people (#29668).

The old algorithm tried to be extra smart with impls, but it mostly led to unpredictable behavior and bugs like #28325.
The new algorithm tries to be as simple as possible - an impl is considered public iff its type is public and its trait is public (if presents).
A type or trait is considered public if all its components are public, [complications](https://internals.rust-lang.org/t/limits-of-type-inference-smartness/2919) with private types leaking to other crates/modules through trait impls and type inference are deliberately ignored so far.

The new algorithm is not recursive and uses the nice new facility `Crate::visit_all_items`!

Obsolete pre-1.0 feature `visible_private_types` is removed.

This is a [breaking-change].
The two main vectors of breakage are type aliases (#28450) and impls (#28325).
I need some statistics from a crater run (cc @alexcrichton) to decide on the breakage mitigation strategy.
UPDATE: All the new errors are reported as warnings controlled by a lint `private_in_public` and lint group `future_incompatible`, but the intent is to make them hard errors eventually.

Closes #28325
Closes #28450
Closes #29524
Closes #29627
Closes #29668
Closes #30055

r? @nikomatsakis

@bors bors closed this in #29973 Dec 18, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment