fix(resolve): update the ambiguity glob binding recursively

[bvanjoi]

Fixes #112713

The error was not reported for #112713 because the ambiguity in the binding of root::sub::C was lost.

The resolve_imports process is shown below:

Iter	importer	module	resolve_glob_import
0	use sub::*	root::sub	Since no bindings had been added, nothing was defined
1	use mod1::*	root::sub::mod1	1. (root::sub, C) -> root::sub::mod1::C by try_define; 2. (block_module, C) -> NameBinding { kind: Import { binding: root::sub::mod1::C }, .. } by try_define recursively for module.glob_importers
2	use mod2::*	root::sub::mod2	(root::sub, C) -> root::sub::mod1::C with ambiguity

However, the ambiguity binding generated by iteration 2 points to a different value than the one generated by iteration 1. Therefore, the binding to root::sub::mod::C in iteration 1 does not have ambiguity, and no error is thrown in the end.

In this PR, I had made update the ambiguity glob binding recursively.

Perhaps changing &'a NameBinding<'a> to &'a mut NameBinding<'a> in NameResolution would be a better choice.

r? @petrochenkov

[bvanjoi]

Additionally, I am unsure whether fixing errors such as "it should throw an error, but it's not" constitutes a breaking change or not.

[petrochenkov]

Additionally, I am unsure whether fixing errors such as "it should throw an error, but it's not" constitutes a breaking change or not.

It's a breaking change, but it is also a bugfix.
Could you rebase this PR so I could run crater to estimate the amount of breakage?
@rustbot author

[petrochenkov]

@bors try @rust-timer queue

[bors]

⌛ Trying commit 1576b8ed41c10aa2263e55f774217b07c8c17966 with merge b1375b2484512d46eeace16657e5b4a5450c610b...

[bors]

☀️ Try build successful - checks-actions
Build commit: b1375b2484512d46eeace16657e5b4a5450c610b (b1375b2484512d46eeace16657e5b4a5450c610b)

[petrochenkov]

@craterbot check

[craterbot]

👌 Experiment pr-112743 created and queued.
🤖 Automatically detected try build b1375b2484512d46eeace16657e5b4a5450c610b
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[rust-timer]

Finished benchmarking commit (b1375b2484512d46eeace16657e5b4a5450c610b): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 658.179s -> 657.807s (-0.06%)

[petrochenkov]

@bors rollup=maybe

[craterbot]

🚧 Experiment pr-112743 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[bvanjoi]

This is a breaking change too: root::Thing currently refers to root::thing::Thing, but it will refer to root::Thing after this PR.

[petrochenkov]

The comment has a typo, but I assume you mean root::thing::Thing -> root::Thing.
It's also clearly a bugfix.

Crater will finish soon and show what actually breaks after these changes.

[bvanjoi]

Update:

I have rebased and add some tests in this PR. I would like to declare that this PR may have much impact, as it:

• Fixes Wildcard import fails to resolve if a derive is present #56593 and introduces a breaking change, see this comment.
• Fixes Type is not exported if type with same name is introduced via use #105235.
• Fixes Ambiguous item error fails to trigger when use is declared before the source #112713 and also introduces a breaking change, see this comment.

[bvanjoi]

I think that removing _ if old_binding() => return t is the correct fix, but it may cause significant compatibility issues. Should I explore alternative fixes?

[petrochenkov]

All regressions: https://crater-reports.s3.amazonaws.com/pr-112743/logs-archives/regressed.tar.gz

[craterbot]

🎉 Experiment pr-112743 is completed!
📊 8909 regressed and 6 fixed (303008 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[bvanjoi]

reduced: rustc +stage1 code.rs

macro_rules! m {
    () => {
      pub fn EVP_PKEY_id() {}
    };
}

mod openssl {
    pub use self::evp::*;
    pub use self::handwritten::*;

    mod evp {
      m!();
    }

    mod handwritten {
      m!();
    }
}
use openssl::*;

fn main() {
    EVP_PKEY_id();
}

[bvanjoi]

It appears to be a bugfix as both rustc code.rs and rustc +stage1 code.rs produce ambiguous errors for the expanded code.

mod openssl {
    pub use self::evp::*;
    pub use self::handwritten::*;

    mod evp {
      pub fn EVP_PKEY_id() {}
    }

    mod handwritten {
      pub fn EVP_PKEY_id() {}
    }
}
use openssl::*;

fn main() {
    EVP_PKEY_id(); //  `EVP_PKEY_id` is ambiguous
}

[petrochenkov]

Yes, also a bugfix.
Could you try fixing openssl and asking them to release a new version?
It should remove majority of those 8909 regressions and then we'll think what to do next (probably introduce deprecation warnings).

[bvanjoi]

I have written a script to analyze this regression log and found that it contains 58 types of issues, some of which are unrelated to name resolution:

regression.zip

[petrochenkov]

The right fix here is probably adding a new boolean flag somewhere around struct AmbiguityError that would discern between ambiguities reported previously and new ambiguities added by this PR.
Then fn report_ambiguity_error would report a hard error for the former, and a deprecation lint for the latter.

In any case we need to wait until the new release of openssl is published, because it's responsible for the majority of regressions, and because it's not just an ambiguity error, it's a "cannot find" error, likely due to a cross-crate use of this ambiguity.

[petrochenkov]

Superseded by #113099.