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 cfg(rustdoc) #61351

Open
wants to merge 2 commits into
base: master
from

Conversation

@GuillaumeGomez
Copy link
Member

commented May 30, 2019

Part of #43781.

I *think* I did everything required for a release (first time after all!):

  • Moving doc
  • Updating tests

cc @rust-lang/rustdoc

r? @Manishearth

platform-specific code if it *does* receive it.

Because Rustdoc doesn't need to fully compile a crate to binary, it replaces function bodies with
`loop {}` to prevent having to process more than necessary. This means that any code within a

This comment has been minimized.

Copy link
@Centril

Centril May 30, 2019

Member

How does this cope with functions that have -> impl Trait where ! (because of loop {}) does not implement Trait?

This comment has been minimized.

Copy link
@GuillaumeGomez

This comment has been minimized.

Copy link
@ollie27

ollie27 May 30, 2019

Contributor

There is currently an exception for -> impl Trait:

// FIXME: Currently the `everybody_loops` transformation is not applied to:
// * `const fn`, due to issue #43636 that `loop` is not supported for const evaluation. We are
// waiting for miri to fix that.
// * `impl Trait`, due to issue #43869 that functions returning impl Trait cannot be diverging.
// Solving this may require `!` to implement every trait, which relies on the an even more
// ambitious form of the closed RFC #1637. See also [#34511].

This means that #[doc(cfg(...))] doesn't work for those functions yet so I don't think this feature is ready for stabilization yet.

This comment has been minimized.

Copy link
@Centril

Centril May 30, 2019

Member
//  * `impl Trait`, due to issue #43869 that functions returning impl Trait cannot be diverging.
//    Solving this may require `!` to implement every trait, which relies on the an even more
//    ambitious form of the closed RFC #1637. See also [#34511].

This bit seems unlikely from my POV as a T-Lang member... cc @rust-lang/lang if y'all have thoughts...

This comment has been minimized.

Copy link
@QuietMisdreavus

QuietMisdreavus May 31, 2019

Member

I'm with @ollie27 here, i had assumed the reason we couldn't stabilize doc(cfg) yet was because the implementation didn't work for all situations.

@Centril Centril added this to the 1.37 milestone May 30, 2019

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented May 30, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:10b45dc0:start=1559216656207704247,finish=1559216743868434663,duration=87660730416
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
$ export AWS_ACCESS_KEY_ID=AKIA46X5W6CZEJZ6XT55
---
[01:08:26] .................................................................................................... 1200/5599
[01:08:29] .................................................................................................... 1300/5599
[01:08:31] .................................................................................................... 1400/5599
[01:08:34] .................................................................................................... 1500/5599
[01:08:37] ...................................F................................................................ 1600/5599
[01:08:44] .................................................................................................... 1800/5599
[01:08:47] .................................................................................................... 1900/5599
[01:08:51] .................................................................................................... 2000/5599
[01:08:54] ..................................................................................i................. 2100/5599
---
[01:11:13] ---- [ui] ui/feature-gates/feature-gate-doc_cfg.rs stdout ----
[01:11:13] 
[01:11:13] error: ui test compiled successfully!
[01:11:13] status: exit code: 0
[01:11:13] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/feature-gates/feature-gate-doc_cfg.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/feature-gates/feature-gate-doc_cfg" "-Crpath" "-O" "-Cdebuginfo=0" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/feature-gates/feature-gate-doc_cfg/auxiliary" "-A" "unused"
[01:11:13] ------------------------------------------
[01:11:13] 
[01:11:13] ------------------------------------------
[01:11:13] stderr:
---
[01:11:13] thread 'main' panicked at 'Some tests failed', src/tools/compiletest/src/main.rs:521:22
[01:11:13] note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
[01:11:13] 
[01:11:13] 
[01:11:13] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/ui" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "ui" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-6.0/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "6.0.0\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[01:11:13] 
[01:11:13] 
[01:11:13] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:11:13] Build completed unsuccessfully in 0:04:46
[01:11:13] Build completed unsuccessfully in 0:04:46
[01:11:13] make: *** [check] Error 1
[01:11:13] Makefile:48: recipe for target 'check' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0817ba86
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Thu May 30 12:57:06 UTC 2019

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@petrochenkov petrochenkov self-assigned this May 30, 2019

processing early in the compilation process. However, Rustdoc has a trick up its sleeve to handle
platform-specific code if it *does* receive it.

Because Rustdoc doesn't need to fully compile a crate to binary, it replaces function bodies with

This comment has been minimized.

Copy link
@Manishearth

Manishearth May 30, 2019

Member

Unsure if this implementation detail needs to be exposed.

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez May 30, 2019

Author Member

I found it interesting so I didn't remove it. :)

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:stabilize-cfg-rustdoc branch from 117c244 to cfb63d8 May 30, 2019

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented May 30, 2019

Forgot to remove a feature test. Updated.

@Manishearth

This comment has been minimized.

Copy link
Member

commented May 30, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

📌 Commit cfb63d8 has been approved by Manishearth

@Centril

This comment has been minimized.

Copy link
Member

commented May 30, 2019

@bors r-

There's an ongoing discussion in #61351 (comment) and a T-rustdoc team member who things this is not ready to stabilize yet.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented May 30, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:1f1ee840:start=1559252560131854340,finish=1559252676774082914,duration=116642228574
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
$ export AWS_ACCESS_KEY_ID=AKIA46X5W6CZEJZ6XT55
---
travis_time:start:tidy
tidy check
[00:04:27] * 574 error codes
[00:04:27] * highest error code: E0729
[00:04:30] Expected a gate test for the feature 'doc_cfg'.
[00:04:30] Hint: create a failing test file named 'feature-gate-doc_cfg.rs'
[00:04:30]       in the 'ui' test suite, with its failures due to
[00:04:30]       missing usage of #![feature(doc_cfg)].
[00:04:30] Hint: If you already have such a test and don't want to rename it,
[00:04:30]       you can also add a // gate-test-doc_cfg line to the test file.
[00:04:30] tidy error: Found 1 features without a gate test.
[00:04:32] some tidy checks failed
[00:04:32] 
[00:04:32] 
[00:04:32] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:32] 
[00:04:32] 
[00:04:32] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:32] Build completed unsuccessfully in 0:01:21
[00:04:32] Build completed unsuccessfully in 0:01:21
[00:04:32] Makefile:67: recipe for target 'tidy' failed
[00:04:32] make: *** [tidy] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:33c9ce70
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Thu May 30 21:49:19 UTC 2019
---
travis_time:end:121190f7:start=1559252960000256035,finish=1559252960004647825,duration=4391790
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0dd55b4e
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:13cb1c5a
travis_time:start:13cb1c5a
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:1e3596f6
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@petrochenkov petrochenkov removed their assignment May 31, 2019

@kennytm kennytm changed the title Stabilize cfg(doc(...)) and cfg(rustdoc) Stabilize doc(cfg(...)) and cfg(rustdoc) May 31, 2019

@kennytm kennytm added the needs-fcp label May 31, 2019

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

commented May 31, 2019

I'm against stabilizing doc(cfg), because of the following snippet:

#[cfg(any(rustdoc, windows))]
#[doc(cfg(windows))]
pub fn my_handle() -> winapi::shared::ntdef::HANDLE { ... }

We currently don't have a way to handle this code in rustdoc if it's being compiled on non-windows targets, because the winapi crate is totally empty on non-windows targets. This sample will create a type error when rustdoc tries to build it. This sample was discussed at the Rust All-Hands this year, but as far as i know we haven't acted on the discussion that occurred there. In addition, as @ollie27 mentioned, functions returning impl Trait do not receive the benefits of everybody_loops, so the compilation is not perfect in those cases.


...however, i would like to stabilize cfg(rustdoc) separately, since that doesn't have the same ramifications of the rest of doc(cfg). My recommendation would be to either strip out doc(cfg) from this PR and just keep cfg(rustdoc), or close this and open a new one that just stabilizes cfg(rustdoc).

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Jun 1, 2019

I initially wanted to have only cfg(rustdoc) but saw that the two were "linked" so I just stabilized both. I clearly don't mind to only stabilize cfg(rustdoc) (quite the opposite!). I'll update the PR soon.

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:stabilize-cfg-rustdoc branch 4 times, most recently from a33e4e4 to 135e5f3 Jun 1, 2019

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jun 1, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:1aba78dc:start=1559426211948584947,finish=1559426299470295469,duration=87521710522
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
$ export AWS_ACCESS_KEY_ID=AKIA46X5W6CZEJZ6XT55
---
[00:03:57] tidy error: /checkout/src/test/ui/feature-gates/feature-gate-doc_cfg.rs:1: trailing whitespace
[00:04:02] some tidy checks failed
[00:04:02] 
[00:04:02] 
[00:04:02] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:02] 
[00:04:02] 
[00:04:02] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:02] Build completed unsuccessfully in 0:01:15
---
travis_time:end:04c4dc90:start=1559426551889588206,finish=1559426551894457709,duration=4869503
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0d320960
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:03c8c284
travis_time:start:03c8c284
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:076be766
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2019

Updated. I'm not the sure the change in the unstable docs is the one you had in mind @QuietMisdreavus ?

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

That edit to the Rustdoc Book's unstable chapter is good. I still want you to modify the Unstable Book, though:

Please update the Unstable Book (src/doc/unstable-book/src/language-features/doc-cfg.md) to remove the paragraph about cfg(rustdoc).

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Jun 28, 2019

Done!

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:stabilize-cfg-rustdoc branch from 3fbff6d to d22fd9b Jun 28, 2019

@@ -214,3 +214,31 @@ the `strip-hidden` pass is removed.
Since primitive types are defined in the compiler, there's no place to attach documentation
attributes. This attribute is used by the standard library to provide a way to generate
documentation for primitive types.

## `#[cfg(rustdoc)]`: Documenting platform-/feature-specific information

This comment has been minimized.

Copy link
@QuietMisdreavus

QuietMisdreavus Jun 28, 2019

Member

I just realized something about these docs. First, the heading here is a little misleading, since cfg(rustdoc) isn't just about platform-specific stuff. It's feasible that people could "invent" items that only exist for documentation, for whatever reason. However, writing something like "exposing items to documentation" feels more misleading, since it implies that you need that to add items to docs in the first place. I think this header text is acceptable, since platform-specific docs was the driving force for it, but it's something to consider.

Second, there isn't really a good chapter for these docs. This chapter is specifically about the #[doc] attribute, but this section is about something you add to the #[cfg] attribute. However, all the other chapters make even less sense to add this to. I'm wondering whether we should create a new chapter that talks about "advanced" features that are stable. I'm not sure what else would go with it, though... 🤔 Maybe we can just leave this here. I'm curious what @ollie27 and @steveklabnik think, though.

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

As far as i'm concerned, this PR is ready to go, barring the one comment i just left, so i'm going to open up the FCP.

@rust-lang/rustdoc This PR is pulling out the cfg(rustdoc) feature and stabilizing it by itself. The feature allows users to expose items directly to rustdoc, even if they might not otherwise be available. It used to be tied up in the #[doc(cfg)] feature, but we've determined that it has enough utility on its own to stabilize it separately.

@rfcbot fcp merge

@rfcbot concern where-do-the-docs-go

@rfcbot

This comment has been minimized.

Copy link

commented Jun 28, 2019

Team member @QuietMisdreavus has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@Centril Centril modified the milestones: 1.37, 1.38 Jul 2, 2019

Centril added a commit to Centril/rust that referenced this pull request Jul 7, 2019

Rollup merge of rust-lang#62213 - QuietMisdreavus:cfg-doctest, r=Guil…
…laumeGomez

rustdoc: set cfg(doctest) when collecting doctests

Note: This PR builds on top of rust-lang#61199; only the last commit is specific to this PR.

As discussed in rust-lang#61199, we want the ability to isolate items to only when rustdoc is collecting doctests, but we can't use `cfg(test)` because of libcore's `#![cfg(not(test))]`. This PR proposes a new cfg flag, `cfg(doctest)`, specific to this situation, rather than reusing an existing flag. I've isolated it behind a feature gate so that we can contain the effects to nightly only. (A stable workaround that can be used in lieu of `#[cfg(doctest)]` is `#[cfg(rustdoc)] #[doc(hidden)]`, at least once rust-lang#61351 lands.)

Tracking issue: rust-lang#62210
@chocol4te

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2019

@ollie27 @onur Ping from triage regarding your reviews! :)

(I saw the RFC bot's comment, I don't think 2/4 counts as a majority?)

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Jul 23, 2019

And I need to update this PR once again...

@onur

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

👍

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2019

With @onur approval (thanks a lot!), it can go forward now. I'll update the PR then it's ready to go.

@ollie27

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

Is there a reason this isn't cfg(doc)? Given that we have #[test] and cfg(test) I would expect #[doc] and cfg(doc). I guess it doesn't matter too much though.

I've noticed that cfg(rustdoc) is set inside doctests. This doctest currently passes:

//! ```
//! #![feature(doc_cfg)]
//! assert!(cfg!(rustdoc));
//! ```

Is that intentional? It looks like a bug to me.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2019

Originally, I made it cfg(doc) but I completely forgot the reason why we decided to change.

I've noticed that cfg(rustdoc) is set inside doctests. This doctest currently passes:

To me it doesn't look like a bug?

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:stabilize-cfg-rustdoc branch from d22fd9b to 2dcff42 Jul 31, 2019

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

Updated.

@ollie27: Anything blocking on your side or can we make this move forward?

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

commented Aug 10, 2019

It seems very strange for the cfg flag to be set inside doc tests, and since I can't imagine a use case where that would be helpful, I think we shouldn't set it to minimize exposed surface area. All doc tests are guaranteed to be run only by rustdoc :)

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Aug 11, 2019

I'll remove it then.

@Centril Centril modified the milestones: 1.38, 1.39 Aug 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.