use dep1::foo as dep1 is considered ambiguous

[codyps]

src/lib.rs:

pub mod foo {
    pub use dep1::foo as dep1;
}

dep1/src/lib.rs:

pub mod foo {}

I expected to see this happen: build succeeds.

Instead, this happened:

error[E0432]: unresolved import `dep1::foo`
 --> src/lib.rs:2:13
  |
2 |     pub use dep1::foo as dep1;
  |             ^^^^^^^^^^^^^^^^^ no `foo` in `foo`

error[E0659]: `dep1` is ambiguous (name vs any other name during import resolution)
 --> src/lib.rs:2:13
  |
2 |     pub use dep1::foo as dep1;
  |             ^^^^ ambiguous name
  |
  = note: `dep1` could refer to a crate passed with `--extern`
  = help: use `::dep1` to refer to this crate unambiguously
note: `dep1` could also refer to the module imported here
 --> src/lib.rs:2:13
  |
2 |     pub use dep1::foo as dep1;
  |             ^^^^^^^^^^^^^^^^^
  = help: use `self::dep1` to refer to this module unambiguously

error: aborting due to 2 previous errors

Some errors have detailed explanations: E0432, E0659.
For more information about an error, try `rustc --explain E0432`.

Meta

working beta: rustc --version --verbose:

rustc 1.47.0-beta.9 (d6646f647 2020-10-03)
binary: rustc
commit-hash: d6646f64790018719caebeafd352a92adfa1d75a
commit-date: 2020-10-03
host: x86_64-unknown-linux-gnu
release: 1.47.0-beta.9
LLVM version: 11.0

non-working nightly: rustc +nightly --version --verbose

rustc 1.49.0-nightly (beb5ae474 2020-10-04)
binary: rustc
commit-hash: beb5ae474d2835962ebdf7416bd1c9ad864fe101
commit-date: 2020-10-04
host: x86_64-unknown-linux-gnu
release: 1.49.0-nightly
LLVM version: 11.0

This broke rust-systemd's build (https://travis-ci.com/github/jmesmon/rust-systemd/jobs/395293939) (broken versions are v0.6.0 and v0.5.0 which use edition = "2018").

I'm not sure if there is a simpler single-crate break (there could be).

[codyps]

searched nightlies: from nightly-2020-10-03 to nightly-2020-10-04
regressed nightly: nightly-2020-10-04
searched commits: from 8876ffc to 25c8c53
regressed commit: 6ebad43

bisected with cargo-bisect-rustc v0.5.2

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc --start=2020-10-03 --end=2020-10-04 --preserve

[codyps]

Out of the prs in that roll up, I suspect #77421 could be related. @petrochenkov

[petrochenkov]

Yes, #77421 should be the reason.
#77421 makes some code compiling on Rust 1.44-1.46 an error to address #74556.

Strictly speaking, #77421 is not a bug fix, but rather an implementation of a slightly more conservative language rule giving us some more freedom in the future.
If it causes a noticeable amount of regressions in practice, then we can return to the 1.44-1.46 rules and declare #74556 as not-an-issue.

[codyps]

Interesting. It appears I must have threaded the needle on versions here: 1.43 emits the same error as nightly, as do earlier versions.

For reference: the crate was converted to edition = "2018" recently during the 1.44-1.46 time frame. edition = "2015" is totally fine with this import style, which is where this code originated.

While a bit annoying, it should be straight forward to fix for me. Just need to add some :: prefixes.

[tesuji]

@rustbot prioritize

[pnkfelix]

Discussed at T-compiler meeting.

nominating for discussion at T-lang meeting.

[pnkfelix]

self-assigning as reminder to do prep to describe issue at next T-lang meeting.

[nikomatsakis]

Discussed in today's lang-team meeting:

• @pnkfelix was not present but based on the comments in this issue we reached some conclusions
• This does seem like a bug fix and we would be content to let it ride the trains presuming it's not too disruptive
• We'd like crater data, but in the meantime we can see how many folks report issues. It seems relevant that @jmesmon is able to fix their code readily enough and that the behavior here was only allowed for a few releases (at least that was our understanding).
• This (and several other issues on our docket this week) highlights the need to draft better and clearer standards for what sort of breakage of code in the wild we will accept and when (and in particular when we ought to use future compatibility warnings, deny-by-default lints, or editions).

[nikomatsakis]

Discussed in today's meeting:

• Unnominating. We consider this a bug fix.

[Mark-Simulacrum]

It seems like this broke several more crates/repositories, in particular:

• https://crater-reports.s3.amazonaws.com/beta-1.48-1/beta-2020-10-09/gh/CursedFlames.IntCodeVM/log.txt
• https://crater-reports.s3.amazonaws.com/beta-1.48-1/beta-2020-10-09/gh/kdar.smitemotd/log.txt
• https://crater-reports.s3.amazonaws.com/beta-1.48-1/beta-2020-10-09/gh/mrkline.modman/log.txt
• https://crater-reports.s3.amazonaws.com/beta-1.48-1/beta-2020-10-09/gh/pandorasNox.rust-isbn/log.txt
• https://crater-reports.s3.amazonaws.com/beta-1.48-1/beta-2020-10-09/reg/systemd-0.6.0/log.txt

It feels like this pattern is (somewhat surprisingly) common-ish.

I'm going to renominate given crater data, but given that this pattern was permitted on stable for several cycles and my impression from @petrochenkov's comments it that it's not too annoying for us to support, we should keep it around for at least 2018 edition (I wouldn't object to an edition break here though).

[nikomatsakis]

@petrochenkov how difficult would it be to continue supporting this pattern?

[petrochenkov]

@nikomatsakis
Very simple, it only requires reverting #77421.

I'd say that for the current implementation of resolve it's more natural to support it than to not support.

[nikomatsakis]

We discussed this in the @rust-lang/lang meeting today. Our consensus was that in light of the larger-than-expected impact, as well as @petrochenkov's comment, that we should revert #77421 and restore the older behavior. There is a reasonable case to be made for either behavior: there is a self-cycle here, but there is also only one reasonable way to resolve the cycle at present. (I presume that if

However @petrochenkov I'd like to get your take. I was reading a bit into #74556 and especially this comment of yours which seems to be the best summary of what we are "giving up" by accepting this pattern. I'm having a bit of trouble parsing it though, I admit.

I did find the example from #74556 interesting and I don't feel like we explored it in the @rust-lang/lang meeting. The example is:

mod foo {
    pub mod bar {
        pub mod bar {
            pub fn foobar() {}
        }
    }
}

use foo::*;
use bar::bar;
use bar::foobar;

fn main() {
    bar::foobar();
}

What happens here, as best as I can tell, is that use foo::* imports bar as crate::foo::bar but it is "from a glob". We then see the use bar::bar which is resolved based on that import but then shadows it. This is precisely the "time traveling" we were trying to avoid, where the name bar has multiple meanings across different paths. What's worse, the new version of bar (crate::foo::bar::bar) is then used to resolve foobar.

It seems important to me to reject this case, but perhaps it is not so easy to reject this case while also permitting cases like the one in the OP?

[nikomatsakis]

I'm going to leave this nominated.

[petrochenkov]

@nikomatsakis

there is a self-cycle here, but there is also only one reasonable way to resolve the cycle at present.

Namely, causality.
The current resolve's logic is that a use item is not planted into its module until its input (the dep1::foo part) is determined, so the input cannot refer to the output (the as dep1 part) in any way because the output doesn't yet exist when the input is determined.

I was reading a bit into #74556 and especially this comment of yours which seems to be the best summary of what we are "giving up" by accepting this pattern. I'm having a bit of trouble parsing it though, I admit.

The current causality-based approach is different from my "import resolution as inference" idea (this comment of mine) where the output is planted into the module first as a fresh inference variable, and then participates in the input's resolution creating an inference failure (dep1 is required to be equal to two different things in the end).
EDIT: I think it's safe to say that I'll never have time to try implementing this myself.

use foo::* imports bar as crate::foo::bar but it is "from a glob". We then see the use bar::bar which is resolved based on that import but then shadows it. This is precisely the "time traveling" we were trying to avoid, where the name bar has multiple meanings across different paths.

A name can have different meaning in different places because due to the causality logic different names are effectively resolved in different environments.

• Names in input of import A are resolved in environment everything - outputs(A).
• Names in input of import B are resolved in environment everything - outputs(B).
  So the resolution of a name dep1 may be different depending on whether it's resolved in an import producing dep1 as one of its outputs or not.

This is a deterministic rule, it doesn't change if we randomize some item resolution order or macro expansion order, so I'm not sure whether it's directly related to time traveling or not.

It seems important to me to reject this case, but perhaps it is not so easy to reject this case while also permitting cases like the one in the OP?

Perhaps it is not, since it's the same feature in action.

[petrochenkov]

The causality rule do not apply to glob imports because they don't have an easily obtainable set of output names, and people are actually complaining about this (#62769)!

If we excluded Foo-the-variant from the environment when resolving Foo in the import because it's an output of the import, the example wouldn't produce an error.

[nikomatsakis]

This is a deterministic rule, it doesn't change if we randomize some item resolution order or macro expansion order, so I'm not sure whether it's directly related to time traveling or not.

Yeah, I think there's a secondary constraint that I had in mind, which is basically that, in the end, we can come up with a single "resolution result" for each module which is a mapping of name -> full-path and that all of our resolutions are consistent with that final result. That is the property I think you are going for with your "inference variable" idea as well, and it is precisely the property that we lose here, since dep1::foo is resolved in a context where dep1 is not bound, but dep1 does appear in the final mapping, and hence if we "re-resolved" dep1::foo with that final set of names, we would get an ambiguity.

I guess that we can think of use dep1::foo as being a kind of inference itself, that is elaborated to use ::dep1::foo (or use self::dep1::foo, depending), and then we can say that these final elaborated forms are consistent with the resolution result I named above. (Do you understand what I mean here?)

[Diggsey]

One option would be to allow overlap between "pub use" and "use", but not allow overlap within the two.

This would allow having bar in scope for a module whilst exporting a different bar, but doesn't introduce too much ambiguity.

[Mark-Simulacrum]

Assigning to self for a revert of #77421.

[Mark-Simulacrum]

We discussed this in the language meeting today. We thought that we may want the more constrained behavior, but we were not at this point inclined to land breakage. It is possible that in a future edition, we may want to move to a different resolution story (perhaps @petrochenkov's inference idea), and in which case we would lint on such patterns (and presumably others?).

@pnkfelix brought up that we may want to lint on this pattern regardless (since it is rather confusing in most cases), but that should likely go through the standard dance of a proposal dedicated to such a lint which T-lang can FCP (or start in Clippy, perhaps).

[nikomatsakis]

So here is a brain puzzler. Which version of foobar runs in the following code?

mod foo {
    pub mod bar {
        pub fn foobar() -> u32 { 1 }
        pub mod bar {
            pub fn foobar() -> u32 { 2 }
        }
    }
}

use foo:*;
use bar::bar;
use bar::foobar;

Answer: version 2, and I imagine this is because we refuse to resolve use bar::foobar until all possible sources of bar have been resolved. Is that correct, @petrochenkov?

(In any case, @joshtriplett, to answer your question in the meeting, what we are giving up is that we are accepting the current interpretation of cases like this, but these are the sorts of corner cases where different models or evaluation orders or what have you might give slightly different answers.)

[joshtriplett]

@nikomatsakis Interesting. That does seem like it should be rejected (specifically, at the point of use bar::bar), and it's unfortunate we didn't do so from the start. But I'd be hesitant to cause widespread breakage.

Our current error messages seem painfully confusing; "ambiguity" isn't the issue here, that does indeed feel wrong because of causality. Ideally we'd give a clear error about a name conflict, instead.

A lint to gently steer people away from this kind of name conflict (and to take advantage of cap-lints to avoid causing breakage in the process) seems reasonable though.

[petrochenkov]

So here is a brain puzzler. Which version of foobar runs in the following code?

Here's how to solve this step-by step.

• foobar is unambiguously introduced by use bar::foobar;, next we need to determine where bar (1) comes from.
• bar (1) is introduced by use bar::bar;, it could also be introduced by the glob or some prelude, but it doesn't matter because explicit imports shadow anything else, next we need to determine where bar (2) comes from.
• bar (2) is unambiguously introduced by use foo::*; because the other bar is excluded as an output of use bar::bar; and therefore "not in the environment", next we see that foo is a real item and not an import, chain completed.

[petrochenkov]

Another case where people explicitly wanted disambiguation based on causality - #56414 (comment) ("An import should never be ambiguous with itself.").