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

Add --check for rustdoc #8859

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/bin/cargo/commands/check.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
use crate::command_prelude::*;
use crate::commands::doc;

use cargo::core::features;
use cargo::ops;

pub fn cli() -> App {
Expand Down Expand Up @@ -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() {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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 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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

doc::exec_doc(
config,
args,
CompileMode::DocCheck,
ProfileChecking::Checked,
)?;
}

Ok(())
}
54 changes: 46 additions & 8 deletions src/bin/cargo/commands/doc.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use crate::command_prelude::*;

use cargo::core::features;
use cargo::ops::{self, DocOptions};

pub fn cli() -> App {
Expand All @@ -23,6 +24,7 @@ pub fn cli() -> App {
"Document only the specified binary",
"Document all binaries",
)
.arg(opt("check", "Runs `rustdoc --check` (nightly only)"))
.arg_release("Build artifacts in release mode, with optimizations")
.arg_profile("Build artifacts with the specified profile")
.arg_features()
Expand All @@ -35,18 +37,54 @@ pub fn cli() -> App {
}

pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
if args.is_present("check") {
if !features::nightly_features_allowed() {
Err(CliError::new(
anyhow::format_err!("This option is only available in nightly"),
1,
))?;
}
exec_doc(
config,
args,
CompileMode::DocCheck,
ProfileChecking::Unchecked,
)
} else {
exec_doc(
config,
args,
CompileMode::Doc {
deps: !args.is_present("no-deps"),
},
ProfileChecking::Checked,
)
}
}

pub fn exec_doc(
config: &mut Config,
args: &ArgMatches<'_>,
mode: CompileMode,
profile: ProfileChecking,
) -> CliResult {
let ws = args.workspace(config)?;
let mode = CompileMode::Doc {
deps: !args.is_present("no-deps"),
};
let mut compile_opts =
args.compile_options(config, mode, Some(&ws), ProfileChecking::Checked)?;
compile_opts.rustdoc_document_private_items = args.is_present("document-private-items");

let doc_opts = DocOptions {
open_result: args.is_present("open"),
let mut compile_opts = args.compile_options(config, mode, Some(&ws), profile)?;

if !mode.is_check() {
compile_opts.rustdoc_document_private_items = args.is_present("document-private-items");
}

let mut doc_opts = DocOptions {
open_result: false,
compile_opts,
};

if !mode.is_check() {
doc_opts.open_result = args.is_present("open");
}

ops::doc(&ws, &doc_opts)?;
Ok(())
}
9 changes: 7 additions & 2 deletions src/cargo/core/compiler/build_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,8 @@ pub enum CompileMode {
/// Building a target with `rustc` to emit `rmeta` metadata only. If
/// `test` is true, then it is also compiled with `--test` to check it like
/// a test.
///
/// It then runs `rustdoc --check`.
Check { test: bool },
/// Used to indicate benchmarks should be built. This is not used in
/// `Unit`, because it is essentially the same as `Test` (indicating
Expand All @@ -143,6 +145,8 @@ pub enum CompileMode {
Doc { deps: bool },
/// A target that will be tested with `rustdoc`.
Doctest,
/// Runs `rustdoc --check`.
DocCheck,
/// A marker for Units that represent the execution of a `build.rs` script.
RunCustomBuild,
}
Expand All @@ -160,6 +164,7 @@ impl ser::Serialize for CompileMode {
Bench => "bench".serialize(s),
Doc { .. } => "doc".serialize(s),
Doctest => "doctest".serialize(s),
DocCheck => "doccheck".serialize(s),
RunCustomBuild => "run-custom-build".serialize(s),
}
}
Expand All @@ -168,12 +173,12 @@ impl ser::Serialize for CompileMode {
impl CompileMode {
/// Returns `true` if the unit is being checked.
pub fn is_check(self) -> bool {
matches!(self, CompileMode::Check { .. })
matches!(self, CompileMode::Check { .. } | CompileMode::DocCheck)
}

/// Returns `true` if this is generating documentation.
pub fn is_doc(self) -> bool {
matches!(self, CompileMode::Doc { .. })
matches!(self, CompileMode::Doc { .. } | CompileMode::DocCheck)
}

/// Returns `true` if this a doc test.
Expand Down
5 changes: 4 additions & 1 deletion src/cargo/core/compiler/build_context/target_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,10 @@ impl TargetInfo {
}
}
CompileMode::Check { .. } => Ok((vec![FileType::new_rmeta()], Vec::new())),
CompileMode::Doc { .. } | CompileMode::Doctest | CompileMode::RunCustomBuild => {
CompileMode::Doc { .. }
| CompileMode::Doctest
| CompileMode::RunCustomBuild
| CompileMode::DocCheck => {
panic!("asked for rustc output for non-rustc mode")
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/core/compiler/context/compilation_files.rs
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,7 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
// outputs.
vec![]
}
CompileMode::Doctest => {
CompileMode::Doctest | CompileMode::DocCheck => {
// Doctests are built in a temporary directory and then
// deleted. There is the `--persist-doctests` unstable flag,
// but Cargo does not know about that.
Expand Down
20 changes: 15 additions & 5 deletions src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -562,12 +562,14 @@ fn rustdoc(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult<Work> {

let doc_dir = cx.files().out_dir(unit);

// Create the documentation directory ahead of time as rustdoc currently has
// a bug where concurrent invocations will race to create this directory if
// it doesn't already exist.
paths::create_dir_all(&doc_dir)?;
if !unit.mode.is_check() {
// Create the documentation directory ahead of time as rustdoc currently has
// a bug where concurrent invocations will race to create this directory if
// it doesn't already exist.
paths::create_dir_all(&doc_dir)?;

rustdoc.arg("-o").arg(doc_dir);
rustdoc.arg("-o").arg(doc_dir);
}

for feat in &unit.features {
rustdoc.arg("--cfg").arg(&format!("feature=\"{}\"", feat));
Expand Down Expand Up @@ -596,6 +598,14 @@ 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("-Z");
rustdoc.arg("unstable-options");
rustdoc.arg("--check");
// rustdoc.arg("--warn");
// rustdoc.arg("rustdoc");
}

Ok(Work::new(move |state| {
if let Some(script_metadata) = script_metadata {
if let Some(output) = build_script_outputs
Expand Down
4 changes: 3 additions & 1 deletion src/cargo/core/compiler/timings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,9 @@ impl<'cfg> Timings<'cfg> {
CompileMode::Test => target.push_str(" (test)"),
CompileMode::Build => {}
CompileMode::Check { test: true } => target.push_str(" (check-test)"),
CompileMode::Check { test: false } => target.push_str(" (check)"),
CompileMode::Check { test: false } | CompileMode::DocCheck => {
target.push_str(" (check)")
}
CompileMode::Bench => target.push_str(" (bench)"),
CompileMode::Doc { .. } => target.push_str(" (doc)"),
CompileMode::Doctest => target.push_str(" (doc test)"),
Expand Down
5 changes: 4 additions & 1 deletion src/cargo/core/profiles.rs
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,10 @@ impl Profiles {
)
}
}
CompileMode::Build | CompileMode::Check { .. } | CompileMode::RunCustomBuild => {
CompileMode::Build
| CompileMode::Check { .. }
| CompileMode::RunCustomBuild
| CompileMode::DocCheck => {
// Note: `RunCustomBuild` doesn't normally use this code path.
// `build_unit_profiles` normally ensures that it selects the
// ancestor's profile. However, `cargo clean -p` can hit this
Expand Down
28 changes: 15 additions & 13 deletions src/cargo/ops/cargo_compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,8 @@ pub fn create_bcx<'a, 'cfg>(
| CompileMode::Build
| CompileMode::Check { .. }
| CompileMode::Bench
| CompileMode::RunCustomBuild => {
| CompileMode::RunCustomBuild
| CompileMode::DocCheck => {
if std::env::var("RUST_FLAGS").is_ok() {
config.shell().warn(
"Cargo does not read `RUST_FLAGS` environment variable. Did you mean `RUSTFLAGS`?",
Expand Down Expand Up @@ -678,17 +679,18 @@ impl CompileFilter {
match mode {
CompileMode::Test | CompileMode::Doctest | CompileMode::Bench => true,
CompileMode::Check { test: true } => true,
CompileMode::Build | CompileMode::Doc { .. } | CompileMode::Check { test: false } => {
match *self {
CompileFilter::Default { .. } => false,
CompileFilter::Only {
ref examples,
ref tests,
ref benches,
..
} => examples.is_specific() || tests.is_specific() || benches.is_specific(),
}
}
CompileMode::Build
| CompileMode::Doc { .. }
| CompileMode::Check { test: false }
| CompileMode::DocCheck => match *self {
CompileFilter::Default { .. } => false,
CompileFilter::Only {
ref examples,
ref tests,
ref benches,
..
} => examples.is_specific() || tests.is_specific() || benches.is_specific(),
},
CompileMode::RunCustomBuild => panic!("Invalid mode"),
}
}
Expand Down Expand Up @@ -1177,7 +1179,7 @@ fn filter_default_targets(targets: &[Target], mode: CompileMode) -> Vec<&Target>
.iter()
.filter(|t| t.tested() || t.is_example())
.collect(),
CompileMode::Build | CompileMode::Check { .. } => targets
CompileMode::Build | CompileMode::Check { .. } | CompileMode::DocCheck => targets
.iter()
.filter(|t| t.is_bin() || t.is_lib())
.collect(),
Expand Down