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

`unreachable-pub` lint (as authorized by RFC 2126) #45569

Merged
merged 1 commit into from Nov 3, 2017

Conversation

Reviewers

@estebank estebank

@petrochenkov petrochenkov

Assignees

@petrochenkov petrochenkov

Labels
S-waiting-on-author
Projects
None yet
Milestone
No milestone
5 participants
@zackmdavis @petrochenkov @bors @kennytm @estebank
@zackmdavis
Copy link
Member

zackmdavis commented Oct 27, 2017 •
edited

To whom it may concern:

RFC 2126 commissions the creation of a lint for pub items that are not visible from crate root (#45521). We understand (but seek confirmation from more knowledgable compiler elders) that this can be implemented by linting HIR items that are not cx.access_levels.is_exported cx.access_levels.is_reachable but have a vis (-ibility) field of hir::Visibility::Public.

The lint, tentatively called unexported-pub unreachable-pub (with the understanding that much could be written on the merits of various names, as it is said of the colors of bicycle-sheds), suggests crate as a replacement for pub if the crate_visibility_modifier feature is enabled (see #45388), and pub(crate) otherwise. We also use help messaging to suggest the other potential fix of exporting the item; feedback is desired as to whether this may be confusing or could be worded better.

As a preview of what respecting the proposed lint would look like (and to generate confirmatory evidence that this implementation doesn't issue false positives), we take its suggestions for libcore (save one, which is deferred to another pull request because it brings up an unrelated technical matter). I remain your obedient servant.

unexported_pub

r? @petrochenkov

@rust-highfive rust-highfive assigned petrochenkov Oct 27, 2017

@zackmdavis zackmdavis referenced this pull request Oct 27, 2017

Merged

regenerate libcore/char_private.rs #45571

@kennytm kennytm added the S-waiting-on-review label Oct 27, 2017

@petrochenkov

petrochenkov reviewed Oct 27, 2017
View changes

src/libcore/fmt/builders.rs Outdated
pub fn debug_struct_new<'a, 'b>(fmt: &'a mut fmt::Formatter<'b>,
name: &str)
-> DebugStruct<'a, 'b> {
pub(crate) fn debug_struct_new<'a, 'b>(fmt: &'a mut fmt::Formatter<'b>,

This comment has been minimized.

Sign in to view
Copy link
@petrochenkov

petrochenkov Oct 27, 2017

Contributor

I'd avoid changing libcore until the crate sugar for pub(crate) is available in bootstrap compiler.
@petrochenkov

petrochenkov reviewed Oct 27, 2017
View changes

src/librustc_lint/builtin.rs Outdated

impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnexportedPub {
fn check_item(&mut self, cx: &LateContext, item: &hir::Item) {
if !cx.access_levels.is_exported(item.id) && item.vis == hir::Visibility::Public {

This comment has been minimized.

Sign in to view
Copy link
@petrochenkov

petrochenkov Oct 27, 2017

Contributor

As I mentioned on gitter, this should use is_reachable instead of is_exported.
With is_exported this lint will give false positives on anything that is leaked through any other means than use (type aliases, return types, field types). All these things still need to be pub to be usable from other crates.

This comment has been minimized.

Sign in to view
Copy link
@petrochenkov

petrochenkov Oct 28, 2017

Contributor

Test to add:

mod m {
    pub struct S; // The lint should not fire on this struct
}

pub fn f() -> m::S { m::S }
@petrochenkov

petrochenkov reviewed Oct 28, 2017
View changes

src/librustc_lint/builtin.rs Outdated
pub struct UnexportedPub;

declare_lint! {
UNEXPORTED_PUB,

This comment has been minimized.

Sign in to view
Copy link
@petrochenkov

petrochenkov Oct 28, 2017

Contributor

I'd actually rename the lint into unreachable_pub (+ update all the comments, etc).
@petrochenkov

petrochenkov reviewed Oct 28, 2017
View changes

src/librustc_lint/builtin.rs Outdated
if !cx.access_levels.is_exported(item.id) && item.vis == hir::Visibility::Public {
let mut err = cx.struct_span_lint(UNEXPORTED_PUB, item.span, "unexported `pub` item");
err.help("consider exporting it for use by other crates");
// `pub` is three chars at start of declaration

This comment has been minimized.

Sign in to view
Copy link
@petrochenkov

petrochenkov Oct 28, 2017

Contributor

This is a bold claim!
pub can be passed to an item through a $vis matcher, for example.

This comment has been minimized.

Sign in to view
Copy link
@petrochenkov

petrochenkov Oct 28, 2017

Contributor

Could you add a test for this case? Something like

mod m {
    macro_rules! m {
        ($visibility: vis) => { $visibility struct S; }
    }

    m!(pub);
}
@petrochenkov

petrochenkov reviewed Oct 28, 2017
View changes

src/librustc_lint/builtin.rs Outdated
fn check_item(&mut self, cx: &LateContext, item: &hir::Item) {
if !cx.access_levels.is_exported(item.id) && item.vis == hir::Visibility::Public {
let mut err = cx.struct_span_lint(UNEXPORTED_PUB, item.span, "unexported `pub` item");
err.help("consider exporting it for use by other crates");

This comment has been minimized.

Sign in to view
Copy link
@petrochenkov

petrochenkov Oct 28, 2017

Contributor

Looking at the tests, "or consider exporting it for use by other crates" would look better.
@petrochenkov

petrochenkov reviewed Oct 28, 2017
View changes

src/librustc_lint/builtin.rs Outdated
}

impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnexportedPub {
fn check_item(&mut self, cx: &LateContext, item: &hir::Item) {

This comment has been minimized.

Sign in to view
Copy link
@petrochenkov

petrochenkov Oct 28, 2017

Contributor

check_item doesn't cover pub on impl items, foreign items and struct fields.
@petrochenkov

petrochenkov reviewed Oct 28, 2017
View changes

src/librustc_lint/builtin.rs Outdated
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnexportedPub {
fn check_item(&mut self, cx: &LateContext, item: &hir::Item) {
if !cx.access_levels.is_exported(item.id) && item.vis == hir::Visibility::Public {
let mut err = cx.struct_span_lint(UNEXPORTED_PUB, item.span, "unexported `pub` item");

This comment has been minimized.

Sign in to view
Copy link
@petrochenkov

petrochenkov Oct 28, 2017

Contributor

def_span will probably look better here (see an example of use in #44847)

@zackmdavis zackmdavis force-pushed the zackmdavis:unexported_pub_lint branch from 2e7a731 to 7edd04d Oct 30, 2017

@zackmdavis

This comment has been minimized.

Sign in to view
Copy link
Member Author

zackmdavis commented Oct 30, 2017

updated (force-push)

@zackmdavis zackmdavis changed the title `unexported-pub` lint (as authorized by RFC 2126) `unreachable-pub` lint (as authorized by RFC 2126) Oct 30, 2017

@estebank

estebank reviewed Oct 31, 2017
View changes

Copy link
Contributor

estebank left a comment

Could you do a squash of your commits while you're at it?
src/test/ui/lint/unreachable_pub.rs
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![feature(crate_visibility_modifier)]

This comment has been minimized.

Sign in to view
Copy link
@estebank

estebank Oct 31, 2017

Contributor

Could you add a test without this feature enabled to make sure there're no regressions going forward with the pub(crate) suggestion?
@petrochenkov

This comment has been minimized.

Sign in to view
Copy link
Contributor

petrochenkov commented Nov 1, 2017

r=me with @estebank's comment (#45569 (comment)) addressed

@kennytm kennytm added S-waiting-on-author and removed S-waiting-on-review labels Nov 1, 2017

@zackmdavis
unreachable-pub lint for `pub` items not reachable from crate root 
This is with deepest thanks to Vadim Petrochenkov for thorough review, and
resolves #45521.
Loading status checks…
085ec6d

@zackmdavis zackmdavis force-pushed the zackmdavis:unexported_pub_lint branch from 7edd04d to 085ec6d Nov 3, 2017

@zackmdavis

This comment has been minimized.

Sign in to view
Copy link
Member Author

zackmdavis commented Nov 3, 2017

(added non-crate_visibility_modifier UI test and squashed) 🏁 💖
@petrochenkov

This comment has been minimized.

Sign in to view
Copy link
Contributor

petrochenkov commented Nov 3, 2017

@bors r+
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Nov 3, 2017

📌 Commit 085ec6d has been approved by petrochenkov
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Nov 3, 2017

⌛️ Testing commit 085ec6d with merge 9140b5d...

bors added a commit that referenced this pull request Nov 3, 2017

@bors
Auto merge of #45569 - zackmdavis:unexported_pub_lint, r=petrochenkov 
`unreachable-pub` lint (as authorized by RFC 2126)

To whom it may concern:

RFC 2126 commissions the creation of a lint for `pub` items that are not visible from crate root (#45521). We understand (but seek confirmation from more knowledgable compiler elders) that this can be implemented by linting HIR items that are _not_ ~~`cx.access_levels.is_exported`~~ `cx.access_levels.is_reachable` but have a `vis` (-ibility) field of `hir::Visibility::Public`.

The lint, tentatively called ~~`unexported-pub`~~ `unreachable-pub` (with the understanding that much could be written on the merits of various names, as it is said of the colors of bicycle-sheds), suggests `crate` as a replacement for `pub` if the `crate_visibility_modifier` feature is enabled (see #45388), and `pub(crate)` otherwise. We also use help messaging to suggest the other potential fix of exporting the item; feedback is desired as to whether this may be confusing or could be worded better.

As a preview of what respecting the proposed lint would look like (and to generate confirmatory evidence that this implementation doesn't issue false positives), ~~we take its suggestions for `libcore`~~ (save one, which is deferred to another pull request because it brings up an unrelated technical matter). I remain your obedient servant.

![unexported_pub](https://user-images.githubusercontent.com/1076988/32089794-fbd02420-baa0-11e7-87e5-3ec01f18924a.png)

r? @petrochenkov
Loading status checks…
9140b5d
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Nov 3, 2017

💔 Test failed - status-travis
@kennytm

This comment has been minimized.

Sign in to view
Copy link
Member

kennytm commented Nov 3, 2017

@bors retry
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Nov 3, 2017

⌛️ Testing commit 085ec6d with merge 59d4845...

bors added a commit that referenced this pull request Nov 3, 2017

@bors
Auto merge of #45569 - zackmdavis:unexported_pub_lint, r=petrochenkov 
`unreachable-pub` lint (as authorized by RFC 2126)

To whom it may concern:

RFC 2126 commissions the creation of a lint for `pub` items that are not visible from crate root (#45521). We understand (but seek confirmation from more knowledgable compiler elders) that this can be implemented by linting HIR items that are _not_ ~~`cx.access_levels.is_exported`~~ `cx.access_levels.is_reachable` but have a `vis` (-ibility) field of `hir::Visibility::Public`.

The lint, tentatively called ~~`unexported-pub`~~ `unreachable-pub` (with the understanding that much could be written on the merits of various names, as it is said of the colors of bicycle-sheds), suggests `crate` as a replacement for `pub` if the `crate_visibility_modifier` feature is enabled (see #45388), and `pub(crate)` otherwise. We also use help messaging to suggest the other potential fix of exporting the item; feedback is desired as to whether this may be confusing or could be worded better.

As a preview of what respecting the proposed lint would look like (and to generate confirmatory evidence that this implementation doesn't issue false positives), ~~we take its suggestions for `libcore`~~ (save one, which is deferred to another pull request because it brings up an unrelated technical matter). I remain your obedient servant.

![unexported_pub](https://user-images.githubusercontent.com/1076988/32089794-fbd02420-baa0-11e7-87e5-3ec01f18924a.png)

r? @petrochenkov
Loading status checks…
59d4845
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Nov 3, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing 59d4845 to master...

@bors bors merged commit 085ec6d into rust-lang:master Nov 3, 2017
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

bors added a commit that referenced this pull request Nov 7, 2017

@bors
Auto merge of #45571 - zackmdavis:regenerate_char_private, r=alexcric… 
…hton

regenerate libcore/char_private.rs

(filed separately from the work in #45569, because of this matter of the updated Unicode data; see also #45567)

char_private.rs is generated programmatically by char_private.py, using data retrieved from the Unicode Consortium's website.

The motivation here was to make `is_printable` crate-visible (with `pub(crate)`), but it would seem that the Unicode data has changed slightly since char_private.rs was last generated.
Loading status checks…
a17e724

@zackmdavis zackmdavis deleted the zackmdavis:unexported_pub_lint branch Jan 13, 2018

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.