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.

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

permit crate as a shorthand visibility identifier #45388

Closed
nikomatsakis opened this issue Oct 19, 2017 · 13 comments
Closed

permit crate as a shorthand visibility identifier #45388

nikomatsakis opened this issue Oct 19, 2017 · 13 comments
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Oct 19, 2017

As part of #44660, we plan to support crate as a visibility modifier equivalent to pub(crate). Given that pub(crate) exists, this should be relatively straight-forward.

Visibilities are stored in the AST type Visibility. We will want to extend the Crate variant to include one additional field, indicating whether the sugar crate or the expanded form pub(crate) was used. We can add an enum like

enum CrateSugar {
    /// User wrote `pub(crate)`
    PubCrate,

    /// User wrote `crate`
    JustCrate,
}

and add this field to the Crate variant, so that it looks like Crate(Span, CrateSugar). Actually, at the same time we can remove the Span field, which appears to be unused, so it would just become Crate(CrateSugar). Doing this simultaneously is sorta bad but will save you some editing, since all existing uses look like Crate(_), and 99% of them want to ignore the CrateSugar field anyway. =)

The one user that does NOT want to ignore CrateSugar is the pretty printer. We just want to change this line to something like:

 ast::Visibility::Crate(CrateSugar::PubCrate) => self.word_nbsp("pub(crate)"),
 ast::Visibility::Crate(CrateSugar::JustCrate) => self.word_nbsp("crate"),

Naturally, we also will want to alter the parser itself, in particular the parse_visibility function. Currently, if it doesn't see the word pub, it just returns. We need to extend this to look for crate and -- if it finds it -- to return the new visibility (with sugar JustCrate). We also need to modify the existing pub(crate) code to return PubCrate for the sugar.

(Note: Please limit discussion on this issue strictly to implementation concerns relative to this particular change. Policy discussion as to whether or not to make this change -- or whether to make other changes to the module system -- belong in #44660.)

@nikomatsakis nikomatsakis added E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-compiler-front labels Oct 19, 2017
@kennytm
Copy link
Member

kennytm commented Oct 19, 2017

To implementors: Ensure tests related to :vis (#41022) are updated as well.

@nikomatsakis nikomatsakis added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. hacktoberfest labels Oct 19, 2017
@zackmdavis
Copy link
Member

I volunteer.

zackmdavis added a commit to zackmdavis/rust that referenced this issue Oct 23, 2017
With regrets, this breaks rustfmt and rls.

This is in the matter of rust-lang#45388.
bors added a commit that referenced this issue Oct 24, 2017
…, r=nikomatsakis

`crate` shorthand visibility modifier

cc #45388.

r? @nikomatsakis
@nikomatsakis
Copy link
Contributor Author

Seems like this is done!

bors added a commit that referenced this issue Nov 3, 2017
`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
@dtolnay dtolnay reopened this Apr 1, 2018
@dtolnay
Copy link
Member

dtolnay commented Apr 1, 2018

Reopening as a tracking issue because feature(crate_visibility_modifier) still refers here.

error[E0658]: `crate` visibility modifier is experimental (see issue #45388)
 --> src/main.rs:1:1
  |
1 | crate struct S;
  | ^^^^^
  |
  = help: add #![feature(crate_visibility_modifier)] to the crate attributes to enable

@dtolnay dtolnay added C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. and removed E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. hacktoberfest labels Apr 1, 2018
@nikomatsakis nikomatsakis added T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 4, 2018
@nikomatsakis
Copy link
Contributor Author

cc @rust-lang/lang @petrochenkov -- I really want to stabilize the crate visibility modifier. However, there is one issue we ought to resolve first:

struct Foo(crate :: X)

This is presently interpreted as crate (::X), I believe, as opposed to (crate::X).

I am not sure that this is correct: I would think that, since :: paths are more-or-less deprecated now that we have the extern_prelude feature, the correct interpretation would be struct Foo( crate::X ) -- that is, a single private field of type crate::X.

(But if we stabilized crate as is, people could write that sort of code, and we'd be locked in.)

@petrochenkov
Copy link
Contributor

petrochenkov commented May 4, 2018

@nikomatsakis
struct Foo(crate :: X) is already interpreted as struct Foo( (crate::X) ) actually, i.e. a single private field of type crate::X, it was interpreted like this from the start (#45771).

// `#![feature(crate_visibility_modifier)]` is not required
#![feature(crate_in_paths)] // <-- On the other hand, this is required

struct Z;

mod m {
    pub struct S(crate::Z);
    
    pub fn get_s() -> S { S(::Z) }
}

fn main() {
    m::get_s().0; // ERROR field `0` of struct `m::S` is private
}

@est31
Copy link
Member

est31 commented Jun 12, 2018

Could we get an FCP to stabilize then?

@joshtriplett
Copy link
Member

I don't think we should stabilize this until we finalize the module changes. There's still some active discussion on one last point of those: whether to write crate::foo or ::foo. If we settle on the latter (which, personally, I'd prefer) then that would change the interpretation of the struct declarations above.

Let's wait and stabilize all of them together, as a unit, in the combination we settle on.

@est31
Copy link
Member

est31 commented Jun 12, 2018

@joshtriplett do you have any pointers to places where that discussion is happening?

@joshtriplett
Copy link
Member

@est31 The last discussions I saw were on internals.rust-lang.org threads; I believe they're in a bit of a lull at the moment due to attention placed on the edition.

@Ixrec
Copy link
Contributor

Ixrec commented Jun 25, 2018

I haven't seen any discussion on leading crate vs leading :: since https://internals.rust-lang.org/t/the-great-module-adventure-continues/6678 ended in March.

@joshtriplett
Copy link
Member

@Ixrec Likewise.

@Centril
Copy link
Contributor

Centril commented Sep 15, 2018

This is a duplicate issue of #53120; so I'm closing this one.

@Centril Centril closed this as completed Sep 15, 2018
Centril added a commit to Centril/rust that referenced this issue Jan 26, 2019
…fier-issue-number, r=Centril

Change crate-visibility-modifier issue number in The Unstable Book

rust-lang#45388 is closed.
Because, it's duplicate issue of rust-lang#53120.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

9 participants