rust-lang / rust

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

(Modules) Tracking issue for Picking a Module Path System variant #53130

Closed Centril closed 5 years ago

Centril commented 6 years ago

This is a sub-tracking issue for the RFC "Clarify and streamline paths and visibility" (rust-lang/rfcs#2126) as well as the internals post Relative Paths in Rust 2018.

The issue deals with revamping paths and making a decision about whether we should pick:

Unresolved questions:

norcalli commented 6 years ago

The new uniform paths seemed appealing, but when I tried to write something that I thought should "just work," it didn't.

// #![feature(uniform_paths)] // Fails with error "can refer to self::rgb"
pub use rgb;
use rgb::*;

mod k { pub use rgb; }

I'm trying to re-export a crate and then use that crate myself. The error message does correctly suggest doing the following:

#![feature(uniform_paths)]
pub use ::rgb;
use ::rgb::*;

mod k { pub use ::rgb; }

The first scenario without uniform_paths makes more sense to me, but I suppose it's a small thing.

joshtriplett commented 6 years ago

@norcalli Ah, I see. That does seem unfortunate.

@eddyb I think that pub use some_crate; should probably work, by just exporting the crate and not duplicating the existing name from the extern_prelude. Would that be possible?

As for the ambiguity in use rgb::*, that's a bit harder, but I wonder if we could somehow realize that it's the same rgb in both cases?

sanmai-NL commented 6 years ago

@norcalli: Exactly, this is how you find out that implicitness does not pay off in the long term.

petrochenkov commented 6 years ago

@norcalli @joshtriplett This is a limitation of the current implementation, not a fundamental issue with uniform paths. @sanmai-NL I don't see how implicitness is relevant to that example.

sanmai-NL commented 6 years ago

@petrochenkov: A path that starts with :: to indicate the next segment refers to an external crate is more explicit than one without ::. The latter case, the less explicit one, is not supported now. You can indeed go ahead and improve the implementation. And we can keep improving uniform_paths until all corner cases are covered. Or we could, hypothetically, stick with the plain and simple anchored_use_paths and spend Rust development time in more productive ways.

mark-i-m commented 6 years ago

Personally, I tend to prefer the anchored paths variant. I admittedly haven't had the opportunity to use either on a major project yet, but from the smaller projects I have played around with, I find that both are major ergonomics improvements over the current system.

However, I haven't actually run into a use case where the total uniformity of uniform paths actually helped me; I haven't needed it. In other words, on the smaller projects I have worked on, anchored use paths are uniform enough for my use cases.

At the same, I find there are a couple of aspects of uniform paths that bother me a bit:

Of course, it is possible that such ambiguities don't show up much in practice and I am just overreacting, but overall, my leaning is towards anchored use paths.

joshtriplett commented 6 years ago

@mark-i-m To clarify something, :: still exists in the anchored variant. And absolute paths are not ambiguous.

You can always write absolute paths if you want to.

sanmai-NL commented 6 years ago

Can vs. must. One way vs. multiple ways. It’s a significant philosophical difference.

mark-i-m commented 6 years ago

@joshtriplett sorry, I was unclear. What I mean is that it will be much less necessary to write :: prefixes under anchored paths, and users don't need to be aware of the crate root. It can just be an implementation detail.

To me this allows pathes that start with a name to always be "absolute" in use paths, in the sense that they are not relative to the current module. The same is not true of uniform paths because they may disambiguate to a local module if there is a name conflict, right?

Centril commented 6 years ago

What I mean is that it will be much less necessary to write :: prefixes under anchored paths, and users don't need to be aware of the crate root.

I personally don't think this is likely; It seems to me that conflicts with module names and crate names is rather unlikely and that fixing those problems where they occur will also be quite easy. To me, the prospect of not having to litter code with self:: seems much more appealing on balance and something that will give larger dividends in practice.

lnicola commented 6 years ago

To me, the prospect of not having to litter code with self:: seems much more appealing on balance and something that will give larger dividends in practice.

Agreed. When porting some stuff, I found anchored_use_paths to be a chore with all those self:: prefixes, up to the point where the 2015 edition seemed nicer. I'm on the side of "make the easy things easy, and the hard ones possible". Imagine seeing some Rust code for the first time where a quarter of what's on the first screenfull is a ton of self::.

Having paths implicitly start with self is natural -- you don't see people write "type .\foo.txt".

petrochenkov commented 6 years ago

I'm interested in fully implementing and testing uniform_paths because it gives some fundamentally new abilities compared to both current system and anchored paths, like https://github.com/rust-lang/rust/issues/18084 / https://github.com/rust-lang/rfcs/issues/959. (This doesn't work in the current implementation yet.)

mark-i-m commented 6 years ago

To me, the prospect of not having to litter code with self:: seems much more appealing on balance and something that will give larger dividends in practice.

Fair enough. I think we just disagree. I would rather see self:: than ::, though I'm not fond of either...

Centril commented 6 years ago

@mark-i-m but do you agree that having to write one (self::) is more likely than writing ::?

lnicola commented 6 years ago

Fair enough. I think we just disagree. I would rather see self:: than ::, though I'm not fond of either...

Well, I didn't really have to use ::.

joshtriplett commented 6 years ago

On September 5, 2018 8:24:31 AM PDT, Mazdak Farrokhzad notifications@github.com wrote:

@mark-i-m but do you agree that having to write one (self::) is more likely than writing ::?

I think this is a fundamental point worth emphasizing.

self:: both comes up more often and is harder to explain the need for, since it feels like it should be the default (and since in the absolute_use_paths variant use paths and expressions have different rules).

:: should come up much less often.

So far, the majority of issues I've seen have been little implementation quirks of the disambiguation approach, such as use somecrate; (now handled) or pub use somecrate (needs handling) or use somecrate::* (needs handling to not generate a conflict on somecrate itself).

rpjohnst commented 6 years ago

The fact that we still have so many implementation quirks suggests to me that we should be conservative and stick with (a forward-compatible variant of?) anchored paths for now, which already brings most of the wins.

aturon commented 6 years ago

EDIT: see this important update to the proposal.

We're getting close to the cut-off point for stabilizations for the Edition, and of course this feature is one of the most important ones remaining to get nailed down.

Based on the commentary in this tracking issue, the previous tracking issue, and various forum posts, I think it's fair to say that the strong majority of people who have tried any variant of 2018 modules prefer it to 2015 modules. And rustfix migration seems to be working well to limit the amount of manual churn required. So from a high level, I think we're in good shape to stabilize some variant for Rust 2018.

However, there's less agreement on which variant to prefer:

The "uniform paths" variant makes paths work more consistently, and eliminates a common stumbling block around forgetting self:: in use statements. However:

Given the limited time we have, I want to propose that we take a conservative route. We would ship the anchored paths variant, but with future-proofing that would make it possible to move to uniform paths later. The future proofing is simple: if foo is both an external crate name and a local item name, then a use statement must either say use ::foo or use self::foo, just as it would in the uniform paths variant.

With this conservative approach, we can undergo the major path migration as part of Rust 2018, and reap much of the benefit. Then we can take our time to determine whether to eliminate the need for self:: after we've collectively gotten more comfortable with 2018-style paths.

The main downside, of course, is that churn is potentially split over multiple steps, rather than done all at once. However, the churn of removing no-longer-needed self:: is different from the migration churn; it's purely an idiom adjustment. We already plan to grow the set of "idiom lints" over time, and use rustfix to perform them, so this is probably not a huge issue.

In the interest of getting to a decision, I'm going to formally propose we take this conservative route and stabilize the feature:

@rfcbot fcp merge

rfcbot commented 6 years ago

Team member @aturon 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.

strega-nil commented 6 years ago

I personally think that anchored paths are fantastic, and although uniform paths might be a good extension, anchored paths, as the conservative choice, are good to stabilize now.

nikomatsakis commented 6 years ago

I think a conservative path makes sense. I think @aturon you more or less summarized my own feelings of indecision pretty well.

Centril commented 6 years ago

I would personally be excited to move towards uniform_paths as I think self:: is a stumbling block for beginners and proficient rustaceans alike, and that it results in poor ergonomics; but for now the move to a forward compatible anchored-paths solution is a good first step.

QuietMisdreavus commented 6 years ago

One situation i've avoided thinking about so far is how this interacts with the intra-doc links feature in rustdoc. Since it picks apart the path itself, any change to how use statements process paths will also need to be carried over to rustdoc. Presumably we'll need to keep the current behavior for the 2015 edition as well.

joshtriplett commented 6 years ago

@rfcbot concern forward-compatibility

While I understand the desire to go with the more cautious choice at this phase, I'm not sure that "anchored use paths with forward compatibility" is a cautious choice. In particular, of the few concerns I've seen expressed about uniform_paths, the biggest ones have been the cases in which it raises ambiguity errors in cases where they feel it shouldn't. Effectively, adding "forward compatibility' ambiguity errors to anchored use paths gives it the primary remaining pain point (the "implementation quirks") of uniform paths, without the corresponding benefit, and I think making the result more usable will require exactly the same fixes that would polish the last nits of uniform paths.

For instance, we've already made use somecrate; emit a much more straightforward error (effectively "you can just remove this"). We need to make pub use somecrate; work for a somecrate that's already in scope due to extern_prelude, and make use somecrate::*; not issue an error about somecrate itself already being in scope (when self::somecrate refers to exactly the same thing as ::somecrate).

I'd be particularly interested in hearing @eddyb's thoughts comparing the remaining work needed for uniform paths to the hypothetical (not yet written) code for forward-compatibility of anchored use paths. It seems to me that, aside from implementing the latter, we'd still need the same work to make both more usable in cases like pub use somecrate; and use somecrate::*;.

By way of clarification: I'm absolutely open to making the cautious choice at this point; please don't take this as an argument against doing so at all. I'm specifically suggesting that I don't think the proposed path forward gives us that.

withoutboats commented 6 years ago

Effectively, adding "forward compatibility' ambiguity errors to anchored use paths gives it the primary remaining pain point (the "implementation quirks") of uniform paths, without the corresponding benefit, and I think making the result more usable will require exactly the same fixes that would polish the last nits of uniform paths.

But we can move in either direction with @aturon's proposal: either we can eliminate self:: or we can eliminate the ambiguity errors. In general, the forward compatible choice between two incompatible alternatives usually gives you some of the downsides of both, but only temporarily.

joshtriplett commented 6 years ago

@withoutboats I'm not arguing against that. And to be clear, I'm not trying to use advance one or the other alternative here.

I think part of my concern here is that if we're considering this route, I'd want to see some additional detail on what precise ambiguity errors would be raised. And I'm concerned that this proposal would involve doing some additional work that isn't done (or started) yet, at a point that feels rather last-minute, on a piece of code that has proven quite subtle.

mark-i-m commented 6 years ago

Effectively, adding "forward compatibility' ambiguity errors to anchored use paths gives it the primary remaining pain point (the "implementation quirks") of uniform paths, without the corresponding benefit, and I think making the result more usable will require exactly the same fixes that would polish the last nits of uniform paths.

This was my initial reaction too. However, after thinking a bit, my stance has changed: with uniform paths, the "implementation quirks" show up as errors that "there is no name foo in bar". OTOH, with the forward-compat errors, one would get an error that "bar is ambiguous and you should use ::bar or self::bar". The second is vastly better IMHO.

joshtriplett commented 6 years ago

@mark-i-m Can you clarify the cases you're thinking of here, please? What issue are you referring to?

Redrield commented 6 years ago

Very little of my Rust code, sprinkled between crates and toy endeavours, uses self in use. It only happens when it's necessary, and I can't compile with out. The experience shifting to the new module system would be a lot more seamless if I don't need to run rustfix to make my code valid for every file that I work with. That is without addressing the ergonomics issues around self pointed out above. Even with the ambiguity that can come with it, I think that uniform would be the correct choice.

aturon commented 6 years ago

@joshtriplett Mm, that's an excellent point, and I admit that I don't have a firm grasp on the remaining problem cases here. It'd be really helpful to gather them in this tracking issue in the issue description.

To be clear, though, addressing these remaining cases is necessary for either the conservative approach or stabilizing uniform paths. I don't see this issue as a deciding factor on which approach we take -- and notably, there are multiple other concerns that people have with uniform paths in general, which I tried to outline in my summary comment. Note also the feedback on the users thread, which is largely in favor of anchored paths.

mark-i-m commented 6 years ago

@mark-i-m Can you clarify the cases you're thinking of here, please? What issue are you referring to?

@joshtriplett Sure. Consider a crate foo, which conflicts with module self::foo.

Under uniform paths, using a foo::bar path may result in a compile error due to:

Under the forward-compatibility plan, one would instead get an error telling you to disambiguate your paths in all of these cases. This seems like better UX to me. Or am I misunderstanding?

joshtriplett commented 6 years ago

@mark-i-m No, that's definitely not correct! With uniform_paths, if you use foo::bar, and both the crate ::foo and the local module self::foo exist, you should always get an error saying that foo is ambiguous and could refer to either the crate ::foo or the local module self::foo. Those ambiguity warnings are already present in uniform_paths.

...and now I'm wondering if other people have the same concern.

aturon commented 6 years ago

Given the concerns @joshtriplett raised -- the most important of which is that the future-proofing is not currently implemented and may take some time to land -- I want to revise the proposal slightly:

The main downside here is that you will be able to write code that compiles on the initial beta, but not in the final edition. I'm not sure there's a reasonable alternative.

I'm going to update the previous summary comment to point to this one as well; @joshtriplett I believe this tweaked proposal addresses your concerns, and I will confirm that those who've already reviewed are OK with it.

joshtriplett commented 6 years ago

@aturon Sounds good to me, thank you!

@rfcbot resolved forward-compatibility @rfcbot reviewed

aturon commented 6 years ago

Oh, I should further note that the "stable-as-is" anchored paths will only ever exist on the beta channel; we plan on removing the ability to opt in to the new Edition before it hits stable, for our extended beta.

mark-i-m commented 6 years ago

@aturon I didn't realize anyone actually uses the beta channel. I only ever used stable or nightly (if I need to do bare-metal stuff). Do you think it will get the exposure we need there?


@joshtriplett Thanks for the correction! That certainly assuages my UX concerns. I still feel that I lean towards anchored paths for the same reasons mentioned before, though (in https://github.com/rust-lang/rust/issues/53130#issuecomment-418430367).

Also, I realize that self::foo would be more common than ::foo, but I prefer self:: to ::foo personally. Apart from syntactic preferences, self:: feels more consistent with all the other ways a path could start and simplifies the mental model IMHO.

aturon commented 6 years ago

@mark-i-m The extended beta plan is here.

mikeyhew commented 6 years ago

I like the idea of making the conservative choice here. I think that even with the anchored version, if there is both a crate named foo and a module named foo, then use foo::bar is a potential source of bugs because someone might think they're using the module's bar when they're actually using the crate's bar.

Centril commented 6 years ago

@aturon

For the upcoming release candidate 1, which will serve as a beta for the edition, we stabilize anchored paths as-is, [...]

Does this mean that on RC1, a user will be able to write foo::bar even tho there is a local module foo? If so, I'm slightly uncomfortable with the inertia given to keep anchored_paths.

sanmai-NL commented 6 years ago

@mikeyhew: I would personally not so much consider it a source of bugs, since two modules are unlikely to be API compatible in the first place, so a compilation error will come up. But it is a waste of developer time and limits the ability for novices to get up to speed when they are confused by such things.

eddyb commented 6 years ago

FWIW the uniform_paths canaries (which are used to detect and report ambiguities as errors) are not tied to uniform_paths name resolution, and we could have them enabled with anchored_paths.

Do we want to? If we reach some decision here, let me know to make a quick PR.

EDIT: PR is up at #54011.

aturon commented 6 years ago

@eddyb That definitely seems worth trying! Especially if we can also resolve the issue with pub use some_extern_crate leading to ambiguities -- basically wanting to get resolution to notice if the local item and the external crate are the same thing and not error out.

eddyb commented 6 years ago

I just opened #54005 for allowing use crate_name; with uniform_paths, assuming we want that.

rfcbot commented 6 years ago

:bell: This is now entering its final comment period, as per the review above. :bell:

I60R commented 6 years ago

Can we disambiguate the uniform_paths by using a three colons instead of two when referring to a crate? Can we also fix the ugly leading :: by using crate::: instead?

The result could be the best synthesis of uniform and anchored variants possible that takes the best parts from them and eliminates the worst.

Example:

use futures:::Future;

fn my_poll() -> futures:::Poll { ... }

mod foo {
    pub struct Bar;

    fn func() {
        println!("foo::func is called");
        super::func();     // Calls a function from top level
    }

    pub mod bar {
        fn func() {
            println!("bar::func is called");
            super::func(); // Calls a function from previous module
        }

        crate fn summary() {
            func();        // Calls a function from current module
            super::func(); // Calls a function from previous module
            crate::func(); // Calls a function from top level
            outer:::func();// Calls a function from external crate 
        }
    }
}

use foo::Bar;

enum SomeEnum {
    V1(usize),
    V2(String),
}

pub fn func() {
    println!("crate::func is called");
    let five = std:::sync::Arc::new(5);
    use SomeEnum::*;
    match ... {
        V1(i) => { ... }
        V2(s) => { ... }
    }
}

Advantages over RFC uniform_paths:

Advantages over RFC anchored_use_paths:

Redrield commented 6 years ago

@I60R If anchored is going to be used, I think it should be used as-is. Personally I find that code even harder to read, and it would be very strange to have to get used to disambiguating crates with :::

joshtriplett commented 6 years ago

Can we disambiguate the uniform_paths by using a three colons instead of two when referring to a crate?

That's a really subtle distinction, and I don't think it buys us much for the cost and subtlety.

Can we also fix the ugly leading :: by using crate::: instead?

I don't know why people fixate on a syntax that, in the ideal case, you'll almost never have to write. The only time you will ever need to write an explicit leading :: is if you have a local item with the same name as a crate.

cramertj commented 6 years ago

Some data: I just got done updating Fuchsia to the latest nightly, and I hit a bunch of errors as a result of the change to be future-compatible with uniform_paths. Most of the errors came from one specific import, use log::log; (or some variant thereof).

The errors looked something like this:

error: `log` import is ambiguous
   --> /usr/local/google/home/cramertj/src/fuchsia/garnet/bin/recovery_netstack/core/src/wire/mod.rs:31:17
    |
31  |               use log::{debug, log};
    |                   ^^^          --- shadowed by block-scoped `log`
    | 
   ::: /usr/local/google/home/cramertj/src/fuchsia/garnet/bin/recovery_netstack/core/src/wire/ipv6.rs:11:18
    |
11  |   use log::{debug, log};
    |                    --- may refer to `self::log` in the future
...
115 |               return debug_err!(
    |  ____________________-
116 | |                 Err(ParseError::Format),
117 | |                 "unexpected IP version: {}",
118 | |                 packet.fixed_hdr.version()
119 | |             );
    | |_____________- in this macro invocation
    |
    = help: write `self::log` explicitly instead
    = note: in the future, `#![feature(uniform_paths)]` may become the default

The code that caused the error is available here.

The issue was fixed by adding use ::log;. We should probably suggest using ::log or self::log in the error message, not just self::log (which would have been incorrect in this case).

I had a similar issue importing use fdio::fdio_sys::*; because fdio_sys included a struct named fdio.

joshtriplett commented 6 years ago

@cramertj I think that you can just use log 0.4.4 or newer, and then stop importing log::log at all, at least when you don't call it directly. From a quick look at the code you linked to, I don't see any calls to log!.

eddyb commented 6 years ago

@cramertj It's not suggesting ::log because you have an ambiguity in the macro namespace, not the type/module namespace.

You hit the "nested scopes" problem, where self::foo currently only looks in the same module, but use foo::...;, under uniform_paths, in the future, might end up finding foo in a scope between the current module and the import.

However, you happen to be importing the same entity, so I think we should just silence the error in that case, like we did for imports of an external crate.

EDIT: PR up at #54201.