Skip to content

/rust

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

Define and Document what visibility really means #9735

Merged
merged 4 commits into from Oct 8, 2013

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@alexcrichton @pcwalton @SimonSapin @bors
@alexcrichton
Copy link
Member

alexcrichton commented Oct 6, 2013

This is the culmination and attempted resolution of #8215. The commits have many more details about implementation details and the consequences of this refinement.

I'll point out specific locations which may be possible causes for alarm. In general, I have been very happy with how things have turned out. I'm a little sad that I couldn't remove privacy from resolve as much as I did, but I blame glob imports (although in theory even some of this can be mitigated as well).
src/libstd/rt/mod.rs

// XXX: these probably shouldn't be public...
#[doc(hidden)]
pub mod shouldnt_be_public {

This comment has been minimized.

Sign in to view
Copy link
@alexcrichton

alexcrichton Oct 6, 2013

Author Member

@brson, this is alarming. Modules like std::task::spawn and std::select were accessing std::rt internals, so now they're all accessing the internals through this module instead. I'm not sure if the deprecation plans of the old apis would render this issue moot, but at least in its current state it's pretty obvious you shouldn't be using this module unless you know what you're doing.
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented Oct 6, 2013

Hmm, I also just remembered that this program is accepted when it shouldn't be, so this isn't quite ready for an r+ yet.

use foo::i::A;

pub mod foo {
    pub use foo = self::i::A;

    mod i {
        pub struct A;
    }
}

fn main() {}
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented Oct 6, 2013

Test case should now be fixed, re-ready again for review.

alexcrichton added some commits Oct 5, 2013

@alexcrichton
Extract privacy checking from name resolution 
This commit is the culmination of my recent effort to refine Rust's notion of
privacy and visibility among crates. The major goals of this commit were to
remove privacy checking from resolve for the sake of sane error messages, and to
attempt a much more rigid and well-tested implementation of visibility
throughout rust. The implemented rules for name visibility are:

1. Everything pub from the root namespace is visible to anyone
2. You may access any private item of your ancestors.

"Accessing a private item" depends on what the item is, so for a function this
means that you can call it, but for a module it means that you can look inside
of it. Once you look inside a private module, any accessed item must be "pub
from the root" where the new root is the private module that you looked into.
These rules required some more analysis results to get propagated from trans to
privacy in the form of a few hash tables.

I added a new test in which my goal was to showcase all of the privacy nuances
of the language, and I hope to place any new bugs into this file to prevent
regressions.

Overall, I was unable to completely remove the notion of privacy from resolve.
One use of privacy is for dealing with glob imports. Essentially a glob import
can only import *public* items from the destination, and because this must be
done at namespace resolution time, resolve must maintain the notion of "what
items are public in a module". There are some sad approximations of privacy, but
I unfortunately can't see clear methods to extract them outside.

The other use case of privacy in resolve now is one that must stick around
regardless of glob imports. When dealing with privacy, checking a private path
needs to know "what the last private thing was" when looking at a path. Resolve
is the only compiler pass which knows the answer to this question, so it
maintains the answer on a per-path resolution basis (works similarly to the
def_map generated).

Closes #8215
439e277
@alexcrichton
Fix existing privacy/visibility violations 
This commit fixes all of the fallout of the previous commit which is an attempt
to refine privacy. There were a few unfortunate leaks which now must be plugged,
and the most horrible one is the current `shouldnt_be_public` module now inside
`std::rt`. I think that this either needs a slight reorganization of the
runtime, or otherwise it needs to just wait for the external users of these
modules to get replaced with their `rt` implementations.

Other fixes involve making things pub which should be pub, and otherwise
updating error messages that now reference privacy instead of referencing an
"unresolved name" (yay!).
de7d143
@alexcrichton
Document visibility in the manual/tutorial 
This removes the warning "Note" about visibility not being fully defined, as it
should now be considered fully defined with further bugs being considered just
bugs in the implementation.
2c76cda
@pcwalton

This comment has been minimized.

Sign in to view
Copy link
Contributor

pcwalton commented Oct 7, 2013

This looks great to me.
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Owner Author

alexcrichton commented on 7cd6692 Oct 8, 2013

r=cmr,pcwalton
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented on 7cd6692 Oct 8, 2013

saw approval from cmr
at alexcrichton@7cd6692

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors replied Oct 8, 2013

merging alexcrichton/rust/privacy = 7cd6692 into auto

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors replied Oct 8, 2013

alexcrichton/rust/privacy = 7cd6692 merged ok, testing candidate = 6ddd011

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors replied Oct 8, 2013

fast-forwarding master to auto = 6ddd011

bors added a commit that referenced this pull request Oct 8, 2013

@bors
auto merge of #9735 : alexcrichton/rust/privacy, r=cmr 
This is the culmination and attempted resolution of #8215. The commits have many more details about implementation details and the consequences of this refinement.

I'll point out specific locations which may be possible causes for alarm. In general, I have been very happy with how things have turned out. I'm a little sad that I couldn't remove privacy from resolve as much as I did, but I blame glob imports (although in theory even some of this can be mitigated as well).
6ddd011

@bors bors closed this Oct 8, 2013

@bors bors merged commit 7cd6692 into rust-lang:master Oct 8, 2013
1 check passed

1 check passed

default all tests passed

@alexcrichton alexcrichton referenced this pull request Oct 11, 2013

Closed

No compile-time error when referencing non-public module #8505

doc/rust.md
* A crate needs a global available "helper module" to itself, but it doesn't
want to expose the helper module as a public API. To accomplish this, the root
of the crate's hierarchy would have a private module which then internally has
a "public api". Because the entire crate is an ancestor of the root, then the

This comment has been minimized.

Sign in to view
Copy link
@SimonSapin

SimonSapin Oct 14, 2013

Contributor

Isn’t the entire crate a descendant of the root?

This comment has been minimized.

Sign in to view
Copy link
@alexcrichton

alexcrichton Oct 14, 2013

Author Member

Ah yes, good catch! I'll roll this in with a pull coming up soon.
doc/rust.md
current module and its descendants, but the exact meaning of accessing an item
depends on what the item is. Accessing a module, for example, would mean looking
inside of it (to import more items). On the other hand, accessing a function
would mean that it is invoked.

This comment has been minimized.

Sign in to view
Copy link
@SimonSapin

SimonSapin Oct 14, 2013

Contributor

Doesn’t useing (re-exporting) a function or assigning it to something of type &fn… count as accessing?

This comment has been minimized.

Sign in to view
Copy link
@alexcrichton

alexcrichton Oct 14, 2013

Author Member

Hm, I believe that you are correct, I'll write up a little extra about re-exportations as well.

@jseyfried jseyfried referenced this pull request Feb 23, 2016

Merged

Check paths for privacy as they are resolved and refactor away `LastPrivate` #31824

bors added a commit that referenced this pull request Mar 3, 2016

@bors
Auto merge of #31824 - jseyfried:privacy_in_resolve, r=nikomatsakis 
This PR privacy checks paths as they are resolved instead of in `librustc_privacy` (fixes #12334 and fixes #31779). This removes the need for the `LastPrivate` system introduced in PR #9735, the limitations of which cause #31779.

This PR also reports privacy violations in paths to intra- and inter-crate items the same way -- it always reports the first inaccessible segment of the path.

Since it fixes #31779, this is a [breaking-change]. For example, the following code would break:
```rust
mod foo {
    pub use foo::bar::S;
    mod bar { // `bar` should be private to `foo`
        pub struct S;
    }
}

impl foo::S {
    fn f() {}
}

fn main() {
    foo::bar::S::f(); // This is now a privacy error
}
```

r? @alexcrichton
7cee8b9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.