rustdoc: introduce glob shadowing rules to rustdoc

[longfangsong]

Signed-off-by: longfangsong longfangsong@icloud.com

Fixes #83375.

This pr introduced the glob shadowing rules into the visit ast stage of rustdoc. So we can prevent glob reexport duplicate or wrong modules.

[rust-highfive]

r? @ollie27

(rust-highfive has picked a reviewer for you, use r? to override)

[jyn514]

Just a few more nits :) this looks really good! Thank you for working on it.

[longfangsong]

/cc @pickfire @jyn514 @ollie27

[jyn514]

Can you add some tests for glob re-exports of unnamed items and items with different namespaces?

[pickfire]

Is it possible for new_item.name() to be empty? And is it possible that new_item_ns is empty?

If both is not possible maybe we can remove this branch.

[longfangsong]

Is it possible for new_item.name() to be empty? And is it possible that new_item_ns is empty?

The only situation I found which new_item.name().is_empty() is std::prelude (but IMO we don't need to care about it), remove this check seems won't affect the test result. But this comment mentioned some possible situation. @ollie27 can you give a more detailed example?

On the other hand, new_item_ns can be empty in many situations like macro derives (a minimal example is Debug in src/test/rustdoc/wrapping.rs). If we remove this check, then adding those items may cause panic on L88.

[jyn514]

new_item.name can be empty for impl blocks - or if it's not and it's using {{impl}} or something, that's another bug, because impl blocks shouldn't use globbing logic, they don't have have a namespace.

[pickfire]

So means in this case both checks are still necessary? Maybe we should leave a comment or it's not necessary?

And should the test includes these cases?

[jyn514]

Hmm, are you sure from_glob should apply recursively like this? What happens if you have

pub mod outer {
  pub const X: usize = 0;
  pub mod inner {
    pub use super::*;
    pub const X: usize = 1;
    pub const Y: usize = 2;
  }
}
pub use outer::inner::*;

Shouldn't inner::X take precedence over outer::X?

[pickfire]

This could be set as a test case to prevent regression.

[longfangsong]

I'm still looking for a way to fix the case in this comment, but I found another interesting case which might reveal a bug in rustc:

// resolve.rs
mod sub4 {
    pub const X: usize = 0;
 
    pub mod other {
        pub const X: usize = 1;
    }
    pub mod inner {
        pub use other::*;
        pub use super::*;
    }
}

pub use sub4::inner::*;
// main.rs
println!("{:?}", resolve::X);

Will give us a 0.

And

// resolve.rs
mod sub4 {
    pub const X: usize = 0;
 
    pub mod other {
        pub const X: usize = 1;
    }
    pub mod inner {
        // just swap these `pub use` here
        pub use super::*;
        pub use other::*;
    }
}

pub use sub4::inner::*;
// main.rs
println!("{:?}", resolve::X);

Will cause:

`X` is ambiguous (glob import vs glob import in the same module)

Is this the expected behaviour?

[jyn514]

Is this the expected behaviour?

No, this is a bug: #47525 (and various other ordering bugs: https://github.com/rust-lang/rust/issues?q=is%3Aopen+is%3Aissue+label%3AA-resolve+order+).

I wonder if there's a way to reuse what resolve decides here so we don't have three different opinions of what it should resolve to? Right now there are two, rustc_resolve and rust-analyzer.

[jyn514]

I'm still looking for a way to fix the case in this comment

FWIW I think just passing false unconditionally should fix the bug (right now you propagate from_glob instead).

[longfangsong]

FWIW I think just passing false unconditionally should fix the bug (right now you propagate from_glob instead).

Propagating from_globis a mistake, which I already fixed in 18f031f., but fixing the case here is not that eazy.

Both mod::sub4::X and mod::sub4::inner::X are from_glob for module mod, and we have no way to decide which one to keep.

for code:

mod sub4 {
    pub const X: usize = 0;
    pub mod inner {
        pub use super::*;
        pub const X: usize = 1;
    }
}

#[doc(inline)]
pub use sub4::inner::*;

The call tree start from pub use is roughly like:

• visiting item Use(sub4::inner::*, from_glob = false)
  • maybe_inline_local(sub4::inner, from_glob = true)
    • visiting item Const(sub4::X, from_glob = true)
    • visiting item Mod(sub4::inner, from_glob = true)
      • visit_mod_contents(sub4::inner, from_glob = false)
      • visiting item Use(sub4, from_glob = false)
      • maybe_inline_local(sub4, from_glob=false)
      • visiting item Const(sub4::inner::X, from_glob = true)

[rust-log-analyzer]

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
* 326 features
some tidy checks failed


command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/rust-tidy" "/checkout" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "/checkout/obj/build" "16"


failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test --stage 2 src/tools/tidy
Build completed unsuccessfully in 0:00:13

[jyn514]

Both mod::sub4::X and mod::sub4::inner::X are from_glob for module mod, and we have no way to decide which one to keep.

I'm not quite following the code, but my opinion is we should keep neither since it's ambiguous.

[longfangsong]

Both mod::sub4::X and mod::sub4::inner::X are from_glob for module mod, and we have no way to decide which one to keep.

I'm not quite following the code, but my opinion is we should keep neither since it's ambiguous.

However rustc think mod::X in this situation resolves to mod::sub4::inner::X, which the stable version of rustdoc can handle correctly. IMO we should prevent this regression.

[jyn514]

Ok, I think I misunderstood, this is a different case from before. inner::X is defined in inner, not through a glob, so it should take precedence over sub4::X.

I think the issue is that from_glob should be per-module, not per-item (or at least, it should be per-reexport). I don't have ideas off the top of my head for how that should work.

[bors]

☔ The latest upstream changes (presumably #84525) made this pull request unmergeable. Please resolve the merge conflicts.

[JohnCSimon]

Ping from triage:
@longfangsong can you please address the merge conflicts, address the build failure, and post the status of this PR?

thank you

[longfangsong]

Close this, because this solution can't cover all cases and will cause regression and I have no idea how to fix it easily.
I'll send another pr if I have greater idea.
Thank everybody's concern!