From 0deba5546d5bd69719a8be3d8ec59741d32a9f07 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Mon, 3 May 2021 09:21:52 -0400 Subject: [PATCH 1/2] manually crafted revert of PR #80653, to address issue #82465. (update: placated tidy) (update: rebased post PR #84707 ) merge me --- src/librustdoc/html/render/context.rs | 9 +- src/librustdoc/html/render/mod.rs | 26 ++---- src/librustdoc/passes/collect_trait_impls.rs | 97 +++++++------------- src/test/rustdoc-ui/deref-recursive-cycle.rs | 17 ---- src/test/rustdoc/deref-recursive-pathbuf.rs | 24 ----- src/test/rustdoc/deref-recursive.rs | 40 -------- src/test/rustdoc/deref-typedef.rs | 4 +- 7 files changed, 45 insertions(+), 172 deletions(-) delete mode 100644 src/test/rustdoc-ui/deref-recursive-cycle.rs delete mode 100644 src/test/rustdoc/deref-recursive-pathbuf.rs delete mode 100644 src/test/rustdoc/deref-recursive.rs diff --git a/src/librustdoc/html/render/context.rs b/src/librustdoc/html/render/context.rs index d80b2db00ac..1898f5feed2 100644 --- a/src/librustdoc/html/render/context.rs +++ b/src/librustdoc/html/render/context.rs @@ -6,7 +6,7 @@ use std::sync::mpsc::{channel, Receiver}; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; -use rustc_hir::def_id::{DefId, LOCAL_CRATE}; +use rustc_hir::def_id::LOCAL_CRATE; use rustc_middle::ty::TyCtxt; use rustc_session::Session; use rustc_span::edition::Edition; @@ -51,9 +51,6 @@ pub(super) render_redirect_pages: bool, /// The map used to ensure all generated 'id=' attributes are unique. pub(super) id_map: RefCell, - /// Tracks section IDs for `Deref` targets so they match in both the main - /// body and the sidebar. - pub(super) deref_id_map: RefCell>, /// Shared mutable state. /// /// Issue for improving the situation: [#82381][] @@ -74,7 +71,7 @@ // `Context` is cloned a lot, so we don't want the size to grow unexpectedly. #[cfg(target_arch = "x86_64")] -rustc_data_structures::static_assert_size!(Context<'_>, 152); +rustc_data_structures::static_assert_size!(Context<'_>, 112); /// Shared mutable state used in [`Context`] and elsewhere. crate struct SharedContext<'tcx> { @@ -486,7 +483,6 @@ fn init( dst, render_redirect_pages: false, id_map: RefCell::new(id_map), - deref_id_map: RefCell::new(FxHashMap::default()), shared: Rc::new(scx), cache: Rc::new(cache), }; @@ -504,7 +500,6 @@ fn make_child_renderer(&self) -> Self { dst: self.dst.clone(), render_redirect_pages: self.render_redirect_pages, id_map: RefCell::new(IdMap::new()), - deref_id_map: RefCell::new(FxHashMap::default()), shared: Rc::clone(&self.shared), cache: Rc::clone(&self.cache), } diff --git a/src/librustdoc/html/render/mod.rs b/src/librustdoc/html/render/mod.rs index 4b6faefc2fb..b808c6e04d8 100644 --- a/src/librustdoc/html/render/mod.rs +++ b/src/librustdoc/html/render/mod.rs @@ -1045,17 +1045,12 @@ fn render_assoc_items( RenderMode::Normal } AssocItemRender::DerefFor { trait_, type_, deref_mut_ } => { - let id = - cx.derive_id(small_url_encode(format!("deref-methods-{:#}", type_.print(cx)))); - debug!("Adding {} to deref id map", type_.print(cx)); - cx.deref_id_map.borrow_mut().insert(type_.def_id_full(cache).unwrap(), id.clone()); write!( w, - "

\ + "

\ Methods from {trait_}<Target = {type_}>\ - \ + \

", - id = id, trait_ = trait_.print(cx), type_ = type_.print(cx), ); @@ -1080,6 +1075,9 @@ fn render_assoc_items( ); } } + if let AssocItemRender::DerefFor { .. } = what { + return; + } if !traits.is_empty() { let deref_impl = traits .iter() @@ -1090,13 +1088,6 @@ fn render_assoc_items( .any(|t| t.inner_impl().trait_.def_id_full(cache) == cx.cache.deref_mut_trait_did); render_deref_methods(w, cx, impl_, containing_item, has_deref_mut); } - - // If we were already one level into rendering deref methods, we don't want to render - // anything after recursing into any further deref methods above. - if let AssocItemRender::DerefFor { .. } = what { - return; - } - let (synthetic, concrete): (Vec<&&Impl>, Vec<&&Impl>) = traits.iter().partition(|t| t.inner_impl().synthetic); let (blanket_impl, concrete): (Vec<&&Impl>, _) = @@ -2009,14 +2000,9 @@ fn sidebar_deref_methods(cx: &Context<'_>, out: &mut Buffer, impl_: &Impl, v: &V .flat_map(|i| get_methods(i.inner_impl(), true, &mut used_links, deref_mut, c)) .collect::>(); if !ret.is_empty() { - let deref_id_map = cx.deref_id_map.borrow(); - let id = deref_id_map - .get(&real_target.def_id_full(c).unwrap()) - .expect("Deref section without derived id"); write!( out, - "Methods from {}<Target={}>", - id, + "Methods from {}<Target={}>", Escape(&format!("{:#}", impl_.inner_impl().trait_.as_ref().unwrap().print(cx))), Escape(&format!("{:#}", real_target.print(cx))), ); diff --git a/src/librustdoc/passes/collect_trait_impls.rs b/src/librustdoc/passes/collect_trait_impls.rs index 4e621e100e3..6d7c45f6eea 100644 --- a/src/librustdoc/passes/collect_trait_impls.rs +++ b/src/librustdoc/passes/collect_trait_impls.rs @@ -3,8 +3,7 @@ use crate::core::DocContext; use crate::fold::DocFolder; -use rustc_data_structures::fx::{FxHashMap, FxHashSet}; -use rustc_hir::def_id::DefId; +use rustc_data_structures::fx::FxHashSet; use rustc_middle::ty::DefIdTree; use rustc_span::symbol::sym; @@ -53,6 +52,39 @@ } } + let mut cleaner = BadImplStripper { prims, items: crate_items }; + + // scan through included items ahead of time to splice in Deref targets to the "valid" sets + for it in &new_items { + if let ImplItem(Impl { ref for_, ref trait_, ref items, .. }) = *it.kind { + if cleaner.keep_impl(for_) && trait_.def_id() == cx.tcx.lang_items().deref_trait() { + let target = items + .iter() + .find_map(|item| match *item.kind { + TypedefItem(ref t, true) => Some(&t.type_), + _ => None, + }) + .expect("Deref impl without Target type"); + + if let Some(prim) = target.primitive_type() { + cleaner.prims.insert(prim); + } else if let Some(did) = target.def_id() { + cleaner.items.insert(did.into()); + } + } + } + } + + new_items.retain(|it| { + if let ImplItem(Impl { ref for_, ref trait_, ref blanket_impl, .. }) = *it.kind { + cleaner.keep_impl(for_) + || trait_.as_ref().map_or(false, |t| cleaner.keep_impl(t)) + || blanket_impl.is_some() + } else { + true + } + }); + // `tcx.crates()` doesn't include the local crate, and `tcx.all_trait_implementations` // doesn't work with it anyway, so pull them from the HIR map instead let mut extra_attrs = Vec::new(); @@ -84,53 +116,6 @@ } } - let mut cleaner = BadImplStripper { prims, items: crate_items }; - - let mut type_did_to_deref_target: FxHashMap = FxHashMap::default(); - // Gather all type to `Deref` target edges. - for it in &new_items { - if let ImplItem(Impl { ref for_, ref trait_, ref items, .. }) = *it.kind { - if trait_.def_id() == cx.tcx.lang_items().deref_trait() { - let target = items.iter().find_map(|item| match *item.kind { - TypedefItem(ref t, true) => Some(&t.type_), - _ => None, - }); - if let (Some(for_did), Some(target)) = (for_.def_id(), target) { - type_did_to_deref_target.insert(for_did, target); - } - } - } - } - // Follow all `Deref` targets of included items and recursively add them as valid - fn add_deref_target( - map: &FxHashMap, - cleaner: &mut BadImplStripper, - type_did: &DefId, - ) { - if let Some(target) = map.get(type_did) { - debug!("add_deref_target: type {:?}, target {:?}", type_did, target); - if let Some(target_prim) = target.primitive_type() { - cleaner.prims.insert(target_prim); - } else if let Some(target_did) = target.def_id() { - // `impl Deref for S` - if target_did == *type_did { - // Avoid infinite cycles - return; - } - cleaner.items.insert(target_did.into()); - add_deref_target(map, cleaner, &target_did.into()); - } - } - } - for type_did in type_did_to_deref_target.keys() { - // Since only the `DefId` portion of the `Type` instances is known to be same for both the - // `Deref` target type and the impl for type positions, this map of types is keyed by - // `DefId` and for convenience uses a special cleaner that accepts `DefId`s directly. - if cleaner.keep_impl_with_def_id(FakeDefId::Real(*type_did)) { - add_deref_target(&type_did_to_deref_target, &mut cleaner, type_did); - } - } - let items = if let ModuleItem(Module { ref mut items, .. }) = *krate.module.kind { items } else { @@ -138,19 +123,7 @@ fn add_deref_target( }; items.extend(synth_impls); - for it in new_items.drain(..) { - if let ImplItem(Impl { ref for_, ref trait_, ref blanket_impl, .. }) = *it.kind { - if !(cleaner.keep_impl(for_) - || trait_.as_ref().map_or(false, |t| cleaner.keep_impl(t)) - || blanket_impl.is_some()) - { - continue; - } - } - - items.push(it); - } - + items.extend(new_items); krate } diff --git a/src/test/rustdoc-ui/deref-recursive-cycle.rs b/src/test/rustdoc-ui/deref-recursive-cycle.rs deleted file mode 100644 index 4cb518cbbbd..00000000000 --- a/src/test/rustdoc-ui/deref-recursive-cycle.rs +++ /dev/null @@ -1,17 +0,0 @@ -// check-pass -// #26207: Ensure `Deref` cycles are properly handled without errors. - -#[derive(Copy, Clone)] -struct S; - -impl std::ops::Deref for S { - type Target = S; - - fn deref(&self) -> &S { - self - } -} - -fn main() { - let s: S = *******S; -} diff --git a/src/test/rustdoc/deref-recursive-pathbuf.rs b/src/test/rustdoc/deref-recursive-pathbuf.rs deleted file mode 100644 index 459a30060c6..00000000000 --- a/src/test/rustdoc/deref-recursive-pathbuf.rs +++ /dev/null @@ -1,24 +0,0 @@ -// #26207: Show all methods reachable via Deref impls, recursing through multiple dereferencing -// levels and across multiple crates. - -// @has 'foo/struct.Foo.html' -// @has '-' '//*[@id="deref-methods-PathBuf"]' 'Methods from Deref' -// @has '-' '//*[@class="impl-items"]//*[@id="method.as_path"]' 'pub fn as_path(&self)' -// @has '-' '//*[@id="deref-methods-Path"]' 'Methods from Deref' -// @has '-' '//*[@class="impl-items"]//*[@id="method.exists"]' 'pub fn exists(&self)' -// @has '-' '//*[@class="sidebar-title"][@href="#deref-methods-PathBuf"]' 'Methods from Deref' -// @has '-' '//*[@class="sidebar-links"]/a[@href="#method.as_path"]' 'as_path' -// @has '-' '//*[@class="sidebar-title"][@href="#deref-methods-Path"]' 'Methods from Deref' -// @has '-' '//*[@class="sidebar-links"]/a[@href="#method.exists"]' 'exists' - -#![crate_name = "foo"] - -use std::ops::Deref; -use std::path::PathBuf; - -pub struct Foo(PathBuf); - -impl Deref for Foo { - type Target = PathBuf; - fn deref(&self) -> &PathBuf { &self.0 } -} diff --git a/src/test/rustdoc/deref-recursive.rs b/src/test/rustdoc/deref-recursive.rs deleted file mode 100644 index b96b5397ad7..00000000000 --- a/src/test/rustdoc/deref-recursive.rs +++ /dev/null @@ -1,40 +0,0 @@ -// #26207: Show all methods reachable via Deref impls, recursing through multiple dereferencing -// levels if needed. - -// @has 'foo/struct.Foo.html' -// @has '-' '//*[@id="deref-methods-Bar"]' 'Methods from Deref' -// @has '-' '//*[@class="impl-items"]//*[@id="method.bar"]' 'pub fn bar(&self)' -// @has '-' '//*[@id="deref-methods-Baz"]' 'Methods from Deref' -// @has '-' '//*[@class="impl-items"]//*[@id="method.baz"]' 'pub fn baz(&self)' -// @has '-' '//*[@class="sidebar-title"][@href="#deref-methods-Bar"]' 'Methods from Deref' -// @has '-' '//*[@class="sidebar-links"]/a[@href="#method.bar"]' 'bar' -// @has '-' '//*[@class="sidebar-title"][@href="#deref-methods-Baz"]' 'Methods from Deref' -// @has '-' '//*[@class="sidebar-links"]/a[@href="#method.baz"]' 'baz' - -#![crate_name = "foo"] - -use std::ops::Deref; - -pub struct Foo(Bar); -pub struct Bar(Baz); -pub struct Baz; - -impl Deref for Foo { - type Target = Bar; - fn deref(&self) -> &Bar { &self.0 } -} - -impl Deref for Bar { - type Target = Baz; - fn deref(&self) -> &Baz { &self.0 } -} - -impl Bar { - /// This appears under `Foo` methods - pub fn bar(&self) {} -} - -impl Baz { - /// This should also appear in `Foo` methods when recursing - pub fn baz(&self) {} -} diff --git a/src/test/rustdoc/deref-typedef.rs b/src/test/rustdoc/deref-typedef.rs index 47009559e6f..3fc48b46d74 100644 --- a/src/test/rustdoc/deref-typedef.rs +++ b/src/test/rustdoc/deref-typedef.rs @@ -1,12 +1,12 @@ #![crate_name = "foo"] // @has 'foo/struct.Bar.html' -// @has '-' '//*[@id="deref-methods-FooJ"]' 'Methods from Deref' +// @has '-' '//*[@id="deref-methods"]' 'Methods from Deref' // @has '-' '//*[@class="impl-items"]//*[@id="method.foo_a"]' 'pub fn foo_a(&self)' // @has '-' '//*[@class="impl-items"]//*[@id="method.foo_b"]' 'pub fn foo_b(&self)' // @has '-' '//*[@class="impl-items"]//*[@id="method.foo_c"]' 'pub fn foo_c(&self)' // @has '-' '//*[@class="impl-items"]//*[@id="method.foo_j"]' 'pub fn foo_j(&self)' -// @has '-' '//*[@class="sidebar-title"][@href="#deref-methods-FooJ"]' 'Methods from Deref' +// @has '-' '//*[@class="sidebar-title"][@href="#deref-methods"]' 'Methods from Deref' // @has '-' '//*[@class="sidebar-links"]/a[@href="#method.foo_a"]' 'foo_a' // @has '-' '//*[@class="sidebar-links"]/a[@href="#method.foo_b"]' 'foo_b' // @has '-' '//*[@class="sidebar-links"]/a[@href="#method.foo_c"]' 'foo_c' From b894f75594fbf44bf6a4d504e604c4b2762a2a69 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Mon, 3 May 2021 10:06:57 -0400 Subject: [PATCH 2/2] regression test for issue 82465. --- .../issue-82465-asref-for-and-of-local.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 src/test/rustdoc/issue-82465-asref-for-and-of-local.rs diff --git a/src/test/rustdoc/issue-82465-asref-for-and-of-local.rs b/src/test/rustdoc/issue-82465-asref-for-and-of-local.rs new file mode 100644 index 00000000000..618ac20ac48 --- /dev/null +++ b/src/test/rustdoc/issue-82465-asref-for-and-of-local.rs @@ -0,0 +1,16 @@ +use std::convert::AsRef; +pub struct Local; + +// @has issue_82465_asref_for_and_of_local/struct.Local.html '//code' 'impl AsRef for Local' +impl AsRef for Local { + fn as_ref(&self) -> &str { + todo!() + } +} + +// @has - '//code' 'impl AsRef for str' +impl AsRef for str { + fn as_ref(&self) -> &Local { + todo!() + } +}