From ba13225ba1e3e94b5656181926cd38750eed1503 Mon Sep 17 00:00:00 2001 From: Camelid Date: Sat, 8 May 2021 15:03:15 -0700 Subject: [PATCH 1/2] Remove `FakeDefId::expect_local()` This function returned a fake `DefIndex`, with no indication that it was fake, when it was provided with a `FakeDefId::Fake`. Every use of the function uses the returned `DefIndex` in a call to `tcx.local_def_id_to_hir_id()`, which I'm pretty sure would panic if it were given a fake `DefIndex`. I removed the function and replaced all calls to it with a call to `expect_real()` followed by `DefId::expect_local()` (that's a function on the *real* `DefId`). --- src/librustdoc/clean/types.rs | 18 +----------------- .../passes/calculate_doc_coverage.rs | 8 +++++++- .../passes/collect_intra_doc_links.rs | 6 +++++- src/librustdoc/passes/doc_test_lints.rs | 4 +++- 4 files changed, 16 insertions(+), 20 deletions(-) diff --git a/src/librustdoc/clean/types.rs b/src/librustdoc/clean/types.rs index 9861e838e33f1..550df203a9fee 100644 --- a/src/librustdoc/clean/types.rs +++ b/src/librustdoc/clean/types.rs @@ -18,7 +18,7 @@ use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_data_structures::thin_vec::ThinVec; use rustc_hir as hir; use rustc_hir::def::{CtorKind, DefKind, Res}; -use rustc_hir::def_id::{CrateNum, DefId, DefIndex, LocalDefId, CRATE_DEF_INDEX, LOCAL_CRATE}; +use rustc_hir::def_id::{CrateNum, DefId, DefIndex, CRATE_DEF_INDEX, LOCAL_CRATE}; use rustc_hir::lang_items::LangItem; use rustc_hir::{BodyId, Mutability}; use rustc_index::vec::IndexVec; @@ -85,22 +85,6 @@ impl FakeDefId { } } - #[inline] - crate fn as_local(self) -> Option { - match self { - FakeDefId::Real(id) => id.as_local(), - FakeDefId::Fake(idx, krate) => { - (krate == LOCAL_CRATE).then(|| LocalDefId { local_def_index: idx }) - } - } - } - - #[inline] - crate fn expect_local(self) -> LocalDefId { - self.as_local() - .unwrap_or_else(|| panic!("FakeDefId::expect_local: `{:?}` isn't local", self)) - } - #[inline] crate fn expect_real(self) -> rustc_hir::def_id::DefId { self.as_real().unwrap_or_else(|| panic!("FakeDefId::expect_real: `{:?}` isn't real", self)) diff --git a/src/librustdoc/passes/calculate_doc_coverage.rs b/src/librustdoc/passes/calculate_doc_coverage.rs index c8b82fb1563df..e65fcf057f15c 100644 --- a/src/librustdoc/passes/calculate_doc_coverage.rs +++ b/src/librustdoc/passes/calculate_doc_coverage.rs @@ -213,7 +213,13 @@ impl<'a, 'b> fold::DocFolder for CoverageCalculator<'a, 'b> { let filename = i.span(self.ctx.tcx).filename(self.ctx.sess()); let has_doc_example = tests.found_tests != 0; - let hir_id = self.ctx.tcx.hir().local_def_id_to_hir_id(i.def_id.expect_local()); + // The `expect_real()` should be okay because `local_def_id_to_hir_id` + // would presumably panic if a fake `DefIndex` were passed. + let hir_id = self + .ctx + .tcx + .hir() + .local_def_id_to_hir_id(i.def_id.expect_real().expect_local()); let (level, source) = self.ctx.tcx.lint_level_at_node(MISSING_DOCS, hir_id); // `missing_docs` is allow-by-default, so don't treat this as ignoring the item // unless the user had an explicit `allow` diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 4317b9747353d..8838dc57d5a05 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -1207,7 +1207,11 @@ impl LinkCollector<'_, '_> { // item can be non-local e.g. when using #[doc(primitive = "pointer")] if let Some((src_id, dst_id)) = id .as_local() - .and_then(|dst_id| item.def_id.as_local().map(|src_id| (src_id, dst_id))) + // The `expect_real()` should be okay because `local_def_id_to_hir_id` + // would presumably panic if a fake `DefIndex` were passed. + .and_then(|dst_id| { + item.def_id.expect_real().as_local().map(|src_id| (src_id, dst_id)) + }) { use rustc_hir::def_id::LOCAL_CRATE; diff --git a/src/librustdoc/passes/doc_test_lints.rs b/src/librustdoc/passes/doc_test_lints.rs index 7362cfcb71b7e..ba698474f1629 100644 --- a/src/librustdoc/passes/doc_test_lints.rs +++ b/src/librustdoc/passes/doc_test_lints.rs @@ -71,7 +71,9 @@ crate fn should_have_doc_example(cx: &DocContext<'_>, item: &clean::Item) -> boo { return false; } - let hir_id = cx.tcx.hir().local_def_id_to_hir_id(item.def_id.expect_local()); + // The `expect_real()` should be okay because `local_def_id_to_hir_id` + // would presumably panic if a fake `DefIndex` were passed. + let hir_id = cx.tcx.hir().local_def_id_to_hir_id(item.def_id.expect_real().expect_local()); if cx.tcx.hir().attrs(hir_id).lists(sym::doc).has_word(sym::hidden) || inherits_doc_hidden(cx.tcx, hir_id) { From 4b7c8b0b53c2bea54e9cd029c68ebdc6f668b1b8 Mon Sep 17 00:00:00 2001 From: Camelid Date: Sat, 8 May 2021 15:14:21 -0700 Subject: [PATCH 2/2] Add `#[track_caller]` to `FakeDefId::expect_real()` Now, in the case that the function is not inlined, the panic location will be the caller's location, which is more helpful since the panic is not `expect_real()`'s fault. --- src/librustdoc/clean/types.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/librustdoc/clean/types.rs b/src/librustdoc/clean/types.rs index 550df203a9fee..33aa42b137a23 100644 --- a/src/librustdoc/clean/types.rs +++ b/src/librustdoc/clean/types.rs @@ -86,6 +86,7 @@ impl FakeDefId { } #[inline] + #[track_caller] crate fn expect_real(self) -> rustc_hir::def_id::DefId { self.as_real().unwrap_or_else(|| panic!("FakeDefId::expect_real: `{:?}` isn't real", self)) }