Fix `doc_cfg` not working as expected on trait impls by GuillaumeGomez · Pull Request #153964 · rust-lang/rust

GuillaumeGomez · 2026-03-16T16:27:54Z

I spent waaaaay too much time on this fix. So the current issue is that rustdoc gets its items in two passes:

All items
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.

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

GuillaumeGomez · 2026-03-16T19:45:38Z

Not great that ImplItem and PlaceholderImplItem need to be handled in rustdoc lints. :-/

GuillaumeGomez · 2026-03-16T21:02:38Z

And CI passed. \o/

lolbinarycat

Code looks good to me but I still need to do a more in-depth investigation of the logic. For now, here's some nits and clarifying questions.

View changes since this review

lolbinarycat · 2026-03-17T05:30:06Z

src/librustdoc/clean/mod.rs

    impl_: &hir::Impl<'tcx>,
    def_id: LocalDefId,
    cx: &mut DocContext<'tcx>,
+    // If `renamed` is some, then this is an inlined impl and it will be handled later on in the


nit: ambiguous pronoun, this would be less ambiguous if "this" was replaced with an actual name.

src/librustdoc/json/conversions.rs

src/librustdoc/passes/calculate_doc_coverage.rs

lolbinarycat · 2026-03-17T05:35:46Z

src/librustdoc/passes/stripper.rs


            clean::ImplItem(..) => {}

+            // They have no use anymore, so always remove them.


nit: great comments explain why code does something.

lolbinarycat · 2026-03-17T05:38:36Z

tests/rustdoc-html/doc-cfg/trait-impls.rs

@@ -0,0 +1,59 @@
+// This test ensures that `doc_cfg` feature is working as expected on trait impls.


just to be sure: we do have a test for non-trait impls, right? we definitely don't want to regress that.

also, do we have a test for non-auto doc(cfg(...)) on trait items?

Doesn't hurt to add one in the same test I guess. Gonna add a test for non-auto doc(cfg(...)) too.

lolbinarycat · 2026-03-17T05:44:52Z

src/librustdoc/clean/mod.rs

        match item.kind {
-            ItemKind::Impl(ref impl_) => return clean_impl(impl_, item.owner_id.def_id, cx),
+            ItemKind::Impl(ref impl_) => {
+                return clean_impl(impl_, item.owner_id.def_id, cx, renamed);


what does it mean for an impl to be renamed?

The explanation is on the clean_impl function but I can copy/paste it here too.

GuillaumeGomez · 2026-03-17T11:47:55Z

Added comments and greatly extended the tests.

GuillaumeGomez · 2026-03-17T13:20:53Z

Seems like a flaky failure. Restarted CI.

lolbinarycat

I took a closer look at the actual logic this time and it seems solid and straightforwards, I can't come up with any edge cases that it would cause problems in.

My only issue of immediate relevance is rustc_span::symbol::kw::Impl and renamed being used in a somewhat unintuitive way, not sure what the best way to address that is but it definitely has an impact on code readability. I also found some missing test coverage but that's mostly unrelated so that can be handled in a followup PR if you want.

View changes since this review

lolbinarycat · 2026-03-18T14:46:03Z

src/librustdoc/passes/check_doc_test_visibility.rs

                | clean::ProvidedAssocConstItem(..)
                | clean::ImplAssocConstItem(..)
                | clean::RequiredAssocTypeItem(..)
+                | clean::PlaceholderImplItem


This made me realize that we don't actually have a test for how rustdoc::missing_doc_code_examples interacts with inherent impls (we do have them for trait impls, which is what is relevant to this PR, and what I was initially checking).

here's a search for all the test files using that lint

Gonna send another PR for it then. :)

lolbinarycat · 2026-03-18T14:50:23Z

src/librustdoc/passes/calculate_doc_coverage.rs

+            clean::PlaceholderImplItem => {
+                // The "real" impl items are handled below.
+                return;
+            }


note: we do have test coverage for doc coverage involving both types of impl. good.

lolbinarycat · 2026-03-18T14:54:51Z

src/librustdoc/passes/propagate_doc_cfg.rs

+        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 {


nit: if let with no new bindings introduced. might be more intuitive if these were written with matches!? but this pattern is used a bit around rustdoc, so if you'd rather keep it how it is that's fine.

I tend to prefer inline code over macros if I can, and I have to admit I find this let binding funny. Brain fart of the day I guess. ^^'

lolbinarycat · 2026-03-18T15:03:14Z

src/librustdoc/visit_ast.rs

+                self.add_to_current_mod(
+                    item,
+                    if impl_.of_trait.is_none() {
+                        None
+                    } else {
+                        Some(rustc_span::symbol::kw::Impl)
+                    },
+                    None,
+                );


This seems like a little bit of a magic value, I'd at least like if this special value was documented somewhere, perhaps on the doc comment of Module::items. I understand the use of a sentinel here because the alternative involves adding a new field to the tuple in the items Vec, or refactoring that into a more principled enum, which I'm sure would be quite the undertaking.

Actually, this isn't quite a sentinel value, is it, because Impl items in this context don't actually care about the inner value of renamed, just if it is None, correct?

Yeah. I'll add a comment to make that clear though because it really comes out of nowhere.

GuillaumeGomez · 2026-03-18T15:49:24Z

I added code comments to explain that the symbol is used as a sentinel value.

fmease · 2026-03-18T17:04:32Z

src/librustdoc/visit_ast.rs

+                    if impl_.of_trait.is_none() {
+                        None
+                    } else {
+                        Some(rustc_span::symbol::kw::Impl)
+                    },


Would replacing parameter type Option<Symbol> with Option<BikeshedTy> be possible / much of a hassle where BikeshedTy is a new type that could be defined like enum BikeshedTy { BikeshedName(Symbol), BikeshedAnon }? I haven't read the rest of the diff, lol

Context: this argument is what an inlined item is renamed into. So no, I don't want to rename it. 😝

@lolbinarycat

…amples-improvement, r=lolbinarycat Don't emit rustdoc `missing_doc_code_examples` lint on impl items @lolbinarycat realized in [this comment](rust-lang#153964 (comment)) that we weren't testing some cases for the `missing_doc_code_examples` lint. Turns out that it was not handling this case well. =D So in short: `missing_doc_code_examples` lint should not be emitted on impl items and this PR fixes that. r? @lolbinarycat

@lolbinarycat

…amples-improvement, r=lolbinarycat Don't emit rustdoc `missing_doc_code_examples` lint on impl items @lolbinarycat realized in [this comment](rust-lang#153964 (comment)) that we weren't testing some cases for the `missing_doc_code_examples` lint. Turns out that it was not handling this case well. =D So in short: `missing_doc_code_examples` lint should not be emitted on impl items and this PR fixes that. r? @lolbinarycat

@lolbinarycat

…amples-improvement, r=lolbinarycat Don't emit rustdoc `missing_doc_code_examples` lint on impl items @lolbinarycat realized in [this comment](rust-lang#153964 (comment)) that we weren't testing some cases for the `missing_doc_code_examples` lint. Turns out that it was not handling this case well. =D So in short: `missing_doc_code_examples` lint should not be emitted on impl items and this PR fixes that. r? @lolbinarycat

rustbot · 2026-03-20T16:49:57Z

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

rustbot · 2026-03-20T16:49:58Z

⚠️ Warning ⚠️

There are issue links (such as #123) in the commit messages of the following commits.
Please move them to the PR description, to avoid spamming the issues with references to the commit, and so this bot can automatically canonicalize them to avoid issues with subtree.
- 2d4b185

GuillaumeGomez · 2026-03-20T16:49:59Z

Fixed merge conflict.

rustbot assigned lolbinarycat Mar 16, 2026

This comment has been minimized.

Sign in to view

GuillaumeGomez changed the title ~~Fix trait impl doc cfg~~ Fix doc_cfg not working as expected on trait impls Mar 16, 2026

GuillaumeGomez mentioned this pull request Mar 16, 2026

rustdoc reports wrong config conditions for impl LowerExp for integers #153655

Open

This comment has been minimized.

Sign in to view

lolbinarycat reviewed Mar 17, 2026

View reviewed changes

GuillaumeGomez force-pushed the fix-trait-impl-doc-cfg branch from ce71ac8 to 2dd7677 Compare March 17, 2026 11:47

This comment has been minimized.

Sign in to view

Darksonn mentioned this pull request Mar 18, 2026

chore: Do not show "Available on non-loom only." doc label tokio-rs/tokio#7977

Merged

lolbinarycat reviewed Mar 18, 2026

View reviewed changes

GuillaumeGomez force-pushed the fix-trait-impl-doc-cfg branch from 2dd7677 to 3f0f7d7 Compare March 18, 2026 15:49

GuillaumeGomez mentioned this pull request Mar 18, 2026

Don't emit rustdoc missing_doc_code_examples lint on impl items #154048

Merged

fmease reviewed Mar 18, 2026

View reviewed changes

This comment has been minimized.

Sign in to view

GuillaumeGomez added 3 commits March 20, 2026 17:49

Fix doc_cfg not working as expected on trait impls

65a1e09

Add regression test for rust-lang#153655

2d4b185

Correctly handle rustdoc PlaceholderImplItem in lints

244af0f

Add more comments to explain code

2752dc5

GuillaumeGomez force-pushed the fix-trait-impl-doc-cfg branch from 3f0f7d7 to 2752dc5 Compare March 20, 2026 16:49


		clean::ImplItem(..) => {}

		// They have no use anymore, so always remove them.

		@@ -0,0 +1,59 @@
		// This test ensures that `doc_cfg` feature is working as expected on trait impls.

Uh oh!

Conversation

GuillaumeGomez commented Mar 16, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

GuillaumeGomez commented Mar 16, 2026

Uh oh!

GuillaumeGomez commented Mar 16, 2026

Uh oh!

lolbinarycat left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

GuillaumeGomez commented Mar 17, 2026

Uh oh!

This comment has been minimized.

GuillaumeGomez commented Mar 17, 2026

Uh oh!

lolbinarycat left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez commented Mar 18, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

rustbot commented Mar 20, 2026

Uh oh!

rustbot commented Mar 20, 2026

Uh oh!

GuillaumeGomez commented Mar 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lolbinarycat left a comment •

edited by rustbot

Loading

GuillaumeGomez Mar 17, 2026 •

edited

Loading

lolbinarycat left a comment •

edited by rustbot

Loading

GuillaumeGomez Mar 18, 2026 •

edited

Loading

GuillaumeGomez Mar 18, 2026 •

edited

Loading