New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stabilize uniform paths on Rust 2018 #55618

Open
withoutboats opened this Issue Nov 2, 2018 · 94 comments

Comments

Projects
None yet
@withoutboats
Contributor

withoutboats commented Nov 2, 2018

@rfcbot fcp merge
cc #53130

This issue is to track the discussion of the following proposal:

I propose we stabilize the uniform_paths feature in Rust 2018, eliminating the anchored_paths variant. I further propose we backport this stabilization to version 1.31 beta, so that all stable versions of Rust 2018 have this feature.

We've taken informal polls about this question in a paper document. Most of the lang team has favored uniform_paths, I and @cramertj were the only two members who initially favored anchored_paths. Yesterday, we discussed this in the lang team video meeting; in the meeting we sort of crystallized the various pros and cons of the two variants, but didn't reach a consensus.

In the time since the meeting, I've come around to thinking that stabilizing on uniform_paths is probably the best choice. I imagine @cramertj has not changed his mind, and though he said in the meeting he wouldn't block a decision, I hope we can use this FCP period to see if there's any agreeable changes that would make uniform_paths more palatable to him.

I've also written a blog post summarizing the meeting discussion and how my position has evolved, which provides more context on this decision.

I also want to make sure we continue to get community feedback as we reach this final decision! Please contribute new thoughts you have to the discussion. However, we will hopefully make this decision within the next few weeks, and certain proposals are out of scope. Any third proposal which is not completely backwards compatible with the current behavior of paths in Rust 2018 is impossible to ship in Rust 2018 at this point. Variations on anchored or uniform paths that are backwards compatible may be considered, but that also seems unlikely.

@withoutboats withoutboats added the T-lang label Nov 2, 2018

@withoutboats

This comment has been minimized.

Contributor

withoutboats commented Nov 2, 2018

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

rfcbot commented Nov 2, 2018

Team member @withoutboats has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@joshtriplett

This comment has been minimized.

Member

joshtriplett commented Nov 2, 2018

@withoutboats Thank you for your detailed writeup, as well as your careful and considered evaluation.

Having been the sole opposition of more than one language proposal in the past, I would like to explicitly extend the following invitation to @cramertj: If you feel you need more time to write up some thoughts on how uniform_paths could adapt to better meet the needs of Rust users in your group and elsewhere at Google, as well as any concerns about uniform_paths that didn't make it into yesterday's discussion as captured by @withoutboats' writeup, please feel free to either register a concern or simply state that you'd like more time. This is not a vote, and despite rfcbot's default behavior for what it takes to move forward with an FCP, I'd like us to reach a full consensus if possible.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Nov 2, 2018

please feel free to either register a concern or simply state that you'd like more time

It's good to say it explicitly, of course, but -- to be clear -- I feel this is always true for any FCP =)

@Centril

This comment has been minimized.

Contributor

Centril commented Nov 2, 2018

Excellent writeup @withoutboats. ❤️

Report & Tests

@rfcbot concern stabilization-report-and-tests

Before we stabilize uniform_paths, I think we need a stabilization report that documents what behavior is being stabilized (and what is not), and tests for that behavior. Since @petrochenkov and @eddyb have been at the forefront of implementing the behavior, maybe they could write up the report?

File-centric behavior

@rfcbot concern file-centric-behavior

As for what we're going to stabilize here, @nikomatsakis noted in the paper document that a more file-centric adaptation of uniform_paths is favourable such that you may write:

use std::sync::Arc;

mod foo {
    mod bar {
        // uses the `use` from parent module, because it is in the same file
        fn baz() -> Arc<u32> { ... }
    }
}

Benefits

The file-centric approach is beneficial mainly because it is quite common for languages to take such an approach and Rust is the odd fish in the bunch; By adopting an approach where the above snippet is legal, the speed-bump when learning Rust can be reduced. For example, in Java, inner classes can see the imports at the top of the file.

Another benefit of the file-centric approach is that you no longer have to write the use super::*; boilerplate, which is arguably a misfeature, anymore. This makes writing unit tests more ergonomic:

use some_crate::Thing;

fn foo(x: Bar) -> Baz { ... }

#[cfg(test)]
mod test {
    // use super::*;
    // ^-- you don't have to do this anymore.

    ...
}

Drawbacks

I see two principal drawbacks with the file-centric behavior:

  1. It encourages writing large files and thus may diminish the maintainability of large projects.

  2. Inline modules are no longer units that can be so easily refactored. For example, if you want to move an inline mod foo { ... } into a dedicated file, then you may need to scroll up to the beginning of the file and copy the use statements. This can be mitigated with IDEs -- but not everyone have those.

One mitigating factor here is that it is unlikely to have inline modules which nest; the most common use of inline modules are for unit tests or for small modules that contain platform specific behavior -- in the language team meeting, everyone agreed that this was the case and that nested inline modules were not commonly used.

Conclusion

All in all, while I was initially somewhat skeptical about the file-centric behavior, I think it is in line with one of the core goals of uniform_paths -- to make the module system intuitive and easy to learn. Therefore, I'd like to propose that we incorporate the file-centric behavior into the stabilization (EDIT: or at least make it possible after stabilization...).

Why does it have to happen now? Consider the following (playground):

// Nightly, Edition 2018 on.

#![feature(uniform_paths)]

// Let's assume that there's a crate `foo` in the extern prelude with `foobar` in it.

mod foo {
    pub fn foobar() -> u8 { 1 }
}

mod bar {
    fn barfoo() -> u8 {
        // With the current `uniform_paths`,
        // there's no conflict as far as the resolution system sees it.
        //
        // Thus, if `foo` is in the extern prelude, it will resolve to the
        // `foo` crate and not `foo` the sibling module. If we allow that
        // to pass, then a *breaking change* will be the result if we change
        // to the file centric behavior.
        foo::foobar()
    }
}
@lucozadeez

This comment has been minimized.

lucozadeez commented Nov 2, 2018

Apologies if this has already been considered but would extern::foo work to disambiguate uniform paths?

So the logic would always be

  • crate::foo:: for modules in the crate
  • extern::foo:: for external modules
  • foo:: for submodules if there is a matching submodule or external if not

It has the advantage of using an existing keyword. I couldn't see any reason for there to be a parsing ambiguity but I could be wrong there.

@Centril

This comment has been minimized.

Contributor

Centril commented Nov 2, 2018

@lucozadeez

Apologies if this has already been considered but would extern::foo work to disambiguate uniform paths?

To disambiguate, you can write ::foo instead; this will have the same effect.
We could also permit extern::foo but that would be sort of redundant (more than one ways to do it without clear benefits...).

I couldn't see any reason for there to be a parsing ambiguity but I could be wrong there.

I don't think there would be any ambiguity.

@joshtriplett

This comment was marked as resolved.

Member

joshtriplett commented Nov 2, 2018

@Centril I feel that the "file-centric approach" is something that could be introduced as an extension later, with sufficient care. And as @withoutboats put it, "Any third proposal which is not completely backwards compatible with the current behavior of paths in Rust 2018 is impossible to ship in Rust 2018 at this point. Variations on anchored or uniform paths that are backwards compatible may be considered, but that also seems unlikely."

As such, please consider whether the "file-centric approach" could be written as a compatible add-on.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Nov 2, 2018

@Centril

Before we stabilize uniform_paths, I think we need a stabilization report that documents what behavior is being stabilized (and what is not)

😍 Yes please! 😍

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Nov 2, 2018

I think we need a stabilization report that documents what behavior is being stabilized (and what is not), and tests for that behavior

Implementation is in progress, so the behavior will change slightly.
I landed some preliminary refactorings last weekend and hope to finish the work this weekend.
I want to do it in such way that uniform paths are always enabled on 2018, but gated if actually used, so we get feature gate errors instead of "unresolved name" errors.
This is possible because "anchored paths with future-proofing" is a strict subset of uniform paths.

I'd prefer to test the new implementation for one more cycle and not backport stabilization to 1.31.

@nikomatsakis

This comment was marked as resolved.

Contributor

nikomatsakis commented Nov 2, 2018

So @Centril points out that extending to support "all enclosing scopes within the file" would not, strictly speaking, be backwards compatible (without some sort of fallback or prioritization, I guess). For example this would compile today but yield ambiguity tomorrow:

mod rayon { ... }

mod foo {
    use rayon::join; // today, compiles to extern crate `rayon`
}

Similarly, this builds today:

#![feature(uniform_paths)]

mod bar {
    pub fn x() { }
}

fn main() {
    mod bar { pub fn y() { } }
    
    use bar::x;
    
    x();
}

Regardless, I'm not inclined to block much longer on this stuff. Que sera sera. Gotta ship someday. =)

@nikomatsakis

This comment was marked as resolved.

Contributor

nikomatsakis commented Nov 2, 2018

Good point and I suppose this is the cause for their "concern".

@Centril

This comment was marked as resolved.

Contributor

Centril commented Nov 2, 2018

I would be OK with skipping the uniform_paths_file_centric adaptation and just go with uniform_paths. But I am concerned about "que sera sera" when we know the better approach (if we can agree it's better...) and it isn't a pipe-dream... It would be one thing to avoid shipping if there was a hypothetical better solution, but it isn't hypothetical here. My main concern with doing changes in 2021 towards the file-centric approach would be technical debt; I'd like to avoid that technical debt even if we have to wait 1/2 more months. I suppose we could do a forward-compat warning and then just do it inside Rust 2018 as well but that is sort of breaking our stability promises...

It seems to me that instituting changes to uniform_paths that ensures forward-compatibility with _file_centric may be as complex as implementing _file_centric itself...? Maybe @petrochenkov can talk a bit about that?

@petrochenkov

This comment was marked as resolved.

Contributor

petrochenkov commented Nov 2, 2018

uniform_paths_file_centric

  • First, I still don't see why this is an improvement and desirable change.
  • Second, I don't think decisions like this (or implementation work like this) need to be done in the last moment.
  • Third, it's always possible to do backward-compatibly with some opt-in syntax #[transparent] mod m { ... } or something. (Pro: can work on out-of-line modules as well.)
  • Fourth, some opt-out syntax is probably needed anyway. What if you actually need the isolation provided by modules? Moving code into a separate file is not always possible or convenient (e.g. what if the module is generated by a macro).

So, my suggestion would be to pursue this in some opt-in form after the edition is released. Then if it's implemented, 2-3 years of usage experience should be enough to decide whether it should be enabled by default in some cases in Rust 2021 or not.

@Centril

This comment was marked as resolved.

Contributor

Centril commented Nov 2, 2018

  • Second, I don't think decisions like this (or implementation work like this) need to be done in the last moment.

So I take it you don't think it can be feasibly implemented in the short period remaining until Edition 2018 ships?

  • Third, it's always possible to do backward-compatibly with some opt-in syntax #[transparent] mod m { ... } or something. (Pro: can work on out-of-line modules as well.)

I don't think that would be worth it and it would just be even more technical debt; the idea is that it should be intuitive by default... using that attribute wouldn't be.

  • Fourth, some opt-out syntax is probably needed anyway. What if you actually need the isolation provided by modules? Moving code into a separate file is not always possible or convenient (e.g. what if the module is generated by a macro).

This makes a lot of sense, so it seems like the file-centric idea might need some design work + process given that.

So, my suggestion would be to pursue this in some opt-in form after the edition is released. Then if it's implemented, 2-3 years of usage experience should be enough to decide whether it should be enabled by default in some cases in Rust 2021 or not.

I suppose that the file-centric approach isn't very actionable right now and neither are the forward-compatibility hacks to make it possible later...

I think we might just have to either use forward-compat warnings later (and implement them mid-edition-2018), or do it in Rust 2021 (tho the technical debt isn't great...). Therefore...
@rfcbot resolve file-centric-behavior

@mark-i-m

This comment has been minimized.

Contributor

mark-i-m commented Nov 2, 2018

@withoutboats You're blog post is really helpful. Thanks!


Personally, I've been using edition 2018 beta/nightly for while now, and I haven't really run into self:: or ::foo, so I don't have strong feelings. Overall, my inclination is still towards anchored paths because use statements are easier to mentally parse when reading code, as @withoutboats put it:

The use statements of anchored paths are syntactically disambiguated as to where they come from: either they come from a special namespace crate, super, and self, or it comes from an extern crate. This makes it easy to make sure your use statements are grouped together properly during code review without referencing other information.

@djc

This comment has been minimized.

Contributor

djc commented Nov 2, 2018

After reading @withoutboats post, I wanted to make sure the following compromise has been considered and rejected: to do uniform paths, but always requiring the leading :: for extern crate paths.

By disambiguating-by-default, the two disadvantages listed in the blog post get neutralized:

  • Use statements for extern crates are more distinctive again, making it easier to group them
  • Since the leading :: will become much more common, it will be that much less surprising

At the expense of undoing the initial advantage that :: would no longer be needed in non-use paths.

This seems like an elegant compromise to me after reading the blog post, but I fully realize it probably has already been discussed to death. Just wanted to throw this out there and maybe allow someone from the lang team to articulate why this alternative is less attractive than uniform-paths as proposed.

@mark-i-m

This comment has been minimized.

Contributor

mark-i-m commented Nov 3, 2018

I would personally really not like to write or read :: everywhere. My code would end up littered with them. I would rather see self:: in a few places than :: everywhere I use an external crate. I guess that's a matter of taste, though.

@TimNN

This comment has been minimized.

Contributor

TimNN commented Nov 3, 2018

Could someone please remind me how the following would work for uniform and anchored paths?

// Assume there is also an
// extern crate foo;
mod foo;

fn main() {
    // Does this call the module or the crate? (Or is it an error?)
    foo::bar();
    // How would I call the other?
}

Edit: Also, would use self::* be allowed with uniform paths? Or would that syntax be forbidden?

@tommit

This comment has been minimized.

tommit commented Nov 3, 2018

Whichever path design you choose, I'd wish everybody wrote use self::foo::Foo; rather than use foo::Foo; because then it's immediately clear to someone reading the code that foo is not an external crate but probably a submodule given the letter casing. And like the old adage says, code is written once and read many times, so it should be optimized for ease of reading rather than writing. I also think that the common case is when you have multiple imports from the current crate, in which case you wouldn't even be writing use self::foo::Foo; but rather there would be a whole section of imports from the current crate inside which the Foo import would go, and this would be the best option for reading comprehension:

use crate::{
    thismod::foo::Foo,
    // ...
};

But if you choose the uniform paths design, then there will probably be some who write their imports using absolute paths and some using relative paths, and it's the readers of the code who suffer from this mishmash of styles. So, I would definitely prefer you choose the anchored paths design, but I'd make one change, or addition to it: I'd allow you to start the path in a use declaration with the name of an enum type that is currently in scope. This would provide a nicer syntax for importing enum variants (typically in function local scope) by simply: use Bar::*;. And the reason why this doesn't hurt readability is the fact that enum names tend to start with an upper case letter, whereas module and crate names typically start with a lower case letter. That is a good enough hint for the reader that Bar is most likely an enum that's currently in scope.

@tommit

This comment has been minimized.

tommit commented Nov 3, 2018

@TimNN

Could someone please remind me how the following would work for uniform and anchored paths?

// Assume there is also an
// extern crate foo;
mod foo;

fn main() {
    // Does this call the module or the crate? (Or is it an error?)
    foo::bar();
    // How would I call the other?
}

I just tried that out, and in both uniform and anchored paths designs, your code calls the module function. You can unambiguously call the module one with crate::foo::bar() or the extern crate one with ::foo::bar().

But what's more interesting is that this is a really bad behaviour. The act of adding a module should not silently hijack a function coming from an external crate. I think that is one of the main design principles of how imports are designed in the D language.

EDIT: I just re-read about how imports work in D language (which I had mostly forgotten about). The main design principle of imports in D says that: "Adding or removing imports can't change the decision of resolving a function name". Where "resolving a function name" means: "figuring out which function to call when the programmer writes foo()". And in D, importing a module brings all the symbols in that module into the current module's scope, after which there's a specific way in which the symbols coming from all the imported modules fight each other over which one of them is the best match for the call to foo().

This is so different from how things work in Rust, that I seem to be unable to compare the two, probably due to not knowing exactly how this stuff works in either language. But in Rust specifically, an external crate's name seems to get hidden by any other symbol in scope, including symbols coming from glob imports. For example, given the Cargo dependency num = "0.2.0", the following code prints "x: 3", and if you comment out the module num, then the code prints "x: 2", and if you then also comment out the module sub along with use crate::sub::*;, then the code prints "x: 1".

use crate::sub::*;

mod sub {
    // This, whether from a glob import or regular import, hides the extern crate `num`
    pub mod num {
        pub fn one() -> u64 {
            2
        }
    }
}

// This hides both the extern crate `num` and `crate::sub::num` coming from glob import
mod num {
    pub fn one() -> u64 {
        3
    }
}

fn main() {
    let x: u64 = num::one();
    println!("x: {:?}", x);
}

I realize that this kind of hiding behaviour is not the same thing as the "function hijacking" which D language's import rules are designed to prevent: a function that's imported from one module hijacking another function with the same name that's imported from a different module. That kind of hijacking is not possible in Rust either. In the example above, it's just a matter of a "more local" symbol hiding a "less local" or "more external" symbol. I think this is fine, and in fact D language does that kind of thing as well - an imported symbol gets hidden by a module local symbol.

@steveklabnik

This comment has been minimized.

Member

steveklabnik commented Nov 4, 2018

I would be shocked if the “file centric approach” was accepted. This is a major new thing that would be stabilized almost immediately without RFC-like discussion. I don’t like to speak too strongly but I’m shocked it was even suggested.

In terms of “anchored” vs “uniform”, I still prefer anchored, for basically all of these reasons: https://www.reddit.com/r/rust/comments/9toa5j/comment/e8xxs2p

I have a bit more to say but I’m about to catch a flight; in the end, I’m not willing to die on the hill of anchored paths, but do prefer them quite strongly.

@kornelski

This comment has been minimized.

Contributor

kornelski commented Nov 4, 2018

In the discussions submodules and disambiguation are often taken into account, but I'm not seeing much consideration of workspaces, so here's my use case.

In projects I write I use workspaces a lot. I have a natural progression for chunks of code:

  1. In its own module in the same crate, until it outgrows it
  2. In its own crate in the same workspace, unless its useful outside the project
  3. As a separate crate on crates.io

For example, see how many "internal" crates crates.rs has: https://gitlab.com/crates.rs

Currently, refactoring this way is actually convenient as there is no syntactic difference between modules and crates. I can replace mod foo with extern crate foo and it works beautifully (I generally don't nest modules, so implied extern would work fine too).

AFAIK anchored paths variant intentionally removes that ability, which is why I don't like it. Not only I'm not interested in marking whether something is a module or extern crate, I deliberately don't want that distinction and take advantage of modules and crates having uniform syntax.

@withoutboats

This comment has been minimized.

Contributor

withoutboats commented Nov 4, 2018

In terms of “anchored” vs “uniform”, I still prefer anchored, for basically all of these reasons: https://www.reddit.com/r/rust/comments/9toa5j/comment/e8xxs2p

Thanks for linking to this comment, I think it succinctly sums up the most common arguments in favor of anchored paths. But I must say, this summary reveals what seems to me to be a flaw in understanding about how paths work in the anchored paths proposal (or indeed, in any version of Rust that exists): the post claims quite emphatically that the appeal is that you "ALWAYS" anchor paths, but this is not true: you only anchor paths in use statements, all other paths use the mechanism that uniform paths propose to apply to use statements as well.

All but the last argument in this comment (which is a claim about Rust's philosophy I don't agree with) seem to fall apart when you consider that even under anchored paths, paths sometimes have the uniform path semantics. You are still subject to the same refactoring hazards, you still have to understand the contextual information, its just not true in the case of use statements.

This is why uniform paths is called uniform paths: it makes this semantics true everywhere, instead of having a separate logic that only applies to one item form.

@tommit

This comment has been minimized.

tommit commented Nov 4, 2018

@kornelski

Currently, refactoring this way is actually convenient as there is no syntactic difference between modules and crates. I can replace mod foo with extern crate foo and it works beautifully (I generally don't nest modules, so implied extern would work fine too).

AFAIK anchored paths variant intentionally removes that ability, which is why I don't like it. Not only I'm not interested in marking whether something is a module or extern crate, I deliberately don't want that distinction and take advantage of modules and crates having uniform syntax.

Both of the new path variants remove that ability inside modules. The uniform paths variant retains that ability only in the current crate's root, which isn't usually where most of the code is.

@withoutboats

This comment has been minimized.

Contributor

withoutboats commented Nov 4, 2018

Apologies if this has already been considered but would extern::foo work to disambiguate uniform paths?

This works fine and is even preferable in my opinion; since its only used for disambiguating, being kinda long isnt the problem, and use extern::foo is very clear in its disambiguation.

However, I think we're currently using :: because its consistent with the behavior on 2015. Basically I think the most likely path toward using extern:: is to introduce extern:: as a synonym for ::, later deprecate :: (at least outside of 2015), and possibly someday remove :: in an edition update.

I wanted to make sure the following compromise has been considered and rejected: to do uniform paths, but always requiring the leading :: for extern crate paths.

I'm fairly certain we've discussed this and ultimately rejected it. I see this as the third of the three potentially viable variants on the 2018 path changes, each of which have two of these three desirable properties:

  • 1path consistency
  • Never disambiguating in a use statement
  • Less syntactic noise

We've already decided that the third bullet is desirable, and its been a question between the other two bullets, which is what anchored and uniform represent.


More broadly, I want to point out that while having ambiguity might sound troubling, the reality is that name resolution is already full of ambiguity, for example, glob imports and the prelude introduce potential name conflicts which are partially solved through shadowing rather than disambiguation.

@phaylon

This comment has been minimized.

phaylon commented Nov 8, 2018

@rpjohnst I'm interested in what the current module/file is connected to. Cargo.toml is of no use for that. See the above example, where a complete header gives a nice overview. It's by no means absolute, I'll also have trait imports inside functions. But I still find it very informative. It's also nice when things show up when I grep for use failure.

But as said above, that's another thing that a lint can fix easily, since writing down the imports is still possible.

@joshtriplett

This comment has been minimized.

Member

joshtriplett commented Nov 8, 2018

Based on discussions in the lang team meeting, we observed that it is the de facto standard (rustc's own code notwithstanding) to use separate blocks of use statements (separated by a blank line) for external crates and for internal modules. That should address part of @cramertj's concern; the rest will need future tooling to help enforce that, whether in the form of rustfmt or lints.

Speaking with my style team hat on, I think we should add that de facto standard to the style guide.

@aturon

This comment has been minimized.

Member

aturon commented Nov 8, 2018

@cramertj can you mark your idiom concern as resolved?

@cramertj

This comment has been minimized.

Member

cramertj commented Nov 8, 2018

@rfcbot resolve idiomatic_use

General consensus is that idiomatic use of this feature requires users to separate items being imported from the current module from items imported from external crates, either through a separate use import or by introducing a blank line between the imports. I'm concerned that these properties can't be upheld by rustfmt without implementing expansion and name resolution, which isn't possible when formatting a single file. However, I recognize that the concern I have is difficult to weigh against the undeniable learnability benefits uniform_paths brings.

unform_paths gets us closer to the one-path vision, and I'm looking forward to seeing a new generation of Rustaceans who aren't confused by the module system ;)

@aturon

This comment has been minimized.

Member

aturon commented Nov 9, 2018

@rfcbot concern implementation

I wanted to log the concern raised by @petrochenkov on Discord:

I'm wary of stabilizing uniform paths until

  • the reimplementation lands
  • some large project (e.g. Servo) migrates to it and doesn't encounter a bunch of "import resolution is stuck" errors
    It's a quite fragile feature and the current approximate implementation doesn't fully represent the possible issues.

Given that the edition release is 4 weeks away, and we should have artifacts ready in probably 2 weeks time, I feel like the above is probably a show-stopper for moving to uniform paths at the Edition release, and we should hold off until the next cycle.

Thoughts?

@Centril

This comment has been minimized.

Contributor

Centril commented Nov 9, 2018

Thoughts?

We have the future compat bits in place so if this has to wait until the next cycle it has to wait until the next cycle. Better safe than sorry!

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Nov 9, 2018

@Centril

We have the future compat bits in place

We don't have all the future compat bits in place - that's the point, so the reimplementation is going to land on the current beta (next week, most likely), but it's somewhat risky to stabilize it immediately after that without testing on complex crates with macros, globs and everything (e.g. Servo).

@Centril

This comment has been minimized.

Contributor

Centril commented Nov 9, 2018

@petrochenkov right, OK; so we should get all the future compat bits in place and then wait a bit until you are comfortable with the implementation having been tested by Servo and other crates. I'm down with that. I think we can defer the appropriate moment to stabilize to you. :) (and we need the stabilization report done anyways...)

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Nov 12, 2018

Reimplementation PR - #55884.

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Nov 12, 2018

Just in case, if something goes wrong with enabling uniform paths by default (e.g. too many "resolution is stuck" errors), then it's always an option to turn on the in-scope resolution on demand with something like in::foo::bar, so that local imports like

fn f() {
    enum E { A, B }
    use in::E::*;
    match A {
        A => {}
        B => {}
    }
}

are still possible to write.

bors added a commit that referenced this issue Nov 13, 2018

Auto merge of #55884 - petrochenkov:uni, r=<try>
[beta] resolve: Implement uniform paths 2.0

With this PR paths in imports on 2018 edition are resolved as relative in the scope in which they are written, similarly to any other paths.
The previous implementation worked differently - it tried to resolve the import in the current module (`self::import`) and in the "crate universe" (`::import`), and then merge these two resolutions if possible.

The difference is that the new scheme can refer to strictly larger set of names in scope - names from unnamed blocks, names from all kinds of preludes, including macros imported with `#[macro_use] extern crate`, built-in types/macros, macros introduced with `macro_rules`.
This means strictly more potential ambiguities and therefore ambiguity errors, since we keep the rule that any two different candidate names in scope conflict with each other during import resolution. So this is a breaking change for 2018 edition, but it should be relatively minor.

All paths that don't start with an extern crate are also gated with the `uniform_paths` feature, paths that refer to extern crates are not gated (so we effectively get something like "future-proofed anchored paths" on stable).

Another difference is treatment of paths in visibilities (`pub(in path)`). Few people remember about paths in visibilities, so before this PR they still used the old 2015 rules even on 2018 edition. Namely, paths in visibilities were crate-relative, analogous to 2015 edition imports.
This PR resolves paths in visibilities as uniform as well, or rather future proofs them in this direction.
Paths in visibilities are restricted to parent modules, so relative paths almost never make sense there, and `pub(in a)` needs to be rewritten as `pub(in crate::a)` in the uniform scheme, de-facto cementing the discouraged status of non-`pub(crate)` and non-`pub(super)` fine-grained visibilities.
This is clearly a breaking change for 2018 edition as well, but also a minor one.

The in-scope resolution strategy for import paths mirrors what is currently done for macro paths on stable (on both editions), so it will continue working even if the "ambiguity always means error" restriction is relaxed in the future.

This PR also significantly improves diagnostics for all kinds of resolution ambiguities, from the newly introduced import ones to pretty old "glob vs glob" conflicts. (That's probably what I've spent most of the time on.)

Why beta:
- This is a breaking change on 2018 edition.
- This is a large PR, it's less risky to forward-port it to nightly, than back-port to beta.

cc #55618
cc #53130
cc rust-lang/rfcs#1289
Closes #18084
Closes #54525
Fixes #54390
Fixes #55668

r? @ghost

bors added a commit that referenced this issue Nov 14, 2018

Auto merge of #55884 - petrochenkov:uni, r=<try>
[beta] resolve: Implement uniform paths 2.0

With this PR paths in imports on 2018 edition are resolved as relative in the scope in which they are written, similarly to any other paths.
The previous implementation worked differently - it tried to resolve the import in the current module (`self::import`) and in the "crate universe" (`::import`), and then merge these two resolutions if possible.

The difference is that the new scheme can refer to strictly larger set of names in scope - names from unnamed blocks, names from all kinds of preludes, including macros imported with `#[macro_use] extern crate`, built-in types/macros, macros introduced with `macro_rules`.
This means strictly more potential ambiguities and therefore ambiguity errors, since we keep the rule that any two different candidate names in scope conflict with each other during import resolution. So this is a breaking change for 2018 edition, but it should be relatively minor.

All paths that don't start with an extern crate are also gated with the `uniform_paths` feature, paths that refer to extern crates are not gated (so we effectively get something like "future-proofed anchored paths" on stable).

Another difference is treatment of paths in visibilities (`pub(in path)`). Few people remember about paths in visibilities, so before this PR they still used the old 2015 rules even on 2018 edition. Namely, paths in visibilities were crate-relative, analogous to 2015 edition imports.
This PR resolves paths in visibilities as uniform as well, or rather future proofs them in this direction.
Paths in visibilities are restricted to parent modules, so relative paths almost never make sense there, and `pub(in a)` needs to be rewritten as `pub(in crate::a)` in the uniform scheme, de-facto cementing the discouraged status of non-`pub(crate)` and non-`pub(super)` fine-grained visibilities.
This is clearly a breaking change for 2018 edition as well, but also a minor one.

The in-scope resolution strategy for import paths mirrors what is currently done for macro paths on stable (on both editions), so it will continue working even if the "ambiguity always means error" restriction is relaxed in the future.

This PR also significantly improves diagnostics for all kinds of resolution ambiguities, from the newly introduced import ones to pretty old "glob vs glob" conflicts. (That's probably what I've spent most of the time on.)

Why beta:
- This is a breaking change on 2018 edition.
- This is a large PR, it's less risky to forward-port it to nightly, than back-port to beta.

cc #55618
cc #53130
cc rust-lang/rfcs#1289
Closes #18084
Closes #54525
Fixes #54390
Fixes #55668

r? @ghost
@liigo

This comment has been minimized.

Contributor

liigo commented Nov 14, 2018

@aturon

we should hold off until the next cycle.

Thoughts?

Should hold off edition 2018 until next year, and rename it to edtion 2019. Not just uniform paths, we have other important features not completed, e.g. async/await.

@mark-i-m

This comment has been minimized.

Contributor

mark-i-m commented Nov 14, 2018

I was under the impression that boat has already sailed. Either way, the only thing that really needs to be in 1.31 is any breaking changes that can't be done except across an edition boundary.

@mark-i-m

This comment has been minimized.

Contributor

mark-i-m commented Nov 16, 2018

So it seems that language team is pretty settled with choosing uniform paths? I still prefer anchored paths, but if the decision is made, then I guess that's fine. Mostly, it would be nice to get an official confirmation that that's still how the language team is leaning.

@withoutboats

This comment has been minimized.

Contributor

withoutboats commented Nov 16, 2018

@mark-i-m the position of the language team is pretty well tracked by RFCbot's post, which shows all but one member have checked to approve the proposal & the only concerns are about implementation and testing.

I think your comment might come more from a sense that more people on this thread spoke in favor of anchored paths than uniform paths. That's my impression of the situation, numerically at least. But that's not surprising: people usually comment when they disagree with a decision we're making, since there is little need to comment when it seems like what you would be trying to make happen is already going to happen. So I at least tend to only consider the arguments, and not the "mood of the thread," when making decisions.

None of the arguments led me to have concerns about uniform paths (and remember, I was on the anchored paths side until very recently). In fact, most seemed confused to me (as I tried to express several times), since they were objecting to the fallback mechanism in itself, which is a part of both proposals, but only relevant outside of use statements for anchored paths. I didn't find any compelling argument that the fallback was especially bad in use statements beyond the problem of sorting and separating use statements we've already all been aware of with uniform paths, and so none of this was convincing to me personally. Can't speak for other members of the team.

bors added a commit that referenced this issue Nov 17, 2018

Auto merge of #55884 - petrochenkov:uni, r=eddyb
[beta] resolve: Implement uniform paths 2.0

With this PR paths in imports on 2018 edition are resolved as relative in the scope in which they are written, similarly to any other paths.
The previous implementation worked differently - it tried to resolve the import in the current module (`self::import`) and in the "crate universe" (`::import`), and then merge these two resolutions if possible.

The difference is that the new scheme can refer to strictly larger set of names in scope - names from unnamed blocks, names from all kinds of preludes, including macros imported with `#[macro_use] extern crate`, built-in types/macros, macros introduced with `macro_rules`.
This means strictly more potential ambiguities and therefore ambiguity errors, since we keep the rule that any two different candidate names in scope conflict with each other during import resolution. So this is a breaking change for 2018 edition, but it should be relatively minor.

All paths that don't start with an extern crate are also gated with the `uniform_paths` feature, paths that refer to extern crates are not gated (so we effectively get something like "future-proofed anchored paths" on stable).

Another difference is treatment of paths in visibilities (`pub(in path)`). Few people remember about paths in visibilities, so before this PR they still used the old 2015 rules even on 2018 edition. Namely, paths in visibilities were crate-relative, analogous to 2015 edition imports.
This PR resolves paths in visibilities as uniform as well, or rather future proofs them in this direction.
Paths in visibilities are restricted to parent modules, so relative paths almost never make sense there, and `pub(in a)` needs to be rewritten as `pub(in crate::a)` in the uniform scheme, de-facto cementing the discouraged status of non-`pub(crate)` and non-`pub(super)` fine-grained visibilities.
This is clearly a breaking change for 2018 edition as well, but also a minor one.

The in-scope resolution strategy for import paths mirrors what is currently done for macro paths on stable (on both editions), so it will continue working even if the "ambiguity always means error" restriction is relaxed in the future.

This PR also significantly improves diagnostics for all kinds of resolution ambiguities, from the newly introduced import ones to pretty old "glob vs glob" conflicts. (That's probably what I've spent most of the time on.)

Why beta:
- This is a breaking change on 2018 edition.
- This is a large PR, it's less risky to forward-port it to nightly, than back-port to beta.

cc #55618
cc #53130
cc rust-lang/rfcs#1289
Closes #18084
Closes #54525
Fixes #54390
Fixes #55668

r? @ghost

bors added a commit that referenced this issue Nov 18, 2018

Auto merge of #55884 - petrochenkov:uni, r=eddyb
[beta] resolve: Implement uniform paths 2.0

With this PR paths in imports on 2018 edition are resolved as relative in the scope in which they are written, similarly to any other paths.
The previous implementation worked differently - it tried to resolve the import in the current module (`self::import`) and in the "crate universe" (`::import`), and then merge these two resolutions if possible.

The difference is that the new scheme can refer to strictly larger set of names in scope - names from unnamed blocks, names from all kinds of preludes, including macros imported with `#[macro_use] extern crate`, built-in types/macros, macros introduced with `macro_rules`.
This means strictly more potential ambiguities and therefore ambiguity errors, since we keep the rule that any two different candidate names in scope conflict with each other during import resolution. So this is a breaking change for 2018 edition, but it should be relatively minor.

All paths that don't start with an extern crate are also gated with the `uniform_paths` feature, paths that refer to extern crates are not gated (so we effectively get something like "future-proofed anchored paths" on stable).

Another difference is treatment of paths in visibilities (`pub(in path)`). Few people remember about paths in visibilities, so before this PR they still used the old 2015 rules even on 2018 edition. Namely, paths in visibilities were crate-relative, analogous to 2015 edition imports.
This PR resolves paths in visibilities as uniform as well, or rather future proofs them in this direction.
Paths in visibilities are restricted to parent modules, so relative paths almost never make sense there, and `pub(in a)` needs to be rewritten as `pub(in crate::a)` in the uniform scheme, de-facto cementing the discouraged status of non-`pub(crate)` and non-`pub(super)` fine-grained visibilities.
This is clearly a breaking change for 2018 edition as well, but also a minor one.

The in-scope resolution strategy for import paths mirrors what is currently done for macro paths on stable (on both editions), so it will continue working even if the "ambiguity always means error" restriction is relaxed in the future.

This PR also significantly improves diagnostics for all kinds of resolution ambiguities, from the newly introduced import ones to pretty old "glob vs glob" conflicts. (That's probably what I've spent most of the time on.)

Why beta:
- This is a breaking change on 2018 edition.
- This is a large PR, it's less risky to forward-port it to nightly, than back-port to beta.

cc #55618
cc #53130
cc rust-lang/rfcs#1289
Closes #18084
Closes #54525
Fixes #54390
Fixes #55668

r? @ghost

bors added a commit that referenced this issue Nov 18, 2018

Auto merge of #55884 - petrochenkov:uni, r=eddyb
[beta] resolve: Implement uniform paths 2.0

With this PR paths in imports on 2018 edition are resolved as relative in the scope in which they are written, similarly to any other paths.
The previous implementation worked differently - it tried to resolve the import in the current module (`self::import`) and in the "crate universe" (`::import`), and then merge these two resolutions if possible.

The difference is that the new scheme can refer to strictly larger set of names in scope - names from unnamed blocks, names from all kinds of preludes, including macros imported with `#[macro_use] extern crate`, built-in types/macros, macros introduced with `macro_rules`.
This means strictly more potential ambiguities and therefore ambiguity errors, since we keep the rule that any two different candidate names in scope conflict with each other during import resolution. So this is a breaking change for 2018 edition, but it should be relatively minor.

All paths that don't start with an extern crate are also gated with the `uniform_paths` feature, paths that refer to extern crates are not gated (so we effectively get something like "future-proofed anchored paths" on stable).

Another difference is treatment of paths in visibilities (`pub(in path)`). Few people remember about paths in visibilities, so before this PR they still used the old 2015 rules even on 2018 edition. Namely, paths in visibilities were crate-relative, analogous to 2015 edition imports.
This PR resolves paths in visibilities as uniform as well, or rather future proofs them in this direction.
Paths in visibilities are restricted to parent modules, so relative paths almost never make sense there, and `pub(in a)` needs to be rewritten as `pub(in crate::a)` in the uniform scheme, de-facto cementing the discouraged status of non-`pub(crate)` and non-`pub(super)` fine-grained visibilities.
This is clearly a breaking change for 2018 edition as well, but also a minor one.

The in-scope resolution strategy for import paths mirrors what is currently done for macro paths on stable (on both editions), so it will continue working even if the "ambiguity always means error" restriction is relaxed in the future.

This PR also significantly improves diagnostics for all kinds of resolution ambiguities, from the newly introduced import ones to pretty old "glob vs glob" conflicts. (That's probably what I've spent most of the time on.)

Why beta:
- This is a breaking change on 2018 edition.
- This is a large PR, it's less risky to forward-port it to nightly, than back-port to beta.

cc #55618
cc #53130
cc rust-lang/rfcs#1289
Closes #18084
Closes #54525
Fixes #54390
Fixes #55668

r? @ghost
@ErichDonGubler

This comment has been minimized.

ErichDonGubler commented Nov 21, 2018

I see that @steveklabnik actually linked to my comment from Reddit! Woot! Since I haven't seen much response to that, I'll inline the points from that link here in hopes that they can be addressed.


I'm one of the people firmly in the camp of the anchored paths variant. "Why?", you might ask. Well:

  1. For me, personally, I find it much easier to remember that I ALWAYS need an "anchor" to my path to indicate where my root starts from. It's easier to teach about paths if they're consistent everywhere.
  2. I think paying the price of a few extra characters is worth the disambiguation -- that's because I'm very ADHD and am sensitive to cognitive load that contextually-sensitive stuff like this might require to "resolve".
  3. In line with point 2, uniform paths introduce retain potential breakages when refactoring code in the (rare) case I decide to name submodules the same thing as a crate dependency. That's very surprising, and I don't think it jives with Rust's otherwise amazing flow with refactoring things that's only been helped by this module revamp.
  4. I understand the people that think uniform paths might be "nicer", and I won't gripe if we end up using uniforms paths because it ain't going to stop me from using Rust. That said, to me Rust's philosophy has always been "eat yer veggies, son", and anchored paths only seem consistent with that philosophy. I recognize that I might be suffering from a bias, now that the pendulum is swinging the other way for modules after a long time with us getting familiar with the downsides of the current (amazing) system.

I'm sure that my points aren't necessarily original, and that rebuttals may already exist for them. I'd love to hear them! :)

@withoutboats

This comment has been minimized.

Contributor

withoutboats commented Nov 21, 2018

@ErichDonGubler I responded to your original comment here. Your comment really pulls out the contradiction that I don't understand in the argument in favor of anchored paths (referenced here, the most recent comment before yours):

For me, personally, I find it much easier to remember that I ALWAYS need an "anchor" to my path to indicate where my root starts from. It's easier to teach about paths if they're consistent everywhere.

The whole argument for uniform paths is that they're consistent everywhere - that's why its called uniform paths! Specifically, anchored paths are not consistent between the behavior of paths outside of use statements and in use statements.

My impression from your post is that you didn't read the thread. I know that its a big time investment, but in future please at least try to do that instead of just making a post that may be repetitious of previous comments; its about respecting everyone else's time.

@ErichDonGubler

This comment has been minimized.

ErichDonGubler commented Nov 21, 2018

@withoutboats:

My impression from your post is that you didn't read the thread.

You're right. That embarassing, especially given that your response to Steve's comment is the second message after it. I apologize -- please let me try to fix that mistake! To your responses:

The whole argument for uniform paths is that they're consistent everywhere - that's why its called uniform paths! Specifically, anchored paths are not consistent between the behavior of paths outside of use statements and in use statements.

As you've indicated, I think the vast majority of my misunderstanding comes from the fact that I didn't even think that uniform path handling is what will be used to handle non-use paths anyway -- independent of the variant we choose.

Erich thinking out loud. Feel free to ignore this.

So, if I understand right, these snippets are correct:

// src/random_module.rs
// This is how I normally use Rust 2015 when it comes to importing things.

// List everything that comes from other modules/deps as a "`use` manifest".
// 
// The whitespace for the `use`s below isn't normal Rust style -- I actually prefer to use
// this style, but I'm only using it here for demonstrative purposes.
//
// None of this `use` statement would really change with Rust 2018, IIUC.
use {
    self::sentience::think_deeply,
    super::{
        try_frobicate,
        FrobnicateError,
    },
    std::fmt::{
        Debug,
        Formatter,
        Result as FmtResult,
    },
};

pub(crate) mod sentience {
    // Another "`use` manifest", as it were.
    // Note that `shallow_thoughts` could be better disambiguated in Rust 2018 with:
    // use crate::shallow_thoughts;
    use {    
        ::shallow_thoughts,
        std::mem::transmute,
    };

    pub(crate) struct DeepThoughts { /* ... */ }

    pub(crate) fn think_deeply() -> DeepThoughts { // No need to use `self::...` here
        let mut stuff = shallow_thoughts();
        // ...
        let stuff_bytes: [u8; 4] = unsafe {
            transmute(stuff);
        };
        // ...
    }
}

pub enum ProcessError {
    FrobnicateFailed(FrobnicateError),
}

impl Debug for ProcessError {
    fn fmt(&self, f: &mut Formatter) -> FmtResult {
        // ...
    }
}

fn process() -> Result<(), ProcessError> {
    use self::ProcessError::*; // Needs `self::...` because this is a `use` statement
    let mut deep_thoughts = think_deeply();
    try_frobnicate(deep_thoughts).map_err(FrobnicateFailed)?;
    Ok(())
}

If you look at the code I wrote above, you might note that the flow I prefer totally avoids non-use paths, which I know plenty of other people DO use. This would definitely explain how my point-of-view differs! I hadn't considered this before, and now that I have I think I understand much better your blog post and...ergh, agree with it. I'd prefer to be consistent everywhere. I consider that more valuable than removing the necessity for contextual information in the case of use statements.

...dang it, did I just become convinced that uniform paths are the way to go? 😮


a claim about Rust's philosophy I don't agree with

This is off-topic, so feel free to reply somewhere else, but I'm genuinely interested in how your vision of Rust differs from how I expressed it. :) I understand that your time might be better spent elsewhere, but I'm curious!

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Dec 1, 2018

Some follow-up issues:

  • #56417 Stabilize uniform paths on Rust 2018 (technical details)
  • #56414 Ambiguity errors in 2018 uniform import paths are not technically necessary
  • #56413 Known deviations of macro paths and 2018 import paths from the "uniform path" model

@petrochenkov petrochenkov removed their assignment Dec 2, 2018

bors bot added a commit to rust-analyzer/rust-analyzer that referenced this issue Dec 5, 2018

Merge #253 #256
253: Fix diagnostic fixes showing up everywhere r=matklad a=flodiebold

The LSP code action request always returned the fixes for all diagnostics anywhere in the file, because of a shadowed variable.


There's no test yet; I wasn't sure where to add it. I tried adding one in `heavy_tests`, but that's a bit uncomfortable because the code action response contains the (random) file paths. I could make it work, but wanted to ask beforehand what you think.

256: Improve/add use_item documentation r=matklad a=DJMcNab

Adds some documentation to use_item explaining all code paths (use imports are hard, especially with the ongoing discussion of anchored v. uniform paths - see rust-lang/rust#55618 for what appears to be the latest developments)

Co-authored-by: Florian Diebold <flodiebold@gmail.com>
Co-authored-by: DJMcNab <36049421+djmcnab@users.noreply.github.com>

bors bot added a commit to rust-analyzer/rust-analyzer that referenced this issue Dec 6, 2018

Merge #256
256: Improve/add use_item documentation r=matklad a=DJMcNab

Adds some documentation to use_item explaining all code paths (use imports are hard, especially with the ongoing discussion of anchored v. uniform paths - see rust-lang/rust#55618 for what appears to be the latest developments)

Co-authored-by: DJMcNab <36049421+djmcnab@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment