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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modules #2121

Closed
wants to merge 4 commits into
base: master
from

Conversation

@withoutboats
Contributor

withoutboats commented Aug 23, 2017

Here is another RFC describing a change to the Rust module system (hopefully the last for the near future).

Because it is a relatively large RFC, I have broken the text into multiple files. You can find the rendered root of the RFC here.

The overview document gives a high level overview of the proposed new system and how it differs from the current system.

See RFC #2108, the previous module RFC, for previous work (and links to even more previous work).


For people involved in the previous discussion, here are the highlights of the differences. I would say in summary its a similar skeleton, but with some very significant changes to the skin to make it a much less radical departure.

  • The local keyword: Instead of changing the meaning of pub, a new visibility called local is added for restricted visibility between private and pub. To make this meaning of local easier to learn pedagogically, the crate:: form of absolute paths have been changed to be written local::.

  • mod statements: Mod statements continue to exist instead of being removed. Even when autoloading modules, they will still be used to control visibility (such as making them more public or more private).

  • The export keyword: Now that its not a visibility, export is just a standard mechanism for re-exporting things, similar to a use statement but optimized for this use case.

  • Autoloading opt outs: Two mechanisms of opt out for the autoloading mechanism are added, an #[ignore] attribute for temporarily opting out of loading a particular file, and a Cargo.toml flag which prevents cargo from preparing the --modules list at all.


Quoting this from the previous RFC:

Based on past experience, I predict this RFC will be high traffic. So a couple of reminders:

  • If you find yourself in a conversation with a small number of other users "workshopping" a particular topic, consider finding an out-of-band medium (such as IRC) to hash out the details, and bringing the substantial proposal back to the thread afterward.
  • If you've staked out a position, consider when making a new comment whether you're restating the same position or if your thoughts have been clarified or evolved. Unless your prior post was significantly misunderstood, the conversation doesn't progress much by the same arguments being repeated in cycles.
  • When the conversation grows long, its unreasonable to expect someone coming into it to have read every prior comment. But if you have an objection that seems very obvious to you, there's a good chance its been discussed previously. Consider searching the thread to see if maybe that subject has already reached an endpoint.

Lastly, if you want to leave a comment on the RFC but for any reason at all don't want to leave it publicly, I receive email addressed to woboats@gmail.com. I can't promise I'll respond, but I will read it.

As a final note: this is a polite reminder that the RFCs repo, like all Rust project venues, is governed by the Code of Conduct and also that we, the RFC authors, are acting in good faith (honestly!). Please always be charitable in disagreement. 馃槃 馃寛 鉂わ笍

@ubsan

This comment has been minimized.

Contributor

ubsan commented Aug 23, 2017

I like the proposal as a whole, but local is... not good. It'd break my compiler code - I'd have to find a different way to refer to stack locals. Also, it doesn't really say "crate visibility" to me. I'd prefer if we used the existing crate keyword for this.

Otoh, we could make crate visibility the default, and use the existing priv keyword for module-private? Or just in general, use the priv keyword. That wouldn't break any code.

@arthurprs

This comment has been minimized.

arthurprs commented Aug 23, 2017

I still have to carefully read everything but I feel that this proposal is underestimating the cost of adding 2 more keywords.

would be a valid way to invoke rustc by hand:
```
rustc src/lib.rs --modules src/*.rs src/**/*.rs

This comment has been minimized.

@TimNN

TimNN Aug 23, 2017

Contributor

Nit: For clarity, I think this should either be --modules "src/*.rs src/**/*.rs" or --modules src/*.rs --modules src/**/*.rs (otherwise src/**/*.rs would be a "free" argument).

This comment has been minimized.

@TimNN

TimNN Aug 23, 2017

Contributor

In this example, are the arguments expanded by rustc or the shell? If they are expanded by rustc, I think the reasoning behind the following statement from the alternatives makes less sense:

We could also put the file-lookup in rustc, instead of cargo, and have rustc perform its own directory walk. We believe this would be a bad choice of layering.

Since rustc is already doing a directory walk, why not have only rustc do the directory walk.

This comment has been minimized.

@withoutboats

withoutboats Aug 23, 2017

Contributor

no they are expanded by the shell

This comment has been minimized.

@est31

est31 Aug 23, 2017

Contributor

Should be pointed out in the RFC IMO. I've stumbled across the same question when reading the rendered doc.

This comment has been minimized.

@Aaronepower

Aaronepower Aug 23, 2017

@withoutboats Expansion by shell would never work since there is hard limit on the number arguments you can pass to a program. 128KB worth or 1/4 of your stack size.

@abonander

This comment has been minimized.

abonander commented Aug 23, 2017

I'd have to 馃憥 the implicit module declaration, specifically on the utility of commenting out mod statements while making large sweeping changes to a codebase.

The load-modules directive and #[ignore] are mediocre bandaids given that the former needs to be added for every target and neither may be immediately discoverable; commenting out a declaration is immediately intuitive and doesn't need to be taught or discovered.

Not to mention that this could break crates which published with broken Rust files that were unreachable with the current rules but would not be with the new rules.

* The file name is not a valid Rust identifier followed by `.rs`.
* The file is not in a subdirectory of the directory containing the root
module.

This comment has been minimized.

@TimNN

TimNN Aug 23, 2017

Contributor

Depending on how this "subdirectory of" check is implemented it could get pretty tricky:

  • Do the prefixes given to --modules have to match? If not:
  • How are paths normalised? How are soft/hard links handled?

In most cases this probably does not matter, but it may still be worth considering.

This comment has been minimized.

@withoutboats

withoutboats Aug 23, 2017

Contributor

These are implementation details that don't need to be decided at the RFC stage. This is just a sanity check that we can transform the directory into paths.

@TimNN

This comment has been minimized.

Contributor

TimNN commented Aug 23, 2017

I like the local keyword. I initially had concerns that it wouldn't properly describe the actual visibility of an item, however since it is now also used in absolutes paths, things are pretty clear:

Items marked as local can be accessed from ::local::* paths.

@withoutboats

This comment has been minimized.

Contributor

withoutboats commented Aug 23, 2017

Not to mention that this could break crates which published with broken Rust files that were unreachable with the current rules but would not be with the new rules.

This is why the cargo flag is not defaulted to true under the 2015 checkpoint. I have worked very hard to make sure that this version of the RFC is very strictly nonbreaking.

@repax

This comment has been minimized.

repax commented Aug 23, 2017

I'm wondering about the local keyword. To me, global is its colloquial opposite, however when we speak of something global (a variable for instance) then it's about scope not visibility.

term opposite term
priv pub
local global
internal external
mod crate

Edit: Also, we have thread local storage.

@withoutboats

This comment has been minimized.

Contributor

withoutboats commented Aug 23, 2017

Constraints for the local keyword are listed in the Alternatives section of that document & I would be happy to find a better alternative.

actually `pub`. We make this much clearer by the combination of two choices in
this RFC:
- All items that are `export` (equivalent of today's `pub`) must *actually* be

This comment has been minimized.

@nikomatsakis

nikomatsakis Aug 23, 2017

Contributor

Nit: I believe this is out of sync with the current version of the RFC.

@epdtry

This comment has been minimized.

epdtry commented Aug 23, 2017

For file loading, I still think it would be better to move the directory traversal into rustc, so that command-line usage of rustc src/lib.rs picks up all the relevant modules just like it does today (but without mod statements). Why is this alternative described as "a bad choice of layering"? To me it sounds like it would leave the division of labor between cargo and rustc much the same as it is now.

@repax

This comment has been minimized.

repax commented Aug 23, 2017

First off, I'm really excited about this RFC! 馃憤

Now, how about just priv instead of local? As private is the opposite of public:

  • pub: public api.
  • priv: private / non-public.
  • priv(crate): exactly the same as just priv, just elaborated!
  • priv(in $path): even more specific.
  • priv(super): ...
  • priv(self): The smallest visibility, and also the most common, so this can be completely elided.

鉁 Shorter tends to be better than longer.
鉁 It needs to be sensible as the prefix to a path (making sense as a spatial designator is preferable.)

use priv::foo::Bar;
::priv::foo::Bar;

鉁 It needs to be sensible as a visibility (an adjective is strongly preferable).
鉁 It can't be an existing crate on crates.io (unless its been reserved for this purpose, as local has been).
鉁 It needs to have support for the (restricted) syntax.

@TimNN

This comment has been minimized.

Contributor

TimNN commented Aug 23, 2017

@repax: The problem that I have with private is this: By default, items have visibility priv(self). This means that adding priv (equivalent to priv(crate)) to an item increases it's visibility instead of decreasing it, which is contrary to what (at least) I would expect.

@repax

This comment has been minimized.

repax commented Aug 23, 2017

@TimNN: Yeah, I think I confused myself there. I dunno.

@TimNN

This comment has been minimized.

Contributor

TimNN commented Aug 23, 2017

@repax: That's exactly what I'm saying, if I understand your analogy correctly:

  • If I say just private it should mean "private to the smallest possible group of people, myself".
  • However, thats not what is proposed, instead:
    • Saying nothing means "private to myself"
    • Saying private means "private to me & my whole family"

So saying something is private makes it less private than if I hadn't said anything, which is what I'm having problems with.

During the design process, we considered other, more radical redesigns, such as
making all files "inlined" into their directory and/or using the filename to
determine the visibility of a module. We've decided not to steps that are this

This comment has been minimized.

@est31

est31 Aug 23, 2017

Contributor

decided not to steps

@repax

This comment has been minimized.

repax commented Aug 23, 2017

@TimNN: This is tricky, indeed. But local has the same problem:
Saying something is local makes it less local than saying nothing.

Besides, local has the problems I mentioned in my first post. It reads more like a scope definition than a visibility definition due to the global/local variable connotation, as well as the thread local storage scope. Therefore I think that priv is better, overall.

@phil-opp

This comment has been minimized.

phil-opp commented Aug 23, 2017

I don't like that I have to change almost all pub visibilities to local (since pub is only for items that are part of a library's API now). It means a lot of code churn and changes a short, common, and widely used keyword to the more obscure local, which is not used in other languages. I don't think that this will make rust more accessible.

The previously proposed special visibility for items that are part of a library's public API made much more sense to me. For me, something like the following would feel most natural:

priv: private to the current module (instead of local(self)); default for structs, functions, etc.
priv(self): equivalent to priv
priv(super): private to the parent module
priv(crate): accessible from the whole crate, default for modules
pub: equivalent to priv(crate)
pub(api) (or pub(exported)): part of a library's public API

This would minimize code churn, since only the (few) exported types/functions of libraries need new visibility qualifiers.

@aturon aturon referenced this pull request Aug 23, 2017

Closed

RFC for module redesign. #2108

@est31

This comment has been minimized.

Contributor

est31 commented Aug 23, 2017

My comment is very non-verbose to minimize noise.

On the preceeding proposal I expressed concerns that privacy level was restricted to three levels (public to the world, public to the crate, private to the module), but that was based on a misunderstanding of mine on the RFC, and I think this RFC doesn't feature this problem either as you can restrict privacy of entire modules via the mod keyword.

Stuff I like

  1. The crate's api being contained in a local:: module. I don't like the name very much but its very very good to be able to distinguish modules from crates.
  2. Having some "public to the world" system where you know whether something is public by looking at the definition. This is nice to know.

Stuff I'm not really a great fan of

  1. The goals of the export introduction the RFC cites seem not useful enough
    to me to justify the churn it introduces. Surely, making it relative is good, but I don't think it is worth the churn :).
  2. Item-ness of use has, I admit, some weirdnesses, but it is also useful, and actually causes consistency in the language. Comparing use statements to impl blocks seems weird to me because there is no way impl blocks would have a mapping to items while there is an obvious one for use statements.

My main concerns

  1. Don't call the crate's module local as it contains the full public and private API of the crate, not just the pub(crate) API.
  2. local(foo) is 2 characters longer to type now. Its already long enough to put the brackets and spell out whatever foo is, don't make it even longer. Just call the keyword loc instead, this will make it line up nicely with other stuff.
  3. EDIT(almost forgot this one): Don't let cargo figure out the file layout, but let rustc do it instead. Maybe pass some rules to rustc to e.g. exclude bin directories or similar, but don't pass every single file. This will make build system integration of rust harder, despite making it easier being a goal of this year's roadmap (whatever worth that fact has...).
  4. Automatic loading of modules without a mod keyword. Don't do that, please. Don't want to rehash my points here, just say that this sole point is enough for me to 馃憥 the RFC.
  5. Requiring visibility info on mod keywords. Why not just keep them local(self) by default like it is now? This requirement seems arbitrary to me.
  6. The very substance of this RFC is introducing new mechanisms and deprecating old ones. I think one needs to be very cautious with breaking changes. If you do one, it needs to be an improvement on all fronts. But this RFC is not like that, so the fact that this is a breaking change is one more point against it. (Even if its an improvement on all fronts you might not want to do some change because its too radical).

Summing it all up

馃憥

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Aug 23, 2017

@abonander

commenting out a declaration is immediately intuitive and doesn't need to be taught or discovered.

I appreciate your point, however, for many people the idea of "declaring a module" has proven to be a major source of confusion. In other words, mod itself has to be taught -- once you fully understand it, then yes, commenting it out isn't so bad. But you can also teach #![ignore], in that case.

@kornelski

This comment has been minimized.

Contributor

kornelski commented Aug 23, 2017

While use local::foo is more obvious than use crate::foo, the local keyword on items is inconsistent with pub 鈥 it's not abbreviated like pub, and it's opposite of global, rather than public. I'd prefer to go back to use crate:: and keep pub(crate). It's a safer change, and more consistent naming.

@glaebhoerl

This comment has been minimized.

Contributor

glaebhoerl commented Aug 23, 2017

As I raised on the previous thread and @TimNN already touched on as well, I think internal or maybe intern might be a better name than local, as the opposite of extern.

@kornelski

This comment has been minimized.

Contributor

kornelski commented Aug 23, 2017

load-files directive sounds nice. Maybe it could be false by default, and use epochs to migrate it from default off to default on?

  • make cargo new set it to load-files=true explicitly
  • make cargo warn users to add load-files if it's not there yet
  • if possible, make all old crates on crates.io have load-files=false
  • eventually, when nearly all crates have an explicit setting, change the default without breaking anyone
function, etc), without creating declared items of those names at this point in
the module hierarchy.
`use` statmeents with a visibility (such as `pub use`) are deprecated, and

This comment has been minimized.

@Keats
@thepowersgang

This comment has been minimized.

Contributor

thepowersgang commented Aug 24, 2017

I would like to put another voice towards splitting this RFC and moving the autoloading to another RFC.
The visibility and use changes sound like they will be a net gain overall (and don't seem to have many non-bikeshed disagreements), while the loading changes seem quite contentious.

For the record (again, from the other RFC) - I'm against changing the module loading method to be implicit in general, but if it had to be done - using the current proposal of passing a set of globs to rustc is preferable to explicitly listing files (or worse, having rustc know of cargo's rules)

@est31

This comment has been minimized.

Contributor

est31 commented Aug 24, 2017

@Keats

Does that mean that if I look at a lib.rs with the new pub, I can't really figure out have a sense for the API?

From how I interpret the RFC, you will have to either make a pub mod foo; statement if you wanted a module to be public, or you'd have to pub export foo::stuff; it if you wanted stuff to appear at a different location. AFAIK the RFC proposes no automatism of the kind "if there is a public item in a module, the module gets public as well".

@nox

This comment has been minimized.

Contributor

nox commented Aug 24, 2017

General notes

As a preamble, I would like to note that this RFC acts as if RFC #2088 will land for sure, and AFAIK this has not been decided yet. The same can be said about the reliance on checkpoints in this RFC.

I would also like to note that even if a survey said that beginners have issues with the current system, it doesn't automatically mean that the current system should change, and it doesn't mean that the new system (that introduces many very invasive breaking changes) will not make everyone's life more complicated at scale.

Motivation

The survey of crates.io didn't include most largest crates, such as the ones in Servo, that uses way more local imports than external ones. In projects that cannot be split into multiple crates, use crate:: will get old pretty fast.

"If a crate is renamed, you do not need to rename all internal imports" shouldn't be an argument in favour of this RFC, that sounds like optimising for crate renaming, which is a very rare thing.

The RFC mentions the previous take, where all files were inlined in their directories and directories were modules. It also says that "We've decided not to steps that are this radical right now." Please remove the "right now".

Things I don't like

I think that the very existence of implicit module loading will be an additional burden for me to maintain things or contribute to satellite dependencies of Servo that aren't handled by us directly.

I think that local and export as keywords are way worse than catch and that they should stay as identifiers for people to be able to use them in their code. What is wrong with use and pub use anyway?

I have a vague feeling that this RFC will make future versions of the language unable to ever have a module system that is useful for other purposes than simple namespacing (parameterised modules, etc).

I think that the mandatory explicit visibility bring absolutely nothing to the table and even hurts ergonomics. Why make mod foo; become local(self) mod foo;? It doesn't fit with struct fields and everything else that can have a visibility and is private by default. It also means rewriting of most code in existence. Don't break mod has been my main complaint all along and it is still being broken in this version of the RFC.

Things I think should be blockers

The RFC should not ever, ever land without the automatic migration tool being ready.

Things I like

馃, 馃, and the fact that we can have this discussion.

@TimNN

This comment has been minimized.

Contributor

TimNN commented Aug 24, 2017

I'm gonna write a comment that I've been thinking about since the previous iterations of this RFC:

The thing I call the "Rust Module System" in my mind is dead simple:

  1. You can have a "block module" with the syntax mod block { ... }. You can nest those.
  2. You can declare an "external module" as present in an external file using mod ext;.
  3. An external module is either located in ext/mod.rs or ext.rs.
  4. Only (non-block) modules can declare other external modules and then only if they are located in mod.rs files or are the crate root.
  5. #[path = "..."] can be used to specify an arbitrary location for an external module. The previous rule is ignored (AFAIK).

The fact that a few mod statements need to be written doesn't really matter to me: It may be a bit of an inconvenience at times but again 馃し鈥嶁檪锔 it's simple. Also this is something that can easily be handle by an IDE / editor, eg., with Intellij Rust I currently create new modules by writing mod foo; and then use an IDE action to have the file created.


So, to me, the whole complexity comes from visibility, use and paths, since these topics are more interconnected than how modules are declared. However the core rules around the three topics are also pretty simple, in my opinion:

The core rules around visibility are simple:

  1. By default, everything is private and can only be used/seen in the module it is defined in or the modules children.
  2. Something declared as pub can be used/seen from the parent (and anywhere the contents of the parent module can be seen from)

The rules around paths are simple:

  1. Anywhere, except in use, paths are relative to (the contents of) the current module. They can be made absolute by prefixing them with ::.
  2. Absolute paths are relative to (the contents of) the root module.

The primary rules around use are simple:

  1. use $path::foo ensures that foo can be used as if it was defined in the current module.

An then there are some rules, or interaction between rules, which I think cause the additional complexity:

  1. pub use, making items from somewhere else available to the parents of the current module.
  2. pub(restricted), which makes pub more complicated, although this often desirable.
  3. The "private type in public interface" error, which can be annoying / hard to understand/comprehend.
  4. Absolute paths which are relative to the root module and extern crates being part of the root module.

In summary: I think the core rules of the current system are pretty simple (and, more importantly, pretty uniform): They fit on a single sheet of paper. However there are a few rules which are themselves either complex or confusing (the last four rules above). So in my opinion the RFC should focus on improving the situation / learnability around those rules (or accept, eg. for pub use, that the current situation, while difficult, is still good):

I think this RFC does an admirable job of that with eg. removing extern crates from the root namespace and adding the crate:: / local:: prefix to absolute paths. However the "automatic module loading" fixes a problem which doesn't exist, in my opinion.

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Aug 24, 2017

@TimNN

The "private type in public interface" error, which can be annoying / hard to understand/comprehend.

FYI, since rust-lang/rust#42125 landed, there's only one final push required to resolve this for good - make "private-in-public" a lint and not an error and make it use more intuitive reachability-based heuristics (lints can use heuristics, unlike errors).
Unfortunately, I recently spend all my Rust time processing new RFCs and not implementing anything 馃槃

@burdges

This comment has been minimized.

burdges commented Aug 24, 2017

I also believe the survey of crates.io is flawed, like @nox says. I'd suggest surveying some large projects like rustc, cargo, servo, ring, and their dependencies.

As an aside, there is a cost to users when crates break themselves up into many smaller crates, like the RustCrypto project did, and like the rand crate is discussing, if only because the documentation becomes less cohesive. These crates have security reasons for doing so, in that a person auditing users of the code can see at a glance if approved or unapproved algorithms get included. And tokio makes sense too, due to a very complex dependency graph and that users benefit from understanding. We should not take these hierarchies of micro crates as an ideal though.

I have a vague feeling that this RFC will make future versions of the language unable to ever have a module system that is useful for other purposes than simple namespacing (parameterised modules, etc).

I'd love to know how parameterised modules and module aliases fit with this story. I'd assume the module must then contain a line mod<...>;, which holds with no changes too, and the use statement provides the parameters. I donno if much even changes syntacticly here? Would the visibility changes impact anything like reexporting items from a parameterised modules after nailing down the parameters? In fact, how should we reexport local items with this change? pub use .. still works? Or we need local use or something?

@arielb1

This comment has been minimized.

Contributor

arielb1 commented Aug 24, 2017

As a preamble, I would like to note that this RFC acts as if RFC #2088 will land for sure, and AFAIK this has not been decided yet.

This RFC includes #2088 under its omnibus umbrella. Actually, this is pretty justified - with the change to use roots, not having #2088 would be stupid.

@est31

This comment has been minimized.

Contributor

est31 commented Aug 24, 2017

@arielb1 even though I'm for splitting out the implicit mod proposal from this RFC, I think it would be better if #2088 PR got integrated into this RFC. Having #2088 without the use root change would be pretty bad for clarity.

@rpjohnst

This comment has been minimized.

rpjohnst commented Aug 24, 2017

Another way to look at it is to go back to @aturon's blog post or the RFC's motivation section which described the issues we're trying to solve. The things auto-loading seems aimed at are "too many declaration forms"/"lots of frontloaded syntax," "filesystem organization," "multiple keywords import things," and "repetition."

Some of these goals it still meets, at least individually. mod declarations are less front-loaded. Adding a file no longer requires also adding mod the_file;. Implicit modules no longer have two ways to import them (i.e. today, mod and use both bring them into scope).

However, many of these goals it no longer serves quite as much, or at all. mod declarations are still around, only now they have two purposes that change depending on Cargo configuration. There are still more keywords being introduced to deal with modules (local and export). You still have to keep the filesystem and mod declarations in sync.

So, while implicit module loading is tied into the local visibility reasoning goal, I'm beginning to think it's not worth it, for a few reasons:

  • It doesn't really solve any of the problems it sets out to, especially on its own. Further, path confusion always seemed the bigger of the learnability issues to me.
  • While it's technically backwards compatible and can be (mostly?) automated, the churn it introduces is still quite large.
  • I have always appreciated how self-contained a Rust crate build can be (mod statements, #[link], #![no_std] as opposed to -ffreestanding, #[crate_type], etc.), and auto-loading removes more of that.
@carllerche

This comment has been minimized.

Member

carllerche commented Aug 24, 2017

One thing I haven't seen discussed is how much an IDE would help the beginner approach the current module system.

It seems to me that most of the pains could be avoided at the IDE level.

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Aug 24, 2017

Some editorial points: (I鈥檒l leave opinions for a separate comment.)

What is seemingly the same visibility keyword is called export in motivation.md and pub in overview.md

What is seemingly the same path prefix is called crate:: in motivation.md and local:: in overview.md

Items marked pub inside of files which are not public are not visible outside of this crate, because the full path to that item is not pub. Instead, they need to provide a public mod statement to make that module public:

Grammatically, part of this first sentence seems to be missing. What about these items? Are they disallowed? (Guessing based on context.)

鈥淭hey鈥 in the second sentence seems to refer to these items, but I think something else (the crate鈥檚 author?) was intended.

In a library, items which are marked pub must be publicly accessible in the library's API; a lint warns users when they are not.

Other parts of the RFC suggest (by using 鈥渕ust鈥) this is an error rather than a warning. Which is it?

If you want a file to be ignored, you can use the #[ignore] attribute on the top of it.

Wouldn鈥檛 that need to be #![ignore]?

@stevenblenkinsop

This comment has been minimized.

stevenblenkinsop commented Aug 24, 2017

@SimonSapin

The attempt to modularize the modules rfc backfired somewhat because the components were overly coupled, so changes in one part caused undetected breakages elsewhere in the system ;)

(something something non-local reasoning)

@mark-i-m

This comment has been minimized.

Contributor

mark-i-m commented Aug 24, 2017

@SimonSapin

What is seemingly the same visibility keyword is called export in motivation.md and pub in overview.md

In this proposal, export is not a visibility modifier. It is statement whose only purpose is to re-export an item from another place. In fact, a visibility modifier can (and usually would be) used with export.

For example, pub export foo::bar; would publicly re-export foo::bar.

Items marked pub inside of files which are not public are not visible outside of this crate, because the full path to that item is not pub. Instead, they need to provide a public mod statement to make that module public:

I believe the correct way to parse the sentence is this:

(Items marked pub inside of (files which are not public)) are not visible outside of this crate, because (the full path to that item) is not pub.

i.e. the items in question are not public to the world even though they are marked pub "because the full path to that item is not pub". You would need to make the whole path of modules also pub or re-export from a pub module.

Other parts of the RFC suggest (by using 鈥渕ust鈥) this is an error rather than a warning. Which is it?

I think it will be a warning at first; then in a later epoch, it'll be a hard error.

@mark-i-m

This comment has been minimized.

Contributor

mark-i-m commented Aug 24, 2017

One thing I haven't seen discussed is how much an IDE would help the beginner approach the current module system.

Or increased focus on The Book's chapters on modules...

@ExpHP

This comment has been minimized.

ExpHP commented Aug 24, 2017

Yesterday, I tried to start cataloging all of the various modules proposals from a birds-eye view in an attempt to identify the differences between them, to record their various advantages/disadvantages, and to help understand the cascading changes in design. I wanted to try and understand the complicated design space that we're dealing with.

...well, that was the plan.
I only got through listing the conceptual design and syntax choices in the two most recent RFCs before nearly collapsing from exhaustion.

After doing this so far, however, it feels to me like the feature of automodules actually is orthogonal to the rest of the design. I notice that, between these two RFCs, this one provides an improved design for #[ignore], but I can't see how this change is motivated by (or motivates) any of the other changes. Meanwhile, most other aspects of these proposals (such as the visibility changes, path changes and item-ness) do actually fit together like a beautiful puzzle that cannot be teased apart.

@mark-i-m

This comment has been minimized.

Contributor

mark-i-m commented Aug 24, 2017

Also, would it be good to create a separate RFC for the public-in-private lint. It seems like we would want something like that whether this RFC passes or not, and I think it can be done in a forward-compatible way...

@stevenblenkinsop

This comment has been minimized.

stevenblenkinsop commented Aug 24, 2017

The only problem with the public-in-private lint without this rfc is it forces people to write pub(crate) everywhere in order to satisfy it. People want to be able to just say pub because pub(crate) is a bit unwieldy, so tying the lint to having a nicer way to say pub(crate) makes it more palatable.

Presumably the proposal would still have to include a migration tool to do the rewriting in existing code.

@mark-i-m

This comment has been minimized.

Contributor

mark-i-m commented Aug 24, 2017

@stevenblenkinsop

We could just make the lint have a default of allow. If you want to opt-in, you can just add #[deny(pub_in_priv)] (EDIT:) on the items you want in your API. This has the benefit of being

  • grep-able
  • completely non-breaking
  • TMK, completely forward-compatible with any possible Modules RFC, including this one.

Sure, it's not super pretty and it's not short, but I think it would do nicely until we have a good solution.

@steveklabnik

This comment has been minimized.

Member

steveklabnik commented Aug 24, 2017

@mark-i-m

One thing I haven't seen discussed is how much an IDE would help the beginner approach the current module system.

Or increased focus on The Book's chapters on modules...

We have done this, twice already. It hasn't helped. Nobody has come up with a magic tutorial that makes people understand things easily. And in fact, @carols10cents , in one of the earlier threads, showed a revised version of the chapter with these changes, and it was a lot better.

@stevenblenkinsop

This comment has been minimized.

stevenblenkinsop commented Aug 24, 2017

@mark-i-m

The whole point is to push deployed code towards having the correct visibility modifiers. If it's opt-in, most code won't use it, so it won't have the desired effect. Yeah, people who particularly care can opt in, but that doesn't do anything to address the readability issue when reading other people's code.

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Aug 24, 2017

@carllerche

One thing I haven't seen discussed is how much an IDE would help the beginner approach the current module system.

There's one more thing - how compiler's own diagnostics can help the beginner to approach the current module system!

Currently there's nearly zero (maybe literally zero) help messages related to module loading (missing mods in particular).
The compiler certainly can report more useful help and auto-load files as part of error recovery.
(However, there's fundamental difference between best-effort recovery and guaranteed auto-loading from build system point of view).

EDIT: The first module discussion thread on internals had an immensely useful user report about this - https://internals.rust-lang.org/t/revisiting-rusts-modules/5628/33

@mark-i-m

This comment has been minimized.

Contributor

mark-i-m commented Aug 24, 2017

@stevenblenkinsop

If it's opt-in, most code won't use it, so it won't have the desired effect.

It is still an intermediate step, though. And whatever the final solution is, I think it will probably include such a lint anyway...

@aturon

This comment has been minimized.

Member

aturon commented Aug 24, 2017

The modules saga has truly broken all records for comment velocity!

After some soul-searching discussions among the lang team, and considering the small amount of time left before the impl period, here's what I'd like to propose:

  • We close both this RFC and the extern crate RFC

  • We put together a yet-more-conservative proposal that has the following elements (glossing over some important details!):

    • mod statements remain in exactly their current form; no loading from the filesystem
    • extern crate is deprecated, as in #2088
    • Absolute paths always begin with either a crate name (for external crates) or crate (much like with this RFC, except using crate rather than local)
    • crate is added as a visibility, shorthand for pub(crate)
      • This enables linting on pub items that are not, in fact, public.
    • We allow foo.rs and foo/ to coexists; i.e., you don't have to use mod.rs when you want to have submodules in a subdirectory.
  • We open an experimental RFC for determining modules from the file system (as in this RFC). We recognize that this is a major point of controversy and so will put aside trying to complete a full RFC on the topic at this time; however, we believe the idea has enough merit that it's worth an experimental implementation in the compiler that we can use to gather more data, e.g. around the impact on workflow. We would still like to do this before the impl period, so that we can do that exploration during the impl period. (To be clear: experimental RFCs are to approve landing unstable features that seem promising but where we need more experience; they require a standard RFC to be merged before they can be stabilized.)


The new main RFC here involves no new keywords, and can be done purely with deprecations; it does not require a new epoch. I believe it retains many (but not all) of the learnability benefits of the current RFC. It enables us to explore those other benefits in a slower-paced way, through an experimental RFC.

@withoutboats is going to be tied up with moving house, so I plan to write this new main RFC in the next couple of days. I expect it to be much shorter and to draw a lot from the extern crate RFC. I expect this will be the last iteration.

Meantime, I'm gonna close this one out...

@aturon aturon closed this Aug 24, 2017

@ubsan

This comment has been minimized.

Contributor

ubsan commented Aug 25, 2017

@aturon I'm sad that we'll continue to need to write mod statements, but I think that this is the right choice for now. 馃憤

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Aug 25, 2017

@aturon

Absolute paths always begin with either a crate name (for external crates) or crate (much like with this RFC, except using crate rather than local)
crate is added as a visibility, shorthand for pub(crate)
This enables linting on pub items that are not, in fact, public.

Can path and visibility changes be submitted as separate RFCs?
Both are going to attract bikeshedding about specific syntax alternatives.
I finally read the RFC text and now I'm sure they are totally independent.

We allow foo.rs and foo/ to coexists; i.e., you don't have to use mod.rs when you want to have submodules in a subdirectory.

Oh, this is nice.

@emk

This comment has been minimized.

emk commented Aug 25, 2017

(EDIT: Ah, sorry, it looks like I missed the closing above. I started writing this yesterday, and much of it has been overtaken by events. The underlying reactions, however, might still be useful to someone.)

Speaking as commercial user of Rust who's been bringing multiple in-house developers up to speed, and who already has a non-trivial code base to migrate, my feelings about this whole plan are mixed: some positive, some negative, and some wary. This is also the first time I've seen the whole "epochs" RFC, which is apparently a done deal. This is what I get for not reading r/rust all the time.

  • The whole "epochs" plan makes me very nervous. There are some good points: automatic migration tools, very gradual migrations, and the ability to run mixed-epoch programs indefinitely. But there are also bad points: This means that Stack Overflow is once again going to be filled with obsolete Rust code, and I'm going to have to retrain everybody on the new module system. As far as I'm concerned, this is a case of breaking the "No Rust 2.0 any time soon, Rust 1.0 is stable" promises. Yes, you're taking many steps to ameliorate that, but it's nonetheless bringing flashbacks of the bad old days before Rust 1.0. We absolutely need Rust to be a stable platform, even if that means it's not always perfect. So, my personal verdict on epochs: If the average epoch will involve a change as big as replacing the module system, then epochs are a sufficiently bad idea to make me rethink further investment in Rust. If the average epoch involves nothing worse than a new keyword or something, then the epoch plan is OK, given automatic migration tools and permanent compatibility. I mean, we are used to running cargo fmt on our code regularly.
  • On the whole, the new module system looks pretty nice. Changing pub(crate) to local is fine. The whole "pub means pub" change is nice.
  • I will be very happy to never again type extern crate! Is there are plan for #[macro_use]?
  • Auto module loading is... mostly meh. Since I can still #[cfg(feature = "foo")] mod foo_support;, I have no major complaints. I imagine it will improve developer ergonomics.
  • One hard constraint: Packages on crates.io need to keep working indefinitely, even if they have broken_garbage.rs somewhere internally that wasn't getting loaded. We depend on quite a few crates, a few of which are only semi-maintained, and it would be a huge blow against our trust in Rust if we couldn't keep using those crates鈥攐r worse, crates which depend on crates which depend on those crates.

So overall, my personal verdict is: This change is too large, and it would reduce my trust in Rust's stability and fitness for commercial use. If this is a one-time thing with excellent migration tools, then I can grudgingly accept it to improve Rust onboarding. But if the typical epoch is going to make changes this big, I would need to spend a lot more time defending Rust to my colleagues at work. The mere fact that this will break example code on Stack Overflow is a big deal.

Basically, the thought running through the back of my head is "We've had a golden era since Rust 1.0, but is this the end? Will I need to keep up with constant change again?" I simply have too many Rust crates, both at work and personally, to ever chase the upgrade treadmill again. The more I look at the automatic migration tools, parallel epoch support, etc., the more I realize that this isn't an entirely fair reaction. But that's my underlying gut reaction. And that will probably be the gut reaction of some of my colleagues, at least until I explain the whole epochs plan to them, which will take time. Like it or not, those kinds of conversations are also part of the developer experience that's being proposed.

@aturon

This comment has been minimized.

Member

aturon commented Aug 25, 2017

The new RFC is up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment