Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix spurious trait privacy error #31920

Merged
merged 3 commits into from
Mar 6, 2016
Merged

Fix spurious trait privacy error #31920

merged 3 commits into from
Mar 6, 2016

Conversation

jseyfried
Copy link
Contributor

This PR allows using methods from traits that are visible but are defined in an inaccessible module (fixes #18241). For example,

mod foo {
    pub use foo::bar::Tr;
    mod bar { // This module is inaccessible from `g`
        pub trait Tr { fn f(&self) {} }
    }
}
fn g<T: foo::Tr>(t: T) {
    t.f(); // Currently, this is a privacy error even though `foo::Tr` is visible
}

After this PR, it will continue to be a privacy error to use a method from a trait that is not visible. This can happen when a public trait inherits from a private trait (in violation of the public_in_private lint) -- see @petrochenkov's example in #28504.
r? @nikomatsakis

@jseyfried
Copy link
Contributor Author

cc @alexcrichton

jseyfried
jseyfried reviewed Feb 26, 2016
View reviewed changes
src/librustc_privacy/lib.rs
Some(ast_map::NodeForeignItem(_)) => {
self.tcx.map.get_foreign_vis(closest_private_id)
}
Some(ast_map::NodeVariant(..)) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This arm was unreachable. Variants are always DefModifiers::PUBLIC in resolve, so the LastPrivate is never a variant.

@jseyfried jseyfried force-pushed the fix_spurious_privacy_error branch from 7d12dab to ee090ec Compare February 26, 2016 20:01
@petrochenkov
Copy link
Contributor

LGTM

@alexcrichton
Copy link
Member

Weird, I thought that loop was at the core of one of the required parts of the privacy check... But hey if the tests pass looks like a great cleanup to me!

@jseyfried jseyfried force-pushed the fix_spurious_privacy_error branch 12 times, most recently from 2b3db8c to 0b06658 Compare March 2, 2016 09:59
@nikomatsakis
Copy link
Contributor

r=me

@jseyfried jseyfried force-pushed the fix_spurious_privacy_error branch from 0b06658 to a84268b Compare March 4, 2016 08:40
@jseyfried
Copy link
Contributor Author

@nikomatsakis According to my measurements, loading the public external modules eagerly slows down the stage1-cfail tests by ~14% and slows down a minimal example with crates rustc, syntax, and typeck by ~7%.

I decided to fix #21670 without slowing anything down in separate PR.

@jseyfried
Copy link
Contributor Author

@bors: r=nikomatsakis

@bors
Copy link
Contributor

bors commented Mar 4, 2016

📌 Commit a84268b has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Mar 4, 2016

⌛ Testing commit a84268b with merge 59fa34a...

@bors
Copy link
Contributor

bors commented Mar 4, 2016

💔 Test failed - auto-linux-64-x-android-t

@jseyfried
Copy link
Contributor Author

The test failed because of implicit merge conflicts (unused function)

@jseyfried jseyfried force-pushed the fix_spurious_privacy_error branch from a84268b to d908ff1 Compare March 4, 2016 18:31
@jseyfried
Copy link
Contributor Author

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Mar 4, 2016

📌 Commit d908ff1 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Mar 6, 2016

⌛ Testing commit d908ff1 with merge 45f0ce7...

bors added a commit that referenced this pull request Mar 6, 2016
@bors
Auto merge of #31920 - jseyfried:fix_spurious_privacy_error, r=nikoma…
45f0ce7 
…tsakis

This PR allows using methods from traits that are visible but are defined in an inaccessible module (fixes #18241). For example,
```rust
mod foo {
    pub use foo::bar::Tr;
    mod bar { // This module is inaccessible from `g`
        pub trait Tr { fn f(&self) {} }
    }
}
fn g<T: foo::Tr>(t: T) {
    t.f(); // Currently, this is a privacy error even though `foo::Tr` is visible
}
```

After this PR, it will continue to be a privacy error to use a method from a trait that is not visible. This can happen when a public trait inherits from a private trait (in violation of the `public_in_private` lint) -- see @petrochenkov's example in #28504.
r? @nikomatsakis
@bors
Copy link
Contributor

bors commented Mar 6, 2016

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

@bors bors merged commit d908ff1 into rust-lang:master Mar 6, 2016
@jseyfried jseyfried mentioned this pull request Mar 9, 2016
Closed
@jseyfried jseyfried deleted the fix_spurious_privacy_error branch March 25, 2016 22:56
This was referenced May 1, 2016
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nikomatsakis nikomatsakis

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Trait re-exports fail due to privacy of containing module
5 participants
@jseyfried @petrochenkov @alexcrichton @nikomatsakis @bors