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

Tracking issue for RFC 2126: Clarify and streamline paths and visibility #44660

Closed
6 of 11 tasks
aturon opened this issue Sep 17, 2017 · 120 comments
Closed
6 of 11 tasks

Tracking issue for RFC 2126: Clarify and streamline paths and visibility #44660

aturon opened this issue Sep 17, 2017 · 120 comments
Assignees
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. 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.
Milestone

Comments

@aturon
Copy link
Member

aturon commented Sep 17, 2017

This is a tracking issue for the RFC "Clarify and streamline paths and visibility" (rust-lang/rfcs#2126).

Steps:

Unresolved questions:

  • How should we approach migration? Via a fallback, as proposed, or via epochs? It is probably best to make this determination with more experience, e.g. after we have a rustfix tool in hand.

  • The final syntax for absolute paths; there's more bikeshedding to be done here in a context where we can actually try out the various options. In particular, there are some real advantages to having both crate:: and extern:: paths, but ideally we could do it in a more succinct way.

@aturon aturon added B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Sep 17, 2017
@TimNN TimNN added the C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. label Sep 17, 2017
@retep998
Copy link
Member

That RFC has 4 distinct features and yet we still only have one tracking issue for all of them. Can we please not shove together disparate features like this?

@aturon
Copy link
Member Author

aturon commented Sep 17, 2017

@retep998 As explained throughout the RFC discussions, these features are connected through global design considerations. E.g., providing an external mechanism for renaming crates is motivated in part by the eventual deprecation of extern crate. We can and will gate various aspects separately (and will likely have some overlapping gates for trying out different syntaxes), but for discussion on the overall design and stabilization, it's important to keep global coherence in mind.

@retep998
Copy link
Member

retep998 commented Sep 17, 2017

Having an external mechanism to rename crates is something which has already been desired and needed for years (rust-lang/cargo#1311) and could have stood alone just fine, but instead it's being used as nothing more than a pawn to support killing off extern crate.

We've had no problems with having separate RFCs for closely related features in the past (the RFCs for repr(align(N)) and repr(packed(N)) come to mind), yet now we're claiming that changing foo/mod.rs to foo.rs is so closely related to extern:: and crate:: that they have to be in the same RFC and use the same tracking issue?

@rpjohnst
Copy link
Contributor

Just to make sure this point sticks around, since it's a relatively subtle aspect of the syntax unresolved question: Using crate as a visibility modifier as well as a path prefix introduces a parsing ambiguity between crate ::absolute::path and crate::relative::path. Using a contextual keyword introduces the same ambiguity, and there are no other reserved keywords that really make sense as visibility modifiers.

I would thus like to (at least) experiment with leaving out the crate visibility modifier and sticking with the existing pub(crate).

@rpjohnst
Copy link
Contributor

I wouldn't mind splitting the foo.rs/foo/mod.rs point into a separate tracking issue, since it truly does seem independent of the path and visibility modifier changes.

As far as external crate renaming... there already is a separate issue for it? It is pretty important for the path changes so I think it's fine being part of this one as well.

@neon64
Copy link

neon64 commented Sep 18, 2017

Despite lots of discussion a few weeks ago regarding the choice of crate as a visibility modifier (not a path prefix), I'm disappointed to see that although this choice of keyword was listed as an 'unresolved question' in the RFC, it has now been apparently forgotten. I myself, and several others I've noted, find this choice confusing since it isn't an adjective/modifier, and can also be ambiguous: does crate mean part of the crate's public API? No! It means local or internal or pub(lished) to the crate (hence why I prefer the latter keywords). So I'm not demanding immediate change, but at least acknowledge it as an unresolved question in this tracking issue, so that it is not forgotten upon stabilisation.

It's great that this module redesign has progressed so far, but at the same time it's important that we don't blunder ahead just to make the 'impl period', and end up making decisions without consulting the majority of Rust users. And, unfortunately, I think the people involved in Github RFC discussions aren't representative of the whole user base, because of the sheer flood of information/comments/opinions there can be discouraging. So somehow that needs to be dealt with too.

@lilianmoraru
Copy link

It isn't clear how the implementation ended-up...

@vitiral
Copy link
Contributor

vitiral commented Sep 18, 2017

@rpjohnst @retep998 I have opened a new RFC to discuss foo.rs + foo/ as well as propose a few improvements to this RFC.

Edit: I suggest we open another RFC to discuss crate as a visibility modifier. Personally I would like to see the opposite: pub(extern) be added and required for all externally published symbols. This would make bare pub be the equivalent of pub(crate) in the next epoch.

@est31
Copy link
Member

est31 commented Sep 18, 2017

@rpjohnst

introduces a parsing ambiguity between crate ::absolute::path and crate::relative::path

Isn't crate ::relative::path invalid code anyway? Can't it be rejected during parsing?

@rpjohnst
Copy link
Contributor

rpjohnst commented Sep 18, 2017

@est31 No, that requires doing name resolution during parsing which is definitely not something we want to do. It's one of the more annoying parts of parsing C and C++, which have similar ambiguities around a * b being a multiplication or a declaration, and around a b(c, d); being a variable declaration or a function prototype.

If we ever start allowing dependencies and top-level modules with the same name, even tangling up name resolution with parsing won't save us- crate :: some :: item could be a crate-visible item from dependency some, or a private item from top-level module some.

To keep this in perspective, we could just arbitrarily resolve the ambiguity one way or the other and require parentheses to write the other case (probably the crate-visibility absolute path case, which seems rarest), but that's still a foot-gun that we don't need if we stick with pub(crate), which already solved the syntactic ambiguity.

@petrochenkov
Copy link
Contributor

Using crate as a visibility modifier as well as a path prefix introduces a parsing ambiguity between crate ::absolute::path and crate::relative::path.

This is a tiny issue, IMO, given that both visibilities on tuple struct fields and "inline" absolute paths are rare.
Paths are always parsed greedily currently, so it makes sense for crate :: x :: y to mean crate::x::y.
If the opposite meaning is wanted, then pub(crate) ::x::y or crate (::x::y) can be used.

@est31
Copy link
Member

est31 commented Sep 18, 2017

@rpjohnst @petrochenkov could you please share a code example where pub ::relative::path is valid code? I don't get the entire ambiguity thing because I think one of the two cases is invalid.

Edit: the only place where this could be relevant is inside macros when you match for a visibility qualifier + a path. But that's a very small breakage so acceptable IMO.

@rpjohnst
Copy link
Contributor

@est31 You seem to have the wrong idea- it's not about relative paths, they're never an issue. It's about building the AST before you know what any of the names refer to. Here's a full sample:

struct S(crate :: x :: y);

Given that you're not allowed to look up any of those names yet, how do you convert that string of characters into an AST? There are two possible answers. One has a private field of a type y defined in module x of the current crate. The other has a crate-visible field of a different type y defined at the top level of dependency x. No macros needed.

@est31
Copy link
Member

est31 commented Sep 18, 2017

@rpjohnst I see thanks for clarifying. Its indeed an ambiguity.

@CasualX
Copy link

CasualX commented Sep 19, 2017

@rpjohnst

A simple solution is to define it unambiguously parse to the crate visibility modifier. If you want to parse it as a tuple struct with private member of the given type, do it like this:

    struct S(:: crate :: x :: y);

(note the :: prefix to indicate the root 'namespace')

This is consistent with how other root namespaces need to be referred to in submodules (eg ::std::x::y).

@rpjohnst
Copy link
Contributor

rpjohnst commented Sep 19, 2017

Just disambiguating to crate-as-a-visibility would be somewhat surprising, I think. But it might be a good idea to "canonicalize" that workaround a bit and enforce that crate:: is always used with a leading :: (outside of uses of course). As you point out, this increases symmetry with ::std::x::y-like paths, and leaves the un-prefixed form for truly relative paths (self::/super::/something_in_scope::).

Should we do this? Allowing crate::x::y in non-use paths makes crate kind of magical, while enforcing ::crate::x::y scopes it to the same level as dependencies, which is what we're trying to imply anyway.

@petrochenkov
Copy link
Contributor

@rpjohnst

enforce that crate:: is always used with a leading :: (outside of uses of course)

This may be reasonable, at least for a start (nothing prevents relaxing this later).

@Zoxc
Copy link
Contributor

Zoxc commented Sep 21, 2017

I'm not a fan of the use extern::bar::foo or use crate::bar::foo syntax. It seems very noisy. I'd prefer some sugar for this. I suggest extern bar::foo for this.

@zengsai
Copy link

zengsai commented Sep 22, 2017

How about add one more implicit rule? Automatically import extern crate into root namespace, make a more coincident path expression.( I'm not good at English, pls learn my view from the following example)

e.g.

  1. Our project structure like this:

src
|--lib.rs
|--foo.rs
|--foo
|----|--bar.rs

  1. you config a dependence in Cargo.toml, like
    [dependencies] serde = "3.0.0"

  2. we add mod bar into mod foo, and add mod foo into lib.rs,

  3. we define a function finlib in lib.rs, define a function finfoo in foo.rs, define a function finbar in bar.rs

Make the extern crate's scope as the top level module in crate, then we can write code like this anywhere.

for fully/qualified path

::serde::Deserialize
::serde::x::y
::finlib                            // not ::crate::finlib
::foo::finfoo                   // not ::crate::foo::finfoo
::foo::bar::finbar           // not ::crate::foo::bar::finbar

relative path write like this

serde::Deserialize      // no need to write `use serde`
serde::x::y                   // no need to write `use serde`
finlib                          
foo::finfoo                  
bar::finbar       

we first look up serde\finlib\foo in current mod scope, if not found lookup in supper mod, until the root namespace. if there have name conflict, we write fully path instead.

also we can use self::bar::finbar in foo.rs to avoid name look up.

@rpjohnst
Copy link
Contributor

I'm not able to find the earlier thread that discussed this, but the compiler used to work that way and it caused major problems for name resolution. IIRC @pcwalton or @arielb1 probably know more.

@neon64
Copy link

neon64 commented Sep 23, 2017

Wouldn't the easiest solution to this ambiguity be to use a different keyword for the crate-local visibility modifier (ie: thepub(crate) replacement). For example, local, as suggested many times before in previous RFC discussions. Then struct S(local :: x :: y) is distinct from struct S(crate :: x :: y).

@rpjohnst
Copy link
Contributor

That just introduces another ambiguity between <visibility> <absolute path> and <relative path starting with the new keyword>, since the new keyword would have to be contextual.

@neon64
Copy link

neon64 commented Sep 24, 2017

@rpjohnst ah damn.. But isn't that a problem the epochs were designed to solve? Eg: in the current epoch we just use pub(crate), and then, in the next epoch, introduce a new non-contextual keyword that is more 'ergonomic'.

@johnthagen
Copy link
Contributor

@parasyte The problem I see with export is that it could be confused with "this crate exports ____" (which is what pub is for), but I see how you're advocating for a different perspective.

It seems like most people agree pub(crate) isn't ideal, and many have raised concerns that the noun keyword crate, which now used in other contexts, can be jarring as a replacement. Making sure we've fully considered other (potentially new) keywords seems like it would be a very good use of time before Rust 2018 sets this in stone.

I haven't heard any "official" feedback if this is still open for discussion though?

@jonhoo
Copy link
Contributor

jonhoo commented Aug 2, 2018

In the interest of introducing some more words along the lines of @parasyte's suggestion (I agree with @johnthagen that export seems more like pub than pub(crate)):

shared use ::generic::{Combine, Func};
shared struct ColoredText {
    export color: types::Color,
    export text: &'static str,
}
global use ::generic::{Combine, Func};
global struct ColoredText {
    export color: types::Color,
    export text: &'static str,
}

We could also follow in Java's footsteps and use something like protected, but I'm not sure that that's particularly easy to intuit the understanding of. There's also local (in the sense of crate-local), but I think global is actually less likely to confuse (e.g., local could be file-local).

@mark-i-m
Copy link
Member

mark-i-m commented Aug 2, 2018

How would people feel about pub(cr) or even just cr?

@CasualX
Copy link

CasualX commented Aug 2, 2018

Using crate seems too good to pass up on. It is already a keyword, its previous use in extern crate is going away. Its other use is already related to visibility pub(crate).

If you squint a little using crate as an adjective doesn't sound that disconcerting. Words such as 'house' can be used as adjectives just fine. IANAEnglishProfessor.

I think reexporting a crate visible item isn't super problematic. Personally I've only ever reexported an item in a direct parent of the module which defines it, which means you (probably) want to use self instead of crate (seanmonstar's case of code organizing notwithstanding). Eg.

    mod detail {
        crate struct Foo;
    }

    crate use self::detail::Foo;

Using crate as a visibility combined with absolute path to a type (again using seanmonstar's example) does look more problematic: crate struct Foo(crate crate::Bar); as opposed to pub(crate) struct Foo(pub(crate) ::Foo);. My only hope is that this construct isn't popular and thus transitioning won't create this crate soup. Users can avoid this by explicit importing:

use crate::Bar;

crate struct Foo(crate Bar);

@parasyte
Copy link

parasyte commented Aug 2, 2018

I do like the suggestion of share or something like it. give, provide, deliver, offer, serve, post, forward ... If it must be an adjective, these can all be transformed with a suffix: shared, givable, providable, etc.

To pick on JavaScript for a moment, ES6 doesn't even have a concept of package versus module built into the language. There are only modules. The export keyword in ES6 always exports the reference from the module; it's then up to another module to import that reference by path if it wants to use it.

This isn't greatly different from pub and use, respectively. and I guess that's where some of the confusion would come from with using an export keyword. pub(crate) is actually kind of niche. Which is why I opted to just use pub in the past. ;(

@Centril
Copy link
Contributor

Centril commented Aug 6, 2018

The issue of the crate visibility modifier has been extracted out to #53120. Further debate and decisions should continue over there.

@Centril
Copy link
Contributor

Centril commented Aug 6, 2018

The issue of the mod.rs changes has been extracted out to #53125. Further debate and decisions should continue over there.

@Centril
Copy link
Contributor

Centril commented Aug 6, 2018

The issue of the extern crate deprecation as well as having use crate_name::foo and crate_name::foo Just Work™ has been extracted out to #53128. Further debate and decisions should continue over there.

@Centril
Copy link
Contributor

Centril commented Aug 6, 2018

The issue of picking a Module Path System has been extracted out to #53130.

@Centril
Copy link
Contributor

Centril commented Aug 6, 2018

Having extracted out all bits from this issue into separate issues; I am hereby closing this one.

@sanmai-NL
Copy link

@Centril: https://doc.rust-lang.org/unstable-book/print.html#extern_prelude links here. Where is this being discussed? If this info is indeed missing, could you add it to the OP?

@Centril
Copy link
Contributor

Centril commented Sep 20, 2018

@sanmai-NL can you refresh my memory on what the "extern prelude" was?

If it is just the ability to do use some_crate::foo::bar; then that should be #53128.

@sanmai-NL
Copy link

Some of it is being discussed in #54230.

@Centril
Copy link
Contributor

Centril commented Sep 20, 2018

@sanmai-NL I believe that issue is mainly for diagnostics.

@eddyb
Copy link
Member

eddyb commented Sep 20, 2018

extern_prelude is crate_name::foo::bar outside of use (import) paths.

@petrochenkov
Copy link
Contributor

@Centril

can you refresh my memory on what the "extern prelude" was?

In general, "prelude" is currently used for all names that are in scope for the whole crate and not attached to a specific module. (There are a lot of those actually.)

@alexreg
Copy link
Contributor

alexreg commented Sep 20, 2018

@petrochenkov So what's "extern" prelude in relation to that?

@petrochenkov
Copy link
Contributor

@alexreg
Crates passed with --extern are in scope for the whole crate while not being attached to any specific module.

@sanmai-NL
Copy link

sanmai-NL commented Sep 21, 2018

It would be great if these nuggets of info, no matter how volatile, are recorded in some official source of documentation. Esp. if there are outside mentions of the concept, e.g. in the Unstable book. I’m not saying the maintainers who implement these concepts should do that, though.

@alexreg
Copy link
Contributor

alexreg commented Sep 22, 2018

@petrochenkov Thanks, makes sense.

@shepmaster
Copy link
Member

Having extracted out all bits from this issue into separate issues; I am hereby closing this one.

@Centril Tracking issues in the current beta version link here. Would you update the original comment with the most recent information so that people don't have to spelunk the comments?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. 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