Add --check for rustdoc #8859

GuillaumeGomez · 2020-11-13T21:39:43Z

Since rust-lang/rust#78984 has landed, it'd be nice to integrate it into cargo nightly.

Currently, the code seems to be running the checks, but doesn't run rustdoc. Any help to help me figure out what's missing would be very appreciated. :)

As you can see here, the second check is run as expected, but the rustdoc command isn't started for some reason...

$ ../../cargo/target/release/cargo check -v
warning: /home/imperio/rust/gtk-rs/graphene/sys/Cargo.toml: unused manifest key: package.link
       Fresh unicode-xid v0.2.1
       Fresh unicode-segmentation v1.6.0
       Fresh strum v0.19.5
       Fresh pkg-config v0.3.19
       Fresh version-compare v0.0.11
       Fresh version_check v0.9.2
       Fresh futures-core v0.3.8
       Fresh once_cell v1.5.2
       Fresh pin-utils v0.1.0
       Fresh slab v0.4.2
       Fresh either v1.6.1
       Fresh smallvec v1.4.2
       Fresh heck v0.3.1
       Fresh futures-task v0.3.8
       Fresh itertools v0.9.0
       Fresh futures-channel v0.3.8
       Fresh proc-macro2 v1.0.24
       Fresh serde v1.0.117
       Fresh proc-macro-hack v0.5.19
       Fresh libc v0.2.80
       Fresh quote v1.0.7
       Fresh toml v0.5.7
       Fresh proc-macro-nested v0.1.6
       Fresh anyhow v1.0.34
       Fresh bitflags v1.2.1
       Fresh syn v1.0.48
       Fresh proc-macro-error-attr v1.0.4
       Fresh proc-macro-crate v0.1.5
       Fresh thiserror-impl v1.0.22
       Fresh strum_macros v0.19.4
       Fresh pin-project-internal v1.0.1
       Fresh futures-macro v0.3.8
       Fresh proc-macro-error v1.0.4
       Fresh thiserror v1.0.22
       Fresh pin-project v1.0.1
       Fresh glib-macros v0.13.0 (/home/imperio/rust/gtk-rs/glib-macros)
       Fresh system-deps v2.0.1
       Fresh futures-util v0.3.8
       Fresh futures-executor v0.3.8
       Fresh glib-sys v0.13.0 (/home/imperio/rust/gtk-rs/glib/sys)
       Fresh gobject-sys v0.13.0 (/home/imperio/rust/gtk-rs/glib/gobject-sys)
    Checking glib v0.13.0 (/home/imperio/rust/gtk-rs/glib)
     Running `rustc --crate-name glib glib/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi --crate-type lib --emit=dep-info,metadata -C embed-bitcode=no -C debuginfo=2 -C metadata=70d43c382e683ed1 -C extra-filename=-70d43c382e683ed1 --out-dir /home/imperio/rust/gtk-rs/target/debug/deps -C incremental=/home/imperio/rust/gtk-rs/target/debug/incremental -L dependency=/home/imperio/rust/gtk-rs/target/debug/deps --extern bitflags=/home/imperio/rust/gtk-rs/target/debug/deps/libbitflags-33cf578042896f22.rmeta --extern futures_channel=/home/imperio/rust/gtk-rs/target/debug/deps/libfutures_channel-2e44f97b4af970fb.rmeta --extern futures_core=/home/imperio/rust/gtk-rs/target/debug/deps/libfutures_core-bafe9494a629a15e.rmeta --extern futures_executor=/home/imperio/rust/gtk-rs/target/debug/deps/libfutures_executor-940684229381a4a0.rmeta --extern futures_task=/home/imperio/rust/gtk-rs/target/debug/deps/libfutures_task-bbbcaee922a34b22.rmeta --extern futures_util=/home/imperio/rust/gtk-rs/target/debug/deps/libfutures_util-f73631bfdc9b54d0.rmeta --extern glib_macros=/home/imperio/rust/gtk-rs/target/debug/deps/libglib_macros-acbc09e903b4b734.so --extern glib_sys=/home/imperio/rust/gtk-rs/target/debug/deps/libglib_sys-2712898b600d8a37.rmeta --extern gobject_sys=/home/imperio/rust/gtk-rs/target/debug/deps/libgobject_sys-f753b4fcba7c0d36.rmeta --extern libc=/home/imperio/rust/gtk-rs/target/debug/deps/liblibc-4444037e32c5a7fc.rmeta --extern once_cell=/home/imperio/rust/gtk-rs/target/debug/deps/libonce_cell-d3ff5d109eb6c70d.rmeta --extern smallvec=/home/imperio/rust/gtk-rs/target/debug/deps/libsmallvec-09d6995c1d1a32f6.rmeta`
    Finished dev [unoptimized + debuginfo] target(s) in 1.59s
warning: /home/imperio/rust/gtk-rs/graphene/sys/Cargo.toml: unused manifest key: package.link
       Fresh unicode-xid v0.2.1
       Fresh unicode-segmentation v1.6.0
       Fresh version_check v0.9.2
       Fresh strum v0.19.5
       Fresh pkg-config v0.3.19
       Fresh version-compare v0.0.11
       Fresh futures-core v0.3.8
       Fresh once_cell v1.5.2
       Fresh either v1.6.1
       Fresh pin-utils v0.1.0
       Fresh slab v0.4.2
       Fresh smallvec v1.4.2
       Fresh heck v0.3.1
       Fresh futures-task v0.3.8
       Fresh futures-channel v0.3.8
       Fresh proc-macro2 v1.0.24
       Fresh serde v1.0.117
       Fresh itertools v0.9.0
       Fresh proc-macro-hack v0.5.19
       Fresh libc v0.2.80
       Fresh proc-macro-nested v0.1.6
       Fresh anyhow v1.0.34
       Fresh bitflags v1.2.1
       Fresh quote v1.0.7
       Fresh toml v0.5.7
       Fresh syn v1.0.48
       Fresh proc-macro-error-attr v1.0.4
       Fresh proc-macro-crate v0.1.5
       Fresh thiserror-impl v1.0.22
       Fresh strum_macros v0.19.4
       Fresh pin-project-internal v1.0.1
       Fresh futures-macro v0.3.8
       Fresh proc-macro-error v1.0.4
       Fresh thiserror v1.0.22
       Fresh pin-project v1.0.1
       Fresh glib-macros v0.13.0 (/home/imperio/rust/gtk-rs/glib-macros)
       Fresh system-deps v2.0.1
       Fresh futures-util v0.3.8
       Fresh futures-executor v0.3.8
       Fresh glib-sys v0.13.0 (/home/imperio/rust/gtk-rs/glib/sys)
       Fresh gobject-sys v0.13.0 (/home/imperio/rust/gtk-rs/glib/gobject-sys)
       Fresh glib v0.13.0 (/home/imperio/rust/gtk-rs/glib)
    Finished dev [unoptimized + debuginfo] target(s) in 1.77s

r? @ehuss

ehuss

The decision of whether to run rustc or rustdoc is done here, so it looks like the is_doc method needs to be updated.

ehuss · 2020-11-15T00:15:34Z

src/cargo/core/compiler/mod.rs

@@ -596,6 +598,10 @@ fn rustdoc(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult<Work> {
    let pkg_id = unit.pkg.package_id();
    let script_metadata = cx.find_build_script_metadata(unit.clone());

+    if unit.mode.is_check() {
+        rustdoc.arg("--test");


Is this intended to be --check?

It is! Good catch.

ehuss · 2020-11-15T00:23:39Z

src/bin/cargo/commands/check.rs

@@ -55,5 +57,15 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
    let compile_opts = args.compile_options(config, mode, Some(&ws), ProfileChecking::Unchecked)?;

    ops::compile(&ws, &compile_opts)?;
+
+    if features::nightly_features_allowed() {


This will probably need an explicit opt-in.

Also, I'm a little concerned about the performance impact of adding this to the default set. Have you checked to see how slow it is on medium-to-large libraries?

Is it possible that rustdoc could generate duplicate warnings from rustc? I believe missing_docs would be duplicated, are there other warnings that could cause that?

Related to the two above issues, if this is to be included in the default set, then the logic would go in filter_default_targets. Running the build logic twice would be a pretty big performance hit, and would cause cached warnings to be replayed twice, among other issues. Unfortunately I think it could make the "duplicate errors" problem even more apparent, which makes me more concerned about having it in the default set.

I didn't think about lint duplicates, but it's a good point. We can always allow the ones impacted to prevent this duplication. As for the compilation duplication, normally, rustdoc doesn't recompile anything. Or did I miss something?

Personally I think this should be an opt-in with cargo check --doc or cargo check --all-targets.

Is it possible that rustdoc could generate duplicate warnings from rustc? I believe missing_docs would be duplicated, are there other warnings that could cause that?

The only warnings rustdoc emits are here: https://github.com/rust-lang/rust/blob/a1a13b2bc4fa6370b9501135d97c5fe0bc401894/src/librustdoc/core.rs#L336-L350

I think of those renamed_and_removed_lints is the only one run in rustc other than missing_docs. I find the code around lints really hard to follow though, so I could be wrong. -A missing_docs -A renamed_and_removed_lints to rustdoc seems like a good idea.

Have you checked to see how slow it is on medium-to-large libraries?

Slow. rust-lang/rust#78761

Since rust-lang/rust#80527 I think cargo can solve the duplicate lint problem generally by passing -A warnings -W rustdoc::all.

I guess that's subtly different because it also warns about lints that are allowed by default. So -A missing_docs -A renamed_and_removed_lints -A unknown_lints is probably better. https://github.com/rust-lang/rust/blob/8fd946c63a6c3aae9788bd459d278cb2efa77099/src/librustdoc/core.rs#L261-L267

GuillaumeGomez · 2020-11-16T20:27:52Z

So, it's now working as expected. Remains the one question: should we disable the missing_docs lint run on rustc and only allow it on rustdoc? We can handle that easily by setting the rustdoc lint level to allow. What do you think?

ehuss · 2020-11-30T15:04:50Z

I think initially, we probably shouldn't worry too much about missing_docs, since it is allow-by-default. We've also discussed adding de-duplication logic to Cargo to automatically remove duplicate messages, so maybe that could be used in the future to solve that problem.

From a high level, some changes that I think should be made:

This still needs explicit unstable flag for cargo check.
The unit graph should only be built once when running cargo check, so calling doc::exec_doc as a second step is probably not the right way to approach it.
This will need some tests.

GuillaumeGomez · 2020-12-01T15:45:45Z

* This still needs explicit unstable flag for `cargo check`.

What is the process in cargo? Do you prefer to wait for the option to become stable in rustdoc first?

* The unit graph should only be built once when running `cargo check`, so calling `doc::exec_doc` as a second step is probably not the right way to approach it.

When running it, it didn't recompile anything there, or did I miss something?

* This will need some tests.

👍

ehuss · 2020-12-01T16:13:33Z

What is the process in cargo? Do you prefer to wait for the option to become stable in rustdoc first?

Usually just add a -Z flag to Cargo to enable the behavior. More information about adding -Z flags can be found in https://github.com/rust-lang/cargo/blob/master/src/cargo/core/features.rs (see the CliUnstable struct) and https://doc.crates.io/contrib/process/unstable.html.

When running it, it didn't recompile anything there, or did I miss something?

It's not about recompiling, but re-running the resolver and other steps which can be expensive. It also causes some messages to be displayed multiple times (such as the Finished line).

GuillaumeGomez · 2020-12-03T11:00:06Z

@ehuss Wouldn't it be better to just run it by default if you're on nightly?

jyn514 · 2020-12-03T12:53:23Z

@GuillaumeGomez I really don't like unstable features being on by default in nightly, that gave people a lot of trouble when we did it for intra-doc-links: rust-lang/rust#63305
It also meant we had to stabilize them because we couldn't realistically remove them, too many people were using them without realizing they were unstable. I would much rather use feature gates so people at least have to opt in to using the unstable feature.

GuillaumeGomez · 2020-12-03T12:56:41Z

The problem is that if it requires action from users, well, it will never be used...

ehuss · 2021-01-13T00:17:40Z

Ping @GuillaumeGomez: do you think you'll be able to address the concerns noted above? I recognize that when unstable features are gated behind an explicit opt-in that it is difficult to get users to test it, but that is the procedure that we use.

GuillaumeGomez · 2021-01-13T10:19:29Z

I'll try to get back to it.

bors · 2021-03-04T20:19:03Z

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

daxpedda · 2021-03-05T10:35:25Z

Missing this broke tooling for me a bit:
rust-analyzer uses check and can be configured to run clippy (which runs on top of check) instead too, this way all warnings showed up in the editor. rust-analyzer can't be configured to run two commands.

I guess this could be solved on rust-analyzers side too.

ehuss · 2021-08-06T19:36:54Z

I'm gonna close this due to inactivity. Feel free to reopen or create a new PR when you've got time to work on this again. Thanks!

rust-highfive assigned ehuss Nov 13, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 13, 2020

ehuss reviewed Nov 15, 2020

View reviewed changes

Add --check for rustdoc

991eb2d

GuillaumeGomez force-pushed the rustdoc-check branch from 07660f2 to 991eb2d Compare November 16, 2020 13:22

GuillaumeGomez mentioned this pull request Dec 27, 2020

Add missing_docs lint to the rustdoc lint group rust-lang/rust#77364

Closed

ehuss added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 6, 2021

jyn514 mentioned this pull request Jan 18, 2021

cargo fix doesn't work for rustdoc lints #13025

Open

ehuss mentioned this pull request Jul 12, 2021

Cargo check does not check doctests #6424

Open

ehuss closed this Aug 6, 2021

GuillaumeGomez mentioned this pull request Nov 1, 2021

Add --check option for cargo doc #10026

Closed

sharnoff mentioned this pull request Oct 4, 2022

Run rustdoc --check in CI to prevent broken intra-doc links neondatabase/neon#2557

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --check for rustdoc #8859

Add --check for rustdoc #8859

GuillaumeGomez commented Nov 13, 2020 •

edited

Loading

ehuss left a comment

ehuss Nov 15, 2020

GuillaumeGomez Nov 15, 2020

ehuss Nov 15, 2020

GuillaumeGomez Nov 15, 2020

jyn514 Nov 22, 2020

jyn514 Mar 5, 2021

jyn514 Mar 5, 2021

GuillaumeGomez commented Nov 16, 2020

ehuss commented Nov 30, 2020

GuillaumeGomez commented Dec 1, 2020

ehuss commented Dec 1, 2020

GuillaumeGomez commented Dec 3, 2020

jyn514 commented Dec 3, 2020

GuillaumeGomez commented Dec 3, 2020

ehuss commented Jan 13, 2021

GuillaumeGomez commented Jan 13, 2021

bors commented Mar 4, 2021

daxpedda commented Mar 5, 2021

ehuss commented Aug 6, 2021

Add --check for rustdoc #8859

Add --check for rustdoc #8859

Conversation

GuillaumeGomez commented Nov 13, 2020 • edited Loading

ehuss left a comment

Choose a reason for hiding this comment

ehuss Nov 15, 2020

Choose a reason for hiding this comment

GuillaumeGomez Nov 15, 2020

Choose a reason for hiding this comment

ehuss Nov 15, 2020

Choose a reason for hiding this comment

GuillaumeGomez Nov 15, 2020

Choose a reason for hiding this comment

jyn514 Nov 22, 2020

Choose a reason for hiding this comment

jyn514 Mar 5, 2021

Choose a reason for hiding this comment

jyn514 Mar 5, 2021

Choose a reason for hiding this comment

GuillaumeGomez commented Nov 16, 2020

ehuss commented Nov 30, 2020

GuillaumeGomez commented Dec 1, 2020

ehuss commented Dec 1, 2020

GuillaumeGomez commented Dec 3, 2020

jyn514 commented Dec 3, 2020

GuillaumeGomez commented Dec 3, 2020

ehuss commented Jan 13, 2021

GuillaumeGomez commented Jan 13, 2021

bors commented Mar 4, 2021

daxpedda commented Mar 5, 2021

ehuss commented Aug 6, 2021

GuillaumeGomez commented Nov 13, 2020 •

edited

Loading