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

pub(restricted) item #1422

Merged
merged 7 commits into from Mar 22, 2016

Conversation

Projects
None yet
@pnkfelix
Member

pnkfelix commented Dec 21, 2015

Summary

Expand the current pub/non-pub categorization of items with the ability to say "make this item visible solely to a (named) module tree."

The current crate is one such tree, and would be expressed via: pub(crate) item. Other trees can be denoted via a path employed in a use statement, e.g. pub(a::b) item, or pub(super) item.

(accepted) RFC text

rendered draft

pnkfelix added some commits Dec 21, 2015

first draft.
update: zebra stripe the embedded laundry list of bugs.

update: try to clarify what I am talking about in the "In practice" section.

update: add example of re-export.

update: added pub(crate) example.

update: added discussion of precedent in Scala.
(You may be wondering: "Could we move that `impl S` out to the
top-level, out of `mod a`?" Well ... see discussion in the
[unresolved questions][def-outside-restriction].)

This comment has been minimized.

@oli-obk

oli-obk Dec 21, 2015

Contributor

I don't find this to be a convincing example. You could just leave out the pub(a) and the example would work right now just as expected. Adding impls inside submodules is imo the correct way to say it's only accessible in there. The example should probably be showing two submodules accessing the same function. But then it doesn't fit here, but to to the unresolved question point.

@oli-obk

oli-obk Dec 21, 2015

Contributor

I don't find this to be a convincing example. You could just leave out the pub(a) and the example would work right now just as expected. Adding impls inside submodules is imo the correct way to say it's only accessible in there. The example should probably be showing two submodules accessing the same function. But then it doesn't fit here, but to to the unresolved question point.

This comment has been minimized.

@pnkfelix

pnkfelix Dec 21, 2015

Member

You're right, this was not a good example. I will try to fix.

@pnkfelix

pnkfelix Dec 21, 2015

Member

You're right, this was not a good example. I will try to fix.

@sfackler

This comment has been minimized.

Show comment
Hide comment
@sfackler

sfackler Dec 21, 2015

Member

I currently emulate this with traits defined at the level that needs visibility, but it's a lot of annoying boilerplate: https://github.com/sfackler/rust-postgres/blob/master/src/lib.rs#L1437-L1487

Member

sfackler commented Dec 21, 2015

I currently emulate this with traits defined at the level that needs visibility, but it's a lot of annoying boilerplate: https://github.com/sfackler/rust-postgres/blob/master/src/lib.rs#L1437-L1487

The main problem with this approach is that we tried it, and it
did not work well: The implementation was buggy, and the user-visible
error messages were hard to understand.

This comment has been minimized.

@petrochenkov

petrochenkov Dec 21, 2015

Contributor

Note, that reachability analysis is already performed by the privacy pass. Its results are used by rustdoc, dead code lint, stability and link-time-reachability passes, but not by privacy itself.
It's also less buggy than it was.

@petrochenkov

petrochenkov Dec 21, 2015

Contributor

Note, that reachability analysis is already performed by the privacy pass. Its results are used by rustdoc, dead code lint, stability and link-time-reachability passes, but not by privacy itself.
It's also less buggy than it was.

Or does it just mean that `J`, despite being defined in `mod b`, is
itself not accessible in `mod b`?
pnkfelix is personally inclined to make this sort of thing illegal,

This comment has been minimized.

@petrochenkov

petrochenkov Dec 21, 2015

Contributor

+1

@petrochenkov

petrochenkov Dec 21, 2015

Contributor

+1

This comment has been minimized.

@petrochenkov

petrochenkov Dec 22, 2015

Contributor

If arbitrary non-nested paths are permitted + a path list can be used instead of a single path pub(path1, path2, path3), then it will look more and more like friend from C++
:horror:

@petrochenkov

petrochenkov Dec 22, 2015

Contributor

If arbitrary non-nested paths are permitted + a path list can be used instead of a single path pub(path1, path2, path3), then it will look more and more like friend from C++
:horror:

@ticki

This comment has been minimized.

Show comment
Hide comment
@ticki

ticki Dec 21, 2015

Contributor

I like the idea, but I'm not sure about the syntax, you propose.

Contributor

ticki commented Dec 21, 2015

I like the idea, but I'm not sure about the syntax, you propose.

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Dec 21, 2015

Contributor

I like the general direction, although pub(arbitrary_path) feels a bit to powerful than needed and pub(crate) may be enough. A crate is usually moderately sized and maintained by a team or one person and it's never a problem to modify or reorganize it, or make something public or private.
I also feel that the current system is pretty good already and the need in this extension is not especially dire.

I mostly have concerns about the implementation side, the current privacy system is not well formalized and three its main components - privacy checker itself, private-in-public checker and reachability analysis often work based on different assumptions and contradict to each other. Together with this and the new macro privacy system from @nrc it may bring some chaos.
So, I'd prefer to roll this out in parts:
Make the parts of the current system coherent and document them well -> Macro privacy -> pub(crate) -> pub(path) if necessary

Also, fixing rust-lang/rust#30079 or relaxing the rules to allow

mod m {
    struct S;
    mod n {
        // Allowed, but you can't reexport `f` out of `m` without making `S` public
        pub fn f(arg: super::S) {}
    }
}

will probably involve implementing some rudimentary form of pub(restricted) from this RFC, so I still see it as a good long term direction.

Contributor

petrochenkov commented Dec 21, 2015

I like the general direction, although pub(arbitrary_path) feels a bit to powerful than needed and pub(crate) may be enough. A crate is usually moderately sized and maintained by a team or one person and it's never a problem to modify or reorganize it, or make something public or private.
I also feel that the current system is pretty good already and the need in this extension is not especially dire.

I mostly have concerns about the implementation side, the current privacy system is not well formalized and three its main components - privacy checker itself, private-in-public checker and reachability analysis often work based on different assumptions and contradict to each other. Together with this and the new macro privacy system from @nrc it may bring some chaos.
So, I'd prefer to roll this out in parts:
Make the parts of the current system coherent and document them well -> Macro privacy -> pub(crate) -> pub(path) if necessary

Also, fixing rust-lang/rust#30079 or relaxing the rules to allow

mod m {
    struct S;
    mod n {
        // Allowed, but you can't reexport `f` out of `m` without making `S` public
        pub fn f(arg: super::S) {}
    }
}

will probably involve implementing some rudimentary form of pub(restricted) from this RFC, so I still see it as a good long term direction.

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Dec 21, 2015

Contributor

It would be really nice to try making pub(super) (or pub(crate)) a default for pub instead of pub(universe). At least it is in line with the general "private by default" ideology. It would also allow reachability analysis to make more things internal at link time. But it would break the world unfortunately.

Contributor

petrochenkov commented Dec 21, 2015

It would be really nice to try making pub(super) (or pub(crate)) a default for pub instead of pub(universe). At least it is in line with the general "private by default" ideology. It would also allow reachability analysis to make more things internal at link time. But it would break the world unfortunately.

tree."
The current `crate` is one such tree, and would be expressed via:
`pub(crate) item`. Other trees can be denoted via a path employed in a

This comment has been minimized.

@nrc

nrc Dec 21, 2015

Member

We could have pub(::) instead of pub(crate), this would mean only having a single syntax, at the expense of grotesque ugliness.

@nrc

nrc Dec 21, 2015

Member

We could have pub(::) instead of pub(crate), this would mean only having a single syntax, at the expense of grotesque ugliness.

This comment has been minimized.

@pnkfelix

pnkfelix Dec 21, 2015

Member

my eyes!

@pnkfelix

pnkfelix Dec 21, 2015

Member

my eyes!

This comment has been minimized.

@oli-obk

oli-obk Dec 22, 2015

Contributor

alternatively crate could be allowed at the beginning of paths, thus making this a single syntax

@oli-obk

oli-obk Dec 22, 2015

Contributor

alternatively crate could be allowed at the beginning of paths, thus making this a single syntax

This comment has been minimized.

@pnkfelix

pnkfelix Feb 11, 2016

Member

But then people will complain about having more than one way to do things (in this case, writing absolute paths) right?

@pnkfelix

pnkfelix Feb 11, 2016

Member

But then people will complain about having more than one way to do things (in this case, writing absolute paths) right?

@withoutboats

This comment has been minimized.

Show comment
Hide comment
@withoutboats

withoutboats Dec 21, 2015

Contributor

Big 👍 on something in this direction. I've repeatedly felt contorted trying to make items public to the rest of my crate without exposing them outside the crate.

Maybe I haven't worked on large enough libraries, but I am not sure that anything more powerful than pub(crate) or maybe pub(super) is really needed. I wonder if we couldn't just add a keyword for that instead of going for the heavy duty solution. It would be kind of strange to use priv for something less private than default, but it is still a reserved word.

It would be really nice to try making pub(super) (or pub(crate)) a default for pub instead of pub(universe).

👍 on this if Rust ever increments the major version number.

Contributor

withoutboats commented Dec 21, 2015

Big 👍 on something in this direction. I've repeatedly felt contorted trying to make items public to the rest of my crate without exposing them outside the crate.

Maybe I haven't worked on large enough libraries, but I am not sure that anything more powerful than pub(crate) or maybe pub(super) is really needed. I wonder if we couldn't just add a keyword for that instead of going for the heavy duty solution. It would be kind of strange to use priv for something less private than default, but it is still a reserved word.

It would be really nice to try making pub(super) (or pub(crate)) a default for pub instead of pub(universe).

👍 on this if Rust ever increments the major version number.

subtrees, narrow the feature to just `pub(crate) item`, so that one
chooses either "module private" (by adding no modifier), or
"universally visible" (by adding `pub`), or "visible to just the
current crate" (by adding `pub(crate)`).

This comment has been minimized.

@nrc

nrc Dec 21, 2015

Member

I'm strongly against this alternative - I think it is ill-advised to make crates more significant than they already are - we would end up with two layers of modularisation. I prefer to keep crates as essentially file-system artefacts.

@nrc

nrc Dec 21, 2015

Member

I'm strongly against this alternative - I think it is ill-advised to make crates more significant than they already are - we would end up with two layers of modularisation. I prefer to keep crates as essentially file-system artefacts.

This comment has been minimized.

@pnkfelix

pnkfelix Dec 21, 2015

Member

Obviously I agree with @nrc; otherwise this RFC would have been written to just suggest pub(crate) on its own.

Of course we can see there are people on the other side of this debate; I've seen two commenters already state that they think pub(crate) may suffice on its own.


@nrc I am curious if you can come up with other use-cases for pub(path) beyond the hypothetical crate-merging refactoring tool that I sketched in the text of this subsection.

  • Well, of course there is the obvious "some crates are large enough to warrant privacy controls within the crate, that are more fine-grained than just pub/pub(crate)/private

I actually rather likes the proposal from @petrochenkov that we might roll this out in stages, with pub(crate) very early on, and adding pub(path) later after convincing the community at large that it is a good thing too.

Except I might advocate putting in pub(crate) before we tighten up the rules for privacy at large, for the precise reason described in the RFC: giving people the ability to express pub(crate) will encourage them to fix their code to say something closer to what they mean now and stop exploiting bugs in the privacy system to express such structures, which will give us more freedom to tighten up privacy (and tell them to use pub(crate) in the warnings emitted during warning cycle period).

@pnkfelix

pnkfelix Dec 21, 2015

Member

Obviously I agree with @nrc; otherwise this RFC would have been written to just suggest pub(crate) on its own.

Of course we can see there are people on the other side of this debate; I've seen two commenters already state that they think pub(crate) may suffice on its own.


@nrc I am curious if you can come up with other use-cases for pub(path) beyond the hypothetical crate-merging refactoring tool that I sketched in the text of this subsection.

  • Well, of course there is the obvious "some crates are large enough to warrant privacy controls within the crate, that are more fine-grained than just pub/pub(crate)/private

I actually rather likes the proposal from @petrochenkov that we might roll this out in stages, with pub(crate) very early on, and adding pub(path) later after convincing the community at large that it is a good thing too.

Except I might advocate putting in pub(crate) before we tighten up the rules for privacy at large, for the precise reason described in the RFC: giving people the ability to express pub(crate) will encourage them to fix their code to say something closer to what they mean now and stop exploiting bugs in the privacy system to express such structures, which will give us more freedom to tighten up privacy (and tell them to use pub(crate) in the warnings emitted during warning cycle period).

This comment has been minimized.

@petrochenkov

petrochenkov Dec 21, 2015

Contributor

before we tighten up the rules for privacy at large

I don't expect noticeable breakage from privacy in the future, most practically significant holes are closed now. Regarding the existing private_in_public warnings, I don't think pub(crate) will help much, but e.g. relaxing rules on trait bounds or alias substitution certainly will.

@petrochenkov

petrochenkov Dec 21, 2015

Contributor

before we tighten up the rules for privacy at large

I don't expect noticeable breakage from privacy in the future, most practically significant holes are closed now. Regarding the existing private_in_public warnings, I don't think pub(crate) will help much, but e.g. relaxing rules on trait bounds or alias substitution certainly will.

This comment has been minimized.

@withoutboats

withoutboats Dec 21, 2015

Contributor

I think it is ill-advised to make crates more significant than they already are - we would end up with two layers of modularisation.

I agree that this is not ideal, but how is this not already the case? Coherence rules operate at the crate rather than module level, extern crate declarations and cargo dependencies are in themselves not transparent, etc. There is already a pretty big difference in the relation between modules inside my crate and relations with modules outside of it.

@withoutboats

withoutboats Dec 21, 2015

Contributor

I think it is ill-advised to make crates more significant than they already are - we would end up with two layers of modularisation.

I agree that this is not ideal, but how is this not already the case? Coherence rules operate at the crate rather than module level, extern crate declarations and cargo dependencies are in themselves not transparent, etc. There is already a pretty big difference in the relation between modules inside my crate and relations with modules outside of it.

This comment has been minimized.

@petrochenkov

petrochenkov Dec 21, 2015

Contributor

With regards to privacy, crates are also important, as units of compilation - some code or metadata can potentially be omitted from the compiled crate if it can be proven that it is not accessible from other crates.

@petrochenkov

petrochenkov Dec 21, 2015

Contributor

With regards to privacy, crates are also important, as units of compilation - some code or metadata can potentially be omitted from the compiled crate if it can be proven that it is not accessible from other crates.

This comment has been minimized.

@bstrie

bstrie Dec 22, 2015

Contributor

@nrc I think the battle against crates as a separate unit of modularization has long been lost. The crate vs. mod distinction already matters for circular dependencies, inlining, and incremental recompilation. I don't see any danger to embracing this distinction even further.

@pnkfelix I have indeed seen people asking for "crate-level visibility" for years now (I'd be surprised if there isn't an old RFC for it somewhere), so paring down this proposal to merely pub(crate) may very well be viable. And in that case you can even consider simplifying the syntax to just pub crate or crate pub or something.

@bstrie

bstrie Dec 22, 2015

Contributor

@nrc I think the battle against crates as a separate unit of modularization has long been lost. The crate vs. mod distinction already matters for circular dependencies, inlining, and incremental recompilation. I don't see any danger to embracing this distinction even further.

@pnkfelix I have indeed seen people asking for "crate-level visibility" for years now (I'd be surprised if there isn't an old RFC for it somewhere), so paring down this proposal to merely pub(crate) may very well be viable. And in that case you can even consider simplifying the syntax to just pub crate or crate pub or something.

This comment has been minimized.

@pnkfelix

pnkfelix Dec 22, 2015

Member

@bstrie

paring down this proposal to merely pub(crate) may very well be viable. And in that case you can even consider simplifying the syntax to just pub crate or crate pub or something.

I am pretty strongly opposed to adopting a syntax that isn't future-extensible to pub(path) ... pub in crate <item> (which then supports e.g. pub in path <item>) might work, though visually I think I prefer the parentheses.

@pnkfelix

pnkfelix Dec 22, 2015

Member

@bstrie

paring down this proposal to merely pub(crate) may very well be viable. And in that case you can even consider simplifying the syntax to just pub crate or crate pub or something.

I am pretty strongly opposed to adopting a syntax that isn't future-extensible to pub(path) ... pub in crate <item> (which then supports e.g. pub in path <item>) might work, though visually I think I prefer the parentheses.

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Dec 21, 2015

Contributor

The typeck point-of-view is that associated items are as private as their containing traits and that all types are basically public. I am not sure about the motivation for the no-privates-in-public rule (even reading the comments does not make that clear) - we should clarify what properties we want from privacy.

Contributor

arielb1 commented Dec 21, 2015

The typeck point-of-view is that associated items are as private as their containing traits and that all types are basically public. I am not sure about the motivation for the no-privates-in-public rule (even reading the comments does not make that clear) - we should clarify what properties we want from privacy.

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Dec 21, 2015

Contributor

From my typeck POV, the NPIP/RFC136 rule feels to me rather lintish - it is not sure exactly what it is trying to prevent. Local reasoning on reachability is always kind of odd - you need to be sure nobody exposes your API in some indirect way - but having some block on direct uses would be fine.

Contributor

arielb1 commented Dec 21, 2015

From my typeck POV, the NPIP/RFC136 rule feels to me rather lintish - it is not sure exactly what it is trying to prevent. Local reasoning on reachability is always kind of odd - you need to be sure nobody exposes your API in some indirect way - but having some block on direct uses would be fine.

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Dec 21, 2015

Member

@arielb1 Just to check, have you seen Felix's related blog post? It tries to address at least some of the questions you're raising.

Member

aturon commented Dec 21, 2015

@arielb1 Just to check, have you seen Felix's related blog post? It tries to address at least some of the questions you're raising.

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Dec 21, 2015

Contributor

@aturon

Yes I've read it.

Contributor

arielb1 commented Dec 21, 2015

@aturon

Yes I've read it.

@huonw

This comment has been minimized.

Show comment
Hide comment
@huonw

huonw Dec 21, 2015

Member

It seems somewhat unfortunate that pub fn can't be written as pub(X) fn for some X (like extern fn == extern "C" fn). Although it looks like pub fn foo should be pretty similar to pub(super) fn foo, except maybe that if the module containing foo is pub then foo can be used outside super?

What happens if the restriction is to a subpath? e.g. is the call to foo legal?

pub(module) fn foo() {}

fn bar() {
    foo() // is foo visible here?
}

mod module {}
Member

huonw commented Dec 21, 2015

It seems somewhat unfortunate that pub fn can't be written as pub(X) fn for some X (like extern fn == extern "C" fn). Although it looks like pub fn foo should be pretty similar to pub(super) fn foo, except maybe that if the module containing foo is pub then foo can be used outside super?

What happens if the restriction is to a subpath? e.g. is the call to foo legal?

pub(module) fn foo() {}

fn bar() {
    foo() // is foo visible here?
}

mod module {}
@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Dec 22, 2015

Member

@huonw

It seems somewhat unfortunate that pub fn can't be written as pub(X) fn for some X

Hmm, you're right; if I'm going to allow pub (self) item as a synonym for no pub at all, then one would think I should also provide a form that one could feed into the pub(_) that stands for the universe of all crates. (pub(pub) ? Oh man, that is truly terrible, nearly as bad as @nrc's pub(::) ...)

What happens if the restriction is to a subpath? e.g. is the call to foo legal?

See the unresolved question section on this topic.

Member

pnkfelix commented Dec 22, 2015

@huonw

It seems somewhat unfortunate that pub fn can't be written as pub(X) fn for some X

Hmm, you're right; if I'm going to allow pub (self) item as a synonym for no pub at all, then one would think I should also provide a form that one could feed into the pub(_) that stands for the universe of all crates. (pub(pub) ? Oh man, that is truly terrible, nearly as bad as @nrc's pub(::) ...)

What happens if the restriction is to a subpath? e.g. is the call to foo legal?

See the unresolved question section on this topic.

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Dec 22, 2015

Contributor

@arielb1

we should clarify what properties we want from privacy

In addition to user-oriented properties, I'd like to have the next guarantee:
If a type is private or pub(crate)/pub(path), then all its methods (inherent or from traits) can have internal linkage (these methods shouldn't also be used from inlinable code, but it's separate story).
It's mostly true today with exception of some cases where type inference is too smart.

Update: Ideally this should be done based on reachability and not pub annotations, but the rule "type inference can not result in unreachable types" would look kind of strange compared to the rule "type inference can not result in private types". Or we can calculate somehow which exactly private types can be reached through type inference (I have no idea how to do this), add them to the reachable set and abandon the privacy-based reasoning completely.

Contributor

petrochenkov commented Dec 22, 2015

@arielb1

we should clarify what properties we want from privacy

In addition to user-oriented properties, I'd like to have the next guarantee:
If a type is private or pub(crate)/pub(path), then all its methods (inherent or from traits) can have internal linkage (these methods shouldn't also be used from inlinable code, but it's separate story).
It's mostly true today with exception of some cases where type inference is too smart.

Update: Ideally this should be done based on reachability and not pub annotations, but the rule "type inference can not result in unreachable types" would look kind of strange compared to the rule "type inference can not result in private types". Or we can calculate somehow which exactly private types can be reached through type inference (I have no idea how to do this), add them to the reachable set and abandon the privacy-based reasoning completely.

@diwic

This comment has been minimized.

Show comment
Hide comment
@diwic

diwic Dec 22, 2015

There is no example with struct impls, but I assume they are covered as well?

I've hit this in my bindings a few times, I wanted to do:

mod ctl_int {
    pub struct ElemInfo(*mut alsa::snd_ctl_elem_info_t);
    impl ElemInfo {
        // Accessible in this crate, but not other crates
        pub($crate) handle(&self) -> *mut alsa::snd_ctl_elem_info_t { self.0 }
        // Other public methods accessible to other crates
    }
}
pub use ctl_int::ElemInfo;

Now I'm instead doing this workaround:

mod ctl_int {
    pub struct ElemInfo(*mut alsa::snd_ctl_elem_info_t);
    pub fn elem_info_ptr(a: &ElemInfo) -> *mut alsa::snd_ctl_elem_info_t { a.0 }
    impl ElemInfo {
        // Other public methods accessible to other crates
    }
}
pub use ctl_int::ElemInfo;

Edit: Changed a to self

diwic commented Dec 22, 2015

There is no example with struct impls, but I assume they are covered as well?

I've hit this in my bindings a few times, I wanted to do:

mod ctl_int {
    pub struct ElemInfo(*mut alsa::snd_ctl_elem_info_t);
    impl ElemInfo {
        // Accessible in this crate, but not other crates
        pub($crate) handle(&self) -> *mut alsa::snd_ctl_elem_info_t { self.0 }
        // Other public methods accessible to other crates
    }
}
pub use ctl_int::ElemInfo;

Now I'm instead doing this workaround:

mod ctl_int {
    pub struct ElemInfo(*mut alsa::snd_ctl_elem_info_t);
    pub fn elem_info_ptr(a: &ElemInfo) -> *mut alsa::snd_ctl_elem_info_t { a.0 }
    impl ElemInfo {
        // Other public methods accessible to other crates
    }
}
pub use ctl_int::ElemInfo;

Edit: Changed a to self

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Dec 22, 2015

Contributor

@diwic

mod ctl_int {
    pub struct ElemInfo(*mut alsa::snd_ctl_elem_info_t);
    impl ElemInfo {
        // Other public methods accessible to other crates
    }
}
pub use ctl_int::ElemInfo;

impl ElemInfo {
    // Accessible in this crate, but not other crates
    fn handle(&self) -> *mut alsa::snd_ctl_elem_info_t { a.0 };
}

?

Contributor

petrochenkov commented Dec 22, 2015

@diwic

mod ctl_int {
    pub struct ElemInfo(*mut alsa::snd_ctl_elem_info_t);
    impl ElemInfo {
        // Other public methods accessible to other crates
    }
}
pub use ctl_int::ElemInfo;

impl ElemInfo {
    // Accessible in this crate, but not other crates
    fn handle(&self) -> *mut alsa::snd_ctl_elem_info_t { a.0 };
}

?

@diwic

This comment has been minimized.

Show comment
Hide comment
@diwic

diwic Dec 23, 2015

@petrochenkov - that doesn't work because self.0 is private. Which is expected. Potentially,

pub struct ElemInfo(pub($crate) *mut alsa::snd_ctl_elem_info_t);

...could work, but it leaks the internal layout of ElemInfo to the entire crate, so an accessor method is better.

diwic commented Dec 23, 2015

@petrochenkov - that doesn't work because self.0 is private. Which is expected. Potentially,

pub struct ElemInfo(pub($crate) *mut alsa::snd_ctl_elem_info_t);

...could work, but it leaks the internal layout of ElemInfo to the entire crate, so an accessor method is better.

@blaenk

This comment has been minimized.

Show comment
Hide comment
@blaenk

blaenk Dec 24, 2015

Contributor

Huge 👍

I'm very glad and grateful you came up with this @pnkfelix . I've run into situations where this would be very useful and much more straightforward than the contortions that are required today, but I had this (what I hope to be mis)conception that changes to item visibility rules/functionality would be very difficult to convince the team about. Glad it's happening from the inside! 😁

Contributor

blaenk commented Dec 24, 2015

Huge 👍

I'm very glad and grateful you came up with this @pnkfelix . I've run into situations where this would be very useful and much more straightforward than the contortions that are required today, but I had this (what I hope to be mis)conception that changes to item visibility rules/functionality would be very difficult to convince the team about. Glad it's happening from the inside! 😁

Show outdated Hide outdated text/0000-pub-restricted.md Outdated
Show outdated Hide outdated text/0000-pub-restricted.md Outdated
Show outdated Hide outdated text/0000-pub-restricted.md Outdated
Show outdated Hide outdated text/0000-pub-restricted.md Outdated
@liigo

This comment has been minimized.

Show comment
Hide comment
@liigo

liigo Dec 25, 2015

Contributor

to describe a crate-scoped-item:
protected item?
internal item?
crate item?

protected mod xxx should be sufficient, imho.

Contributor

liigo commented Dec 25, 2015

to describe a crate-scoped-item:
protected item?
internal item?
crate item?

protected mod xxx should be sufficient, imho.

@nixpulvis

This comment has been minimized.

Show comment
Hide comment
@nixpulvis

nixpulvis Dec 26, 2015

I was giving something along the lines of pub(...) for a more general stability annotation. Since we should only care about an item's stability when it's public. So how the syntax here could tie into a syntax for that might be something to consider. For example pub in a::b as stable as an offhand idea.

nixpulvis commented Dec 26, 2015

I was giving something along the lines of pub(...) for a more general stability annotation. Since we should only care about an item's stability when it's public. So how the syntax here could tie into a syntax for that might be something to consider. For example pub in a::b as stable as an offhand idea.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Mar 11, 2016

Contributor

Hear ye, hear ye. This RFC is hereby promoted to final comment period.

Note that @pnkfelix still plans to make some edits, but they are not intended to change the semantics, just address some nits.

Contributor

nikomatsakis commented Mar 11, 2016

Hear ye, hear ye. This RFC is hereby promoted to final comment period.

Note that @pnkfelix still plans to make some edits, but they are not intended to change the semantics, just address some nits.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Mar 11, 2016

Contributor

How does this interact with glob reexports? Glob reexports currently reexport only pub(universe) items.

Did we ever clarify this in particular? Longer term, I would expect glob re-exports to re-export all things that are visible to the "re-exporter" (with the same privacy). However, our current rules about shadowing make it a problematic breaking change to add these semantics, because use super::* would now import private items. So we could either continue to do imports so long as the pub keyword is used -- which sort of violates my mental model, in which no keyword is short for pub(self) -- or just keep the current model of "only do pub (with no restriction)" and then expand things all at once. I don't care that much except that long term I'd like to go for the more expansive model. But it seems like the best thing short term is to only have glob re-exports affect pub with no restriction.

Some possible solutions to #1422 (comment) are discussed in rust-lang/rust#31783

But I should go make sure I'm caught up on this thread...

Contributor

nikomatsakis commented Mar 11, 2016

How does this interact with glob reexports? Glob reexports currently reexport only pub(universe) items.

Did we ever clarify this in particular? Longer term, I would expect glob re-exports to re-export all things that are visible to the "re-exporter" (with the same privacy). However, our current rules about shadowing make it a problematic breaking change to add these semantics, because use super::* would now import private items. So we could either continue to do imports so long as the pub keyword is used -- which sort of violates my mental model, in which no keyword is short for pub(self) -- or just keep the current model of "only do pub (with no restriction)" and then expand things all at once. I don't care that much except that long term I'd like to go for the more expansive model. But it seems like the best thing short term is to only have glob re-exports affect pub with no restriction.

Some possible solutions to #1422 (comment) are discussed in rust-lang/rust#31783

But I should go make sure I'm caught up on this thread...

@nodakai

This comment has been minimized.

Show comment
Hide comment
@nodakai

nodakai Mar 13, 2016

What happens if we want to expose a name to multiple modules? pub(krate1::m11, krate2::m21) ? Yuck.

nodakai commented Mar 13, 2016

What happens if we want to expose a name to multiple modules? pub(krate1::m11, krate2::m21) ? Yuck.

If a re-export occurs within a non-`pub` module, can we treat it as
implicitly satisfying a restriction to `super` imposed by the item it
is re-exporting?

This comment has been minimized.

@petrochenkov

petrochenkov Mar 13, 2016

Contributor

I'd prefer to start with the strict rules (i.e. the answer "No"). They fit better into the whole picture and can always be relaxed if necessary (I doubt it will be necessary).

@petrochenkov

petrochenkov Mar 13, 2016

Contributor

I'd prefer to start with the strict rules (i.e. the answer "No"). They fit better into the whole picture and can always be relaxed if necessary (I doubt it will be necessary).

@aturon aturon removed the I-nominated label Mar 14, 2016

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Mar 21, 2016

Contributor

Huzzah! The @rust-lang/lang team has decided to accept this RFC. With respect to unresolved questions -- such as the strictness of certain rules -- I think we tend towards the stricter interpretation, but also intend to answer such questions after gaining some experience with an implementation.

Contributor

nikomatsakis commented Mar 21, 2016

Huzzah! The @rust-lang/lang team has decided to accept this RFC. With respect to unresolved questions -- such as the strictness of certain rules -- I think we tend towards the stricter interpretation, but also intend to answer such questions after gaining some experience with an implementation.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Mar 21, 2016

Contributor

Tracking issue: rust-lang/rust#32409

Contributor

nikomatsakis commented Mar 21, 2016

Tracking issue: rust-lang/rust#32409

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Mar 21, 2016

Contributor

@pnkfelix I think you still had some minor clarifications that you planned to make?

Contributor

nikomatsakis commented Mar 21, 2016

@pnkfelix I think you still had some minor clarifications that you planned to make?

@tomaka tomaka referenced this pull request Mar 21, 2016

Merged

reports 2016-03-21 #32

@glaebhoerl

This comment has been minimized.

Show comment
Hide comment
@glaebhoerl

glaebhoerl Mar 21, 2016

Contributor

cc @glaebhoerl -- not sure if you've been following this, but now would be a good time to leave your thoughts.

Welp. This has been sitting in my notifications for so long that I never noticed the cc.

Anyway:

  • As far as #136 is concerned, I never figured out what I really felt was the right way to formulate the rules I wanted (which was a major contributing factor to the fact that we ended up with completely different ones), and the details are even less in my memory now, but I do know that I didn't intend it to be any kind of "global privacy (or publicity) inference" scheme -- that's something I deliberately wanted to avoid. It was always my goal for it to be based solely on simple, local, compositional rules.

    For instance, in @nikomatsakis's "Problem 0", it wouldn't check at the definition site of outer_fn, "are there any pub uses of this in outer modules?", to determine whether or not it is allowed to refer to the privately-declared OuterStruct in its signature. mod inner is exposing pub outer_fn to its parent module (outer), and what to do with it from there (whether or not to expose it further) is entirely outer's concern. The privacy checker sees that the type OuterStruct is public to the parent module, so the definition is accepted.

    If in fact there were a pub use self::inner::outer_fn in outer, then the check for whether this pub use is allowed -- whether the types in the signature of item being re-exported are public to its parent module -- would happen at that point. (I think this might be equivalent to substituting the signature of the pub use's target in place of the pub use, and re-checking it there.) The privacy checker would see that OuterStruct is not public to outer's parent module, and so the pub use would be rejected. (Likewise, if it were pub mod inner instead of private, the relevant test would be, "Is mod outer allowed to declare this module public? Are the types in the signatures of its public items public to outer's parent module?" -- whether or not its parent module (or maybe I should say owner) declares it public is not inner's concern.)

  • But that's water under the bridge. I don't have any well-defined feelings about this pub(restricted) RFC, or even about whether the referencing-private-items rules in their new form are worth keeping. The new syntax that has to be learned seems undesirable, but I suppose that's banal and obvious.

Contributor

glaebhoerl commented Mar 21, 2016

cc @glaebhoerl -- not sure if you've been following this, but now would be a good time to leave your thoughts.

Welp. This has been sitting in my notifications for so long that I never noticed the cc.

Anyway:

  • As far as #136 is concerned, I never figured out what I really felt was the right way to formulate the rules I wanted (which was a major contributing factor to the fact that we ended up with completely different ones), and the details are even less in my memory now, but I do know that I didn't intend it to be any kind of "global privacy (or publicity) inference" scheme -- that's something I deliberately wanted to avoid. It was always my goal for it to be based solely on simple, local, compositional rules.

    For instance, in @nikomatsakis's "Problem 0", it wouldn't check at the definition site of outer_fn, "are there any pub uses of this in outer modules?", to determine whether or not it is allowed to refer to the privately-declared OuterStruct in its signature. mod inner is exposing pub outer_fn to its parent module (outer), and what to do with it from there (whether or not to expose it further) is entirely outer's concern. The privacy checker sees that the type OuterStruct is public to the parent module, so the definition is accepted.

    If in fact there were a pub use self::inner::outer_fn in outer, then the check for whether this pub use is allowed -- whether the types in the signature of item being re-exported are public to its parent module -- would happen at that point. (I think this might be equivalent to substituting the signature of the pub use's target in place of the pub use, and re-checking it there.) The privacy checker would see that OuterStruct is not public to outer's parent module, and so the pub use would be rejected. (Likewise, if it were pub mod inner instead of private, the relevant test would be, "Is mod outer allowed to declare this module public? Are the types in the signatures of its public items public to outer's parent module?" -- whether or not its parent module (or maybe I should say owner) declares it public is not inner's concern.)

  • But that's water under the bridge. I don't have any well-defined feelings about this pub(restricted) RFC, or even about whether the referencing-private-items rules in their new form are worth keeping. The new syntax that has to be learned seems undesirable, but I suppose that's banal and obvious.

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Mar 22, 2016

Member

@nikomatsakis I'll try to update doc today

Member

pnkfelix commented Mar 22, 2016

@nikomatsakis I'll try to update doc today

@pnkfelix pnkfelix merged commit 51daa98 into rust-lang:master Mar 22, 2016

@aturon aturon referenced this pull request Apr 6, 2016

Closed

Private enum variants #32770

@mitchmindtree

This comment has been minimized.

Show comment
Hide comment
@mitchmindtree

mitchmindtree Apr 6, 2016

An alternative idea for the syntax:

pub::foo item (rather than pub(foo) item).

This syntax might be more intuitively associable with paths than the proposed syntax?

mitchmindtree commented Apr 6, 2016

An alternative idea for the syntax:

pub::foo item (rather than pub(foo) item).

This syntax might be more intuitively associable with paths than the proposed syntax?

@Xion

This comment has been minimized.

Show comment
Hide comment
@Xion

Xion Apr 29, 2016

I know I'm late to the party, but I just read this RFC and noticed the section where the author isn't sure about precedents to the suggested solution.

The visibility system in Bazel (the build system) is basically identical to the this pub scoping, with some additional features that are mostly listed in the "Why not more ambitious?" section. For more information, see here.

Xion commented Apr 29, 2016

I know I'm late to the party, but I just read this RFC and noticed the section where the author isn't sure about precedents to the suggested solution.

The visibility system in Bazel (the build system) is basically identical to the this pub scoping, with some additional features that are mostly listed in the "Why not more ambitious?" section. For more information, see here.

@iqualfragile

This comment has been minimized.

Show comment
Hide comment
@iqualfragile

iqualfragile Mar 17, 2017

nomenclature idea: lets make "pub" be short for publish, if you don't specify where you publish it to everywhere, when you restrict it then you just publish your function/struct/member to them.

iqualfragile commented Mar 17, 2017

nomenclature idea: lets make "pub" be short for publish, if you don't specify where you publish it to everywhere, when you restrict it then you just publish your function/struct/member to them.

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