resolve: Mark items under exported ambiguous imports as exported

[petrochenkov]

After #147984 one of the imports in an ambiguous import set becomes accessible under a deny-by-default deprecation lint.

So if it points to something, that something needs to be marked as exported, so its MIR is encoded into metadata, its symbol is not lost from object files, etc.
The added test shows an example.
This fixes around 10-20 crater regressions found in #149195 (comment).

Unblocks #149195.

[rustbot]

These commits modify tests/rustdoc-json.
rustdoc-json is a public (but unstable) interface.

Please ensure that if you've changed the output:

• It's intentional.
• The FORMAT_VERSION in src/librustdoc-json-types is bumped if necessary.

cc @aDotInTheVoid, @obi1kenobi

[rustbot]

r? @jackh726

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

cc @obi1kenobi #149195 (comment)

From the cargo-semver-checks's point of view introducing a public name through an ambiguous reexport is clearly a breaking change, even if it technically can be allowed (and is allowed implicitly in dependencies).
Cases like that are actually reported by rustc using another lint called ambiguous_glob_reexports (regular, not future-compatibility).

Not marking items accessible through ambiguous imports as exported can cause errors like "missing optimized MIR" or linking errors, but I'm not sure if it can cause ICEs.
Reporting errors like that instead of ambiguous_glob_imports would probably be okay for directly compiled crates because they already produce an error (the lint is deny-by-default), but is not so good for ambiguous glob imports in dependencies because unlike lints such errors cannot be capped and break all other crates depending on the crate using them.

[obi1kenobi]

Thanks for the tag. I read the contents of this PR and also the description at #147984 (comment) and I still have a few questions.

#147984 (comment) mentioned the desire to make both local and cross-crate ambiguous errors behave consistently, which makes sense to me. But is that change limited solely to improving errors only? Or is it possible that a Rust program that previously would have failed to compile now is able to compile without errors, even if some lints had to be allowed?

cargo-semver-checks currently treats ambiguity as "attempting to use either item is an error." I'm attempting to determine if that's ever not correct, including with lints, so we can raise the right diagnostic with full and accurate info.

Right now I'm worried that I don't know how to model the case of "one of the items is made available but it isn't defined which one".

[petrochenkov]

Right now I'm worried that I don't know how to model the case of "one of the items is made available but it isn't defined which one".

That's the main problem, the compiler cannot define it either - it depends on internal unstable details that we don't want to expose, like specific expansion and import resolution order.
Otherwise this wouldn't be an error or future error.

If this is reported as a deprecation lint and not an error, the specific compiler's choice can be visible from the the effective visibility tables - the chosen alternative will be marked as exported, and the other alternatives will not.

Or is it possible that a Rust program that previously would have failed to compile now is able to compile without errors, even if some lints had to be allowed?

It's a language change, not just diagnostics, more names appear in modules and they may cause more names to appear in other modules through imports, and may cause more name conflicts.

In case of glob conflicts new programs will be able to compile under deprecation lints, some programs that previously compiled will report deprecation lints and will stop compiling in the future, in corner cases some compiling programs even start reporting hard errors like in the examples from #147984 (comment).
It probably cannot make previously erroneous programs compile without deprecation lints.

[petrochenkov]

r? @yaahc
Apologies for reassigning one more PR to you, this is logically a part of #147984 which you reviewed.

[rustbot]

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]

ping @yaahc, it's been 2 weeks

[yaahc]

Hey, sorry for the delay. I took a look at it and it looks fine to me but to be honest, I don't have any prior familiarity with rustc_resolve/src/effective_visibilities.rs. I haven't had to dig into it for the reference documentation work so far so this is my first time exploring it that I can recall.

I did my best to go through this and try to understand what it does. I'm going to try to summarize it as I understand the logic and, assuming my summary is effectively correct, consider this an approval for r=me, but if my summary has any clear misunderstandings in it please help me correct them before merging.

---

I'm starting with set_bindings_effective_visibilities because despite it's changes coming later in the diff its invoked before the changes in compute_effective_visibilities so those changes apply first in practice.

set_bindings_effective_visibilities seems to do two important operations, updating the import_effective_visibilities and updating the def_effective_visibilities. Before and after this change we always update the import_effective_visibilities, regardless of ambiguity. However, when there is ambiguity, we would previously bail out from further iteration through the chain of imports and would not update the effective visibilities of any of the imports deeper in that chain of imports. Assuming we encountered an ambiguity that wasn't set to warn, after bailing we'd also skip updating the def effective visibility because decl would still be pointing to the ambiguous import in the chain instead of the def behind the chain of imports.

compute_effective_visibilities then goes and updates the effective_visibilities struct in the resolver based on the import_effective_visibilities (this struct is initialized with the content of def_effective_visibilities, so it seems to be a unification of those two structs from the visitor). Prior to this change it would check if the import was ambiguous, if it wasn't it would propagate its effective visibility into the resolver's EffectiveVisibilties member, else if the import was ambiguous and re-exported it would add it to exported_ambiguities. Importantly, these actions were mutually exclusive before, the information about the import would either go into one struct or the other depending on ambiguity.

After this change, these updates are treated as orthogonal, If there is ambiguity on a re-exported import, that gets tracked as before, but now even if there is ambiguity the effective visibility information about that import still gets propagated into the resolver's effective_visibilities struct. The changes to set_bindings_effective_visibilities mean that we're not skipping any updates to either of the def_/import_ effective visibilities structs which then all gets propagated into the resolver's unified copy of effective visibilities regardless of ambiguities, while still tracking ambiguities on the side via exported_ambiguities.

To try to put it into simpler terms, it looks like previously we'd track only the effective visibility of non-ambiguous imports and defs, we'd separately track any ambiguous re-exports, but only the outermost import in the chain, nothing deeper was tracked if it was only reachable through the ambiguity. Now we ignore ambiguity completely for the purposes of tracking effective visibility while still separately tracking which of those imports is ambiguous. On the surface this looks like a nice simplification of existing logic.

---

Let me know if I got that all right.

Also, thank you for splitting the test out into an earlier commit so I could see the diff in the error before and after the change.

[petrochenkov]

Your comment summarizes the logic quite well.

I haven't had to dig into it for the reference documentation work so far

Yeah, the effective visibility tables are not supposed to have a direct effect on language, because they are not necessarily precise (they can conservatively mark things as more reexported/reachable).
They mostly drive lints, link-time visibility and rustdoc.

In particular, the impreciseness and conservativeness allows us to mark slightly more things as exported in this PR. E.g. technically we only need to export things if the ambiguous reexport eventually reaches the crate boundary, because locally it's usually an error rather than a lint. But in practice crate-local parts of the effvis tables are not used much, so we just avoid all the complexity by marking the whole chains reexported.

I think eventually, when all cases of ambiguous_glob_imports become errors (or at least when there are not differences between local and cross-crate cases), a similar logic will need to be reintroduced to make the effective visibility tables more precise again.

[yaahc]

@bors r+

[rust-bors]

📌 Commit 6ada0d2 has been approved by yaahc

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.

[matthiaskrgr]

looks like a test failed in #151710 (comment)

[matthiaskrgr]

@bors r-

[rust-bors]

Commit 6ada0d2 has been unapproved.