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

uniform_paths edition 2018 feature emits spurrious ambiguity for the log crate #53408

Closed
uberjay opened this issue Aug 15, 2018 · 17 comments
Closed
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-rust-2018-preview Area: The 2018 edition preview T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@uberjay
Copy link

uberjay commented Aug 15, 2018

I'm fairly sure this is a bug. At the very least It's confusing! Enabling uniform_paths, there's a detected ambiguous use for log. But I can't figure out where the non-crate log is coming from. Furthermore, if I change use log; to use self::log;, just to see if it'll work, the compiler claims there's no log module in the root!

I didn't run into any other cases like this (with the projects I've tried it with so far).

  1. cargo new uniform-paths
  2. switch crate over to 2018 edition.
  3. cargo add log
  4. in src/main.rs:
#![feature(rust_2018_preview, uniform_paths)]

use log;

fn main() {}

attempting to build:

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

Changing log to self::log:

error[E0432]: unresolved import `self::log`
 --> src/main.rs:3:5
  |
3 | use self::log;
  |     ^^^^^^^^^ no `log` in the root

Playground link: https://play.rust-lang.org/?gist=f749f38540242c143f2e4c41e2f1625c&version=nightly&mode=debug&edition=2018

@eddyb
Copy link
Member

eddyb commented Aug 15, 2018

Well, the ambiguity is there because you're doing use crate_name;, which can find both the crate and this import itself (which defines self::crate_name).
In other words, you have this: use ::log; use self::log;, but combined.
If you were to write use ::log; use self::log::something;, that would work, because the first use introduces the name log in the same module, allowing self::log to find it.

If you tried use log as the_log_crate;, you wouldn't get any error, because there's no self::log anymore.

cc @petrochenkov Do we want to support use crate_name;? What's the least hacky approach to allowing it?

EDIT: even if we don't want to allow this, the error message is abysmal.

@uberjay
Copy link
Author

uberjay commented Aug 15, 2018

Ahh, interesting. I'd minimized it from the case I actually ran across, which was:

use log::{debug, info, log};

fn main() {
    debug!("Hello, world!");
}

I see now that if I only import debug and info the problem goes away. But then fails later:

error: cannot find macro `log!` in this scope
 --> src/main.rs:6:5
  |
6 |     debug!("Hello, world!");
  |     ^^^^^^^^^^^^^^^^^^^^^^^^

I have essentially zero experience with macros, so am a little unclear if this is even a real issue. I vaguely recall seeing something that would allow the external crate to fix this particular (hygiene?) issue.

But... I see why this (my original issue) is happening, and it makes sense now. But it also seems surprising to me that the side-effect of a use statement is essentially conflicting with itself.

@eddyb eddyb added A-diagnostics Area: Messages for errors, warnings, and lints A-rust-2018-preview Area: The 2018 edition preview labels Aug 15, 2018
@eddyb
Copy link
Member

eddyb commented Aug 15, 2018

@uberjay Part of why that happens is the use item is not a statement, and it doesn't have side-effects - even if you have macros expanding to imports, we enforce convergence towards the same result, i.e., we guarantee that if you wrote:

use ::log::log;
use log::{debug, info};

it would have the exact same result, as your example (I disambiguated the first one just to assume it somehow works).

And in this case, the other two imports are definitely ambiguous, so you really want:

use ::log::{debug, info, log};

(if you want to use #![feature(uniform_paths)])

@eddyb
Copy link
Member

eddyb commented Aug 15, 2018

@aturon @joshtriplett @petrochenkov On a slightly separate note, we could allow more things by making the uniform_paths ambiguity checking only look in the module/type namespace, so values and macros aren't treated as ambiguities. Is there value in keeping ambiguity errors for them at all?

@letheed
Copy link
Contributor

letheed commented Aug 15, 2018

Something similar:
use lazy_static::lazy_static; produces:

import from `lazy_static` is ambiguous

could refer to external crate `::lazy_static`

help: write `::lazy_static` or `self::lazy_static` explicitly instead
note: relative `use` paths enabled by `#![feature(uniform_paths)]`

But that makes no sense, use self::lazy_static wouldn't be valid yet.

@uberjay
Copy link
Author

uberjay commented Aug 15, 2018

Oookay, It suddenly makes perfect sense, knowing that the order of use items is not significant. Thank you!

@eddyb
Copy link
Member

eddyb commented Aug 15, 2018

@letheed As I tried to explain above, the import conflicts with itself.
Replacing the import with a self:: isn't equivalent because you removed the succeeding import.

use crate_name; first succeeds as if it were use ::crate_name; and then, when the ambiguity with self::crate_name is checked, self::crate_name exists. It's the import itself!

This, for example, has "always" worked:

use ::log;
use self::log as renamed_log;

The second import refers to the first one (the order doesn't matter, fwiw).

If we want to allow use crate_name;, we have to special-case it, and not check if self::crate_name exists (or check for it but ignore it if it comes back to the same import).
The initial implementation is conservative, in that it will aggressively catch "ambiguities", even here.

@joshtriplett
Copy link
Member

@eddyb I don't think we want to special-case this, because it's redundant. If you have log in your Cargo.toml, then you can already use log::foo and similar without writing use log;. We should generate a special-case error message, though, to make it clear that you don't need to write use somecrate;.

(Note that such an error message would not trigger on, for instance, use somecrate as foo; or use somecrate as _;)

@joshtriplett
Copy link
Member

@uberjay The error you get when you import and use debug! without importing log! indicates that the log crate needs updating to work more smoothly with use_extern_macros. See #51496 for details. In the interim, you can indeed work around the problem by also importing log.

@eddyb eddyb added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Aug 15, 2018
@eddyb
Copy link
Member

eddyb commented Aug 15, 2018

Example of an error message that makes slightly more sense:

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

The reason use log; has such a bad diagnostic that the span of the identifier log is being used for both the ambiguity, and the definition of what self::log resolves to.

OTOH, use log::log; is even worse, you don't even get the "could also refer to self::log" part of the message because log::log is a macro - so maybe it shouldn't even conflict?
That is, an external crate only exists in the module/type namespace, so getting an ambiguity error because of a value or macro with the same name seems wrong.

@uberjay
Copy link
Author

uberjay commented Aug 15, 2018

Mm, yeah. I definitely minimized it too far, taking it all the way to use log. In reality, I was trying to use log::{debug, info}, ended up running across the implicit dependency on the log macro, and then ended up here while trying to figure out where the conflict was coming from. (I checked the prelude, despite it seeming unlikely... :)

@petrochenkov
Copy link
Contributor

This looks purely like a consequence of implementation via import cloning.
I think this should work eventually, imports are normally planted into a module after they are resolved, so self-conflicts are not possible.

@petrochenkov
Copy link
Contributor

On a slightly separate note, we could allow more things by making the uniform_paths ambiguity checking only look in the module/type namespace, so values and macros aren't treated as ambiguities. Is there value in keeping ambiguity errors for them at all?

This is how it should work, if I understand the issue correctly.
use thing; is essentially three independent imports fused only for error reporting purposes.

use thing in type;
use thing in value;
use thing in macro;

Only the first import should be affected by "uniform path"-related cloning

// Desugared into
use self::thing in type;
use ::thing in type;
use self::thing in value;
use self::thing in macro;

since the crate universe "module" can't contain neither values nor macros.

@internetionals
Copy link

internetionals commented Aug 16, 2018

I still don't understand why macros aren't imported by the distinguished name (log!) instead of their base name (log) which overlaps with other symbols.

The only reason I've seen people cite has to do with the fact that you declare them using macro_rules! foo. But if that's the only reason, than by all means also allow defining them using macro_rules! foo! and perhaps in the future add a lint suggesting that's how to declare them. Then they'd always have their own distinguished namespace.

@uberjay
Copy link
Author

uberjay commented Aug 16, 2018

@internetionals my (possibly mistaken!) understanding was that macro names don't include the !, and that it was just how the call-site syntax is defined.

@eddyb
Copy link
Member

eddyb commented Aug 16, 2018

Note that #53427 should fix the false positive macro ambiguity.

bors added a commit that referenced this issue Aug 17, 2018
rustc_resolve: overhaul `#![feature(uniform_paths)]` error reporting.

Fixes #53408 by only considering external crates to conflict within their (type/module) namespace, *not* with the value or macro namespaces, and also by adding a special-cased error for redundant `use crate_name;` imports (without actually allowing them).
Also, all canaries for a given import are grouped into one diagnostic per namespace, in order to make block-scoped ambiguities clearer.
See changed/added tests for more details.

r? @petrochenkov cc @aturon @joshtriplett
@internetionals
Copy link

@internetionals my (possibly mistaken!) understanding was that macro names don't include the !, and that it was just how the call-site syntax is defined.

No your obviously not mistaken. But having something called X that's effectively only available as X!, might as well have been called X!.

Now if you do use ::log::log; you refer to both some symbol (eg. a function) log() in the log crate and to a possible log!() macro, because they both occupy different namespaces. But if you consider macro names to always include the ! at the end (which they effectively always have and how they are distinguised by the parser) then they can still technically be in separate namespaces, but they can always be referred to unambiguously using use ::log::log; for the normal symbol and use log::log!; for the macro.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-rust-2018-preview Area: The 2018 edition preview T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants