resolve: Preserve ambiguous glob reexports in crate metadata

[petrochenkov]

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]

r? @jdonszelmann

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]

@bors try

[petrochenkov]

cc @LorrensP-2158466 @bvanjoi

[rust-bors]

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

[petrochenkov]

@craterbot check

[craterbot]

👌 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]

🚧 Experiment pr-147984 is now running

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

[craterbot]

🎉 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

Footnotes

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

[petrochenkov]

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.

[rustbot]

This PR was rebased onto a different master 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]

@bors try

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

[rust-bors]

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

[petrochenkov]

74 regressions are just due to ambiguous_glob_imports being deny-by-default.
We could potentially introduce a separate warn-by-default lint extern_ambiguous_glob_imports to soften the transition.

There are 3 other non-spurious regressions from this change:

• polywrap_http_plugin-0.1.11 - when resolving to an ambigous glob recovery picks up a different resolution than previously (polywrap_msgpack_serde::Result -> serde_json::Result), it has some chain effects resulting in errors
• polywrap_logger_plugin-0.1.11 - same as polywrap_http_plugin-0.1.11
• Dhayson.six-degrees-bot.96f810355aa802b6df7a51f2d5c5cdd3d4aba468 - when resolving to an ambigous glob recovery picks up a different resolution than previously (crate::client::Error -> nostr_relay_pool::prelude::Error), it has some chain effects resulting in errors

I think this amount of breakage is acceptable.
Fixes to polywrap_* crates were already sent (polywrap/rust-wrap-client#259), but ignored.
Dhayson.six-degrees-bot.96f810355aa802b6df7a51f2d5c5cdd3d4aba468 is unpublished, so probably no need to send a fix.

[petrochenkov]

Change description for the lang team

This PR implements a long stanging missing piece in cross-crate reexports.

Currently ambiguous glob imports/reexports produce an error like this on local use.

// Crate `mylib`

mod m1 {
    pub struct S {}
}
mod m2 {
    pub struct S {}
}

pub use m1::*;
pub use m2::*;

fn main() {
	let s = self::S {}; // ERROR `S` is ambiguous
}

However, from other crates such reexports are entirely invisible, because they are ignored when encoding rlib metadata.

let s = mylib::S {}; // ERROR cannot resolve `S` in module `mylib`

use mylib::*; // OK, empty glob import, puts no names into the current module

This PR implements the missing metadata encoding and the reexports can now be observed from other crates, so the behavior becomes identical in cross-crate and crate-local scenarios.

let s = mylib::S {}; // ERROR `S` is ambiguous

use mylib::*; // OK, puts one name `S` into the current module, that will produce errors on actual use

// In particular, if we had another glob import importing `S` here, then there will be a new conflict (only on actual use).
// That's why the change is potentially breaking, see the breakage analysis in the comment above.
use thylib::*; // Also imports `S`

[traviscross]

We talked about this and ended up liking it. This preserves what I might call "ambiguity monotonicity" -- adding ambiguity should never reduce ambiguity warnings.

We'd also like to see a def-site warning about these cases paired with this -- in this PR or otherwise concurrently -- if possible.

@rfcbot fcp merge

[rust-rfcbot]

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[nikomatsakis]

@rfcbot reviewed

[joshtriplett]

FWIW I'd personally like to see that proposed additional warning be in a separate PR, not mixed into this one. I do want to see that warning too, though.

[tmandry]

I think a def-site warning is important to "shift left" failures like this.

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.

@rfcbot reviewed

[rust-rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[petrochenkov]

We'd also like to see a def-site warning about these cases paired with this

I think a def-site warning is important to "shift left" failures like this.

We already have a lint for this - https://doc.rust-lang.org/rustc/lints/listing/warn-by-default.html#ambiguous-glob-reexports

[petrochenkov]

@traviscross

74 regressions are just due to ambiguous_glob_imports being deny-by-default.
We could potentially introduce a separate warn-by-default lint extern_ambiguous_glob_imports to soften the transition.

Is there some decision on this ^^^ ?

[traviscross]

The FCP, as proposed, is to accept that soft breakage.

(I.e., "soft breakage" due to this being from a deny-by-default lint, that could be allowed and is affected by cap-lints, rather than due to a hard error.)

[traviscross]

We already have a lint for this - https://doc.rust-lang.org/rustc/lints/listing/warn-by-default.html#ambiguous-glob-reexports

Thanks. Yes, that is the lint we were looking for.