Skip to content

Implement RFC 3631: add rustdoc doc_cfg features #138907

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Implement RFC 3631: add rustdoc doc_cfg features #138907

wants to merge 11 commits into from
+961 −366

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Mar 24, 2025 •
edited
Loading

Implementation of rust-lang/rfcs#3631.

This implementation actually resulted in a lot of simplifications:

  • All cfg computation is now done in one place: propagate_doc_cfg.rs. Because (trait) impls are not retrieved at the same time as the other items, we cannot perform this computation in the clean process, it needs to be after.
  • Because there is cfg inheritance, we can keep track of them in one place (in propagate_doc_cfg.rs), meaning we don't need to copy an item's attributes to its children anymore. Only exception: impl items. For them we clone only cfg attributes.
  • propagate_doc_cfg.rs is also now much simpler, much less need to keep track of parents, since everything we need is handled by the new CfgInfo type.
  • I also suspect that Cfg::simplify_with could either be removed or at least used directly into propagate_doc_cfg.rs when we compute cfgs. Considering how big the PR already is, I'll do it in a follow-up.

I didn't remove the doc_cfg* features in this PR because some dependencies used in rustc (like stdarch) are using it, so we need to have a nightly released with this PR before I can switch to the new feature.

r? ghost

@rustbot rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. labels Mar 24, 2025
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@petrochenkov
Copy link
Contributor

What remains to be done:

I'd also want to block the stabilization on landing #138844 to avoid a stable rustdoc feature relying on externally observable hacks in rustc.
The crater run in #138844 returned mostly clean, so I expect it to land soon.

@GuillaumeGomez
Copy link
Member Author

Noted! And that will be a nice improvement, thanks!

Just one thing left for the cfg expansion missing: #[cfg_attr(blabla, derive(Debug))]. In this case, the cfg is not kept in the generated derive items. It's been in my TODO list for a long time now. ^^'

@petrochenkov
Copy link
Contributor

Just one thing left for the cfg expansion missing: #[cfg_attr(blabla, derive(Debug))]

Do you mean like in #138515? :)

@GuillaumeGomez
Copy link
Member Author

You're my hero! Gonna need to handle this new attribute then. :)

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Collaborator

bors commented Mar 25, 2025

☔ The latest upstream changes (presumably #138923) made this pull request unmergeable. Please resolve the merge conflicts.

@GuillaumeGomez GuillaumeGomez force-pushed the rfc-3631 branch 2 times, most recently from d88598f to db25eea Compare March 27, 2025 10:18
@rustbot rustbot added the A-run-make Area: port run-make Makefiles to rmake.rs label Mar 27, 2025
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Collaborator

bors commented Mar 27, 2025

☔ The latest upstream changes (presumably #138927) made this pull request unmergeable. Please resolve the merge conflicts.

@GuillaumeGomez GuillaumeGomez force-pushed the rfc-3631 branch 2 times, most recently from b8cb424 to b581ce1 Compare March 28, 2025 23:54
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@GuillaumeGomez GuillaumeGomez force-pushed the rfc-3631 branch 4 times, most recently from 71369a1 to fbee8a9 Compare April 1, 2025 15:52
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@GuillaumeGomez
Copy link
Member Author

Not sure why CI is failing on the GCC backend but seems unrelated to this PR. Anyway, the implementation is now complete and ready for review. \o/

r? @notriddle

@Vrixyz Vrixyz mentioned this pull request Apr 7, 2025
Open
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@GuillaumeGomez
Copy link
Member Author

Just realized that the failure was actually a ui test... All fixed.

camelid
camelid reviewed Apr 11, 2025
View reviewed changes
Copy link
Member

@camelid camelid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't gotten to the meat of it yet, but one thing is (quoting from one of my comments below):

On second thought, I think I'd prefer we land the implementation of the new features without stabilizing doc_cfg/doc_auto_cfg. Then we can check that it behaves as expected and stabilize soon after. It just seems risky to change so much at the same time as stabilizing.

compiler/rustc_passes/src/check_attr.rs Outdated
@@ -1346,10 +1386,8 @@ impl<'tcx> CheckAttrVisitor<'tcx> {
self.check_attr_crate_level(attr, meta, hir_id);
}

sym::cfg_hide => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we delete sym::cfg_hide as well? Or is it used somewhere else?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can see, it's not used anywhere else.

src/doc/rustdoc/src/unstable-features.md
@@ -56,88 +56,6 @@ It is also not emitted for foreign items, aliases, extern crates and imports.
These features operate by extending the `#[doc]` attribute, and thus can be caught by the compiler
and enabled with a `#![feature(...)]` attribute in your crate.

### `#[doc(cfg)]`: Recording what platforms or features are required for code to be present
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't these docs be updated and moved to the stable part of the book instead of just deleted?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, I think I'd prefer we land the implementation of the new features without stabilizing doc_cfg/doc_auto_cfg. Then we can check that it behaves as expected and stabilize soon after. It just seems risky to change so much at the same time as stabilizing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Erf. I suppose I badly rebased but it was supposed to be moved to the stable part.

And agreed on not stabilizing right away. One thing though: there are 3 features for this RFC, and cfg_hide at least it not used anymore. So I'm gonna put them all behind doc_cfg to make things easier.

@camelid camelid self-assigned this Apr 11, 2025
@GuillaumeGomez
Copy link
Member Author

I put back the features behind the doc_cfg.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Collaborator

bors commented Apr 18, 2025

☔ The latest upstream changes (presumably #139996) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@GuillaumeGomez
Copy link
Member Author

Rebased and replaced usage of removed name_or_empty.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camelid camelid camelid left review comments

Assignees

@notriddle notriddle

@camelid camelid

Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@GuillaumeGomez @rust-log-analyzer @petrochenkov @bors @rustbot @camelid @notriddle