Skip to content
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 the documentation of builtin macro attributes accessible #92456

Merged
merged 1 commit into from Jan 4, 2022

Conversation

danielhenrymantilla
Copy link
Contributor

@danielhenrymantilla danielhenrymantilla commented Dec 31, 2021

use ::std::prelude::v1::derive; compiles on stable, so, AFAIK, there is no reason to have it be #[doc(hidden)].

Since this involves doc people to chime in, and since jyn is on vacation, I'll cc @GuillaumeGomez and tag the rustdoc team as well

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 31, 2021
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 31, 2021
@danielhenrymantilla
Copy link
Contributor Author

danielhenrymantilla commented Dec 31, 2021

This is currently on a draft stage / waiting-on-author since I'd like to see if I can tackle the filtering out that happens from rustdoc as well (see the docs for ::core::prelude::v1::derive to see what I mean)

@petrochenkov
Copy link
Contributor

See the FIXME above, #[doc(hidden)] was added due to rustdoc bugs causing broken links, not due to stability or instability of the attributes.

It may be possible that those bugs are fixed now, in that case the #[doc(hidden)] annotations should be just removed together with the FIXME, instead of being replaced with #[doc(no_inline)].

@rust-log-analyzer

This comment has been minimized.

@petrochenkov
Copy link
Contributor

Ah, wait, #[doc(no_inline)] is indeed the desired behavior for prelude imports.
Then all #[doc(hidden)]s need to be replaced with it, regardless of stability.
(You may also be able to merge some import groups after that.)

@danielhenrymantilla
Copy link
Contributor Author

@petrochenkov So, derive seems a bit more special, since it doesn't have a public defining module to be re-exported from; thus, if using doc(no_inline), we end up with an "unclickable" item. I've thus gone with no doc override from core (so that it currently acts as inline, and would become no_inline should derive be made accessible elsewhere); but had to fall back to an explicit #[doc(inline)] override for the std reexport (I've added a comment to warn about the situation).
But I'll gladly change to whatever seems more appropriate

@petrochenkov
Copy link
Contributor

since it doesn't have a public defining module to be re-exported from

This is true for all other uses of doc(hidden) in the prelude module.

@danielhenrymantilla
Copy link
Contributor Author

So, #[global_allocator] and #[test] seem to be the only non-unstable non-deprecated attributes in the same situation, so I'm gonna apply my suggested change to those as well (whether unstable or deprecated ones deserve to be "clickable" is another question).

@chorman0773
Copy link
Contributor

chorman0773 commented Dec 31, 2021 via email

@danielhenrymantilla danielhenrymantilla changed the title Do not #[doc(hidden)] the #[derive] macro attribute Do not #[doc(hidden)] builtin stable macro attributes Dec 31, 2021
@danielhenrymantilla danielhenrymantilla changed the title Do not #[doc(hidden)] builtin stable macro attributes Do not #[doc(hidden)] builtin stable macro attributes (#[test], #[derive], …) Dec 31, 2021
@jyn514
Copy link
Member

jyn514 commented Dec 31, 2021

I'm on vacation.

r? @petrochenkov

@rust-highfive rust-highfive assigned petrochenkov and unassigned jyn514 Dec 31, 2021
@danielhenrymantilla
Copy link
Contributor Author

@chorman0773 I've implemented 2778d53 to apply your rationale, but, that being said, the only thing one can do with Rustc{De,En}codable is pass it around, since, as a derive, they don't seem to be usable without the unstable rustc_serialize crate. So I'd lean towards making an exception for them and keep them hidden.

@chorman0773
Copy link
Contributor

It's probably useful for implementors, at least for now, to know they exist. That being said, it may be reasonable to exempt them, especially if (as may be a good idea, although technically breaking) they eventually get removed/destabilzed (since they can't be used anyways).

@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 31, 2021
@danielhenrymantilla danielhenrymantilla marked this pull request as ready for review December 31, 2021 16:57
@rust-log-analyzer

This comment has been minimized.

@danielhenrymantilla

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@danielhenrymantilla
Copy link
Contributor Author

danielhenrymantilla commented Jan 1, 2022

I'll now add another commit trying to have all macros documented, regardless of stability, except for Rustc…codable

library/core/src/prelude/v1.rs Outdated Show resolved Hide resolved
library/core/src/prelude/v1.rs Outdated Show resolved Hide resolved
library/std/src/prelude/v1.rs Outdated Show resolved Hide resolved
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 1, 2022
@danielhenrymantilla danielhenrymantilla changed the title Do not #[doc(hidden)] builtin stable macro attributes (#[test], #[derive], …) Make the documentation of builtin macro attributes accessible Jan 1, 2022
library/core/src/prelude/v1.rs Outdated Show resolved Hide resolved
@@ -69,29 +68,26 @@ pub use crate::{
#[doc(no_inline)]
pub use crate::concat_bytes;

// Do not `doc(inline)` these `doc(hidden)` items.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this comment make sense?
The items are hidden anyway, so it doesn't make any difference whether we inline them or not?
(Especially considering that doc(inline) is the default in this situation.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doc(inline) is the default in this situation

So, while this is currently true, I have to admit having chosen not to use doc(inline) explicitly since there is a change I've been considering to suggest to the rustdoc team, which would change, at the very least, the same-crate re-export of a #[doc(hidden)] item when not using #[doc(inline)].

For a crate wanting to feature macro_export-ed macros scoped within a module, currently, there are three options:

  • pub mod module {
        #[macro_export]
        macro_rules! __m__ {() => ()}
        pub use __m__ as m;
    }

    Drawback: __m__! is visible at the crate root.

    • Workaround: use js hacks to hide it in the aftermath (an ugly example)

      Drawback: so hacky and ugly.

  • Another solution: use a helper crate that performs the #[macro_export]s, and then pub use them in a scoped fashion from the frontend crate (an example).

    Drawback: on top of being more cumbersome, it breaks $crate.

The best solution for these things that occurred to me would be for #[doc(inline)] to override #[doc(hidden)] (or have a doc(inline_unhidden) for this, but that seems to be way more work). Indeed, doc(inline) expresses an intent to have the item be documented in situ, so it makes sense to have it express _unhidden semantics as well, for the rare case where it could matter.

And this would be one such case, where we'd thus have not to use doc(inline). I thus find it less error-prone to do it this way, if that makes sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I don't want to dig too much into this.

library/std/src/prelude/v1.rs Show resolved Hide resolved
library/std/src/prelude/v1.rs Show resolved Hide resolved
@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 2, 2022
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 2, 2022
@petrochenkov
Copy link
Contributor

r=me after squashing commits.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 3, 2022
  - Do not `#[doc(hidden)]` the `#[derive]` macro attribute

  - Add a link to the reference section to `derive`'s inherent docs

  - Do the same for `#[test]` and `#[global_allocator]`

  - Fix `GlobalAlloc` link (why is it on `core` and not `alloc`?)

  - Try `no_inline`-ing the `std` reexports from `core`

  - Revert "Try `no_inline`-ing the `std` reexports from `core`"

  - Address PR review

  - Also document the unstable macros
@bors
Copy link
Contributor

bors commented Jan 3, 2022

@danielhenrymantilla: 🔑 Insufficient privileges: Not in reviewers

@danielhenrymantilla
Copy link
Contributor Author

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Jan 3, 2022

@danielhenrymantilla: 🔑 Insufficient privileges: Not in reviewers

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 3, 2022
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jan 4, 2022

📌 Commit f20ccc0 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 4, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 4, 2022
…askrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#91754 (Modifications to `std::io::Stdin` on Windows so that there is no longer a 4-byte buffer minimum in read().)
 - rust-lang#91884 (Constify `Box<T, A>` methods)
 - rust-lang#92107 (Actually set IMAGE_SCN_LNK_REMOVE for .rmeta)
 - rust-lang#92456 (Make the documentation of builtin macro attributes accessible)
 - rust-lang#92507 (Suggest single quotes when char expected, str provided)
 - rust-lang#92525 (intra-doc: Make `Receiver::into_iter` into a clickable link)
 - rust-lang#92532 (revert rust-lang#92254 "Bump gsgdt to 0.1.3")

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4e4e1ec into rust-lang:master Jan 4, 2022
@rustbot rustbot added this to the 1.59.0 milestone Jan 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants