#[feature(uniform_paths)]: allow `use x::y;` to resolve through `self::x`, not just `::x`. #52923

eddyb · 2018-07-31T22:18:08Z

Branch originally by @cramertj, based on @petrochenkov's description on the internals forum.
(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 ::x::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 ::x::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.:

#![feature(uniform_paths)]
pub mod foo {
    use std::io;
    pub mod std { pub mod io {} }
}

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:

#![feature(uniform_paths)]
enum Foo { A, B }
fn main() {
    enum Foo {}
    use Foo::*;
}

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:

cc @aturon @joshtriplett

rust-highfive · 2018-07-31T22:18:11Z

r? @varkor

(rust_highfive has picked a reviewer for you, use r? to override)

varkor · 2018-08-02T16:46:20Z

r? @petrochenkov

I had a look at the changes: everything looks good to me (apart from needing more tests), but I'm not familiar with this part of the code.

eddyb · 2018-08-02T21:10:07Z

src/librustc_resolve/build_reduced_graph.rs

+            self.session.features_untracked().relative_imports &&
+            if let Some(second_segment) = module_path.get(1) {
+                module_path[0].name == keywords::CrateRoot.name() &&
+                self.extern_prelude.contains(&second_segment.name)


Talking to @joshtriplett, turns out this condition is wrong, we should always fork paths that don't start with :: or a keyword.

If we want to be more forward-compatible with "1path", then crates outside of extern_prelude (not passed with ---extern) should not be accessible with non-disambiguated paths.

(So the condition needs to stay in some form.)

@petrochenkov I guess with relative_imports, we'd have to separate relative use paths (which would be the default?) from allowing to resolve through an extern crate (which would be gated by extern_prelude), but preferably still detect ambiguities when a crate could be loaded?

Do you mean something like this?

mod rustc {} // refers to the local module rustc as a relative path // however, there's an extern crate `rustc` that can be loaded even if it's not in extern prelude use rustc;

We can certainly detect this "ambiguity" in a post-processing step if rustc was successfully resolved locally, and possibly report an error.

eddyb · 2018-08-02T21:10:50Z

src/librustc_resolve/build_reduced_graph.rs

+            let self_ident = Ident { name: keywords::SelfValue.name(), span: use_tree.span };
+            let extern_ident = Ident { name: keywords::Extern.name(), span: use_tree.span };
+            self_module_path[0] = self_ident;
+            extern_module_path[0] = extern_ident;


This doesn't need to use Extern, it could rely on CrateRoot which has same effect.

eddyb · 2018-08-03T18:39:55Z

(reminder to self to incorporate these)

Tricky expansion-related tests: https://internals.rust-lang.org/t/relative-paths-in-rust-2018/7883/105

An intricate testcase from @joshtriplett

Expected output:

main.rs:

pub const A: usize = 0;

pub mod foo {
    pub const B: usize = 1;

    pub mod bar {
        pub const C: usize = 2;

        pub enum E {
            V1(usize),
            V2(String),
        }

        pub fn test() {
            println!("{} {} {}", crate::A, crate::foo::B, C);
        }

        pub fn test_use() {
            use crate::A;
            use crate::foo::B;

            println!("{} {} {}", A, B, C);
        }

        pub fn test_enum() {
            use E::*;
            match E::V1(10) {
                V1(i) => { println!("V1: {}", i); }
                V2(s) => { println!("V2: {}", s); }
            }
        }
    }

    pub fn test() {
        println!("{} {} {}", crate::A, B, bar::C);
    }

    pub fn test_use() {
        use crate::A;
        use bar::C;

        println!("{} {} {}", A, B, C);
    }

    pub fn test_enum() {
        use bar::E::*;
        match bar::E::V1(10) {
            V1(i) => { println!("V1: {}", i); }
            V2(s) => { println!("V2: {}", s); }
        }
    }
}

pub fn test() {
    println!("{} {} {}", A, foo::B, foo::bar::C);
}

pub fn test_use() {
    use foo::B;
    use foo::bar::C;

    println!("{} {} {}", A, B, C);
}

pub fn test_enum() {
    use foo::bar::E::*;
    match foo::bar::E::V1(10) {
        V1(i) => { println!("V1: {}", i); }
        V2(s) => { println!("V2: {}", s); }
    }
}

fn main() {
    test();
    foo::test();
    foo::bar::test();
    test_use();
    foo::test_use();
    foo::bar::test_use();
    test_enum();
    foo::test_enum();
    foo::bar::test_enum();
}

EDIT: more testcases, on Discord

joshtriplett · 2018-08-03T21:06:44Z

Updated version of the edition guide for these changes: rust-lang/edition-guide#74

petrochenkov · 2018-08-05T18:18:57Z

src/librustc_resolve/resolve_imports.rs

+                        {
+                            return Ok(());
+                        }
+                    }


Does this mean "both self::path and extern::path exist, but they actually refer to the same non-import entity in the end"?

(Looks like this should be unnecessary if the "backtracking" is not supported.)

I'm trying to remember if I added this or @cramertj but I think it was here when I started hacking on it. IMO this should probably be an ambiguity error to stay on the safe side.

aturon · 2018-08-06T16:45:59Z

@bors: p=100

I'm going to go ahead and set high priority on this, since it's blocking the Preview 2 announcement.

…dition.

…::x`, not just `::x`.

…ths)] is enabled.

…`uniform_paths` imports.

eddyb · 2018-08-14T04:07:56Z

Huh, we don't run those two accursed tests on the PR Travis checks?!

@bors r=petrochenkov

bors · 2018-08-14T04:07:57Z

📌 Commit 13bc0b5 has been approved by petrochenkov

bors · 2018-08-14T04:22:21Z

⌛ Testing commit 13bc0b5 with merge 3e05f80...

@cramertj

#[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 ::x::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 ::x::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 · 2018-08-14T06:31:04Z

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing 3e05f80 to master...

rust-highfive · 2018-08-14T06:32:24Z

📣 Toolstate changed by #52923!

Tested on commit 3e05f80.
Direct link to PR: #52923

💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra).
💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra).
💔 rls on windows: test-pass → build-fail (cc @nrc, @rust-lang/infra).
💔 rls on linux: test-pass → build-fail (cc @nrc, @rust-lang/infra).

@Manishearth

Tested on commit rust-lang/rust@3e05f80. Direct link to PR: <rust-lang/rust#52923> 💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra). 💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra). 💔 rls on windows: test-pass → build-fail (cc @nrc, @rust-lang/infra). 💔 rls on linux: test-pass → build-fail (cc @nrc, @rust-lang/infra).

Manishearth · 2018-08-14T06:43:12Z

Edit: removed bug report for nonexistent bug caused by rustup falling back to an oudated cargo.

This hits an ICE on clippy_lints

   Compiling clippy_lints v0.0.212 (file:///home/manishearth/mozilla/Wall/rust-clippy/clippy_lints)
thread 'main' panicked at 'librustc_resolve/lib.rs:2853: unexpected definition for an identifier in pattern: Mod(DefId(10/0:0))', librustc/util/bug.rs:47:26
note: Run with `RUST_BACKTRACE=1` for a backtrace.

error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

note: rustc 1.30.0-nightly (3e05f80a3 2018-08-14) running on x86_64-unknown-linux-gnu

note: compiler flags: -C debuginfo=2 -C incremental --crate-type lib

note: some of the compiler flags provided by cargo are hidden

eddyb · 2018-08-14T06:58:58Z

@Manishearth Oh right ~~I fixed a bug where paths not starting with :: could load arbitrary extern crates, so that's where the first error comes from - is ansi_term not in build-dependencies?~~
(EDIT: looking again at the code, I didn't actually do that after all, the behavior only changes with #![feature(uniform_paths)] - I would've had to change too many tests otherwise)

I'll have to look into the ICE, that's unexpected! (EDIT: see #53333).

rustc_resolve: crates only exist in the type namespace. Fixes #53333 by resolving `::crate_name` in `TypeNS` alone, which was overlooked in #52923 and didn't break tests, since having `use crate_name;` and a `crate_name` value in the same scope is rare.

rust-highfive assigned varkor Jul 31, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 31, 2018

This comment has been minimized.

Sign in to view

petrochenkov self-assigned this Aug 1, 2018

This comment has been minimized.

Sign in to view

rust-highfive unassigned varkor Aug 2, 2018

This comment has been minimized.

Sign in to view

eddyb commented Aug 2, 2018

View reviewed changes

joshtriplett mentioned this pull request Aug 3, 2018

path-clarity: Document new uniform paths variant rust-lang/edition-guide#74

Merged

This comment has been minimized.

Sign in to view

petrochenkov reviewed Aug 5, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 5, 2018

This comment has been minimized.

Sign in to view

Centril mentioned this pull request Aug 6, 2018

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

Closed

8 tasks

joshtriplett added this to the Rust 2018 Preview 2 milestone Aug 7, 2018

bors added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 14, 2018

eddyb and others added 5 commits August 14, 2018 07:06

syntax: gensym the injected std/core extern crates in the Rust 2018 e…

cd47831

…dition.

rustc_resolve: fix special-case for one-segment import paths.

f9b1176

#[feature(uniform_paths)]: allow use x::y; to resolve through `self…

39ce9ef

…::x`, not just `::x`.

rustc_resolve: inject ambiguity "canaries" when #![feature(uniform_pa…

2ad865d

…ths)] is enabled.

rustc_resolve: also inject canaries to detect block scopes shadowing …

13bc0b5

…`uniform_paths` imports.

eddyb force-pushed the relative-imports branch from 98478ca to 13bc0b5 Compare August 14, 2018 04:07

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 14, 2018

bors merged commit 13bc0b5 into rust-lang:master Aug 14, 2018

eddyb deleted the relative-imports branch August 14, 2018 06:55

This comment has been minimized.

Sign in to view

This was referenced Aug 14, 2018

ICE: librustc_resolve/lib.rs:2853: unexpected definition for an identifier in pattern: Mod(DefId(10/0:0)) #53333

Closed

rustc_resolve: crates only exist in the type namespace. #53335

Merged

This was referenced Sep 6, 2018

rustc crashes with exit code 101 and no error - 2018 edition + futures-await + automatic extern crate #51975

Closed

Internal compiler in Rust 2018 module path resolution #51887

Closed

petrochenkov mentioned this pull request Oct 5, 2018

Add extern crate items to extern prelude #54658

Merged

petrochenkov mentioned this pull request Dec 27, 2018

Stabilize uniform_paths #56759

Merged

neachdainn mentioned this pull request Apr 3, 2019

Change Edition from 2018 to 2015 jeikabu/nng-rust#10

Closed

#[feature(uniform_paths)]: allow use x::y; to resolve through self::x, not just ::x. #52923

#[feature(uniform_paths)]: allow use x::y; to resolve through self::x, not just ::x. #52923

Conversation

eddyb commented Jul 31, 2018 • edited

rust-highfive commented Jul 31, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

varkor commented Aug 2, 2018

This comment has been minimized.

This comment has been minimized.

eddyb Aug 2, 2018

Choose a reason for hiding this comment

petrochenkov Aug 5, 2018 • edited

Choose a reason for hiding this comment

petrochenkov Aug 5, 2018

Choose a reason for hiding this comment

eddyb Aug 6, 2018

Choose a reason for hiding this comment

petrochenkov Aug 6, 2018

Choose a reason for hiding this comment

eddyb Aug 2, 2018 • edited

Choose a reason for hiding this comment

eddyb commented Aug 3, 2018 • edited

joshtriplett commented Aug 3, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

petrochenkov Aug 5, 2018

Choose a reason for hiding this comment

petrochenkov Aug 5, 2018

Choose a reason for hiding this comment

eddyb Aug 6, 2018

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

aturon commented Aug 6, 2018

eddyb commented Aug 14, 2018

bors commented Aug 14, 2018

bors commented Aug 14, 2018

bors commented Aug 14, 2018

rust-highfive commented Aug 14, 2018

Manishearth commented Aug 14, 2018 • edited

eddyb commented Aug 14, 2018 • edited

This comment has been minimized.

This comment has been minimized.

#[feature(uniform_paths)]: allow `use x::y;` to resolve through `self::x`, not just `::x`. #52923

#[feature(uniform_paths)]: allow `use x::y;` to resolve through `self::x`, not just `::x`. #52923

eddyb commented Jul 31, 2018 •

edited

petrochenkov Aug 5, 2018 •

edited

eddyb Aug 2, 2018 •

edited

eddyb commented Aug 3, 2018 •

edited

Manishearth commented Aug 14, 2018 •

edited

eddyb commented Aug 14, 2018 •

edited