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

RFC: Clarify and streamline paths and visibility #2126

Merged
merged 10 commits into from Sep 17, 2017

Conversation

@aturon
Member

aturon commented Aug 25, 2017

Read this first

This is the latest RFC in the ongoing saga around Rust's module system. For those of you who have stopped following due to fatigue 馃槱: this is almost certainly the final iteration, and I'd like to ask you one last time to take a fresh look -- almost all of the text here is new. 鉂わ笍

Relative to earlier discussion, this proposal is drastically more conservative. It does not include any changes to mod (i.e., it does not propose to determine module structure from the file system). It does not require epochs.

The lang team does still want to explore the possibility of determining modules from the file system, but plans to do that through an experimental RFC -- i.e., to seek just for consensus that we can land a prototype implementation to gather more data and experience; a full RFC must subsequently be accepted prior to any stabilization. The experimental RFC will be posted soon.

How to give feedback

A word about feedback. I'm eager for feedback from the entire community, but am also conscious that the comment velocity on this topic has been very large. Thus I would ask:

  • Please read carefully and spend some time digesting before commenting.
  • Please focus on the big picture items; if you have concerns, make clear which ones are serious, and which are more minor, and consider holding off on the latter until there's consensus on the basics.
  • Please avoid bikeshedding on e.g. names until there's a clear consensus on the basics.
  • Please don't use this comment thread as a virtual chatroom; if you're tempted to make multiple replies in a short span of time, consider taking the discussion out of band then bringing it back to thread when things are clearer. The #rust-lang IRC channel is a great place to work through finer details.

Summary

This RFC seeks to clarify and streamline Rust's story around paths and visibility for modules and crates. That story will look as follows:

  • Absolute paths should begin with a crate name, where the keyword crate refers to the current crate (other forms are linted, see below)
  • extern crate is no longer necessary, and is linted (see below); dependencies are available at the root unless shadowed.
  • The crate keyword also acts as a visibility modifier, equivalent to today's pub(crate). Consequently, uses of bare pub on items that are not actually publicly exported are linted, suggesting crate visibility instead.
  • A foo.rs and foo/ subdirectory may coexist; mod.rs is no longer needed when placing submodules in a subdirectory.

These changes do not require a new epoch. The new features are purely additive. They can ship with allow-by-default lints, which can gradually be moved to warn-by-default and deny-by-default over time, as better tooling is developed and more code has actively made the switch.

This RFC incorporates some text written by @withoutboats and @cramertj, who have both been involved in the long-running discussions on this topic.

Rendered

@aturon aturon referenced this pull request Aug 25, 2017

Closed

Modules #2121

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Aug 25, 2017

Once again, thanks so much to @aturon and everyone involved here for continuing to push on this!

I was personaly quite happy with the previous iteration so it's only natural that I'm also quite happy with this version! This version of the RFC persists my favorite feature from before, requiring a keyword for local crate imports. It also allows obvious-in-retrospect things like foo.rs vs foo/mod.rs to boot! I also think that having a dedicated crate visibility as an alias for pub(crate) is going to be a great catalyst for its adoption.

I'd primarily like to advocate for the RFC 2088-like approach to the extern crate statement migration path. I see that requiring an epoch here is a downside where it creates an interim period where we're required to use extern crate but we know we'll have to stop in the next epoch. I don't think one way or the other will require less absolute work on an author's part, but I think that doing it all at once will result in an overall feeling of "less work done" as you only need to change source to fix deprecation warnings once. I think the current RFC 2088-like approach is also fully backwards compatible in the current epoch, so the only downsides I see are a lack of a "clear demarcation" and the weight of the compiler implementation. I'd bet, though, that the compiler implementation would be pretty small (but please correct me if I'm wrong!). I'm personally willing to give up the "clear demarcation" with an epoch :)

Otherwise I've got a few high-level comments but they're relatively nit-picky, so I don't mind resolving many of them during the implementation phase!

  • Right now the Cargo.toml modification has an alias key, but I think we may want to go the other way around. For example something like random = { version = "0.3", crate = 'rand' }. I think this works better with TOML where you can't duplicate keys in a table, and putting the unique identifier as the "key" in the table here allows to have greater customization like:

    random01 = { version = "0.1", crate = 'rand' }
    random02 = { version = "0.2", crate = 'rand' }
    random03 = { git = 'https://github.com/rust-lang/rand', crate = 'rand' }

    Notably we could get "depend on multiple versions" for free (or even from different sources!) if we invert the keys here. I think this is also a little clear as the "names on the left" are what you see in use in the crate. Note that crate probably isn't a great name for this key, but I'm sure others will have thoughts!

  • Right now the final state for extern crate leaves the story for "sysroot crates" open for interpretation. For example this is accepted code in Rust today:

    #![no_std]
    
    #[cfg(feature = "std")]
    extern crate std;
    
    fn main() {}

    Notably you can use extern crate to load crates in the sysroot. This is done in many #![no_std]-like situations where you are unconditionally #![no_std] (bringing in the core crate) and then you optionally load the std crate depending on a Cargo feature, enhancing the API appropriately.

    A strict interpretation of this RFC means that this pattern is no longer possible, so we may want to make sure we accomodate it! I'd personally be ok if use foo looked in the sysroot for a foo crate and automatically load it. We could also leave around extern crate for sysroot-only crates (but I'm not a huge fan of this solution). in any case, may be worth a mention in the RFC!

@est31

This comment has been minimized.

Contributor

est31 commented Aug 25, 2017

Huge 馃憤! I feel that this RFC is definitely a benefit for Rust, and improves the language. And I'm saying this as someone who has been opposing many of the previous module system proposals. I fully agree that thanks to this proposal, the Rust module system will be more clear and easier to use while still being as powerful and versatile as we know it today.

I want to thank @cramertj, @withoutboats and @aturon for their awesome work in making this RFC happen.

@cramertj

This comment has been minimized.

Member

cramertj commented Aug 25, 2017

@alexcrichton

A strict interpretation of this RFC means that this pattern is no longer possible, so we may want to make sure we accomodate it! I'd personally be ok if use foo looked in the sysroot for a foo crate and automatically load it. We could also leave around extern crate for sysroot-only crates (but I'm not a huge fan of this solution). in any case, may be worth a mention in the RFC!

Under the RFC as-is, I think the replacement would be something like this:

#![no_std]

#[cfg(feature = "std")]
crate use std;

fn main() {
    // in some other cfg'd code:
    let foo = crate::std::boxed::Box::new(5);
}

Edit: note, though, that you probably wouldn't need this. Just writing the following would conditionally import std:

#![no_std]

#[cfg(feature = "std")]
mod my_mod {
    use std::<something>;
}

Edit 2: I'm seeing lots of "confused" reactions-- can someone respond and explain what is confusing here? The idea is to import the std crate based on a feature flag, right?

- Second, one benefit of `crate` is that it helps reduce confusion about paths
appearing in `use` versus references to names elsewhere. In particular, it
serves as a reminder that `use` paths are absolute.

This comment has been minimized.

@vadimcn

vadimcn Aug 25, 2017

Contributor

Another alternative might be to improve error messages.

Using example from Motivation, rather than just saying "Use of undeclared type or module futures", as rustc does now, it could add a note to the effect of "...but I did find an extern crate of this name. You can use an absolute path to refer to it as ::futures::Poll, or you can bring it into the current scope with use futures;".

This comment has been minimized.

@est31

est31 Aug 25, 2017

Contributor

The asymmetry between lib.rs and all other files doesn't just show up in the extern crate; declarations, it also appears when you do use std::mem; use mem::swap;. You can only do this in the crate root and as probably your first steps in Rust are by coding stuff inside the crate root and after that moving stuff from there into modules, it gives you this moment of surprise right at the start when you realize that submodules work differently. You can fix this inconsistency in two ways: either change how the crate root works, or change how use works to be always like the crate root, aka relative. Previous proposals wanted to do precisely this change to use, but I think this approach is nicer because its more conservative. Usually most of a crate's code is not inside lib.rs/main.rs.

This comment has been minimized.

@vadimcn

vadimcn Aug 26, 2017

Contributor

My point was that rather than revamping the module system, we could simply provide better error messages, like we do in other circumstances when the compiler can reasonably guess what went wrong. If we can't find module 'foo' in the current scope, but there is one in crate root, it's probably what the user wanted.

I don't think I've seen this option mentioned anywhere (at least lately). If it had been considered, maybe it should be mentioned among the alternatives along with why this isn't a good idea.

Another drawback is that imports from within your crate become more verbose,
since they require a leading `crate`. However, this downside is considerably
mitigated if [nesting in `use`] is permitted.

This comment has been minimized.

@vadimcn

vadimcn Aug 25, 2017

Contributor

... or the compiler could search both the global namespace and the crate root (for relative use's only, use ::... would still be absolute).

@cramertj cramertj added the T-lang label Aug 25, 2017

@DevOrc

This comment has been minimized.

DevOrc commented Aug 25, 2017

I have been loosely following these different proposals and none of them really got me excited. However this feels like a great compromise! Nice job!

Nice job @aturon, @withoutboats, and @cramertj! 馃憤

@ExpHP

This comment has been minimized.

ExpHP commented Aug 25, 2017

For those of you who have stopped following due to fatigue 馃槱: this is almost certainly the final iteration, and I'd like to ask you one last time to take a fresh look -- almost all of the text here is new. 鉂わ笍

Really, everybody please do!

Speaking as somebody who could barely even stand to hear the word "module" any more, I can attest that this proposal is different. It is short, has a single, clear focus, and will take maybe 5-8 minutes of your time to read through from start to finish. (I'm not sure; I should've used a stopwatch!)

@Manishearth Manishearth referenced this pull request Aug 25, 2017

Closed

New modules RFC #37

@hawkw

This comment has been minimized.

hawkw commented Aug 25, 2017

A huge thank-you to @aturon, @withoutboats, @cramertj, and everyone else who's worked on this series of RFCs 鈥 I know it's been an incredibly difficult process for so many people, but I'm really happy to see that it looks like we're close to reaching a compromise that seems acceptable both in improving the ergonomics story for beginners and not breaking existing code too drastically.

Thank you all for your hard work! 鉂わ笍

@letheed

This comment has been minimized.

letheed commented Aug 25, 2017

I like it. Unlike most other proposals, I have no major reservation/pain point with this one. Looks like a clear improvement on all fronts to me. I'm happy about it, and kinda relieved to be honest (looking back on other proposals).

It's been really hard to follow, so a big thank to all that worked on this for reaching this great consensus.

@arthurprs

This comment has been minimized.

arthurprs commented Aug 25, 2017

I'm not a fan of crate being used in the place of pub and the companion linter. I fail to see it bringing a net benefit.

Edit: does the crate/pub also apply for struct fields?

Overall this is great! A lot of those for all involved so far 鉂わ笍

@newpavlov

This comment has been minimized.

newpavlov commented Aug 25, 2017

Great work! Some comments and questions:

Should mod.rs be deprecated?

I don't think so, first without it we reduce need for fixing perfectly fine code, and second I personally like that to rename/move module you just need to rename/move folder and fix mod declarations in the parent module(s). If we leave mod.rs, then it should be mentioned that having both foo.rs and foo/mod.rs will result in compilation error.

Rather than using crate as a visibility specifier, we could use something like local.

I prefer crate much more as it feels like a proper keyword on par with selfand super, plus it will be in sync with crate as visibility modifier. BTW I think it's worth to add to RFC examples of using crate mod/fn/etc.

What about inline modules? As I understand they will be left untouched?

@alexcrichton

Right now the final state for extern crate leaves the story for "sysroot crates" open for interpretation.

In your example you forgot to add #![cfg_attr(not(feature = "std"), no_std)].

If possible I think ideal solution would be to allow use of std and core simultaneously:

use core::ops::Add;
#[cfg(feature = "std")]
use std::io::Read;

And issue compilation errors if use std is detected with activated #![no_std]

@kornelski

This comment has been minimized.

Contributor

kornelski commented Aug 25, 2017

edit: nevermind, I forgot that epochs are explicit opt-in

If I understand correctly, the second epoch requires migrating all use foo to use crate::foo, so it will very likely affect every crate that has modules. Epochs for catch seemed doable, because only edge cases were affected (no breakage in practice for majority of users). This sounds like it affects the common case, so it's a massive breakage for almost all users.

There's probably a long tail of crates on crates.io that are not maintained. Are they going to be left broken? Forcefully rustfix-ed? Is the second epoch going to be delayed for many years, until nobody cares about the abandoned crates?

@steveklabnik

This comment has been minimized.

Member

steveklabnik commented Aug 25, 2017

Are they going to be left broken?

The epochs system means that they will not break; they'd be on epoch 2015.

@Ixrec

This comment has been minimized.

Contributor

Ixrec commented Aug 25, 2017

@pornel Maybe I've missed something, but I thought a major highlight of the epochs proposal was that crates written for different epochs are fully intercompatible, so that abandoned crates for old epochs will
almost never be a problem in practice no matter how widely used they are (and the exceptions involved things like deprecated syntax in public APIs, which doesn't seem to apply here). Did you see some sort of cross-epoch incompatibility in this proposal?

@nicoburns

This comment has been minimized.

nicoburns commented Aug 25, 2017

I think this is the right change.

I would really like to see exploration of a closer mapping of modules to the filesystem. But I feel like doing that cleanly may require removing mod entirely, and thus is a much larger change: better to not start down that path at all than to half-ass it.

I would also like to see some syntax sugar for crate/self/super: something to visually distinguish them from module identifiers like . and .. do in unix path syntax. I am anticipating paths like super::super::super::modulex being potentially quite painful. But perhaps syntax highlighting will alleviate a lot of that pain. And at the end of the day, it's a fairly minor issue.

@Rufflewind

This comment has been minimized.

Rufflewind commented Aug 25, 2017

If a path begins with a name that does not correspond to an external crate, the compiler will generate an error. If there is a top-level module with the same name, the compiler error will point this out and suggest adding crate.

So I take this to mean that in Epoch 2 it will be possible to use an external library named math (use math) while simultaneously having a top-level module named math (use crate::math)?

@aturon

This comment has been minimized.

Member

aturon commented Aug 25, 2017

@Rufflewind

So I take this to mean that in Epoch 2 it will be possible to use an external library named math (use math) while simultaneously having a top-level module named math (use crate::math)?

Yes, that's the idea! And in particular, there's no way to get there without a new epoch.

```rust
// Either of these work, because we brought `std` into scope:
fn make_vec() -> Vec<u8> { ... }
fn make_vec() -> std::vec::Vec<u8> { ... }

This comment has been minimized.

@mark-i-m

mark-i-m Aug 25, 2017

Contributor

Should the comment be

// Either of these work because we brought both `std` and `std::vec::Vec<u8>` into scope

?

```
All `use` declarations are interpreted as fully qualified paths, making the
leading `::` optional for them.

This comment has been minimized.

@aturon

aturon Aug 25, 2017

Member

Yes, the guide is describing how things will look under the full implementation, which means a future epoch.

- Cargo will provide a new `alias` key for dependencies, so that e.g. users who
want to use the `rand` crate but call it `random` instead can now write `rand
= { version = "0.3", alias = "random" }`.

This comment has been minimized.

@mark-i-m

mark-i-m Aug 25, 2017

Contributor

Perhaps I'm missing something obvious, but how does rustc get this alias information? Does it get passed by cargo or does rustc parse the Cargo.toml? The RFC says that the interface between rustc and cargo doesn't change, so I'm a bit confused here...

This comment has been minimized.

@eddyb

eddyb Aug 25, 2017

Member

Cargo already passes --extern rand=path/to/rand.rlib, all it has to do is change the first rand to be random, but the interface remains the same. Maybe the RFC should be more explicit here?

This comment has been minimized.

@epdtry

epdtry Aug 25, 2017

It's not spelled out here, but in previous proposals, the idea was for cargo to pass --extern foo=.../libbar.rlib. This form of aliasing is already supported by rustc, so the only change is to let cargo make use of it.

This comment has been minimized.

@mark-i-m

mark-i-m Aug 26, 2017

Contributor

Ah, that makes sense. Perhaps this could be added to the RFC, please, @aturon ?

@Phrohdoh

This comment has been minimized.

Phrohdoh commented Aug 25, 2017

Just a quick question from me:

This RFC states that extern crate a as b; would be deprecated.
Is use a as b; valid in this context still?

Everything else looks fantastic and is what I had hoped for when I began reading and writing Rust.

Edit:

Actually, mostly everything looks good.

I do not like implicitly bringing things into scope.
One of my favorite bits of Rust is explicitly knowing where something comes into scope / namespace.

@kornelski

This comment has been minimized.

Contributor

kornelski commented Sep 16, 2017

@liigo You're looking to narrowly at the specific syntax, rather than the concept/analogy in other languages. The RFC notes that in other languages name of your own package is used rather than literally crate, e.g. in Java:

import com.your-crate.foo;
import com.someone.elses.crate.bar;

rather than

import foo;
import extern.com.someone.elses.crate.bar;

The point is that all imports and paths appear to use absolute paths and this is true for many languages.

@mark-i-m

This comment has been minimized.

Contributor

mark-i-m commented Sep 16, 2017

@daboross

This comment has been minimized.

daboross commented Sep 16, 2017

I'd just like to say that while I had some reservations mentioned earlier about the crate visibility, I don't anymore. I was trying to come up with a concrete example on how it could be misused, and I can't think of any that are actually feasible. Even when I was objecting the syntax still seemed like a good idea, just one which should have a drawback listed in the drawbacks section for it.

馃憤 for the current proposal in that regard.

For everything else, I do have some objections - but I believe they're influenced too greatly by working with current rust - and thus aren't that useful for this overhaul. I really like having modules and crates and items in the exact same namespace, but I understand that the only real precedent for the current system is Python's.

@aturon aturon merged commit 8248775 into rust-lang:master Sep 17, 2017

@aturon

This comment has been minimized.

Member

aturon commented Sep 17, 2017

This RFC has now been merged! Thank you, all, for what has surely been the most extensive continuous design discussion in Rust history (considering the prior RFCs and internals threads). This discussion helped shape a huge number of iterations, and the final design is much stronger for it -- besides which, it is also appealing to a much wider array of stakeholders. And in particular, I believe that all major points that have been raised on the final design have been addressed, either through changes or discussion about the tradeoffs.

As per recent comments, there are two formal Unresolved Questions left:

  • The exact migration strategy (epochs vs resolution fallback); wants to be informed by hands-on experience with a rustfix tool.
  • The exact absolute path syntax; crate:: vs extern::, or perhaps a combination yet unthought of. Here, too, we need some hands-on experience to reach a final decision, and there is also interaction with migration issues.

Discussion on those points will continue on the tracking issue.

@vitiral

This comment has been minimized.

vitiral commented Sep 18, 2017

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

@nox

This comment has been minimized.

Contributor

nox commented Sep 23, 2017

I just realised that crate visibility has been introduced mainly because some argue that a mistakenly public item could be a safety hazard, but I don't see anyone argue that about the automatic implementations of Send and Sync. My guess is that this is because the latter doesn't actually cause safety hazards, just like I don't think crate visibility is needed.

@ExpHP

This comment has been minimized.

ExpHP commented Sep 23, 2017

I just realised that crate visibility has been introduced mainly because some argue that a mistakenly public item could be a safety hazard

Do you mean safety as in unsafe? Even after getting Github to show everything it has hidden I only see one small little discussion about unsafe.

Privacy is an integral part of safety, no doubt about that; but I suspect that most usage of pub(crate) is for the far more mundane concerns of semver and stability.

My guess is that this is because the latter doesn't actually cause safety hazards

Assuming you mean the unsafe kind of safety hazards: Automatic Sync impls caused a soundness bug in MutexGuard.

@Michael-F-Bryan

This comment has been minimized.

Michael-F-Bryan commented Dec 22, 2017

@aturon, it looks like the "rendered" link in this issue's description is broken. Any chance you can update it? It looks like it's just a case of changing the placeholder 0000 to 2126 in the URL.

@Lokathor

This comment has been minimized.

Lokathor commented Jan 9, 2018

@aturon your "rendered" link is unfortunately still broken

@Centril

This comment has been minimized.

Contributor

Centril commented Jan 9, 2018

@Lokathor Thanks for the reminder - I went ahead and fixed it.

@dlukes

This comment has been minimized.

dlukes commented Jul 22, 2018

I've been searching for that experimental RFC regarding "the possibility of determining modules from the file system" (as mentioned in OP) but can't find it anywhere... Any pointers, please? :)

@joshtriplett

This comment has been minimized.

Member

joshtriplett commented Jul 22, 2018

@dlukes I don't believe that particular facet ever got as far as an eRFC, only discussion. That may get revisited after the 2018 edition.

@dlukes

This comment has been minimized.

dlukes commented Jul 22, 2018

@joshtriplett Oh OK, thanks :) Sorry, I guess this must've been discussed somewhere at some point, but as a newcomer to the discussion, it's a bit daunting to comb through the sheer volume of comments in the various threads devoted to the subject.

@aturon Please maybe remove "The experimental RFC will be posted soon." from OP then, if you have a bit of spare time, so as not to confuse latecomers? :)

In any case, thanks for the huge amount of intellectual and emotional energy you guys spent on this! For a casual user such as myself, it was fairly hard to translate the seemingly simple rules behind Rust's original module system into the right set of intuitions and habits.

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