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: inject uniform_paths canaries regardless of the feature-gate, on Rust 2018. #54011

Merged
merged 2 commits into from
Sep 10, 2018

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Sep 6, 2018

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

Also, on top of #54005, this PR allows this as well:

use crate_name;
use crate_name::foo;

In that any ambiguity between an extern crate and an import of that same crate is ignored.

r? @petrochenkov cc @aturon @Centril @joshtriplett

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 6, 2018
@Centril
Copy link
Contributor

Centril commented Sep 6, 2018

This looks excellent :)

@petrochenkov
Copy link
Contributor

r=me after some decision on #53988 is made, or with #53988 removed

@petrochenkov petrochenkov added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 6, 2018
@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.

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Sep 8, 2018

📌 Commit 449516f 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-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Sep 8, 2018
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
@eddyb
Copy link
Member Author

eddyb commented Sep 9, 2018

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Sep 9, 2018

📌 Commit 49dd5c180bdee6e6c0b2fd0ab43b20e08859a6d4 has been approved by petrochenkov

@kennytm
Copy link
Member

kennytm commented Sep 9, 2018

@bors p=4

@bors
Copy link
Contributor

bors commented Sep 9, 2018

⌛ Testing commit 49dd5c180bdee6e6c0b2fd0ab43b20e08859a6d4 with merge e14424d052ef6d3e4cc926c010791b240e2cfcd4...

@bors
Copy link
Contributor

bors commented Sep 9, 2018

💔 Test failed - status-appveyor

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

bors commented Sep 9, 2018

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

@kennytm kennytm added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 10, 2018
@eddyb
Copy link
Member Author

eddyb commented Sep 10, 2018

That pattern doesn't work, only use crate_name; by itself does.
What I did was based on @petrochenkov's explanation that use self::x; never resolves to itself, i.e. the "cyclical" possibility is ignored.

Do we want to further expand the relaxation to ignore any ambiguity between the crate and any explicit import of the crate in question, in general?

@joshtriplett
Copy link
Member

@eddyb

That pattern doesn't work, only use crate_name; by itself does.

Ah, I see; I managed to test this incorrectly. You're right, this pattern doesn't work.

Do we want to further expand the relaxation to ignore any ambiguity between the crate and any explicit import of the crate in question, in general?

If possible, yes.

@eddyb
Copy link
Member Author

eddyb commented Sep 10, 2018

Assuming we want that relaxation,

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Sep 10, 2018

📌 Commit 962580076ae48fd528c3ce4ed2ccdfb63fe63698 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 10, 2018
@bors
Copy link
Contributor

bors commented Sep 10, 2018

⌛ Testing commit 962580076ae48fd528c3ce4ed2ccdfb63fe63698 with merge 6fe2df1d16b21451b6e0e0a5c9620ee4982c5c14...

@bors
Copy link
Contributor

bors commented Sep 10, 2018

💔 Test failed - status-appveyor

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 10, 2018
@eddyb
Copy link
Member Author

eddyb commented Sep 10, 2018

RLS has a legitimate error in it:

error: `cargo` import is ambiguous
  --> tools\rls\src\build\mod.rs:15:5
   |
15 | use cargo::util::important_paths;
   |     ^^^^^ can refer to external crate `::cargo`
...
35 | mod cargo;
   | ---------- may refer to `self::cargo` in the future
   |
   = help: write `::cargo` or `self::cargo` explicitly instead
   = note: in the future, `#![feature(uniform_paths)]` may become the default
error: aborting due to previous error
[RUSTC-TIMING] rls test:false 5.373
error: Could not compile `rls`.

What should we do here?
EDIT: we're fixing it, it seems.

@eddyb eddyb changed the title rustc_resolve: inject uniform_paths canary always on Rust 2018. rustc_resolve: inject uniform_paths canaries regardless of the feature-gate, on Rust 2018. Sep 10, 2018
@eddyb
Copy link
Member Author

eddyb commented Sep 10, 2018

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Sep 10, 2018

📌 Commit d5da94a 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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 10, 2018
@bors
Copy link
Contributor

bors commented Sep 10, 2018

⌛ Testing commit d5da94a with merge 5953454...

bors added a commit that referenced this pull request Sep 10, 2018
rustc_resolve: inject `uniform_paths` canaries regardless of the feature-gate, on Rust 2018.

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

Also, on top of #54005, this PR allows this as well:
```rust
use crate_name;
use crate_name::foo;
```
In that any ambiguity between an extern crate and an import *of that same crate* is ignored.

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

bors commented Sep 10, 2018

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

@bors bors merged commit d5da94a into rust-lang:master Sep 10, 2018
@eddyb eddyb deleted the anchored-in-the-future branch September 10, 2018 13:00
bors added a commit that referenced this pull request Sep 14, 2018
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
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