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

Stabilize doc_cfg, auto_doc_cfg and cfg_hide features #100883

Closed

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Aug 22, 2022

Fixes #43781

For some reason I can't seem to reopen #79263 so opening a new one here.

The blocker was that cfg(...) didn't imply doc(cfg(...)), it was added through doc_auto_cfg. Discussion about it is below.

r? @rust-lang/rustdoc

@rustbot rustbot added 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. labels Aug 22, 2022
@rustbot
Copy link
Collaborator

rustbot commented Aug 22, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 22, 2022
@GuillaumeGomez GuillaumeGomez added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. and removed 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. labels Aug 22, 2022
@rustbot
Copy link
Collaborator

rustbot commented Aug 22, 2022

Some changes occurred in src/librustdoc/clean/types.rs

cc @camelid

Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

r=me unless you're waiting for more people

@@ -276,3 +276,46 @@ To get around this limitation, we just add `#[doc(alias = "lib_name_do_something
on the `do_something` method and then it's all good!
Users can now look for `lib_name_do_something` in our crate directly and find
`Obj::do_something`.

## `cfg`
Copy link
Member

Choose a reason for hiding this comment

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

kinda wish our docs here were better

Copy link
Member Author

Choose a reason for hiding this comment

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

I already rewrote the original a bit. Which parts are you thinking about?

@Nemo157
Copy link
Member

Nemo157 commented Aug 22, 2022

I don't recall if there was an FCP previously?

doc_auto_cfg in particular I don't know that there was ever any discussion about making it active by default. We could potentially move that behind a #![doc(auto_cfg)] opt-in.

@@ -173,7 +173,7 @@
#![feature(const_refs_to_cell)]
#![feature(decl_macro)]
#![feature(deprecated_suggestion)]
#![feature(doc_cfg)]
#![cfg_attr(bootstrap, doc_cfg)]
Copy link
Member Author

Choose a reason for hiding this comment

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

This is what is creating the big mess. I'm finding out by what it was replaced (because we can't remove it altogether otherwise the first compilation stage will fail).

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Aug 22, 2022

I don't recall if there was an FCP previously?

There wasn't.

doc_auto_cfg in particular I don't know that there was ever any discussion about making it active by default. We could potentially move that behind a #![doc(auto_cfg)] opt-in.

It's an interesting point. Do you have an idea why someone wouldn't want auto_cfg to not be opt-in?

EDIT: I'd also like to precise that if auto_cfg is still under debate, I can opt it out of this stabilization until we figure it out.

@Nemo157
Copy link
Member

Nemo157 commented Aug 22, 2022

It's an interesting point. Do you have an idea why someone wouldn't want auto_cfg to not be opt-in?

Some crates may not be setup to show this information yet, they may be full of internal cfg that they don't want users to see (e.g. used for compiler feature detection). If we made it opt-in (and maybe enabled by default in edition 2024) then they would have time to update to support it better.

EDIT: e.g. I just tried enabling #![feature(doc_auto_cfg)] in serde and the docs contained this:

image

EDIT: I'd also like to precise that if auto_cfg is still under debate, I can opt it out of this stabilization until we figure it out.

I would prefer not to stabilize doc(cfg) without auto_cfg, that would result in a lot of churn adding the doc(cfg) attributes then removing them again when auto_cfg stabilizes. I think we should be able to quite quickly decide on whether anything about auto_cfg needs changing.

@jyn514 jyn514 added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Aug 22, 2022
@jyn514
Copy link
Member

jyn514 commented Aug 22, 2022

It seems that all the blockers were actually fixed a while ago so it seems like we can finally go forward with this.

Please say what the previous blockers were and how they have been fixed.

@jyn514
Copy link
Member

jyn514 commented Aug 22, 2022

This is the procedure for stabilizing a feature: https://rustc-dev-guide.rust-lang.org/stabilization_guide.html

In particular, it would be very helpful to have a stabilization report: https://rustc-dev-guide.rust-lang.org/stabilization_guide.html#write-a-stabilization-report

@rustbot
Copy link
Collaborator

rustbot commented Aug 22, 2022

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

@GuillaumeGomez
Copy link
Member Author

All valid points. Before starting the FCP, we need to discuss about whether or not we want doc_auto_cfg to be enabled by default or not and if we want an attribute to enable/disable it or not.

@GuillaumeGomez
Copy link
Member Author

Also I think we will have a problem with stdarch since they use the feature but I can't update it from this repository directly (as seen with the last CI error).

@GuillaumeGomez GuillaumeGomez force-pushed the stabilize-doc-cfg branch 2 times, most recently from dc640e3 to 94c0675 Compare August 22, 2022 18:42
@GuillaumeGomez
Copy link
Member Author

The blocker was that cfg(...) didn't imply doc(cfg(...)), it was added through doc_auto_cfg. But since @Nemo157 mentioned the possibility to disable it (or not enable it by default), I looked at reasons at why we wouldn't want it and a very good example came up with: #90497 (I'm also adding this into the first comment).

So I see a few possibilities from here:

  1. Enable the doc_auto_cfg by default and that's it. It has the advantage of not requiring a new argument, and since we also stabilize cfg_hide, people can handle it nicely. However, it requires extra actions from users without "warnings" about the change of behaviour.
  2. Add an attribute #![doc(enable(auto_cfg))] (or something along the line) which would enable the doc_auto_cfg feature. The problem with that is that it's very unlikely that many of our users will simply not know it exists and won't use it.
  3. Add an attribute #![doc(disable(auto_cfg))] (or something along the line) which would disable the doc_auto_cfg feature, meaning that the feature is enabled by default. The problem with having it enabled by default, like @Nemo157 said is that libraries will see a change in their output without "warnings".

Overall, my personal preference is 1.: with cfg_hide, it's very easy to hide the information you don't want and pretty easy to do it. But 3. also makes sense since the attribute would allow to simply disable the feature altogether.

So now: did I forget a case? If not, what is your preference?

@rust-log-analyzer

This comment has been minimized.

@euclio
Copy link
Contributor

euclio commented Aug 23, 2022

This feature still has some significant bugs: #43781 (comment)

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Aug 24, 2022

UI bugs except for #83428. I think it'd be nice to fix them all before stabilizing though.

There is a suggested feature with #73922 by adding this information into the search result too. But I'm not sure if it's really relevant to add it there. Well, the issue is open if you want to debate.

Overall, they don't impact the feature itself but the UI (except for #83428 once again).

@GuillaumeGomez
Copy link
Member Author

Sounds good to me. I'll open a PR adding these two attributes there and make a warning. Next release cycle, we can go forward on this. I'll leave the FCP open in the meantime.

@Manishearth
Copy link
Member

Manishearth commented Oct 2, 2022

I think in that case we should cancel the FCP and do it afresh when we have better information.

@jmillikin
Copy link
Contributor

It sounds like the lack of consensus is around doc(auto_cfg) and doc(no_auto_cfg) -- would it be possible to leave those unstabilized for now, and only stabilize doc(cfg(...)) ?

My understanding is that the doc_cfg feature doesn't have any backwards-compatibility concerns, no new warnings, and no interaction with the editions system. It's purely additive, and gives library authors an important capability that is currently missing in stable.

@Nemo157
Copy link
Member

Nemo157 commented Oct 3, 2022

I mentioned earlier:

I would prefer not to stabilize doc(cfg) without auto_cfg, that would result in a lot of churn adding the doc(cfg) attributes then removing them again when auto_cfg stabilizes.

From what I have seen with both features available usage of doc(cfg) is rare, the auto detected cfg is normally all that is needed.

@jmillikin
Copy link
Contributor

As one data point, I have no plans to use auto_cfg within my own code regardless of its stability.

If there is a set of developers who would prefer to avoid code churn, then they have the option of not using doc(cfg(..)). Allowing it to stabilize won't make them worse off (they'll be waiting for auto_cfg either way), and it is beneficial to folks who are OK with the non-auto functionality.

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Oct 3, 2022

auto_cfg doesn't remove the need for doc(cfg). In some cases, you don't want auto_cfg at all and manually do doc(cfg) whereas in some other cases, you just want to add some extra information that is not provided automatically with auto_cfg. I think both are needed. But yes, I can cancel the FCP if it's better to wait for the new release to be out.

@rfcbot cancel

@rfcbot
Copy link

rfcbot commented Oct 3, 2022

@GuillaumeGomez proposal cancelled.

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Oct 3, 2022
@jhpratt
Copy link
Member

jhpratt commented Oct 3, 2022

For what it's worth, while I previously used doc_cfg directly (alongside doc_auto_cfg), earlier tonight I checked every instance where I used doc_cfg explicitly. Turns out doc_auto_cfg had been improved sufficiently such that doc_cfg was not necessary for my use case (the time crate). While I can imagine that there's still a use case, it's likely vastly smaller than it used to be because of the improvements I noticed.

(also, thank you to those that made the improvements)

@GuillaumeGomez
Copy link
Member Author

Glad to hear it! As for an example, the std::fs module is having issues with doc_auto_cfg iirc because the mix between cfg(doc) and auto_cfg gives pretty bad results.

@GuillaumeGomez
Copy link
Member Author

So what should we do about this and about #102599 ? I think stabling doc(cfg) without auto_cfg wouldn't be the best approach, however, having a way to enable/disable auto_cfg seems like a requirement too.

@jhpratt
Copy link
Member

jhpratt commented Oct 10, 2022

Maybe land the attributes and wait one release cycle to make sure there's no concerns? Then it can all be stabilized together.

@GuillaumeGomez
Copy link
Member Author

It's already the plan. The attributes are in #102599. I was mostly asking if it looked like what they expected and if they thought if it was ready to be merged. ;)

@jhpratt
Copy link
Member

jhpratt commented Oct 10, 2022

In my opinion, yes.

@GuillaumeGomez
Copy link
Member Author

We need someone from the @rust-lang/rustdoc team to agree with us then.

@Manishearth
Copy link
Member

Okay, at this point I'm very confused as to what "the plan" is and I think other team members are too. I wouldn't know what I'm agreeing to without trawling through multiple PRs and threads.

@GuillaumeGomez you posted a stabilization report earlier but I think we need a more extensive one, covering:

  • what the various facets of the feature do (in some detail please)
  • what the status quo is, and roughly how long those have been baking, and what parts are stable vs not
  • what the plan is moving forward (are we stabilizing some of it? all of it? do we have a timeline?)

Here's an example stabilization report that I wrote once for a different RFC

The goal is that team members, but also other people, should be able to make an informed decision without having to read a ton of things to figure out what's going on or be unconfident in their impression of the plan.

@GuillaumeGomez
Copy link
Member Author

Sure, let me explain the situation a bit more then. The stabilization report written here is still valid, only the way we're moving forward changed. Once #102599 merged, I planned to write another one before re-opening the FCP. However you're right, more context will make everyone's life easier:

In this PR, we want to merge the doc_cfg, doc_auto_cfg and cfg_hide features (as they're all co-dependant). The problem was that stabilizing doc_auto_cfg meant that the cfgs were retrieved automatically, which was an issue for some people. Someone suggested that we added attributes to enable/disable this feature. The discussion about these two new attributes happened on zulip.

The conclusions were as follow: until the 2024 edition, doc(no_auto_cfg) will be enabled by default so if you want the feature, you'll need to use doc(auto_cfg). If your code is under the 2024 edition though, it's the opposite: doc(auto_cfg) is enabled by default.

Now the problem was that we needed to let users be aware that using feature(doc_auto_cfg) was not enough anymore to get this feature working, they also need doc(auto_cfg). So we will have one release cycle where they will have a warning before we merge this PR, stabilizing these features. This is what #102599 is for.

@Manishearth
Copy link
Member

Manishearth commented Oct 10, 2022

In this PR, we want to merge the doc_cfg, doc_auto_cfg and cfg_hide features (as they're all co-dependant).

Can you please illustrate, in detail, what each of these features does? I think that's a major part of the confusion here, we all have a general idea of them but not a precise idea of what separates them and their status quo.

And again, ideally this is all one single doc so people do not need to trawl through multiple discussions to review it. It might take a while to write, that's fine. Stabilization reports typically take a bunch of work to make.

(Just make a separate hackmd and post it in zulip and we can ask you to add more detail where necessary, and we can post it here once we're happy with it.)

@GuillaumeGomez
Copy link
Member Author

Thanks for the suggestion! I opened a zulip thread here.

@bors
Copy link
Contributor

bors commented Oct 12, 2022

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

@3tilley
Copy link
Contributor

3tilley commented Nov 16, 2022

For anyone else following from afar, this is the best description I've found of the issues.

@jyn514
Copy link
Member

jyn514 commented Apr 26, 2023

There are a lot of conflicts here. I'm going to close the PR for now by the same rationale as #102599 (comment), since this is blocked on an RFC.

@jyn514 jyn514 closed this Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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.

Tracking issue for #[doc(cfg(…))], #[doc(cfg_hide(…))] and doc_auto_cfg