Rollup merge of #153964 - GuillaumeGomez:fix-trait-impl-doc-cfg, r=lolbinarycat

Fix `doc_cfg` not working as expected on trait impls

Fixes #153655.

I spent waaaaay too much time on this fix. So the current issue is that rustdoc gets its items in two passes:
1. All items
2. Trait/blanket/auto impls

Because of that, the trait impls are not stored "correctly" in the rustdoc AST, meaning that the `propagate_doc_cfg` pass doesn't work correctly on them. So initially, I tried to "clean" the impls at the same time as the other items. However, it created a monstruous amount of bugs and issues and after two days, I decided to give up on this approach (might be worth fixing that in the future!). You can see what I tried [here](https://github.com/rust-lang/rust/compare/main...GuillaumeGomez:trait-impls-doc_cfg?expand=1).

So instead, since the impls are stored at the end, I create placeholders for impls and in `propagate_doc_cfg`, I store the `cfg` "context" (more clear when reading the code 😛) and re-use it later on when the "real" impl comes up.

r? @lolbinarycat

Author: jhpratt

src/librustdoc/clean/mod.rs

@@ -2776,7 +2776,12 @@ fn clean_maybe_renamed_item<'tcx>(
         // These kinds of item either don't need a `name` or accept a `None` one so we handle them
         // before.
         match item.kind {
-            ItemKind::Impl(ref impl_) => return clean_impl(impl_, item.owner_id.def_id, cx),
+            ItemKind::Impl(ref impl_) => {
+                // If `renamed` is `Some()` for an `impl`, it means it's been inlined because we use
+                // it as a marker to indicate that this is an inlined impl and that we should
+                // generate an impl placeholder and not a "real" impl item.
+                return clean_impl(impl_, item.owner_id.def_id, cx, renamed.is_some());
+            }
             ItemKind::Use(path, kind) => {
                 return clean_use_statement(
                     item,
@@ -2909,10 +2914,27 @@ fn clean_impl<'tcx>(
     impl_: &hir::Impl<'tcx>,
     def_id: LocalDefId,
     cx: &mut DocContext<'tcx>,
+    // If true, this is an inlined impl and it will be handled later on in the code.
+    // In here, we will generate a placeholder for it in order to be able to compute its
+    // `doc_cfg` info.
+    is_inlined: bool,
 ) -> Vec<Item> {
     let tcx = cx.tcx;
     let mut ret = Vec::new();
-    let trait_ = impl_.of_trait.map(|t| clean_trait_ref(&t.trait_ref, cx));
+    let trait_ = match impl_.of_trait {
+        Some(t) => {
+            if is_inlined {
+                return vec![Item::from_def_id_and_parts(
+                    def_id.to_def_id(),
+                    None,
+                    PlaceholderImplItem,
+                    cx,
+                )];
+            }
+            Some(clean_trait_ref(&t.trait_ref, cx))
+        }
+        None => None,
+    };
     let items = impl_
         .items
         .iter()

src/librustdoc/clean/types.rs

@@ -885,6 +885,9 @@ pub(crate) enum ItemKind {
     TraitItem(Box<Trait>),
     TraitAliasItem(TraitAlias),
     ImplItem(Box<Impl>),
+    /// This variant is used only as a placeholder for trait impls in order to correctly compute
+    /// `doc_cfg` as trait impls are added to `clean::Crate` after we went through the whole tree.
+    PlaceholderImplItem,
     /// A required method in a trait declaration meaning it's only a function signature.
     RequiredMethodItem(Box<Function>, Defaultness),
     /// A method in a trait impl or a provided method in a trait declaration.
@@ -964,7 +967,8 @@ impl ItemKind {
             | AssocTypeItem(..)
             | StrippedItem(_)
             | KeywordItem
-            | AttributeItem => [].iter(),
+            | AttributeItem
+            | PlaceholderImplItem => [].iter(),
         }
     }
 }

src/librustdoc/fold.rs

@@ -97,7 +97,8 @@ pub(crate) trait DocFolder: Sized {
             | RequiredAssocTypeItem(..)
             | AssocTypeItem(..)
             | KeywordItem
-            | AttributeItem => kind,
+            | AttributeItem
+            | PlaceholderImplItem => kind,
         }
     }
 

src/librustdoc/formats/cache.rs

@@ -389,6 +389,8 @@ impl DocFolder for CacheBuilder<'_, '_> {
                 // So would rather leave them to an expert,
                 // as at least the list is better than `_ => {}`.
             }
+
+            clean::PlaceholderImplItem => return None,
         }
 
         // Maintain the parent stack.

src/librustdoc/formats/item_type.rs

@@ -122,7 +122,7 @@ impl<'a> From<&'a clean::Item> for ItemType {
             clean::StaticItem(..) => ItemType::Static,
             clean::ConstantItem(..) => ItemType::Constant,
             clean::TraitItem(..) => ItemType::Trait,
-            clean::ImplItem(..) => ItemType::Impl,
+            clean::ImplItem(..) | clean::PlaceholderImplItem => ItemType::Impl,
             clean::RequiredMethodItem(..) => ItemType::TyMethod,
             clean::MethodItem(..) => ItemType::Method,
             clean::StructFieldItem(..) => ItemType::StructField,

src/librustdoc/json/conversions.rs

@@ -353,6 +353,8 @@ fn from_clean_item(item: &clean::Item, renderer: &JsonRenderer<'_>) -> ItemEnum
             name: name.as_ref().unwrap().to_string(),
             rename: src.map(|x| x.to_string()),
         },
+        // All placeholder impl items should have been removed in the stripper passes.
+        PlaceholderImplItem => unreachable!(),
     }
 }
 

src/librustdoc/passes/calculate_doc_coverage.rs

@@ -203,6 +203,10 @@ impl DocVisitor<'_> for CoverageCalculator<'_, '_> {
                 // don't count items in stripped modules
                 return;
             }
+            clean::PlaceholderImplItem => {
+                // The "real" impl items are handled below.
+                return;
+            }
             // docs on `use` and `extern crate` statements are not displayed, so they're not
             // worth counting
             clean::ImportItem(..) | clean::ExternCrateItem { .. } => {}

src/librustdoc/passes/check_doc_test_visibility.rs

@@ -80,6 +80,7 @@ pub(crate) fn should_have_doc_example(cx: &DocContext<'_>, item: &clean::Item) -
                 | clean::ImplAssocConstItem(..)
                 | clean::RequiredAssocTypeItem(..)
                 | clean::ImplItem(_)
+                | clean::PlaceholderImplItem
         )
     {
         return false;

src/librustdoc/passes/propagate_doc_cfg.rs

@@ -1,10 +1,11 @@
 //! Propagates [`#[doc(cfg(...))]`](https://github.com/rust-lang/rust/issues/43781) to child items.
 
+use rustc_data_structures::fx::FxHashMap;
 use rustc_hir::Attribute;
 use rustc_hir::attrs::{AttributeKind, DocAttribute};
 
 use crate::clean::inline::{load_attrs, merge_attrs};
-use crate::clean::{CfgInfo, Crate, Item, ItemKind};
+use crate::clean::{CfgInfo, Crate, Item, ItemId, ItemKind};
 use crate::core::DocContext;
 use crate::fold::DocFolder;
 use crate::passes::Pass;
@@ -17,7 +18,8 @@ pub(crate) const PROPAGATE_DOC_CFG: Pass = Pass {
 
 pub(crate) fn propagate_doc_cfg(cr: Crate, cx: &mut DocContext<'_>) -> Crate {
     if cx.tcx.features().doc_cfg() {
-        CfgPropagator { cx, cfg_info: CfgInfo::default() }.fold_crate(cr)
+        CfgPropagator { cx, cfg_info: CfgInfo::default(), impl_cfg_info: FxHashMap::default() }
+            .fold_crate(cr)
     } else {
         cr
     }
@@ -26,6 +28,10 @@ pub(crate) fn propagate_doc_cfg(cr: Crate, cx: &mut DocContext<'_>) -> Crate {
 struct CfgPropagator<'a, 'tcx> {
     cx: &'a mut DocContext<'tcx>,
     cfg_info: CfgInfo,
+
+    /// To ensure the `doc_cfg` feature works with how `rustdoc` handles impls, we need to store
+    /// the `cfg` info of `impl`s placeholder to use them later on the "real" impl item.
+    impl_cfg_info: FxHashMap<ItemId, CfgInfo>,
 }
 
 /// This function goes through the attributes list (`new_attrs`) and extract the `cfg` tokens from
@@ -78,7 +84,22 @@ impl DocFolder for CfgPropagator<'_, '_> {
     fn fold_item(&mut self, mut item: Item) -> Option<Item> {
         let old_cfg_info = self.cfg_info.clone();
 
-        self.merge_with_parent_attributes(&mut item);
+        // If we have an impl, we check if it has an associated `cfg` "context", and if so we will
+        // use that context instead of the actual (wrong) one.
+        if let ItemKind::ImplItem(_) = item.kind
+            && let Some(cfg_info) = self.impl_cfg_info.remove(&item.item_id)
+        {
+            self.cfg_info = cfg_info;
+        }
+
+        if let ItemKind::PlaceholderImplItem = item.kind {
+            // If we have a placeholder impl, we store the current `cfg` "context" to be used
+            // on the actual impl later on (the impls are generated after we go through the whole
+            // AST so they're stored in the `krate` object at the end).
+            self.impl_cfg_info.insert(item.item_id, self.cfg_info.clone());
+        } else {
+            self.merge_with_parent_attributes(&mut item);
+        }
 
         let result = self.fold_item_recur(item);
         self.cfg_info = old_cfg_info;

src/librustdoc/passes/propagate_stability.rs

@@ -107,7 +107,8 @@ impl DocFolder for StabilityPropagator<'_, '_> {
                     | ItemKind::AssocTypeItem(..)
                     | ItemKind::PrimitiveItem(..)
                     | ItemKind::KeywordItem
-                    | ItemKind::AttributeItem => own_stability,
+                    | ItemKind::AttributeItem
+                    | ItemKind::PlaceholderImplItem => own_stability,
 
                     ItemKind::StrippedItem(..) => unreachable!(),
                 }