search

rust-lang / rfcs

RFCs for changes to Rust
https://rust-lang.github.io/rfcs/
Apache License 2.0
5.78k stars 1.55k forks source link

RFC for doc_cfg, doc_cfg_auto, doc_cfg_hide and doc_cfg_show features #3631

Open GuillaumeGomez opened 1 month ago

GuillaumeGomez commented 1 month ago

tracking issue

cc @rust-lang/rustdoc

Rendered

clarfonthey commented 1 month ago

I'm in favour of this but I would recommend clarifying the #[cfg(any(doc, ...))] trick in the guide-level explanation a bit further, since folks reading it might be misled into thinking that rustdoc will just ignore conditional compilation entirely to report things, which isn't true; it just shows the conditions when they're met.

GuillaumeGomez commented 1 month ago

Applied first round of review comments. Thanks everyone!

GuillaumeGomez commented 1 month ago

Updated the syntax for auto_cfg to instead accept true or false.

Removed restriction on doc(cfg()) which was marked as not being able to be used as a crate-level attribute as it currently works using it as such, so doesn't seem there is a good enough reason to change this behaviour.

petrochenkov commented 1 month ago

I have re-read the reference part again, but I still don't understand why there are so many attributes.

It looks like having just doc(cfg) and doc(cfg_hide) would be enough to both explicitly enable and disable whatever configs are necessary, with cfg acting as a default.

petrochenkov commented 1 month ago

A couple more questions.

GuillaumeGomez commented 1 month ago

I have re-read the reference part again, but I still don't understand why there are so many attributes.

  • Why is #![doc(auto_cfg = true)] necessary if it is the default?

Because future possibilities include to make this attribute work not only at crate-level but also on any item. And based on this discussion, I think it'll part of this RFC.

  • Why is #![doc(auto_cfg = false)] necessary if it is equivalent to #![doc(cfg_hide(any()))]?

cfg_hide doesn't work like cfg: it only accepts "items". However, the RFC is very unclear about it, so I'll add it in the RFC.

  • If it is not equivalent, then why it needs to do something different? It would be better if they were equivalent and auto_cfg didn't need to exist.

I have to admit I didn't think about cfg_show and cfg_hide to work this way at all. I'm not sure it's a good idea though. For example, how should we handle this case:

cfg_hide(any(windows, not(unix)))

How do we interpret the not(unix)? As a whole or should it invert the "hide unix"? So for simplicity, that's why not, any and all are not supported in cfg_hide and cfg_show and why we have cfg_auto.

  • Why is #![doc(cfg_show(...))] necessary if it is equivalent to #![doc(cfg(...))]?

It is not. As mentioned in the RFC:

It only applies to #[doc(auto_cfg = true)], not to #[doc(cfg(...))].

  • If it is not equivalent, then why it needs to do something different? It would be better if they were equivalent and cfg_show didn't need to exist.

Because they both fill different role. You might want to add your own "doc cfg" information to either override what auto_doc_cfg generates or add your own information.

A couple more questions.

  • Is #[doc(cfg)] currently allowed on an item without #[cfg]? What are the semantics?

The RFC says:

This attribute provides a standardized format to override #[cfg()] attributes to document conditionally available items. ... This attribute has the same syntax as conditional compilation

So the semantics are covered I think. The RFC didn't make it clear it works with and without cfg on the item though, so I'll add a clarification.

  • I'd expect this to be allowed making cfg_show redundant as in the previous comment.

    • Is it possible to show a cfg on an item, but hide it on all children of that item? (Without putting attributes on all those children individually.)

I think I see why you mention it. I suppose you talk a case like:

#[cfg(windows)]
mod module {
    #![doc(cfg_hide(windows))]

    fn item() {}
}

In this case, since the doc(cfg_hide(windows)) is also an attribute of module, it should impact module. I think we should say that inner doc(cfg_hide) and doc(cfg_show) attributes don't impact the item if they are inner attributes.

GuillaumeGomez commented 1 month ago

So only remains the question:

#[cfg(windows)]
mod module {
    #![doc(cfg_hide(windows))]

    fn item() {}
}

How to allow cfg(windows) to be displayed on module while having it hidden on its children? I think what the RFC offer is good enough:

#[cfg(windows)]
#[doc(cfg(windows)] // Will be displayed all the time
#[doc(cfg_hide(windows))] // Hides `windows` from `doc(auto_cfg)` rendering on `module` and its children
pub mod module {
    #[cfg(windows)] // Not displayed because of `cfg_hide(windows)`!
    pub fn item() {}
}

So I'd be willing to not change anything in this regard.

jhpratt commented 1 month ago

Just wanted to make sure that my comment above (https://github.com/rust-lang/rfcs/pull/3631#pullrequestreview-2048876838) wasn't missed, as there has been no response or acknowledgement :slightly_smiling_face:

GuillaumeGomez commented 1 month ago

I don't understand your comments. Do you have an example of what you would want to do?

jhpratt commented 1 month ago

For an overly simplified case, see https://github.com/jhpratt/doc-cfg-demo

Running cargo +nightly doc --all-features on alpha will incorrectly say that alpha::S::foo is "Available on crate feature foo-method only." I hadn't checked this in a while, but previously it would have no annotation whatsoever. Ideally, the feature resolver would be utilized to show the correct annotation — it relies on the feature beta-foo-method.

This situation is quite common with façade crates and is the sole reason I have not migrated large amounts of code from time to time-core. Were I to do this, I could remove ~1000 LOC (off the top of my head) from time-macros that is currently duplicated.

GuillaumeGomez commented 4 days ago

Okay so if I understand correctly what you say, reexported items should only show cfgs from the reexport's point of view (so to speak)? That sounds reasonable. Once you confirm it, I'll update the RFC.

jhpratt commented 3 days ago

Effectively, yes. My point of view is "what does a consumer of the library care about". That would mean showing them what feature they need to enable such that the item is present, taking into account feature transitivity, etc.

GuillaumeGomez commented 2 days ago

I added an example but normally it was covered. For the current case, this is a bug that should be solved.

GuillaumeGomez commented 2 days ago

I think with this all questions/comments were answered/resolved?

jhpratt commented 2 days ago

As long as we agree on intended behavior, and it seems we do, then I'm satisfied. As I'd said above, the behavior had changed since I originally checked; it previously had no annotation.

Feel free to use that example as a rustdoc test whenever that gets fixed, by the way. No credit necessary.

GuillaumeGomez commented 2 days ago

I definitely plan to. :3