rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.36k stars 12.72k forks source link

(Modules) Tracking issue for `crate` as a visibility modifier #53120

Closed Centril closed 2 years ago

Centril commented 6 years ago

This is a sub-tracking issue for the RFC "Clarify and streamline paths and visibility" (rust-lang/rfcs#2126) dealing with the question of crate as a visibility modifier.

Unresolved questions:

letheed commented 6 years ago

IMO our and my have the same weakness as local and internal: they're not very clear about their scope. local in particular is quite confusing, since the scope would be radically different from local variables where local means private to the enclosing scope, which for an item would be the module, not the crate. I find internal a bit unspecific. Internal to what? The module? The type? The crate? It may be obvious for those who come from languages where it is used but it won't necessarily be for others. our and my are even more vague. In contrast crate is very clear about the scope.

Regarding pub(extern), I actually have a question. Does it make any sense to ever have extern "C" fn foo() {}, without the pub in front? Because if not, we could reuse extern fn foo() {} for our regular, non "C" abi Rust functions as well. I was thinking that we could unify that and not keep the extern syntax special for FFIs. That would mean extern now desugars to pub(extern), pub stays the same as today but accepts an optional ABI string when the item supports it, and a lint for pub items being exported without pub(extern) or extern.

extern fn foo() {
    println!("Just called a Rust function from Rust!");
}

#[no_mangle]
extern "C" fn foo_from_c() {
    println!("Just called a Rust function from C!");
}

I haven't seen that mentioned in the threads I've read so apologies if this has been discussed before!

rpjohnst commented 6 years ago

Does it make any sense to ever have extern "C" fn foo() {}, without the pub in front?

Yes- sometimes you only want to use foo as a function pointer, for example. And in fact the syntax extern fn foo() {} can't be reused for this anyway because extern without "C" defaults to the C ABI, and this is considered idiomatic at least by some.

ghost commented 6 years ago

Here's a suggestion that came up long time ago - maybe we can give it another chance now that some aspects of the new module system have been crystallized?

// Public to the world.
pub struct Foo;

// Private to the crate.
priv struct Foo;

// Basically not visible at all (only inside the module).
struct Foo;

I believe this makes sense if one thinks of:

Some people had a knee-jerk reaction to priv not being the most restrictive one, but I want to make two points about that:

  1. In Rust, modules are used to organize things under namespaces, while crates are used to define interfaces. So if a crate is a "unit of API", then it makes sense for visibility modifiers to talk about crates primarily.

  2. I think Java made a mistake of private meaning "private to the class" and no visibility modifier meaning "visible inside the package". To me, it makes more sense for no modifier (i.e. the default) to be the most restrictive.

stevensonmt commented 6 years ago

@stjepang I would argue that in most other circumstances "private" is going to have a more restrictive connotation than an unmodified state. The private-default-public spectrum in a general sense is analogous to protected-available-advertised.

A private club is more exclusive than a club. A private act implies that some effort was taken to hide from general view. A private thought should be a redundant concept given the failure of evolution to endow us with telepathy, but most people recognize it to mean a thought intended not to be shared.

As an inexperienced language learner, I would also suggest that an additional keyword is less of a cognitive burden than a single keyword with multiple context-dependent meanings. See Costello, L and Abbot, B "Who's on first", 1938.

joshtriplett commented 6 years ago

I feel that my and our would have connotations we don't want, leaving aside the difficulty of reserving these as keywords. They make for an amusing joke, but I don't think we should go that route.

I honestly feel like people would learn what crate visibility means. I think the bigger problem comes from code that becomes difficult to read or ambiguous to parse:

crate struct S(crate crate::Foo);

crate struct S(crate ::Foo);

I personally don't see those as showstoppers, but they're definitely legitimate concerns here.

seanmonstar commented 6 years ago

There's a parallel of pub(path) in the Scala language, which is private[path], which acts pretty much the same. It reads slightly differently, saying "this item is private, only allowing people within $path to view it". But making something private in Scala requires an annotation, as the default is public, which isn't the case in Rust.

rpjohnst commented 6 years ago

It occurs to me that the C++ concept of friends is also similar to pub(path), as another point of precedent.

gdzx commented 6 years ago

Ultimately, the issues are:

  1. the crate keyword is used for both relative paths imports and as a visibility modifier,
  2. the crate keyword prevents a unified visibility modifier syntax like pub(...),
  3. some dislike the pub(crate) syntax (too long, looks like a function call).

After 5 minutes of really deep thinking, I came up with the following... :P

Special Syntax for the Visibility Modifiers

(using @ as an example)

@pub use crate::Foo;

@crate struct Bar;

I personally find that quite ugly and I would not want to type either @crate or pub(crate).

Distinct Visibility Modifiers Keywords

Since there's already the bare pub and extern, I think the crate keyword fits in quite well (unsurprising when coming from languages with the usual public, protected, private keywords).

crate struct Foo;

crate fn path() -> PathBuf { ... }

But as I said, that doesn't work well with the crate import prefix and I'm starting to feel like we got this backwards. I feel the real issue lies with the path clarity changes, for instance, with no syntax highlighting:

use crate::utils;

looks like crate hasn't any special meaning.

With great inspiration from the declarative macros syntax, instead of finding some kind of unified syntax for visibility modifiers, I would rather have the following:

use std::io;
use std::path::Path;

use log::info;

use $crate::utils;

crate fn hello() -> io::Result<()> {
    utils::rm_rf(Path::new("/"))?;
    info!("Goodbye, World!");
}

(self, super and crate special anchors for paths imports would need a prefix, for instance, $crate, which makes it quite clear they are special and look like variables)

Convoluted example:

crate struct Foo(crate crate::Bar);

becomes:

crate struct Foo(crate $crate::Bar);
Centril commented 6 years ago

It appears there's a duplicate tracking issue: https://github.com/rust-lang/rust/issues/45388. Closing that one in favor of this one.

czipperz commented 5 years ago

Is there a reason the rustc implementation is dogfooding this feature? Every instance of crate fn could be changed to pub(crate) fn and stop relying on an unstable feature AFAICT.

Centril commented 5 years ago

I don't see a good reason why we'd do that. Both Clippy and rustc use unstable features all the time and they should because it allows us to test them more extensively both in terms of catching bugs in the implementation and because we can get a feeling for how it is to use them.

In this case, usage of crate as a visibility modifier in Clippy and in rustc is in my view showing that it reads better than pub(crate) and that most of the issues referenced in this tracking issue are non-issues in practice. I think https://github.com/rust-lang/rust/issues/53120#issuecomment-413466129, https://github.com/rust-lang/rust/issues/53120#issuecomment-414392549, and https://github.com/rust-lang/rust/issues/53120#issuecomment-413498376 also suggest that crate as a visibility modifier works well outside as well.

As it works well in practice, since many of us in the language team feel positive about it, and because it was RFC accepted, I think we should consider stabilizing crate as a visibility modifier after considering how we should parse struct Foo ( crate :: foo :: Bar ) (currently as a path, and probably that's right).

parasyte commented 5 years ago

FWIW, I use this feature in cargo-n64 and it has been very nice!

petrochenkov commented 5 years ago

I use crate in rustc all the time because it's short and doesn't have parentheses like pub(crate). I still don't like how it reads though. It also doesn't have 3 letters, so formatting changes together with pub <-> crate changes. I still like our unironically.

Centril commented 5 years ago

@petrochenkov My understanding of what you wrote is that you prefer crate over pub(crate) but would like something better than crate as well. Is that an accurate assessment?

johnthagen commented 5 years ago

FWIW, I still think intern is a good option.

That being said, I also agree that pub(crate) should be replaced with something.

cramertj commented 5 years ago

well it's settled then-- three letters are nice, and "intern" is nice, so... int. ;)

joshtriplett commented 5 years ago

Could we please have a lint for the crate ::T (with a space) case?

jhpratt commented 5 years ago

@Centril A few of us were just discussing the way forward on #rocket. Based on your recent-ish comment, is there a possibility of a proposed FCP in the near future?

Centril commented 5 years ago

@jhpratt I've been meaning to do a write-up and eventually propose it but I've been busy with other things. I'll try to find some time in the near future.

ratijas commented 4 years ago

So, I was just browsing issues, looking for something about macros, and found this one.

Now that we have pub(crate) in stable for a long time already, I wonder, shouldn't this issue be closed?

nikomatsakis commented 4 years ago

Interestingly, this just came up at a recent lang-team meeting, where as discussed this as a possible issue that we'd like to revive.

Speaking personally, I definitely miss being able to write crate fn and crate foo: T on fields and that sort of thing. It's not a big syntactic diff from pub(crate) but I find it makes the code much more readable, particularly in structs that have a lot of public fields. I also find it contributes to a reasonable helpful, simplified "privacy model" --

In my view, there is some intersection then between this keyword and the changes envisioned in https://github.com/rust-lang/rust/issues/48054, and I would prefer to ensure that we adopt them together. I forget the details but I remember that there were errors that one would get when trying to put the above model into practice.

I think the first step towards this would be for someone to try and do a write-up documenting the history and making sure we've highlighted all the concerns that were raised.

One specific concern that I do remember is the syntactic ambiguity of

struct Foo(crate ::x)

Today, this is accepted and crate ::x parses as the path crate::x, but plausibly the user meant crate to serve as a visibility modifier with ::x as a path.

I would be inclined to re-introduce the crate modifier and to keep the parsing of the above case just as it is today. Since Rust 2018, ::foo paths have been largely deprecated -- they still exist and can be useful in certain specific contexts, like macros, but most of the term we now encourage absolute paths that look like crate_name::b, where crate_name might be either the keyword crate or the name of some other crate. So it's pretty likely that crate::x (ignoring whitespace) actually was meant as a path and hence the current parsing is correct.

If we want to address the potential user-confusion, then I think a whitespace-sensitive lint is a reasonably good idea. In other words, struct Foo(crate ::x) would warn, but struct Foo(crate::x) would not, although both of them are accepted and equivalent.

ratijas commented 4 years ago

Personally, I prefer the unified syntax and simple lookahead(1) parser more; Also, a crate is a crate, and there's crates.io out there, and all that stuff has nothing to do with visibility directly — only in context of pub(_).

But well, what do I know? It's just yet another point of view. You guys are undoubtedly have more experience and more valuable opinion.

johnthagen commented 4 years ago

@nikomatsakis Curious if you had any thoughts on intern for this use case (or is reserving a new keyword out of scope at this point?).

seanmonstar commented 4 years ago

After a couple years, my feeling is still the same: optimize for readability. I find that I read code much more than I write it. And so, my opinion is that it's clearer to me to read pub(scope), which can be pub, pub(crate), pub(super), pub(in proto::h1).


However, I don't think we're really adding anything new to conversation with these opinions, I'm sure we've said it all in earlier comments. Should the decision be based on something new? Or said another way, how do we decide that now the decision to adopt this syntax should be yes when it was no or postpone a couple years ago?

SimonSapin commented 4 years ago

I also find it contributes to a reasonable helpful, simplified "privacy model" --

  • structs, fields are either local to a module (very narrow reasoning)
  • or they are used somewhere within the current crate (crate, have to ripgrep around)
  • or they are public to the world (pub)

I fail to understand the understand the benefit of this simplification and how it is worth the loss in expressive power. For what it’s worth I often make items public to their parent modules (so that sibling modules can use them) but not the entire crate. Public to the grand-parent module less so, but still occasionally.

I feel that deprecating pub(super) would be a significant loss.


Separately, I’m not thrilled about using the noun crate as a qualifier that modifies an item that can exist independently. For comparison: pub(lic), unsafe, and const(ant) are adjectives. They nouns already used as keywords in item definitions are not qualifiers but indicate the nature of that item: function, trait, module, …

Crates are already a concept we deal with, but in this proposal an item definition that starts with crate does not define a new crate.

matklad commented 4 years ago

A tiny new bit of info:

rust-analyzer provides completions and assists for adding pub(crate), which (subjectively) make it much less annoying to type, at three keystrokes.

nikomatsakis commented 4 years ago

I agree that little new information is being added here. I think a good next step, if we want to champion the proposal, would be to go back, summarize the outstanding concerns, and bring it before the lang team to try and reach a final decision. I would certainly prefer to either accept or reject this proposal, I am tired of having it hang in limbo.

jhpratt commented 4 years ago

From quickly looking over the entirety of this issue, I think @nikomatsakis's recent comment sums things up pretty well. Aside from a select few people still supporting things like intern, crate seems to be the most logical (and was accepted in RFC 2126). The only truly outstanding issue is how to parse fn foo(crate ::bar).

A warn-by-default whitespace-sensitive lint seems the most logical place to start. crate ::bar is currently parsed as a path, and would have to stay so in both Rust 2015 and 2018. I'd imaging it would be a candidate for a breaking change in an edition, however.

petrochenkov commented 4 years ago

The crate ::bar case is entirely unimportant in practice and already behaves consistently with the rest of the language. It has consistently been the red herring of the discussion, please don't focus attention on it.

petrochenkov commented 4 years ago

I would certainly prefer to either accept or reject this proposal, I am tired of having it hang in limbo.

I think I'd vote for the rejection. I've been consistently using crate instead of pub(crate) in rustc and despite being shorter it still looks out of place most of the time, especially on fields or imports, and choosing between crate and pub(crate) feels like choosing between two evils while not being sure which is the lesser one.

ratijas commented 4 years ago

Yeah. What's more important, is what @SimonSapin mentioned that crate bar indeed does not define a new crate, despite it reads like if it does.

jhpratt commented 4 years ago

@petrochenkov You don't think a lint makes sense? Personally, if I saw crate ::bar without seeing this discussion, I'd expect it to behave like pub(crate) ::bar. I think allowing whitespace in paths at all is confusing.

ratijas commented 4 years ago

@jhpratt It would be hard to deny whitespace between path segments, due to the nature of how parsers work with tokens. Also it would break insane amount of tools like quote!, syn and many others.

joshtriplett commented 4 years ago

I think I would support closing this as well. I've seen enough confusion expressed about the meaning of this construct that I don't think the brevity gain is worth the potential loss to readability.

rpjohnst commented 4 years ago

This may fit better in the "larger" (but closed) tracking issue #44660, but it's also directly related to visibility modifiers and doesn't fit the other sub-issues of #44660:

At some point I remember someone suggesting that pub(path) could now be legal, replacing the pub(in path) form and subsuming pub(crate) and pub(super). That seems to be a nice simplification of pub(...), though in a different direction than crate, that we might consider as well.

Edit: Actually not sure that this works... struct S(pub(path) Type) is ambiguous (to LL-esque parsing) with things like struct S(pub (Type,)).

nikomatsakis commented 4 years ago

So I've been giving this thought over the last few months. I think I've come around to the "close" position. I think the summary of my position is as follows:

And, of course, the fact that not adding crate fn now doesn't mean we can't add it later. In particular, I think that the "three levels of privacy" model would work better if we had some way to do "lightweight, inline crates" within another crate. i.e., if instead of having a submodule within the crate, I could declare a private crate with all that implies (most notably, probably, a DAG-like relationship to other things). I'm not sure if lightweight, inline crates would actually work, but they might cover and replace a lot of the pub(super)-like use-cases. If ever were to explore that, then I'd consider re-opening the discussion around crate fn, as it might become significantly more useful/common.

In short, I would be in favor of removing the crate visibility level from the compiler and removing the feature gate.

matthew-mcallister commented 4 years ago

I just wanted to check on when this feature might be stabilized and am surprised and disappointed that there are plans to remove it. I've been using it on a day-to-day basis since not long after it was added and I'm hardly bothered by any of the issues discussed above. I still haven't written anything resembling crate ::path, for example, and probably never will since I never touch the ::path syntax.

There's room for improvement, sure. The keyword could be better chosen, pub(super) is still inconvenient, and it's a nuisance getting dead code warnings all over the place when I use crate-level visibility for helper methods. As a user, though, I would prefer for this feature to be left around (feature-gated) until a better solution is found. The pub(crate) syntax is something of an eyesore and slightly discourages breaking up big modules into smaller, tighter ones.

matklad commented 4 years ago

think the biggest ergonomic hurdles around "mixed levels of privacy" came from lints and errors related to us trying to ensure that (e.g.) all types that appeared in a pub(x) fn were of suitable privacy.

Does this mean that we also don't enable "unreachable pub" lint (mod private { pub fn f() {} })?

nikomatsakis commented 4 years ago

Heh, I've been having second thoughts as I read through various of rustc code and see crate in use, and I imagine that those cases were all pub(crate)... would probably be worth doing the transition experimentally just to see how the diff makes me feel. I remember feeling sad about it for chalk when we moved it from nightly to stable, though I think now that I'm used to it it hasn't bothered me so much.

@matklad I hope not, I think that lint is quite important too. To be honest, I'm not entirely sure why I haven't encountered any kinds of errors and annoyances of the kind I used to get. Maybe I've just not been writing enough Rust code lately! I definitely remember that I used to have these annoying cycles where it seemed like I couldn't satisfy the compiler except by making way more things pub than I wanted to.

DzenanJupic commented 4 years ago

What about keeping pub(crate) and also adding an alias pubc? This reads quite similar, does not break any current code, and removes the necessity of typing parentheses (what makes it a bit faster). This would also allow pubs for visibility in the parent.

camelid commented 4 years ago

In short, I would be in favor of removing the crate visibility level from the compiler and removing the feature gate.

I find pub(crate) to be kind of an eyesore and noisy when I'm reading code. It would be really nice to have the crate visibility modifier instead.

And, of course, the fact that not adding crate fn now doesn't mean we can't add it later. In particular, I think that the "three levels of privacy" model would work better if we had some way to do "lightweight, inline crates" within another crate. i.e., if instead of having a submodule within the crate, I could declare a private crate with all that implies (most notably, probably, a DAG-like relationship to other things). I'm not sure if lightweight, inline crates would actually work, but they might cover and replace a lot of the pub(super)-like use-cases. If ever were to explore that, then I'd consider re-opening the discussion around crate fn, as it might become significantly more useful/common.

I would definitely like these "lightweight" crates that you mention! It would be a lot nicer than immediately going to workspaces, which are kind of heavy.

Nadrieril commented 3 years ago

The awkward situation we're in is that we want to have three privacy levels -- "completely public", "completely private", and "somewhere in between" (i.e., crate-level) -- and due to backwards compatibility constraints we're stuck with the first of these necessarily being pub, the second being the implicit default, and having to come up with something new for the third.

Why does the implicit default need to be "completely private"? Wouldn't it be ok to make the default crate-visible, since it allows strictly more things to be accessed? Cause if it is, I've got an idea to propose.

My experience of writing and refactoring rust is that whenever I move code around modules, I consistently get annoyed at having to sprinkle pub(crate) everywhere to make the code compile. So I've started defining everything as pub(crate) by default, except specifically for things that should stay local to a module for encapsulation reasons. For that reason I would love a shorter way to write pub(crate).

My mental model is that I only care about visibility in the two following cases:

The rest of the time I don't really care about visibility; I've always had a minority of items that had a good reason not to be crate-visible.

So I would love to have everything crate-accessible by default, and have a special keyword like local to restrict access. This extends naturally to local(super) etc if we want. In terms of legibility, seeing something labeled local makes it easy to identify things I should pay closer attention to: a local field might have an important invariant, and a local function might be allowed to break it. I read it like some kind of unsafe: a warning that this should be used/accessed with a good reason. In fact the default would end up being sorta inherited: a method on a local(foo) struct is by default local(foo) by virtue of type privacy; a method on a crate-visible struct is by default crate-visible too.

I've scanned through the comments here and I was surprised that no one proposed that. Is this out of scope for the current discussion? Has this been proposed before and discarded for some reason?

johnthagen commented 3 years ago

So I would love to have everything crate-accessible by default, and have a special keyword like local to restrict access.

In terms of actual keywords, we already have priv reserved.

jhpratt commented 3 years ago

So if this isn't going to be stabilized, which seems to be the case, is there any plan for removing this feature from nightly?

nikomatsakis commented 3 years ago

Since I wrote my last comment, I've been going back and forth. I do think we need to reach a final verdict and go one way or the other. I'm not sure I'm ready to commit yet to whether this is a hill I want to die on. =)

Kimundi commented 3 years ago

My experience in the last few years is that I only ever needed private, pub and pub(crate) visibility, and among those the last one is way noiser when reading and more annoying to write. This gets to the point that I get code review comments that try to nudge me to prefer pub more, which leads to a tendency to mark things as pub when they are inside crate-local modules, but that of course conflicts with the "crate-external" visibility meaning of pub.

As a result I often experience "modfier churn" where I switch back and forth betwen pub and pub(crate) to both handle "private type in public API" errors and to find the most readable solution.

In my opinion this can trivially solved by providing some single-keyword shortcut for pub(crate), I don't particular care which one. 😄

camelid commented 3 years ago

I find that I often create private modules with public items in them, which helps. However, I think that has a different semantic meaning than just using pub(crate).

matklad commented 3 years ago

The "use pub in lieu of pub(crate)" is indeed fairly popular. For example, TiKV styleguide recommends it: https://github.com/pingcap/style-guide/blob/c8101819c542314a0843a08b158a9ea0721a93f1/docs/rust/modules.md#privacy-and-visibility.

However, this defeats one of the biggest advantages of new visibility system: ability to -W unreachable-pub.

Kekker-git commented 3 years ago

So I would love to have everything crate-accessible by default, and have a special keyword like local to restrict access. This extends naturally to local(super) etc if we want. In terms of legibility, seeing something labeled local makes it easy to identify things I should pay closer attention to: a local field might have an important invariant, and a local function might be allowed to break it. I read it like some kind of unsafe: a warning that this should be used/accessed with a good reason. In fact the default would end up being sorta inherited: a method on a local(foo) struct is by default local(foo) by virtue of type privacy; a method on a crate-visible struct is by default crate-visible too.

I came here looking for exactly this, and was also surprised to find that no one had suggested a local keyword.

lu-zero commented 3 years ago

Having a single keyword pub(<path>) reduces the conceptual burden when learning. I'd rather not add additional items if they do not provide additional functionality.