rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
96.61k stars 12.48k forks source link

rustdoc: Don't hide bugs in `clean_extern_crate` #108987

Closed jyn514 closed 1 year ago

jyn514 commented 1 year ago

unwrap_or(LOCAL_CRATE) here is super shady: https://github.com/rust-lang/rust/blob/d5833423a02e2373c5e3cceb238fb19192cd82f8/src/librustdoc/clean/mod.rs#L2456-L2463 extern_mod_stmt_cnum should always return Some for ExternCrate items; if it doesn't it's a bug and using LOCAL_CRATE is hiding it. We should use unwrap() instead to avoid silently doing the wrong thing.

This has been around since the code was first added in https://github.com/rust-lang/rust/commit/0b5b782e392f0e1d000a7e9fefaad9cbebc0e586#diff-4b76d999d0f2e74f6df3724a36a6b96302b16968a2f7af48d2b4c08048f8f871R321-R322 so I doubt there's any great significant to it.

Teapot4195 commented 1 year ago

@rustbot claim

JacobOgle commented 1 year ago

@Teapot4195 still working on this?

Teapot4195 commented 1 year ago

@Teapot4195 still working on this?

You can pick it up if you'd like, I'm on march break :)

JacobOgle commented 1 year ago

@rustbot claim

JacobOgle commented 1 year ago

@jyn514,

Running the CI tests on x86_64-unknown-linux-gnu I am getting an issue:

thread 'rustc' panicked at 'called 'Option::unwrap()' on a 'None' value', src/librustdoc/clean/mod.rs:2463:67

If we edit let cnum = cx.tcx.extern_mod_stmt_cnum(krate.owner_id.def_id).unwrap_or(LOCAL_CRATE); to let cnum = cx.tcx.extern_mod_stmt_cnum(krate.owner_id.def_id).unwrap();

I would think this should work if extern_mod_stmt_cnum is always returning Some. Am I missing something here? GH Action Log

I see in this err log that it states its a bug that should be reported. Is this the same bug you were hinting could be there covered by LOCAL_CRATE usage?


Apologies, I am a newbie to OSS & Rust in general.

jyn514 commented 1 year ago

yeah ok I was afraid of this

can you add some debug logging to find out what item is represented by that DefId and krate? if it's LOCAL_CRATE then it's ok (although we should add an assertion) but anything else is a bug.

jyn514 commented 1 year ago

I'm going to close this, turns out I was wrong.

diff --cc src/librustdoc/clean/mod.rs
index c992a5388d1,52d693463e4..00000000000
--- a/src/librustdoc/clean/mod.rs
+++ b/src/librustdoc/clean/mod.rs
@@@ -2377,8 -2459,8 +2377,8 @@@ fn clean_extern_crate<'tcx>
      orig_name: Option<Symbol>,
      cx: &mut DocContext<'tcx>,
  ) -> Vec<Item> {
-     // this is the ID of the `extern crate` statement
-     let cnum = cx.tcx.extern_mod_stmt_cnum(krate.owner_id.def_id).unwrap_or(LOCAL_CRATE);
+     // this is the ID of the `extern crate` statement -- should always return CrateNum struct from currently-compiled crate
 -    let cnum = cx.tcx.extern_mod_stmt_cnum(krate.owner_id.def_id).unwrap();
++    let cnum = cx.tcx.extern_mod_stmt_cnum(krate.owner_id.def_id).unwrap_or_else(|| span_bug!(krate.span, "missing extern mod stmt"));
      // this is the ID of the crate itself
      let crate_def_id = cnum.as_def_id();
      let attrs = cx.tcx.hir().attrs(krate.hir_id());
error: internal compiler error: src/librustdoc/clean/mod.rs:2381:85: missing extern mod stmt
   --> library/core/src/lib.rs:258:1
    |
258 | extern crate self as core;
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^

I forgot extern crate also works for the current crate.