Fix: Prevent macro-expanded extern crates from shadowing extern arguments

[Delta17920]

prevents an ICE by fixing a logic bug in build_reduced_graph.rs.
the bug caused the compiler to correctly detect and report a shadowing error for a macro-expanded extern crate but then continue processing the invalid item, corrupting the resolver's internal state (extern_prelude) and leading to a crash in later resolution passes the fix adds an early return after the shadowing error is reported to ensure the invalid item is not added to the resolution graph.

Fixes #149821

[rustbot]

r? @WaffleLapkin

rustbot has assigned @WaffleLapkin.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[Delta17920]

r? @Kivooeo

[Kivooeo]

Have you ran ./x test ui locally?

[Delta17920]

Wait checking 😔
I will see where it broke and will push the fix shortly.

[Kivooeo]

This needs a check from @petrochenkov, who is author of this code, I don't know if Vadim have time to take a look or not, but I assign this to him, so this won't be lost

The current problem of this approach from what I see is that after emitting MacroExpandedExternCrateCannotShadowExternArguments it stops emitting any other errors, so for example this test

https://github.com/rust-lang/rust/blob/main/tests/ui/imports/issue-109148.rs

It will not emit this two "std is ambiguous" errors after this error on 6 line, can't say if this is a good tradeoff or not

r? petrochenkov

[Delta17920]

Drafting to avoid stale review

[petrochenkov]

I think #149855 was actually closer to the right solution than this PR, this PR is just hiding the root issue.

The root issue is that issue_145575_hack swallows legitimate ambiguity errors, it's not surprising that they may result in inconsistent resolutions later.
"inconsistent resolution for an import" ICE is already not reported if ambiguity_errors are non-empty, using dcx().has_errors() would be ok too, even if maybe too blunt.

I guess the most surgical solution would looks something like

diff --git i/compiler/rustc_resolve/src/ident.rs w/compiler/rustc_resolve/src/ident.rs
index e38d4370d5d..342c1192090 100644
--- i/compiler/rustc_resolve/src/ident.rs
+++ w/compiler/rustc_resolve/src/ident.rs
@@ -771,36 +771,38 @@ fn maybe_push_ambiguity(
         } else {
             None
         };
-        // Skip ambiguity errors for extern flag bindings "overridden"
-        // by extern item bindings.
-        // FIXME: Remove with lang team approval.
-        let issue_145575_hack = Some(binding) == extern_prelude_flag_binding
-            && extern_prelude_item_binding.is_some()
-            && extern_prelude_item_binding != Some(innermost_binding);
-        if let Some(kind) = ambiguity_error_kind
-            && !issue_145575_hack
-        {
-            let misc = |f: Flags| {
-                if f.contains(Flags::MISC_SUGGEST_CRATE) {
-                    AmbiguityErrorMisc::SuggestCrate
-                } else if f.contains(Flags::MISC_SUGGEST_SELF) {
-                    AmbiguityErrorMisc::SuggestSelf
-                } else if f.contains(Flags::MISC_FROM_PRELUDE) {
-                    AmbiguityErrorMisc::FromPrelude
-                } else {
-                    AmbiguityErrorMisc::None
-                }
-            };
-            self.ambiguity_errors.push(AmbiguityError {
-                kind,
-                ident: orig_ident,
-                b1: innermost_binding,
-                b2: binding,
-                warning: false,
-                misc1: misc(innermost_flags),
-                misc2: misc(flags),
-            });
-            return true;
+        if let Some(kind) = ambiguity_error_kind {
+            // Skip ambiguity errors for extern flag bindings "overridden"
+            // by extern item bindings.
+            // FIXME: Remove with lang team approval.
+            let issue_145575_hack = Some(binding) == extern_prelude_flag_binding
+                && extern_prelude_item_binding.is_some()
+                && extern_prelude_item_binding != Some(innermost_binding);
+            if issue_145575_hack {
+                self.issue_145575_hack_applied = true;
+            } else {
+                let misc = |f: Flags| {
+                    if f.contains(Flags::MISC_SUGGEST_CRATE) {
+                        AmbiguityErrorMisc::SuggestCrate
+                    } else if f.contains(Flags::MISC_SUGGEST_SELF) {
+                        AmbiguityErrorMisc::SuggestSelf
+                    } else if f.contains(Flags::MISC_FROM_PRELUDE) {
+                        AmbiguityErrorMisc::FromPrelude
+                    } else {
+                        AmbiguityErrorMisc::None
+                    }
+                };
+                self.ambiguity_errors.push(AmbiguityError {
+                    kind,
+                    ident: orig_ident,
+                    b1: innermost_binding,
+                    b2: binding,
+                    warning: false,
+                    misc1: misc(innermost_flags),
+                    misc2: misc(flags),
+                });
+                return true;
+            }
         }
 
         false
diff --git i/compiler/rustc_resolve/src/imports.rs w/compiler/rustc_resolve/src/imports.rs
index 4ef87af5605..4e0f3db5982 100644
--- i/compiler/rustc_resolve/src/imports.rs
+++ w/compiler/rustc_resolve/src/imports.rs
@@ -1170,7 +1170,7 @@ fn finalize_import(&mut self, import: Import<'ra>) -> Option<UnresolvedImportErr
                             return;
                         }
                         if let Some(initial_res) = initial_res {
-                            if res != initial_res {
+                            if res != initial_res && !this.issue_145575_hack_applied {
                                 span_bug!(import.span, "inconsistent resolution for an import");
                             }
                         } else if this.privacy_errors.is_empty() {
diff --git i/compiler/rustc_resolve/src/lib.rs w/compiler/rustc_resolve/src/lib.rs
index 8132bf577d8..7e06b8511e3 100644
--- i/compiler/rustc_resolve/src/lib.rs
+++ w/compiler/rustc_resolve/src/lib.rs
@@ -1186,6 +1186,7 @@ pub struct Resolver<'ra, 'tcx> {
     privacy_errors: Vec<PrivacyError<'ra>> = Vec::new(),
     /// Ambiguity errors are delayed for deduplication.
     ambiguity_errors: Vec<AmbiguityError<'ra>> = Vec::new(),
+    issue_145575_hack_applied: bool = false,
     /// `use` injections are delayed for better placement and deduplication.
     use_injections: Vec<UseError<'tcx>> = Vec::new(),
     /// Crate-local macro expanded `macro_export` referred to by a module-relative path.

[Delta17920]

@petrochenkov I have applied the surgical patch you suggested.

Previously, I was attempting a fix using a dummy_binding, which similarly resolved the crash but had slightly different side effects on diagnostics. Switching to your flag-based approach, I verified that it explicitly suppresses ambiguity errors when the hack is active.

Consequently, I have updated (blessed) tests/ui/imports/issue-109148.rs and tests/ui/macros/issue-78325-inconsistent-resolution.rs to match this intended behavior, as they were failing because they expected the ambiguity errors that are now suppressed.

CI should be green now. Ready for review!

[Kivooeo]

I think #149855 was actually closer to the right solution than this PR, this PR is just hiding the root issue

My bad here then, it's been a lot of effortless PRs lately from people who trying to get a contribute just for resume line or something, it's hard to differ them from ones actual trying to understand, learn code and help to the project, especially when PR description is five times bigger than actual fix, and when you try to ask them about why they think this is correct approach of why they used this instead of something different here, it's likely will lead to dialogue with AI

I also have taken a look more closely to all of this

There is comment saying

rust/compiler/rustc_resolve/src/imports.rs

Line 1147 in f520900

// Consistency checks, analogous to `finalize_macro_resolutions`.

rust/compiler/rustc_resolve/src/imports.rs

Lines 1172 to 1175 in f520900

	if let Some(initial_res) = initial_res {
	if res != initial_res {
	span_bug!(import.span, "inconsistent resolution for an import");
	}

I took a look at finalize_macro_resolutions and it uses span_delayed_bug, so I think it'll reasonable to also use it here (yes I know I told that this is a bad solution, but if author explained why this is fine here, I'd step back and accepted this), because for consistency with finalize_macro_resolutions

rust/compiler/rustc_resolve/src/macros.rs

Lines 840 to 846 in f520900

	if let Some(initial_res) = initial_res {
	if res != initial_res {
	// Make sure compilation does not succeed if preferred macro resolution
	// has changed after the macro had been expanded. In theory all such
	// situations should be reported as errors, so this is a bug.
	this.dcx().span_delayed_bug(span, "inconsistent resolution for a macro");
	}

@Delta17920, Sorry for previous closing, seeing you not trying to elaborate on your solution is made me feel so

[Delta17920]

@Kivooeo Thanks for the follow-up! No worries at all about the earlier confusion I totally understand and really appreciate you taking a second look. 🤝 Regarding the fix, I went with @petrochenkov's "surgical" suggestion (adding a specific flag) to keep the suppression tightly scoped to this hack.
(Note: The CI failure in x86_64-gnu-gcc is a network flake SSL connect error downloading dependencies so it seems unrelated to these changes)

[bors]

@Delta17920: 🔑 Insufficient privileges: not in try users

[Kivooeo]

reopen PR to restart CI :D

this sounds fun but real, just close and open back this RP

[Delta17920]

@Kivooeo thnx man

[Delta17920]

sorry, mb

[Delta17920]

@petrochenkov It looks like the error suppression also affects some Clippy tests (e.g., tests/ui/crashes/ice-6255.rs) which were expecting the ambiguity error.

[rustbot]

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy