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

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

Open
Centril opened this Issue Aug 6, 2018 · 101 comments

Comments

@Centril
Contributor

Centril commented Aug 6, 2018

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:

  • Implement all the things.
    • uniform_paths, #52923
    • anchored_use_paths
  • Make both variants available to test in Edition Preview 2 with appropriate documentation on how to test.
  • Adjust documentation (see instructions on forge)
  • Pick a variant
  • Stabilization PR (see instructions on forge)

Unresolved questions:

  • Which variant do we pick?

@Centril Centril added this to the Rust 2018 RC milestone Aug 6, 2018

@Centril Centril changed the title from (Modules) Tracking issue for Picking one of the two proposed Module Path Systems to (Modules) Tracking issue for Picking one of the two Module Path Systems Aug 6, 2018

@Centril Centril changed the title from (Modules) Tracking issue for Picking one of the two Module Path Systems to (Modules) Tracking issue for Picking a Module Path System variant Aug 6, 2018

@sanmai-NL

This comment has been minimized.

Show comment
Hide comment
@sanmai-NL

sanmai-NL Aug 7, 2018

Could you give more details about what has been implemented concretely, and what renaming operations are needed exactly? It comes down to: how can I start testing anything right now?

A priori, I vote for anchored_use_paths, for being the most explicit/teachable. One suggestion: I find the crate keyword a bit odd since it does not seem related to visibility at all. I suppose this has been debated before but it would be better if it becomes pubcrate or whatever.

sanmai-NL commented Aug 7, 2018

Could you give more details about what has been implemented concretely, and what renaming operations are needed exactly? It comes down to: how can I start testing anything right now?

A priori, I vote for anchored_use_paths, for being the most explicit/teachable. One suggestion: I find the crate keyword a bit odd since it does not seem related to visibility at all. I suppose this has been debated before but it would be better if it becomes pubcrate or whatever.

@Centril

This comment has been minimized.

Show comment
Hide comment
@Centril

Centril Aug 8, 2018

Contributor

cc @eddyb re. @sanmai-NL's question.

Contributor

Centril commented Aug 8, 2018

cc @eddyb re. @sanmai-NL's question.

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

Auto merge of #52923 - eddyb:relative-imports, r=petrochenkov
#[feature(uniform_paths)]: allow `use x::y;` to resolve through `self::x`, not just `::x`.

_Branch originally by @cramertj, based on @petrochenkov's [description on the internals forum](https://internals.rust-lang.org/t/relative-paths-in-rust-2018/7883/30?u=petrochenkov)._
_(note, however, that the approach has significantly changed since)_

Implements `#[feature(uniform_paths)]` from #53130, by treating unqualified `use` paths as maybe-relative. That is, `use x::y;`, where `x` is a plain identifier (not a keyword), is no longer the same as `use ::y;`, and before picking an external crate named `x`, it first looks for an item named `x` in the same module (i.e. `self::x`) and prefers that local item instead.

Such a "maybe-relative" `x` can only resolve to an external crate if it's listed in "`extern_prelude`" (i.e. `core` / `std` and all the crates passed to `--extern`; the latter includes Cargo dependencies) - this is the same condition as being able to refer to the external crate from an unqualified, non-`use` path.
All other crates must be explicitly imported with an absolute path, e.g. `use ::y;`

To detect an ambiguity between the external crate and the local item with the same name, a "canary" import (e.g. `use self::x as _;`), tagged with the `is_uniform_paths_canary` flag, is injected. As the initial implementation is not sophisticated enough to handle all possible ways in which `self::x` could appear (e.g. from macro expansion), this also guards against accidentally picking the external crate, when it might actually get "shadowed" later.
Also, more canaries are injected for each block scope around the `use`, as `self::x` cannot resolve to any items named `x` in those scopes, but non-`use` paths can, and that could be confusing or even backwards-incompatible.

Errors are emitted only if the main "canary" import succeeds while an external crate exists (or if any of the block-scoped ones succeed at all), and ambiguities have custom error reporting, e.g.:
```rust
#![feature(uniform_paths)]
pub mod foo {
    use std::io;
    pub mod std { pub mod io {} }
}
```
```rust
error: import from `std` is ambiguous
 --> test.rs:3:9
  |
3 |     use std::io;
  |         ^^^ could refer to external crate `::std`
4 |     pub mod std { pub mod io {} }
  |     ----------------------------- could also refer to `self::std`
  |
  = help: write `::std` or `self::std` explicitly instead
  = note: relative `use` paths enabled by `#![feature(uniform_paths)]`
```
Another example, this time with a block-scoped item shadowing a module-scoped one:
```rust
#![feature(uniform_paths)]
enum Foo { A, B }
fn main() {
    enum Foo {}
    use Foo::*;
}
```
```rust
error: import from `Foo` is ambiguous
 --> test.rs:5:9
  |
4 |     enum Foo {}
  |     ----------- shadowed by block-scoped `Foo`
5 |     use Foo::*;
  |         ^^^
  |
  = help: write `::Foo` or `self::Foo` explicitly instead
  = note: relative `use` paths enabled by `#![feature(uniform_paths)]`
```

Additionally, this PR, because replacing "the `finalize_import` hack" was a blocker:
* fixes #52140
* fixes #52141
* fixes #52705

cc @aturon @joshtriplett

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

Auto merge of #52923 - eddyb:relative-imports, r=petrochenkov
#[feature(uniform_paths)]: allow `use x::y;` to resolve through `self::x`, not just `::x`.

_Branch originally by @cramertj, based on @petrochenkov's [description on the internals forum](https://internals.rust-lang.org/t/relative-paths-in-rust-2018/7883/30?u=petrochenkov)._
_(note, however, that the approach has significantly changed since)_

Implements `#[feature(uniform_paths)]` from #53130, by treating unqualified `use` paths as maybe-relative. That is, `use x::y;`, where `x` is a plain identifier (not a keyword), is no longer the same as `use ::y;`, and before picking an external crate named `x`, it first looks for an item named `x` in the same module (i.e. `self::x`) and prefers that local item instead.

Such a "maybe-relative" `x` can only resolve to an external crate if it's listed in "`extern_prelude`" (i.e. `core` / `std` and all the crates passed to `--extern`; the latter includes Cargo dependencies) - this is the same condition as being able to refer to the external crate from an unqualified, non-`use` path.
All other crates must be explicitly imported with an absolute path, e.g. `use ::y;`

To detect an ambiguity between the external crate and the local item with the same name, a "canary" import (e.g. `use self::x as _;`), tagged with the `is_uniform_paths_canary` flag, is injected. As the initial implementation is not sophisticated enough to handle all possible ways in which `self::x` could appear (e.g. from macro expansion), this also guards against accidentally picking the external crate, when it might actually get "shadowed" later.
Also, more canaries are injected for each block scope around the `use`, as `self::x` cannot resolve to any items named `x` in those scopes, but non-`use` paths can, and that could be confusing or even backwards-incompatible.

Errors are emitted only if the main "canary" import succeeds while an external crate exists (or if any of the block-scoped ones succeed at all), and ambiguities have custom error reporting, e.g.:
```rust
#![feature(uniform_paths)]
pub mod foo {
    use std::io;
    pub mod std { pub mod io {} }
}
```
```rust
error: import from `std` is ambiguous
 --> test.rs:3:9
  |
3 |     use std::io;
  |         ^^^ could refer to external crate `::std`
4 |     pub mod std { pub mod io {} }
  |     ----------------------------- could also refer to `self::std`
  |
  = help: write `::std` or `self::std` explicitly instead
  = note: relative `use` paths enabled by `#![feature(uniform_paths)]`
```
Another example, this time with a block-scoped item shadowing a module-scoped one:
```rust
#![feature(uniform_paths)]
enum Foo { A, B }
fn main() {
    enum Foo {}
    use Foo::*;
}
```
```rust
error: import from `Foo` is ambiguous
 --> test.rs:5:9
  |
4 |     enum Foo {}
  |     ----------- shadowed by block-scoped `Foo`
5 |     use Foo::*;
  |         ^^^
  |
  = help: write `::Foo` or `self::Foo` explicitly instead
  = note: relative `use` paths enabled by `#![feature(uniform_paths)]`
```

Additionally, this PR, because replacing "the `finalize_import` hack" was a blocker:
* fixes #52140
* fixes #52141
* fixes #52705

cc @aturon @joshtriplett
@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Aug 14, 2018

Member

@sanmai-NL Now that #52923 has landed, the next nightly should have both options available:

  • anchored_use_paths has been the Rust 2018 default for some(?) time now
  • uniform_paths you get by being on Rust 2018 and adding #![feature(uniform_paths)]

I'd suggesting trying the latter after upgrading to the edition, and report back if the ambiguity errors end up being too excessive.

AFAIK, the upgrade involves taking a Rust 2015 crate, adding #![feature(rust_2018_preview)] without changing the edition, running rustfix to apply the suggestions it gives for making same-crate paths explicit (i.e. prepending all paths that don't start with a crate name, with crate::), and only then do you switch the edition (usually in Cargo.toml) to Rust 2018.
You can remove #![feature(rust_2018_preview)] afterwards, since it's redundant.

Member

eddyb commented Aug 14, 2018

@sanmai-NL Now that #52923 has landed, the next nightly should have both options available:

  • anchored_use_paths has been the Rust 2018 default for some(?) time now
  • uniform_paths you get by being on Rust 2018 and adding #![feature(uniform_paths)]

I'd suggesting trying the latter after upgrading to the edition, and report back if the ambiguity errors end up being too excessive.

AFAIK, the upgrade involves taking a Rust 2015 crate, adding #![feature(rust_2018_preview)] without changing the edition, running rustfix to apply the suggestions it gives for making same-crate paths explicit (i.e. prepending all paths that don't start with a crate name, with crate::), and only then do you switch the edition (usually in Cargo.toml) to Rust 2018.
You can remove #![feature(rust_2018_preview)] afterwards, since it's redundant.

@joshtriplett

This comment has been minimized.

Show comment
Hide comment
Member

joshtriplett commented Aug 14, 2018

@archer884

This comment has been minimized.

Show comment
Hide comment
@archer884

archer884 Aug 16, 2018

I absolutely despise the "anchored paths variant," and I am very excited to see that we're considering an alternative. Any alternative, honestly. >.>

...That said, uniform paths look great. :) I'd much prefer to let people use absolute paths (the anchored version) if they like, but not force people to do it.

(No idea where the appropriate place to provide feedback for this would be, so I just left it here. Sorry.)

archer884 commented Aug 16, 2018

I absolutely despise the "anchored paths variant," and I am very excited to see that we're considering an alternative. Any alternative, honestly. >.>

...That said, uniform paths look great. :) I'd much prefer to let people use absolute paths (the anchored version) if they like, but not force people to do it.

(No idea where the appropriate place to provide feedback for this would be, so I just left it here. Sorry.)

@kohensu

This comment has been minimized.

Show comment
Hide comment
@kohensu

kohensu Aug 16, 2018

Using uniform_paths, there is an error I find confusing.
Considering:

#![feature(rust_2018_preview)]
#![feature(uniform_paths)]
use hex;
fn main() {
    println!("hex {}", hex::encode("01"));
}

I get this error:

error: import from `hex` is ambiguous
 --> src/main.rs:3:5
  |
3 | use hex;
  |     ^^^
  |     |
  |     could refer to external crate `::hex`
  |     could also refer to `self::hex`
  |
  = help: write `::hex` or `self::hex` explicitly instead
  = note: relative `use` paths enabled by `#![feature(uniform_paths)]`

whereas self::hex doesn't exist in the module.

Because the code is working without use hex maybe suggesting to remove it will be more clear.

kohensu commented Aug 16, 2018

Using uniform_paths, there is an error I find confusing.
Considering:

#![feature(rust_2018_preview)]
#![feature(uniform_paths)]
use hex;
fn main() {
    println!("hex {}", hex::encode("01"));
}

I get this error:

error: import from `hex` is ambiguous
 --> src/main.rs:3:5
  |
3 | use hex;
  |     ^^^
  |     |
  |     could refer to external crate `::hex`
  |     could also refer to `self::hex`
  |
  = help: write `::hex` or `self::hex` explicitly instead
  = note: relative `use` paths enabled by `#![feature(uniform_paths)]`

whereas self::hex doesn't exist in the module.

Because the code is working without use hex maybe suggesting to remove it will be more clear.

@Nemo157

This comment has been minimized.

Show comment
Hide comment
@Nemo157

Nemo157 Aug 16, 2018

Contributor

@kohensu see #53408, I think the plan is to either special case allow that or improve the error message to just suggest removing it.

Contributor

Nemo157 commented Aug 16, 2018

@kohensu see #53408, I think the plan is to either special case allow that or improve the error message to just suggest removing it.

@kohensu

This comment has been minimized.

Show comment
Hide comment
@kohensu

kohensu Aug 16, 2018

@Nemo157
Sorry, I did not see that.
Thanks for the reference to the issue.

kohensu commented Aug 16, 2018

@Nemo157
Sorry, I did not see that.
Thanks for the reference to the issue.

@sanmai-NL

This comment has been minimized.

Show comment
Hide comment
@sanmai-NL

sanmai-NL Aug 16, 2018

@archer884: What’s more interesting is your motivation to feel that way?

sanmai-NL commented Aug 16, 2018

@archer884: What’s more interesting is your motivation to feel that way?

@Xymist

This comment has been minimized.

Show comment
Hide comment
@Xymist

Xymist Aug 16, 2018

As a user (who perhaps hasn't been paying as much attention as I should) I think the uniform_paths option feels strictly better. Special case sugar for when there's no possible ambiguity would be nice, but by no means necessary; optimising for clarity is definitely a greater win.

Xymist commented Aug 16, 2018

As a user (who perhaps hasn't been paying as much attention as I should) I think the uniform_paths option feels strictly better. Special case sugar for when there's no possible ambiguity would be nice, but by no means necessary; optimising for clarity is definitely a greater win.

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Aug 16, 2018

Member

Special case sugar for when there's no possible ambiguity would be nice

Not sure what you mean. Does #53427 (by treating each namespace independently) solve that for you?

Member

eddyb commented Aug 16, 2018

Special case sugar for when there's no possible ambiguity would be nice

Not sure what you mean. Does #53427 (by treating each namespace independently) solve that for you?

@archer884

This comment has been minimized.

Show comment
Hide comment
@archer884

archer884 Aug 16, 2018

archer884 commented Aug 16, 2018

@sanmai-NL

This comment has been minimized.

Show comment
Hide comment
@sanmai-NL

sanmai-NL Aug 17, 2018

That’s an argument from authority, and a very aspecific one. Conciseness is a criterion independent from explicitness and also from simplicity.

sanmai-NL commented Aug 17, 2018

That’s an argument from authority, and a very aspecific one. Conciseness is a criterion independent from explicitness and also from simplicity.

@archer884

This comment was marked as off-topic.

Show comment
Hide comment
@archer884

archer884 Aug 17, 2018

@sanmai-NL, it's not. It's just a chance for you to read the thoughts of another person who covers the issue in greater depth. Google is your friend.

archer884 commented Aug 17, 2018

@sanmai-NL, it's not. It's just a chance for you to read the thoughts of another person who covers the issue in greater depth. Google is your friend.

@sanmai-NL

This comment was marked as off-topic.

Show comment
Hide comment
@sanmai-NL

sanmai-NL Aug 17, 2018

Very poor way to argue your case. You seem not to even recall which author you mean, but you ask me to Google for something.

sanmai-NL commented Aug 17, 2018

Very poor way to argue your case. You seem not to even recall which author you mean, but you ask me to Google for something.

@archer884

This comment was marked as off-topic.

Show comment
Hide comment
@archer884

archer884 Aug 17, 2018

archer884 commented Aug 17, 2018

@liigo

This comment was marked as off-topic.

Show comment
Hide comment
@liigo

liigo Aug 18, 2018

Contributor

cc me

Contributor

liigo commented Aug 18, 2018

cc me

@sanmai-NL

This comment was marked as off-topic.

Show comment
Hide comment
@sanmai-NL

sanmai-NL Aug 18, 2018

Sad to see how some people dramatize a technical discussion. I hope only comments that are motivated and consider the earlier discussion are taken seriously. Just chiming in doesn’t bring us any further.

sanmai-NL commented Aug 18, 2018

Sad to see how some people dramatize a technical discussion. I hope only comments that are motivated and consider the earlier discussion are taken seriously. Just chiming in doesn’t bring us any further.

@eddyb

This comment was marked as off-topic.

Show comment
Hide comment
@eddyb

eddyb Aug 18, 2018

Member

@liigo You can also use the "Subscribe" button on the right, without posting a comment.

Member

eddyb commented Aug 18, 2018

@liigo You can also use the "Subscribe" button on the right, without posting a comment.

@luben

This comment has been minimized.

Show comment
Hide comment
@luben

luben Aug 20, 2018

Is there a reason why use core::mem does not work?

  |
1 | use core::mem;
  |     ^^^^ Did you mean `self`?

error: aborting due to previous error

For more information about this error, try `rustc --explain E0432`.
error: Could not compile `uds`.

Aside, I see that #53427 fixed use log::* but I hit the same ambiguity with use simple_logger:

   |
14 | use simple_logger;
   |     ^^^^^^^^^^^^^
   |     |
   |     refers to external crate `::simple_logger`
   |     defines `self::simple_logger`, shadowing itself
   |
   = help: remove or write `::simple_logger` explicitly instead
   = note: relative `use` paths enabled by `#![feature(uniform_paths)]`

Aren't the modules a separate namespace?

luben commented Aug 20, 2018

Is there a reason why use core::mem does not work?

  |
1 | use core::mem;
  |     ^^^^ Did you mean `self`?

error: aborting due to previous error

For more information about this error, try `rustc --explain E0432`.
error: Could not compile `uds`.

Aside, I see that #53427 fixed use log::* but I hit the same ambiguity with use simple_logger:

   |
14 | use simple_logger;
   |     ^^^^^^^^^^^^^
   |     |
   |     refers to external crate `::simple_logger`
   |     defines `self::simple_logger`, shadowing itself
   |
   = help: remove or write `::simple_logger` explicitly instead
   = note: relative `use` paths enabled by `#![feature(uniform_paths)]`

Aren't the modules a separate namespace?

@joshtriplett

This comment has been minimized.

Show comment
Hide comment
@joshtriplett

joshtriplett Aug 20, 2018

Member

@luben For your second comment: just delete the line use simple_logger; entirely. extern_prelude makes that redundant; you can just call simple_logger::init() or similar.

@eddyb Should the special-case version of that message reference extern_prelude instead? Because it's extern_prelude that makes use cratename; redundant.

Member

joshtriplett commented Aug 20, 2018

@luben For your second comment: just delete the line use simple_logger; entirely. extern_prelude makes that redundant; you can just call simple_logger::init() or similar.

@eddyb Should the special-case version of that message reference extern_prelude instead? Because it's extern_prelude that makes use cratename; redundant.

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Aug 20, 2018

Member

@luben You need to use #![no_std] to get core instead of std.
Any particular usecase for accessing core without #![no_std]?

Member

eddyb commented Aug 20, 2018

@luben You need to use #![no_std] to get core instead of std.
Any particular usecase for accessing core without #![no_std]?

@luben

This comment has been minimized.

Show comment
Hide comment
@luben

luben Aug 20, 2018

Thanks for both the responses. Makes sense. Actually I didn't need core::mem as I can use std::mem instead: I have copy/pasted some futures related code that turns to work without uniform_paths.

luben commented Aug 20, 2018

Thanks for both the responses. Makes sense. Actually I didn't need core::mem as I can use std::mem instead: I have copy/pasted some futures related code that turns to work without uniform_paths.

@Nemo157

This comment has been minimized.

Show comment
Hide comment
@Nemo157

Nemo157 Aug 20, 2018

Contributor

Any particular usecase for accessing core without #![no_std]?

@eddyb futures-rs uses the opposite (accessing std with #![no_std]) a lot for supporting an optional std feature, a good example is futures-core::stream, this has all the core supported code first, then near the bottom the allocation using code behind a #[cfg(feature = "std")] gate.

This works in 2015 by having an unconditional #![no_std] along with #[cfg(feature = "std")] extern crate std; to optionally add the crate. It also worked in 2018 anchored_use_paths with an unconditional #[no_std] and no explicit import because libstd was available in the search path (see #53166 (comment)).

I'm not sure how to support this as easily with uniform_paths. Adding #[cfg(feature = "std")] extern crate std; to the root requires updating all the use std::*; imports to use crate::std::*;. Using #![cfg_attr(not(feature = "std"), no_std)] requires switching all imports between use core::*; and use std::*; based on the feature. Out of these two options I think the first is definitely the most reasonable, but that requires extern crate which may be deprecated in 2018, and it's not quite as nice as the old system of having normal looking core/std imports everywhere.

EDIT: While editing this post I managed to drop one of the main points. The solution I'd most be interested in seeing is exactly what @eddyb asked about above, being able to access core even without #![no_std]. That way futures-rs could use #![cfg_attr(not(feature = "std"), no_std)], when the std feature is inactive it would only import via core rooted paths, when std is active it would import via both std and core rooted paths.

Contributor

Nemo157 commented Aug 20, 2018

Any particular usecase for accessing core without #![no_std]?

@eddyb futures-rs uses the opposite (accessing std with #![no_std]) a lot for supporting an optional std feature, a good example is futures-core::stream, this has all the core supported code first, then near the bottom the allocation using code behind a #[cfg(feature = "std")] gate.

This works in 2015 by having an unconditional #![no_std] along with #[cfg(feature = "std")] extern crate std; to optionally add the crate. It also worked in 2018 anchored_use_paths with an unconditional #[no_std] and no explicit import because libstd was available in the search path (see #53166 (comment)).

I'm not sure how to support this as easily with uniform_paths. Adding #[cfg(feature = "std")] extern crate std; to the root requires updating all the use std::*; imports to use crate::std::*;. Using #![cfg_attr(not(feature = "std"), no_std)] requires switching all imports between use core::*; and use std::*; based on the feature. Out of these two options I think the first is definitely the most reasonable, but that requires extern crate which may be deprecated in 2018, and it's not quite as nice as the old system of having normal looking core/std imports everywhere.

EDIT: While editing this post I managed to drop one of the main points. The solution I'd most be interested in seeing is exactly what @eddyb asked about above, being able to access core even without #![no_std]. That way futures-rs could use #![cfg_attr(not(feature = "std"), no_std)], when the std feature is inactive it would only import via core rooted paths, when std is active it would import via both std and core rooted paths.

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Aug 20, 2018

Member

You can just write use ::std::... and it still works with uniform_paths, btw.
That syntax works with ::core::... as well, too, irrespective of #![no_std].
(paths starting with :: explicitly look up a crate and are not limited to the extern_prelude filter)

Member

eddyb commented Aug 20, 2018

You can just write use ::std::... and it still works with uniform_paths, btw.
That syntax works with ::core::... as well, too, irrespective of #![no_std].
(paths starting with :: explicitly look up a crate and are not limited to the extern_prelude filter)

@joshtriplett

This comment has been minimized.

Show comment
Hide comment
@joshtriplett

joshtriplett Aug 21, 2018

Member

@eddyb Is there any particular reason not to have core accessible even without #[no_std]?

Doing so allows more easily writing code that works with or without #[no_std], if you only use functions in core.

Member

joshtriplett commented Aug 21, 2018

@eddyb Is there any particular reason not to have core accessible even without #[no_std]?

Doing so allows more easily writing code that works with or without #[no_std], if you only use functions in core.

@ehuss ehuss referenced this issue Aug 21, 2018

Open

Syntax Highlighting: Unstable Tracking Issue #333

2 of 7 tasks complete
@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Aug 21, 2018

Member

@joshtriplett Not really any that I can think of (maybe modules named core causing ambiguities?).
We might want to keep only one to avoid confusion over which to use, but it also seems useful to have both.
I wonder what @rust-lang/lang and @rust-lang/libs think of allowing core, as well as std, to be used everywhere without #![no_std].

Member

eddyb commented Aug 21, 2018

@joshtriplett Not really any that I can think of (maybe modules named core causing ambiguities?).
We might want to keep only one to avoid confusion over which to use, but it also seems useful to have both.
I wonder what @rust-lang/lang and @rust-lang/libs think of allowing core, as well as std, to be used everywhere without #![no_std].

@withoutboats

This comment has been minimized.

Show comment
Hide comment
@withoutboats

withoutboats Sep 5, 2018

Contributor

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.

Contributor

withoutboats commented Sep 5, 2018

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

This comment has been minimized.

Show comment
Hide comment
@joshtriplett

joshtriplett Sep 5, 2018

Member

@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.

Member

joshtriplett commented Sep 5, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@mark-i-m

mark-i-m Sep 5, 2018

Contributor

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.

Contributor

mark-i-m commented Sep 5, 2018

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

This comment has been minimized.

Show comment
Hide comment
@joshtriplett

joshtriplett Sep 5, 2018

Member

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

Member

joshtriplett commented Sep 5, 2018

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

@Redrield

This comment has been minimized.

Show comment
Hide comment
@Redrield

Redrield Sep 5, 2018

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.

Redrield commented Sep 5, 2018

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

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Sep 5, 2018

Member

@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.

Member

aturon commented Sep 5, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@mark-i-m

mark-i-m Sep 5, 2018

Contributor

@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:

  • bar is not found in foo because we looked in the wrong foo (probably the most common case by far)
  • bar is the wrong type because there happens to be a bar in both, but they have different types
  • Perhaps some other unlucky coincidences

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?

Contributor

mark-i-m commented Sep 5, 2018

@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:

  • bar is not found in foo because we looked in the wrong foo (probably the most common case by far)
  • bar is the wrong type because there happens to be a bar in both, but they have different types
  • Perhaps some other unlucky coincidences

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

This comment has been minimized.

Show comment
Hide comment
@joshtriplett

joshtriplett Sep 5, 2018

Member

@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.

Member

joshtriplett commented Sep 5, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Sep 5, 2018

Member

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:

  • For the upcoming release candidate 1, which will serve as a beta for the edition, we stabilize anchored paths as-is, but also allow you to opt in to uniform paths (on the beta channel) so that we can continue testing it and especially testing the ambiguity code.

  • Over the next release cycle, we work to address remaining "false ambiguities" and to fully future-proof anchored paths for the final Edition release. (Or, potentially, we reach a firm consensus on one or the other path variants and just ship it directly, rather than the conservative version).

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.

Member

aturon commented Sep 5, 2018

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:

  • For the upcoming release candidate 1, which will serve as a beta for the edition, we stabilize anchored paths as-is, but also allow you to opt in to uniform paths (on the beta channel) so that we can continue testing it and especially testing the ambiguity code.

  • Over the next release cycle, we work to address remaining "false ambiguities" and to fully future-proof anchored paths for the final Edition release. (Or, potentially, we reach a firm consensus on one or the other path variants and just ship it directly, rather than the conservative version).

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

This comment has been minimized.

Show comment
Hide comment
@joshtriplett

joshtriplett Sep 5, 2018

Member

@aturon Sounds good to me, thank you!

@rfcbot resolved forward-compatibility
@rfcbot reviewed

Member

joshtriplett commented Sep 5, 2018

@aturon Sounds good to me, thank you!

@rfcbot resolved forward-compatibility
@rfcbot reviewed

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Sep 5, 2018

Member

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.

Member

aturon commented Sep 5, 2018

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

This comment has been minimized.

Show comment
Hide comment
@mark-i-m

mark-i-m Sep 5, 2018

Contributor

@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 #53130 (comment)).

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.

Contributor

mark-i-m commented Sep 5, 2018

@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 #53130 (comment)).

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

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Sep 5, 2018

Member

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

Member

aturon commented Sep 5, 2018

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

@mikeyhew

This comment has been minimized.

Show comment
Hide comment
@mikeyhew

mikeyhew Sep 6, 2018

Contributor

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.

Contributor

mikeyhew commented Sep 6, 2018

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

This comment has been minimized.

Show comment
Hide comment
@Centril

Centril Sep 6, 2018

Contributor

@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.

Contributor

Centril commented Sep 6, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@sanmai-NL

sanmai-NL Sep 6, 2018

@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.

sanmai-NL commented Sep 6, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Sep 6, 2018

Member

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.

Member

eddyb commented Sep 6, 2018

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.

@Centril Centril added the I-nominated label Sep 6, 2018

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Sep 6, 2018

Member

@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.

Member

aturon commented Sep 6, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Sep 6, 2018

Member

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

Member

eddyb commented Sep 6, 2018

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

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Sep 6, 2018

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

rfcbot commented Sep 6, 2018

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

@I60R

This comment has been minimized.

Show comment
Hide comment
@I60R

I60R Sep 13, 2018

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:

  • Cleaner and more explicit syntax, easier to learn and understand
  • No ambiguities between modules and crates
  • Better code highlighting in code editors

Advantages over RFC anchored_use_paths:

  • Not a verbose, hard to read and write syntax
  • More explicit, since it not clashes with self function parameter
  • Less chances to shoot itself in foot by moving relative path to different location
  • No need to develop a habit to type self very often

I60R commented Sep 13, 2018

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:

  • Cleaner and more explicit syntax, easier to learn and understand
  • No ambiguities between modules and crates
  • Better code highlighting in code editors

Advantages over RFC anchored_use_paths:

  • Not a verbose, hard to read and write syntax
  • More explicit, since it not clashes with self function parameter
  • Less chances to shoot itself in foot by moving relative path to different location
  • No need to develop a habit to type self very often
@Redrield

This comment has been minimized.

Show comment
Hide comment
@Redrield

Redrield Sep 13, 2018

@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 :::

Redrield commented Sep 13, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@joshtriplett

joshtriplett Sep 13, 2018

Member

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.

Member

joshtriplett commented Sep 13, 2018

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

This comment has been minimized.

Show comment
Hide comment
@cramertj

cramertj Sep 13, 2018

Member

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.

Member

cramertj commented Sep 13, 2018

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

This comment has been minimized.

Show comment
Hide comment
@joshtriplett

joshtriplett Sep 13, 2018

Member

@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!.

Member

joshtriplett commented Sep 13, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Sep 13, 2018

Member

@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.

Member

eddyb commented Sep 13, 2018

@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.

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

Auto merge of #54201 - eddyb:reflexive-disambiguation, r=petrochenkov
rustc_resolve: don't treat uniform_paths canaries as ambiguities unless they resolve to distinct Def's.

In particular, this allows this pattern that @cramertj mentioned in #53130 (comment):
```rust
use log::{debug, log};
fn main() {
    use log::{debug, log};
    debug!(...);
}
```
The canaries for the inner `use log::...;`, *in the macro namespace*, see the `log` macro imported at the module scope, and the (same) `log` macro, imported in the block scope inside `main`.

Previously, these two possible (macro namspace) `log` resolutions would be considered ambiguous (from a forwards-compat standpoint, where we might make imports aware of block scopes).

With this PR, such a case is allowed *if and only if* all the possible resolutions refer to the same definition (more specifically, because the *same* `log` macro is being imported twice).
This condition subsumes previous (weaker) checks like #54005 and the second commit of #54011.

Only the last commit is the main change, the other two are cleanups.

r? @petrochenkov cc @Centril @joshtriplett
@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Sep 16, 2018

The final comment period, with a disposition to merge, as per the review above, is now complete.

rfcbot commented Sep 16, 2018

The final comment period, with a disposition to merge, as per the review above, is now complete.

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