Skip to content
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

rustc_resolve: only prepend CrateRoot to a non-keyword segment. #53988

Merged
merged 1 commit into from
Sep 9, 2018

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Sep 6, 2018

Fixes #53770 by treating use paths as absolute in a finer-grained manner, specifically:

use {a, crate::b, self::c, super::d};

Used to be interpreted as if it were (when uniform_paths is not enabled):

use ::{a, crate::b, self::c, super::d};

With this PR, the CrateRoot pseudo-keyword indicating an absolute path is only inserted when the first path segment is found (if it's not a keyword), i.e. the example behaves like:

use {::a, crate::b, self::c, super::d};

This should (finally) make use {path}; fully equivalent to use path;.

r? @petrochenkov cc @cramertj @joshtriplett @nikomatsakis

@petrochenkov
Copy link
Contributor

petrochenkov commented Sep 6, 2018

FWIW, the RFC explicitly specified that the :: prefix is always added, so that we add it once and never add anything into braces at arbitrary and possibly different depth levels.

It also means you can always prepends a single crate:: to the import to migrate from 2015 to 2018.

@petrochenkov
Copy link
Contributor

Without looking at code, how use {{a::b::c}}; behaves now? Can root segment be injected into deeper levels?

@petrochenkov
Copy link
Contributor

I see, root segments can indeed be injected at arbitrary different levels inside braces.
Could you add a test for this case?

@petrochenkov
Copy link
Contributor

petrochenkov commented Sep 6, 2018

I have mixed feelings and not ready to approve this single-handedly.
@eddyb, could you start a quick @rust-lang/lang review?


In non-uniform_paths schemes in both 2015 and 2018 editions use paths are absolute by default, it means implicit :: is added to them.

Should this implicit :: be added once at the start (current rules)

use {a, {self::b}, {c, {super::d}}};
=>
use ::{a, {self::b}, {c, {super::d}}}; // `::self::b` and `::super::d` are errors

or "adaptively", multiple times inside braces when appropriate (this PR)

use {a, {b}, {c, {d}}};
=>
use {::a, {self::b}, {::c, {super::d}}}; // OK

?

@petrochenkov petrochenkov added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 6, 2018
@Centril
Copy link
Contributor

Centril commented Sep 6, 2018

I would expect the following to hold:

(1)

use {$p_0};
=>
use $p_0;

(2)

use {$p_0, $p_1, $p_2};
=>
use $p_0;
use $p_1;
use $p_2;

(3)

use {$p_0, {$p_1, {$p_2}}};
=>
use $p_0;
use $p_1;
use $p_2;

This applies however arbitrarily deep you nest it.
(My understanding is that this is what @eddyb's PR achieves)

I think that prefixing :: to each use item here works poorly particularly with uniform_paths but also with anchored_paths. In the latter case, it works poorly because use {self::x}; becomes use ::self::x; which is always an error. This also holds for crate and super. For uniform_paths it works even worse since local paths can be referred to instead of referring to an extern crate. Thus, you would likely get surprising behavior from use {...}.

@bors
Copy link
Contributor

bors commented Sep 7, 2018

☔ The latest upstream changes (presumably #54005) made this pull request unmergeable. Please resolve the merge conflicts.

@eddyb
Copy link
Member Author

eddyb commented Sep 7, 2018

For uniform_paths it works even worse

uniform_paths already never injects CrateRoot aka prefix :: (if you look at the diff, logic specific that injection being done on non-uniform_paths, was moved to happen later/further-in).

So with #![feature(uniform_paths)], everything already worked like @Centril's and my intuition.

@eddyb
Copy link
Member Author

eddyb commented Sep 7, 2018

@petrochenkov You mean :: not crate::, right? As in, only in Rust 2015 is use {a}; is equivalent to use crate::a;, in Rust 2018 it looks for a crate named a instead.

@petrochenkov
Copy link
Contributor

@eddyb
Yes, I've updated the comment.

@bors
Copy link
Contributor

bors commented Sep 7, 2018

☔ The latest upstream changes (presumably #54021) made this pull request unmergeable. Please resolve the merge conflicts.

@aturon
Copy link
Member

aturon commented Sep 7, 2018

I definitely agree with @Centril's intuition as well -- a nested use statement should always be equivalent to its "flattening" into a list of single-item use statements.

@eddyb
Copy link
Member Author

eddyb commented Sep 8, 2018

@aturon @Centril Do we need some sort of process here (FCP merge, I'd guess?), or are you saying that @petrochenkov can already go ahead and r+ this PR?

@Centril
Copy link
Contributor

Centril commented Sep 8, 2018

Since we're under a bit of a time pressure, let's instead do:

@rfcbot poll lang Can we go ahead and r+ this PR?

@rfcbot
Copy link

rfcbot commented Sep 8, 2018

Team member @Centril has asked teams: T-lang, for consensus on:

Can we go ahead and r+ this PR?

@petrochenkov
Copy link
Contributor

Ok, @bors r+
The remaining people rarely pay attention to ticky boxes anyway.

@bors
Copy link
Contributor

bors commented Sep 8, 2018

📌 Commit e0e7cf3 has been approved by petrochenkov

@bors 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-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Sep 8, 2018
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Sep 8, 2018
rustc_resolve: only prepend CrateRoot to a non-keyword segment.

Fixes rust-lang#53770 by treating `use` paths as absolute in a finer-grained manner, specifically:
```rust
use {a, crate::b, self::c, super::d};
```
Used to be interpreted as if it were (when `uniform_paths` is not enabled):
```rust
use ::{a, crate::b, self::c, super::d};
```
With this PR, the `CrateRoot` pseudo-keyword indicating an absolute path is only inserted when the first path segment is found (if it's not a keyword), i.e. the example behaves like:
```rust
use {::a, crate::b, self::c, super::d};
```
This should (finally) make `use {path};` fully equivalent to `use path;`.

r? @petrochenkov cc @cramertj @joshtriplett @nikomatsakis
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Sep 8, 2018
…rochenkov

rustc_resolve: inject `uniform_paths` canary always on Rust 2018.

**NOTE**: this PR is based on rust-lang#53988, only the second commit is new.

This PR is an attempt at future-proofing "anchored paths" by emitting the same ambiguity errors that `#![feature(uniform_paths)]` would, with slightly changed phrasing (see added UI tests).

r? @petrochenkov cc @aturon @Centril @joshtriplett
@bors
Copy link
Contributor

bors commented Sep 9, 2018

⌛ Testing commit e0e7cf3 with merge 3d2fc45...

bors added a commit that referenced this pull request Sep 9, 2018
rustc_resolve: only prepend CrateRoot to a non-keyword segment.

Fixes #53770 by treating `use` paths as absolute in a finer-grained manner, specifically:
```rust
use {a, crate::b, self::c, super::d};
```
Used to be interpreted as if it were (when `uniform_paths` is not enabled):
```rust
use ::{a, crate::b, self::c, super::d};
```
With this PR, the `CrateRoot` pseudo-keyword indicating an absolute path is only inserted when the first path segment is found (if it's not a keyword), i.e. the example behaves like:
```rust
use {::a, crate::b, self::c, super::d};
```
This should (finally) make `use {path};` fully equivalent to `use path;`.

r? @petrochenkov cc @cramertj @joshtriplett @nikomatsakis
@bors
Copy link
Contributor

bors commented Sep 9, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants