resolve: Preserve ambiguous glob reexports in crate metadata #147984

petrochenkov · 2025-10-22T13:12:42Z

So in cross-crate scenarios they can work in the same way as in crate-local scenarios.

Resurrection of #114682.
One of unblocking steps for #145108.
Fixes #36837.

rustbot · 2025-10-22T13:12:47Z

rustbot has assigned @jdonszelmann.
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

petrochenkov · 2025-10-22T13:13:01Z

@bors try

resolve: Preserve ambiguous glob reexports in crate metadata

petrochenkov · 2025-10-22T13:14:38Z

cc @LorrensP-2158466 @bvanjoi

rust-bors · 2025-10-22T15:32:03Z

☀️ Try build successful (CI)
Build commit: b4c5508 (b4c55082edd8dec08ce8af276d7054d9c4db20c4, parent: f5e2df741b4a9820a7579f0c8eccc951706a8782)

petrochenkov · 2025-10-22T15:33:22Z

@craterbot check

craterbot · 2025-10-22T15:33:47Z

👌 Experiment pr-147984 created and queued.
🤖 Automatically detected try build b4c5508
🔍 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

craterbot · 2025-10-22T15:34:03Z

🚧 Experiment pr-147984 is now running

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

craterbot · 2025-10-24T07:11:57Z

🎉 Experiment pr-147984 is completed!
📊 2312 regressed and 2 fixed (721923 total)
📊 1766 spurious results on the retry-regessed-list.txt, consider a retry¹ if this is a significant amount.
📰 Open the summary report.

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

re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-147984/retry-regressed-list.txt ↩

petrochenkov · 2025-10-24T12:28:34Z

That's a lot of breakage.
Mostly from dependencies, including various version of openssl that we've seen previously.
I'll demote this error to always be reported as a lint in cross-crate scenarios, and then rerun crater.

petrochenkov · 2025-10-24T12:58:17Z

@bors try

@craterbot run mode=check-only p=1 crates=https://crater-reports.s3.amazonaws.com/pr-147984/retry-regressed-list.txt

resolve: Preserve ambiguous glob reexports in crate metadata

rust-bors · 2025-10-24T15:17:26Z

☀️ Try build successful (CI)
Build commit: c6359cd (c6359cd3b4418e8472bae1a89c242796f2b86d56, parent: 75948c8bb3bd37f1e8ee20273a04edea4c1f84f8)

petrochenkov · 2025-10-24T15:24:38Z

@craterbot run mode=check-only p=1 crates=https://crater-reports.s3.amazonaws.com/pr-147984/retry-regressed-list.txt

craterbot · 2025-10-24T15:24:47Z

👌 Experiment pr-147984-1 created and queued.
🤖 Automatically detected try build c6359cd
🔍 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

traviscross · 2025-11-10T07:37:27Z

cc @yaahc @rust-lang/lang-docs @rust-lang/fls

petrochenkov · 2025-11-20T13:01:15Z

ping @jdonszelmann, this is blocking other work.
cc @b-naber maybe you could take over?

jdonszelmann · 2025-11-21T13:41:51Z

@rustbot reroll

sorry, am not sure I can review this right now

petrochenkov · 2025-12-01T15:21:02Z

@yaahc This is now blocking #149195 as well.

yaahc · 2025-12-02T22:09:06Z

r? @yaahc

yaahc · 2025-12-05T19:54:31Z

compiler/rustc_resolve/src/build_reduced_graph.rs

+            (self.arenas.new_res_binding(res, vis, span, expansion), ambig_kind)
+        });
        // Record primary definitions.
        match res {


NIT: When I was reviewing this section I felt like there was a high signal to noise ratio with the way this match is structured. As far as I can tell the only difference between the different define_extern calls is the namespace argument. I'd recommend changing this match to evaluate to the namespace and pass that ns local into a single shared invocation of define_extern regardless of res defkind.

Especially with the new field changing the formatting to be one line per argument, its even more difficult now to visually tell what is different and what isn't between those invocations.

Feel free to ignore this one, just thought I'd share my experience.

I can factor the define_extern calls into a closure.
I'd like to preserve the exhaustive match because of the bug!("unexpected resolution: {:?}", res) part.

yaahc · 2025-12-05T20:41:30Z

compiler/rustc_resolve/src/build_reduced_graph.rs

+            self.build_reduced_graph_for_external_crate_res(child, parent_scope, i, None)
+        }
+        for (i, child) in
+            self.cstore().ambig_module_children_untracked(def_id, self.tcx.sess).enumerate()


Why does this need to be untracked?

I did some digging and as far as I can tell this is referring to how it's not being accessed through the query system unlike the module_children access above, that it'd break incremental compilation, and that it should only be called pre-HIR (during resolve). I'm not super familiar with the query system though or the later stages of resolution so I want to double check that I understand this properly.

My understanding is that this is satisfying the "should only be called" pre-HIR requirement because all of these ambig children are materialized into their appropriate scopes during early resolution and this untracked method is never called after that.

What I don't understand is how this would interact with incremental compilation and break things and what differences there are between the module children and ambig module children that make us only want to track calls to one of them.

Name resolution happens before incremental compilation starts, so we can just read things from CStore instead of using queries, see other uses of .cstore() in rustc_resolve.

yaahc · 2025-12-05T21:32:28Z

compiler/rustc_resolve/src/diagnostics.rs

+                let node_id = match ambiguity_error.b1.0.kind {
+                    NameBindingKind::Import { import, .. } => import.root_id,
+                    NameBindingKind::Res(_) => CRATE_NODE_ID,


I want to make sure I understand why this changed.

My understanding is that previously we'd only hit this code path via ambiguity errors produced from glob imports in try_define_local. Now we can create them from the ambiguity_children of extern crates, but these only contain a simplified copy of the original import chain and the final res from the original nested set of namebindings. Since you couldn't reasonably recreate the original nested set of namebindings you instead construct a new namebinding from just the underlying Res which you mark as an ambiguity warning.

Let me know if that sounds right

The remaining question I have is how does the NodeId selected here impact the diagnostic we emit? From what I can tell the span in use is that of the outermost import in the re-export chain. I would assume that the diagnostic would be able to point to the external ambiguous pub use declaration but I'm not familiar enough with our diagnostic logic to know how the nodeid comes in. I tried poking around and it seems like it may impact the way the lints are buffered and emitted. I'd guess this might impact the order in which this warning is emitted relative to other diagnostics but I stopped digging when I saw that the rustc_dev_guide said that nodeid was being phased out. I'm guessing this is relatively inconsequential.

Let me know if that sounds right

Sounds right, some parts of the original chain are minimally preserved just to report nice diagnostics.

The remaining question I have is how does the NodeId selected here impact the diagnostic we emit?

It determines the placement of allow/warn/deny attributes.
CRATE_NODE_ID means that only crate level allow-warn-deny would work (other crates don't have node ids, so crate id here is used as a fallback).
The node id selection here is not good in general, it would be better to use some use-site node id here, but it's a larger change, and I wouldn't bother doing it in this PR.

yaahc · 2025-12-05T22:59:34Z

tests/ui/imports/glob-conflict-cross-crate-1.rs

-    // ambiguity error instead of a not found error.
+    glob_conflict::f(); //~ ERROR `f` is ambiguous
+                        //~| WARN this was previously accepted
+    glob_conflict::glob::f(); //~ ERROR `f` is ambiguous


I honestly have no idea how this is resolving, the glob import this is resolving through looks like nonsense to me:

rust/tests/ui/imports/auxiliary/glob-conflict.rs

Lines 12 to 17 in 66428d9

pub use m1::*;

pub use m2::*;

pub mod glob {

pub use *;

}

when I try to do this in a test crate it says "cannot export all crates". I wouldn't expect f to be in scope for re-exportation due to the mod glob {..} boundary. How does this work? I'm thoroughly stumped

@P1n3appl3 pointed out that this is a 2015 edition thing

Yes, use * on 2015 edition is just use crate::*.

yaahc · 2025-12-05T23:25:45Z

tests/ui/imports/glob-conflict-cross-crate-3.rs

+    //~^ ERROR `C` is ambiguous
+    //~| ERROR `C` is ambiguous
+    //~| WARN this was previously accepted
+    //~| WARN this was previously accepted


why does this emit so many duplicate warnings? It looks like theres two unique ones, the one for the local ambiguity and the one for the external ambiguity, but the stderr file looks like its emitting each of those twice for a total of four warnings. Why isn't this being prevented by the warning deduplication logic?

rust/compiler/rustc_resolve/src/lib.rs

Lines 2072 to 2075 in 743dc68

if !self.matches_previous_ambiguity_error(&ambiguity_error) {

// avoid duplicated span information to be emit out

self.ambiguity_errors.push(ambiguity_error);

}

fn record_use_inner walks the import chain and reports an ambiguity at every step.
I think we could only report the first one (if it's an error).
I look at the PR introducing the deduplication logic, and I don't think the author really understood what they were doing, not sure why it doesn't deduplicate this specific case.
In any case, probably off-topic for this PR.

Upd: also record_use can run twice on the same "use", at least for imports (for other diagnostic reasons).

yaahc · 2025-12-05T23:32:37Z

tests/ui/imports/issue-114682-2.rs

-// First, should this ambiguous item be omitted considering the maximum visibility
-// of `issue_114682_2_extern::m::max` in the type namespace is only within the extern crate.
-// Second, if we retain the ambiguous item of the extern crate, should it be treated
-// as an ambiguous item within the local crate for the same reasoning?


Can you link to where the decision on these questions was made?

From my understanding of the PR it's looking like you went with

no

no

Is that right?

Correct, seems to be the simplest and conservative choice.

I don't quite understand the sentence, but it seems like the answer is "yes", because issue_114682_2_extern::max is indeed treated as ambiguous in this test (= local crate).

yaahc · 2025-12-05T23:38:28Z

tests/ui/imports/issue-114682-3.rs

    a.ext();
-    //^ FIXME: it should report `ext` not found because `SettingsExt`
-    // is an ambiguous item in `issue-114682-3-extern.rs`.
+    //~^ ERROR no method named `ext` found for type `u8` in the current scope


I've not yet looked into how ambiguous imports interact with which traits are in scope so the logic for this is unfamiliar to me but I find this behavior surprising. I would have expected this to report an ambiguity error while selecting the innermost¹ candidate and continuing, not for it to block the selection of all three candidates in scope with no mention of ambiguity.

This doesn't seem like an ideal diagnostic experience. Is there some structural issue that makes it hard to identify and report that this error was caused by an interaction with ambiguious imports?

Footnotes

I know this is a globvsglob/globvsexpanded issue where they're ambiguous within a single scope so I'm not sure innermost is accurate, but I don't remember off hand how the candidates for this kind of ambiguity are stored and its nearing the end of the day so I wanna get my first review pass posted rather than diving in more deeply. ↩

This is a separate issue (#147992), there are several possible ways to address it, and the latest development can be found in #149058.

I think this PR and that PR combined will result in .ext() producing a compatibility warning about the method call being ambiguous.

yaahc · 2025-12-05T23:50:44Z

tests/ui/imports/issue-114682-4.rs

 use issue_114682_4_extern::*;

-fn a() -> Result<i32, ()> { // FIXME: `Result` should be identified as an ambiguous item.
+//~v ERROR type alias takes 1 generic argument but 2 generic arguments were supplied


yeah see, this one picks one of the extern results and prints this error, that's how I'd expect things to work normally, the previous one with the traits in scope resolution was super inconsistent w/ my previous experience.

yaahc · 2025-12-06T00:00:52Z

compiler/rustc_middle/src/metadata.rs

+#[derive(Debug, TyEncodable, TyDecodable, HashStable)]
+pub enum AmbigModChildKind {
+    GlobVsGlob,
+    GlobVsExpanded,


I'm assuming this means logic should be able to preserve globvsexpanded ambiguities across crate boundaries but I don't see any tests exercising this logic. Can you add one?

I'll add a test.
In any case, cross-crate GlobVsExpanded is unnecessary and will be removed in #149681.

yaahc · 2025-12-06T00:13:55Z

We won't be catching cases where the ambiguity comes from a dependency adding an item in a subsequent version. @nikomatsakis made a suggestion to lint anytime we glob export from two different crates, which I liked. Otherwise, we can at least catch cases where there is a known conflict at the time of writing the code.

@petrochenkov am I correct in understanding that https://doc.rust-lang.org/rustc/lints/listing/warn-by-default.html#ambiguous-glob-reexports represents the def-site lint but not the second suggestion from niko quoted above?

@tmandry am I correct in assuming that lang doesn't consider adding the second lint you mentioned to be a blocker for this PR?

tmandry · 2025-12-06T00:19:00Z

@yaahc Just checking you mean this one. Correct, I don't consider it a blocker.

nikomatsakis made a suggestion to lint anytime we glob export from two different crates, which I liked.

yaahc · 2025-12-06T00:19:51Z

@rustbot author

rustbot · 2025-12-06T00:19:56Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

So in cross-crate scenarios they can work in the same way as in crate-local scenarios.

- Factor `define_extern` into a closure - Add a test for cross-crate GlobVsExpanded ambiguity

rustbot · 2025-12-06T09:33:21Z

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

petrochenkov · 2025-12-06T09:34:27Z

@rustbot ready

rustbot assigned jdonszelmann Oct 22, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 22, 2025

rust-bors bot added a commit that referenced this pull request Oct 22, 2025

Auto merge of #147984 - petrochenkov:expambig, r=<try>

b4c5508

resolve: Preserve ambiguous glob reexports in crate metadata

This comment has been minimized.

Sign in to view

petrochenkov added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 22, 2025

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Oct 24, 2025

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 24, 2025

petrochenkov force-pushed the expambig branch from aa2fb6e to e388d62 Compare October 24, 2025 12:56

This comment has been minimized.

Sign in to view

rust-bors bot added a commit that referenced this pull request Oct 24, 2025

Auto merge of #147984 - petrochenkov:expambig, r=<try>

c6359cd

resolve: Preserve ambiguous glob reexports in crate metadata

petrochenkov added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 24, 2025

traviscross added the relnotes Marks issues that should be documented in the release notes of the next release. label Nov 10, 2025

rustbot assigned SparrowLii and unassigned jdonszelmann Nov 21, 2025

apiraino removed the to-announce Announce this issue on triage meeting label Nov 27, 2025

This comment was marked as resolved.

Sign in to view

petrochenkov mentioned this pull request Dec 1, 2025

resolve: Convert ambiguous_glob_imports lint into a hard error #149195

Open

petrochenkov removed needs-fcp This change is insta-stable, or significant enough to need a team FCP to proceed. S-waiting-on-t-lang Status: Awaiting decision from T-lang labels Dec 2, 2025

rustbot assigned yaahc and unassigned SparrowLii Dec 2, 2025

petrochenkov mentioned this pull request Dec 3, 2025

Resolver: Batched Import Resolution #145108

Open

yaahc reviewed Dec 6, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 6, 2025

petrochenkov added 2 commits December 6, 2025 12:26

resolve: Preserve ambiguous glob reexports in crate metadata

a0806fb

So in cross-crate scenarios they can work in the same way as in crate-local scenarios.

Address review comments

255f6c3

- Factor `define_extern` into a closure - Add a test for cross-crate GlobVsExpanded ambiguity

petrochenkov force-pushed the expambig branch from 743dc68 to 255f6c3 Compare December 6, 2025 09:33

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 6, 2025

	if !self.matches_previous_ambiguity_error(&ambiguity_error) {
	// avoid duplicated span information to be emit out
	self.ambiguity_errors.push(ambiguity_error);
	}

resolve: Preserve ambiguous glob reexports in crate metadata #147984

Are you sure you want to change the base?

resolve: Preserve ambiguous glob reexports in crate metadata #147984

Conversation

petrochenkov commented Oct 22, 2025 • edited by yaahc Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Oct 22, 2025

Uh oh!

petrochenkov commented Oct 22, 2025

Uh oh!

This comment has been minimized.

petrochenkov commented Oct 22, 2025

Uh oh!

rust-bors bot commented Oct 22, 2025

Uh oh!

petrochenkov commented Oct 22, 2025

Uh oh!

craterbot commented Oct 22, 2025

Uh oh!

craterbot commented Oct 22, 2025

Uh oh!

craterbot commented Oct 24, 2025

Footnotes

Uh oh!

petrochenkov commented Oct 24, 2025

Uh oh!

This comment has been minimized.

petrochenkov commented Oct 24, 2025

Uh oh!

This comment has been minimized.

rust-bors bot commented Oct 24, 2025

Uh oh!

petrochenkov commented Oct 24, 2025

Uh oh!

craterbot commented Oct 24, 2025

Uh oh!

traviscross commented Nov 10, 2025

Uh oh!

petrochenkov commented Nov 20, 2025

Uh oh!

jdonszelmann commented Nov 21, 2025

Uh oh!

This comment was marked as resolved.

petrochenkov commented Dec 1, 2025

Uh oh!

yaahc commented Dec 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petrochenkov Dec 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Footnotes

Uh oh!

Choose a reason for hiding this comment

petrochenkov commented Oct 22, 2025 •

edited by yaahc

Loading

petrochenkov Dec 6, 2025 •

edited

Loading

tmandry commented Dec 6, 2025 •

edited

Loading