-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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.
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
Make cfg imply doc(cfg) #79341
Make cfg imply doc(cfg) #79341
Conversation
r? @ollie27 (rust_highfive has picked a reviewer for you, use r? to override) |
1ba3a96
to
178c32c
Compare
Is this relying on the fact that Here's what the reference say:
(Which matches the intention, but not reality, because |
For my own crates that use doc-cfg this works perfectly, I can remove the For For |
Yeah, probably. Would there be some way to look back at the original AST and see what |
Perhaps the best way to recover this functionality would be to expand Feel free to experiment with the impact of this change on real life code, but I'd like to block actually merging this for ~1.5 months until I get to implementing the
Changing behavior depending on feature gate (rather than just producing an error) is not a good practice, this means stabilization will be a (potentially breaking) change in behavior. |
How would you suggest to do this instead? Unconditionally adding |
☔ The latest upstream changes (presumably #79895) made this pull request unmergeable. Please resolve the merge conflicts. |
@rustbot modify labels: -S-waiting-on-review +S-waiting-on-author |
@JohnCSimon as far as I know this is still waiting on response to #79341 (comment). |
I planned to work on If making the feature gate non-behavior-changing is hard, then it can be made behavior changing. r? @jyn514 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, but someone from T-libs may want to take a look at the rendered docs.
cfg changes are going to be deferred to uncertain future, and I don't want to block this PR on them anymore.
Thank you ❤️ If it makes you feel better, I think the doc(cfg)
desugaring you mentioned should be possible even with the feature gate as long as there's a way to tell whether the attribute was injected by the compiler or not.
For tokio 0.3.4 this adds a bunch of Unix cfgs to the docs. There are a couple of cfg(not(loom)) that need suppressing, and some kind of bug to do with the cfg displayed for tokio::{main, test} that I need to look into.
Did you get a chance to look at the bug?
For syn 1.0.50 this exposes quite a few more cfgs that seem correct, it probably needs looking at by a syn dev to see if they really should be exposed. There's also a couple WebAssembly cfgs that need suppressing.
I'll start a try build in just a second so people can test this locally.
TODO: Update doc(cfg) docs, Add doc(cfg_hide) docs
Please add these when you get a chance. This also needs a rebase.
target_pointer_width = "16", | ||
target_pointer_width = "32", | ||
target_pointer_width = "64", | ||
target_has_atomic = "8", | ||
target_has_atomic = "16", | ||
target_has_atomic = "32", | ||
target_has_atomic = "64", | ||
target_has_atomic = "ptr", | ||
target_has_atomic_equal_alignment = "8", | ||
target_has_atomic_equal_alignment = "16", | ||
target_has_atomic_equal_alignment = "32", | ||
target_has_atomic_equal_alignment = "64", | ||
target_has_atomic_equal_alignment = "ptr", | ||
target_has_atomic_load_store = "8", | ||
target_has_atomic_load_store = "16", | ||
target_has_atomic_load_store = "32", | ||
target_has_atomic_load_store = "64", | ||
target_has_atomic_load_store = "ptr", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why hide these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These, along with target_has_atomic_ptr
, are unstable cfg
. While I think it would be really nice to have this all documented on the items, I don't think the doc(cfg)
presentation of them is good enough. I'll attach a screenshot of what it looks like with them unhidden.
I also think it might make sense to enhance doc(cfg_hide)
to support specifying just the key of key-value cfg expressions so that these aren't duplicated for each size.
@@ -272,7 +272,8 @@ impl Clean<Item> for doctree::Module<'_> { | |||
|
|||
impl Clean<Attributes> for [ast::Attribute] { | |||
fn clean(&self, cx: &DocContext<'_>) -> Attributes { | |||
Attributes::from_ast(cx.sess().diagnostic(), self, None) | |||
let doc_cfg_active = cx.tcx.features().doc_cfg; | |||
Attributes::from_ast(cx.sess().diagnostic(), self, None, doc_cfg_active, &cx.hidden_cfg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why put this on Attributes
instead of calling tcx.features()
on demand?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found why - you use it directly in from_ast
, not after Attributes
is constructed. It's either this or pass a tcx
; this seems fine.
fn single(self) -> Option<Self::Item> { | ||
let mut iter = self.into_iter(); | ||
let item = iter.next()?; | ||
iter.next().is_none().then_some(())?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a little too cute :P
iter.next().is_none().then_some(())?; | |
if iter.next().is_some() { | |
return None; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'll likely scrap the whole SingleExt
extension trait as the diagnostics for 0 and >1 items should be different.
@bors try |
Friendly ping @Nemo157 — what's the status of this? |
Ping from triage: |
8870ae1
to
e6ac7aa
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This is only active when the `doc_cfg` feature is active. The implicit cfg can be overridden via #[doc(cfg(...))], so e.g. to hide a #[cfg] you can use something like: ```rust #[cfg(unix)] #[doc(cfg(all()))] pub struct Unix; ``` (since `all()` is always true, it is never shown in the docs)
By adding #![doc(cfg_hide(foobar))] to the crate attributes the cfg #[cfg(foobar)] (and _only_ that _exact_ cfg) will not be implicitly treated as a doc(cfg) to render a message in the documentation.
7458146
to
0225d97
Compare
The job Click to see the possible cause of the failure (guessed by this bot)
|
☔ The latest upstream changes (presumably #84494) made this pull request unmergeable. Please resolve the merge conflicts. |
@Nemo157 , is this PR still being pursued? |
@bstrie this is the last update I heard: https://discord.com/channels/442252698964721669/459149231702278154/836259493741985854
|
Would someone like to make a tracking issue so that this can be closed, then? |
I took this PR, rebased it and updated the code so that it can work with current rustdoc in #89596. |
…jyn514 Make cfg imply doc(cfg) This is a reopening of rust-lang#79341, rebased and modified a bit (we made a lot of refactoring in rustdoc's types so they needed to be reflected in this PR as well): * `hidden_cfg` is now in the `Cache` instead of `DocContext` because `cfg` information isn't stored anymore on `clean::Attributes` type but instead computed on-demand, so we need this information in later parts of rustdoc. * I removed the `bool_to_options` feature (which makes the code a bit simpler to read for `SingleExt` trait implementation. * I updated the version for the feature. There is only one thing I couldn't figure out: [this comment](rust-lang#79341 (comment)) > I think I'll likely scrap the whole `SingleExt` extension trait as the diagnostics for 0 and >1 items should be different. How/why should they differ? EDIT: this part has been solved, the current code was fine, just needed a little simplification. cc `@Nemo157` r? `@jyn514` Original PR description: This is only active when the `doc_cfg` feature is active. The implicit cfg can be overridden via `#[doc(cfg(...))]`, so e.g. to hide a `#[cfg]` you can use something like: ```rust #[cfg(unix)] #[doc(cfg(all()))] pub struct Unix; ``` By adding `#![doc(cfg_hide(foobar))]` to the crate attributes the cfg `#[cfg(foobar)]` (and _only_ that _exact_ cfg) will not be implicitly treated as a `doc(cfg)` to render a message in the documentation.
This is only active when the
doc_cfg
feature is active.The implicit cfg can be overridden via
#[doc(cfg(...))]
, so e.g. to hide a#[cfg]
you can use something like:By adding
#![doc(cfg_hide(foobar))]
to the crate attributes the cfg#[cfg(foobar)]
(and only that exact cfg) will not be implicitly treated as adoc(cfg)
to render a message in the documentation.TODO:
doc(cfg)
doc(cfg)
docsdoc(cfg_hide)
docs