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

resolve: fix the visibility of extern crates #31362

Merged
merged 4 commits into from
Feb 25, 2016
Merged

resolve: fix the visibility of extern crates #31362

merged 4 commits into from
Feb 25, 2016

Conversation

jseyfried
Copy link
Contributor

This PR changes the visibility of extern crate declarations to match that of items (fixes #26775).
To avoid breakage, the PR makes it a public_in_private lint to reexport a private extern crate, and it adds the lint inaccessible_extern_crate for uses of an inaccessible extern crate.

The lints can be avoided by making the appropriate extern crate declaration public.

@jseyfried
Copy link
Contributor Author

cc @nikomatsakis @petrochenkov

@petrochenkov
Copy link
Contributor

except that it allows private extern crate declarations to be re-exported. This exception considerably reduces breakage

It doesn't necessarily have to be a hard error immediately, you can mark such reexports with private_in_public warning (see https://github.com/rust-lang/rust/blob/master/src/librustc_resolve/resolve_imports.rs#L504 for example).
It's not like this warning is going to become an error any time soon.

@jseyfried jseyfried force-pushed the fix_extern_crate_visibility branch 2 times, most recently from 9d5ef32 to 9f6dd8e Compare February 2, 2016 11:18
@jseyfried
Copy link
Contributor Author

@petrochenkov Good point, I added another commit making it a warning.

@jseyfried jseyfried force-pushed the fix_extern_crate_visibility branch 6 times, most recently from cce80e7 to 2168764 Compare February 3, 2016 00:34
@nrc
Copy link
Member

nrc commented Feb 3, 2016

cc @rust-lang/lang

@nrc nrc added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Feb 3, 2016
nrc
nrc reviewed Feb 3, 2016
View reviewed changes
src/librustc_resolve/resolve_imports.rs
if name_binding.is_extern_crate() {
let msg = format!("extern crate `{}` is private, and cannot be reexported \
(error E0364), consider declaring with `pub`",
source);
Copy link
Member

Choose a reason for hiding this comment

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

can you move the consider... bit to a note please

Copy link
Member

Choose a reason for hiding this comment

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

oh wait, you can't it's a lint, nvm

@nrc
Copy link
Member

nrc commented Feb 3, 2016

The PR lgtm. I'll wait for a crater run and for the lang team to see this before giving r+ though.

@aturon
Copy link
Member

aturon commented Feb 3, 2016

Seems to match our previous decision on this question, so LGTM as well.

petrochenkov
petrochenkov reviewed Feb 3, 2016
View reviewed changes
src/librustc/lib.rs
@@ -68,6 +68,7 @@ extern crate serialize as rustc_serialize; // used by deriving
#[cfg(test)]
extern crate test;

#[allow(private_in_public)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is pub extern crate very buggy? I think it can be used here instead of suppressing the warning. It will report the "pub extern crate does not work as expected" warning during stage0, but that's ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, for some reason I thought that the warnings weren't ok.
pub extern crate works fine except that it is not visible to other crates (before this PR).

@bors
Copy link
Contributor

bors commented Feb 5, 2016

☔ The latest upstream changes (presumably #31386) made this pull request unmergeable. Please resolve the merge conflicts.

@jseyfried
Copy link
Contributor Author

This also closes #22146.

@bors
Copy link
Contributor

bors commented Feb 11, 2016

☔ The latest upstream changes (presumably #31461) made this pull request unmergeable. Please resolve the merge conflicts.

@jseyfried jseyfried mentioned this pull request Feb 11, 2016
Merged
@jseyfried jseyfried force-pushed the fix_extern_crate_visibility branch 2 times, most recently from ea0b0d5 to 3acbd2f Compare February 11, 2016 08:05
@bors
Copy link
Contributor

bors commented Feb 11, 2016

☔ The latest upstream changes (presumably #31487) made this pull request unmergeable. Please resolve the merge conflicts.

@nrc
Copy link
Member

nrc commented Feb 11, 2016

@brson or @alexcrichton could we get a crater run for this PR please?

@bors
Copy link
Contributor

bors commented Feb 25, 2016

⌛ Testing commit 7ad7065 with merge f06ff1a...

Manishearth added a commit to Manishearth/rust that referenced this pull request Feb 25, 2016
@Manishearth
Rollup merge of rust-lang#31362 - jseyfried:fix_extern_crate_visibili…
a48f958 
…ty, r=nikomatsakis

This PR changes the visibility of extern crate declarations to match that of items (fixes rust-lang#26775).
To avoid breakage, the PR makes it a `public_in_private` lint to reexport a private extern crate, and it adds the lint `inaccessible_extern_crate` for uses of an inaccessible extern crate.

The lints can be avoided by making the appropriate `extern crate` declaration public.
@Manishearth Manishearth mentioned this pull request Feb 25, 2016
Merged
@Manishearth Manishearth reopened this Feb 25, 2016
@bors
Copy link
Contributor

bors commented Feb 25, 2016

⌛ Testing commit 7ad7065 with merge 4a01bac...

@bors
Copy link
Contributor

bors commented Feb 25, 2016

💔 Test failed - auto-linux-64-nopt-t

bors added a commit that referenced this pull request Feb 25, 2016
@bors
Auto merge of #31882 - Manishearth:rollup, r=Manishearth
c9852e2 
- Successful merges: #31362, #31793, #31800, #31809, #31818, #31827, #31831, #31835, #31837, #31846
- Failed merges:
@alexcrichton
Copy link
Member

@bors: retry

On Thu, Feb 25, 2016 at 12:28 AM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-linux-64-nopt-t
http://buildbot.rust-lang.org/builders/auto-linux-64-nopt-t/builds/8166

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

@bors bors merged commit 7ad7065 into rust-lang:master Feb 25, 2016
@llogiq llogiq mentioned this pull request Feb 28, 2016
Closed
@SimonSapin SimonSapin mentioned this pull request Mar 5, 2016
Merged
@jseyfried jseyfried mentioned this pull request Mar 15, 2016
Closed
@jseyfried jseyfried mentioned this pull request Mar 23, 2016
Closed
@jseyfried jseyfried deleted the fix_extern_crate_visibility branch March 25, 2016 22:53
@bvssvni bvssvni mentioned this pull request Apr 6, 2016
Closed
5 tasks
@jseyfried jseyfried mentioned this pull request Apr 10, 2016
Merged
@mitchmindtree mitchmindtree mentioned this pull request Apr 17, 2016
Closed
@jgilchrist jgilchrist mentioned this pull request Apr 18, 2016
Merged
@petrochenkov petrochenkov mentioned this pull request Jun 28, 2016
Closed
5 tasks
@Moredread Moredread mentioned this pull request Sep 26, 2016
Closed
@petrochenkov petrochenkov mentioned this pull request Oct 1, 2016
Closed
3 tasks
@jseyfried jseyfried mentioned this pull request Oct 7, 2016
Merged
@jseyfried jseyfried changed the title Resolve: Fix the visibility of extern crates resolve: Fix the visibility of extern crates Feb 14, 2017
@jseyfried jseyfried changed the title resolve: Fix the visibility of extern crates resolve: fix the visibility of extern crates Feb 14, 2017
@SimonSapin SimonSapin mentioned this pull request Oct 30, 2017
Merged
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
8 participants
@jseyfried @petrochenkov @nrc @aturon @bors @alexcrichton @nikomatsakis @Manishearth