Skip to content
Open
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
37 changes: 36 additions & 1 deletion src/cargo/core/compiler/unit_dependencies.rs
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently cargo doc generates docs for all transitive dependencies, which creates a lot of noise.

This allows library authors to control their documentation surface by
marking which dependencies are part of their public API.

The reason is that indirect private deps have no impact on the reader of the docs since they cannot be used and this can dramatically speed up documentation builds, especially with packages like windows-sys in a dependency tree

Copy link
Contributor

Choose a reason for hiding this comment

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

No test updates are in this commit. Either the tests in the previous commit are failing or there is a mismatch in expectations between the tests and this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

This was marked as ready for review but this doesn't seem to be addressed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any questions about this has there have been more commits pushed without addressing this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies for the confusion! I did address the feedback:

  1. Simplified the boolean expression from .require().is_ok() to .is_enabled() as you suggested
  2. Verified all tests pass including the public-dependency tests

I should have been clearer in my commit messages about which feedback was being addressed. The code changes are now in place and working correctly. Is there anything specific you'd like me to clarify or improve further?

Copy link
Contributor

Choose a reason for hiding this comment

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

Unsure if this has been said in this thread before but we find it helpful for authors, reviewers, and the community when a PR has the tests added in their own commit before the fix with them passing, reproducing the existing behavior. The commit with the fix would then update the tests to still pass and show the new behavior. The diff would show how behavior changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in latest commit, just curious to confirm we do this before fix test + fix and updated test commit method for all the code changes PRs ?

Thanks

Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,12 @@ struct State<'a, 'gctx> {
/// dependency from a to b was added purely because it was a dev-dependency.
/// This is used during `connect_run_custom_build_deps`.
dev_dependency_edges: HashSet<(Unit, Unit)>,

/// The set of packages explicitly selected by the user (e.g. via `-p`).
/// Used to determine whether to apply the public-dependency doc filter:
/// only root packages bypass the filter; their deps must be public to
/// have their own docs generated.
root_pkg_ids: HashSet<PackageId>,
}

/// A boolean-like to indicate if a `Unit` is an artifact or not.
Expand Down Expand Up @@ -126,6 +132,7 @@ pub fn build_unit_dependencies<'a, 'gctx>(
interner,
scrape_units,
dev_dependency_edges: HashSet::new(),
root_pkg_ids: roots.iter().map(|u| u.pkg.package_id()).collect(),
};

let std_unit_deps = calc_deps_of_std(&mut state, std_roots)?;
Expand Down Expand Up @@ -637,6 +644,21 @@ fn compute_deps_doc(
// built. If we're documenting *all* libraries, then we also depend on
// the documentation of the library being built.
let mut ret = Vec::new();

// Check if public-dependency feature is enabled
let public_deps_enabled = state.gctx.cli_unstable().public_dependency
|| unit
.pkg
.manifest()
.unstable_features()
.is_enabled(Feature::public_dependency());

// Whether this package was explicitly selected by the user (e.g. via `-p`).
// User-selected packages always have all their direct deps documented,
// regardless of public/private status. For everything else, only public
// deps get their own docs generated when public-dependency is enabled.
let is_user_selected = state.root_pkg_ids.contains(&unit.pkg.package_id());

for (id, deps) in state.deps(unit, unit_for) {
let Some(dep_lib) = calc_artifact_deps(unit, unit_for, id, &deps, state, &mut ret)? else {
continue;
Expand All @@ -657,7 +679,20 @@ fn compute_deps_doc(
IS_NO_ARTIFACT_DEP,
)?;
ret.push(lib_unit_dep);
if dep_lib.documented() && state.intent.wants_deps_docs() {

// Decide whether to document this dependency.
// When public-dependency is enabled, only document:
// - Direct dependencies of user-selected packages
// - Public dependencies (recursively)
// This dramatically speeds up documentation builds by excluding indirect
// private dependencies that cannot be used by readers of the docs.
let should_doc_dep = if is_user_selected || !public_deps_enabled {
true
} else {
state.resolve().is_public_dep(unit.pkg.package_id(), id)
};

if dep_lib.documented() && state.intent.wants_deps_docs() && should_doc_dep {
// Document this lib as well.
let doc_unit_dep = new_unit_dep(
state,
Expand Down
330 changes: 330 additions & 0 deletions tests/testsuite/doc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4029,3 +4029,333 @@ fn mergeable_info_dep_collision() {
// ...and the fingerprint content are different (path to dep.json different)
assert_ne!(first_fingerprint, second_fingerprint);
}

#[cargo_test(nightly, reason = "public-dependency feature is unstable")]
fn doc_with_public_dependency_transitive() {
// selected is the user-chosen package
// foo-dep is a direct dep of selected
// public-bar-dep is a public dep of foo-dep
// public-baz-dep is a public dep of public-bar-dep
// All four should be documented since the whole chain is public.

Package::new("public-baz-dep", "0.0.1")
.file("src/lib.rs", "pub fn public_baz_dep() {}")
.publish();

Package::new("public-bar-dep", "0.0.1")
.cargo_feature("public-dependency")
.add_dep(
cargo_test_support::registry::Dependency::new("public-baz-dep", "0.0.1").public(true),
)
.file("src/lib.rs", "pub fn public_bar_dep() {}")
.publish();

Package::new("foo-dep", "0.0.1")
.cargo_feature("public-dependency")
.add_dep(
cargo_test_support::registry::Dependency::new("public-bar-dep", "0.0.1").public(true),
)
.file("src/lib.rs", "pub fn foo_dep() {}")
.publish();

let p = project()
.file(
"Cargo.toml",
r#"
cargo-features = ["public-dependency"]

[package]
name = "selected"
version = "0.0.1"
edition = "2021"

[dependencies]
foo-dep = "0.0.1"
"#,
)
.file("src/lib.rs", "pub fn selected() {}")
.build();

p.cargo("doc -Zpublic-dependency")
.masquerade_as_nightly_cargo(&["public-dependency"])
.with_stderr_data(
str![[r#"
[UPDATING] `dummy-registry` index
[LOCKING] 3 packages to latest compatible versions
[DOWNLOADING] crates ...
[DOWNLOADED] public-baz-dep v0.0.1 (registry `dummy-registry`)
[DOWNLOADED] public-bar-dep v0.0.1 (registry `dummy-registry`)
[DOWNLOADED] foo-dep v0.0.1 (registry `dummy-registry`)
[DOCUMENTING] public-baz-dep v0.0.1
[CHECKING] public-baz-dep v0.0.1
[DOCUMENTING] public-bar-dep v0.0.1
[CHECKING] public-bar-dep v0.0.1
[DOCUMENTING] foo-dep v0.0.1
[CHECKING] foo-dep v0.0.1
[DOCUMENTING] selected v0.0.1 ([ROOT]/foo)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
[GENERATED] [ROOT]/foo/target/doc/selected/index.html

"#]]
.unordered(),
)
.run();

// All four are documented: the whole chain is public.
assert!(p.root().join("target/doc/selected/index.html").is_file());
assert!(p.root().join("target/doc/foo_dep/index.html").is_file());
assert!(
p.root()
.join("target/doc/public_bar_dep/index.html")
.is_file()
);
assert!(
p.root()
.join("target/doc/public_baz_dep/index.html")
.is_file()
);
}

#[cargo_test(nightly, reason = "public-dependency feature is unstable")]
fn doc_direct_deps_always_documented() {
// Direct dependencies should always be documented regardless of public flag
// foo -> bar (public=true), baz (public=false)
// Both bar and baz should be documented since they are direct deps

Package::new("bar", "0.0.1")
.file("src/lib.rs", "pub fn bar() {}")
.publish();

Package::new("baz", "0.0.1")
.file("src/lib.rs", "pub fn baz() {}")
.publish();

let p = project()
.file(
"Cargo.toml",
r#"
cargo-features = ["public-dependency"]

[package]
name = "foo"
version = "0.0.1"
edition = "2021"

[dependencies]
bar = { version = "0.0.1", public = true }
baz = { version = "0.0.1", public = false }
"#,
)
.file("src/lib.rs", "pub fn foo() {}")
.build();

p.cargo("doc -Zpublic-dependency")
.masquerade_as_nightly_cargo(&["public-dependency"])
.with_stderr_data(
str![[r#"
[UPDATING] `dummy-registry` index
[LOCKING] 2 packages to latest compatible versions
[DOWNLOADING] crates ...
[DOWNLOADED] bar v0.0.1 (registry `dummy-registry`)
[DOWNLOADED] baz v0.0.1 (registry `dummy-registry`)
[DOCUMENTING] bar v0.0.1
[CHECKING] bar v0.0.1
[DOCUMENTING] baz v0.0.1
[CHECKING] baz v0.0.1
[DOCUMENTING] foo v0.0.1 ([ROOT]/foo)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
[GENERATED] [ROOT]/foo/target/doc/foo/index.html

"#]]
.unordered(),
)
.run();

// Both direct deps should be documented
assert!(p.root().join("target/doc/foo/index.html").is_file());
assert!(p.root().join("target/doc/bar/index.html").is_file());
assert!(p.root().join("target/doc/baz/index.html").is_file());
}

#[cargo_test(nightly, reason = "public-dependency feature is unstable")]
fn doc_with_private_dependency() {
Copy link
Contributor

Choose a reason for hiding this comment

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

we should call out that this is a transitive private dependency

// foo -> bar (direct dep) -> baz (transitive private dep of bar)

Package::new("baz", "0.0.1")
.file("src/lib.rs", "pub fn baz() {}")
.publish();

Package::new("bar", "0.0.1")
.cargo_feature("public-dependency")
.add_dep(cargo_test_support::registry::Dependency::new("baz", "0.0.1").public(false))
.file("src/lib.rs", "pub fn bar() {}")
.publish();

let p = project()
.file(
"Cargo.toml",
r#"
cargo-features = ["public-dependency"]

[package]
name = "foo"
version = "0.0.1"
edition = "2021"

[dependencies]
bar = "0.0.1"
"#,
)
.file("src/lib.rs", "pub fn foo() {}")
.build();

p.cargo("doc -Zpublic-dependency")
.masquerade_as_nightly_cargo(&["public-dependency"])
.with_stderr_data(
str![[r#"
[UPDATING] `dummy-registry` index
[LOCKING] 2 packages to latest compatible versions
[DOWNLOADING] crates ...
[DOWNLOADED] baz v0.0.1 (registry `dummy-registry`)
[DOWNLOADED] bar v0.0.1 (registry `dummy-registry`)
[CHECKING] baz v0.0.1
[DOCUMENTING] bar v0.0.1
[CHECKING] bar v0.0.1
[DOCUMENTING] foo v0.0.1 ([ROOT]/foo)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
[GENERATED] [ROOT]/foo/target/doc/foo/index.html

"#]]
.unordered(),
)
.run();

assert!(p.root().join("target/doc/foo/index.html").is_file());
assert!(p.root().join("target/doc/bar/index.html").is_file());
assert!(!p.root().join("target/doc/baz/index.html").is_file());
}

#[cargo_test(nightly, reason = "public-dependency feature is unstable")]
fn doc_mixed_public_private_deps() {
// foo -> pub_dep (public), priv_dep (private), priv_dep_with_dep (unannotated)
// priv_dep_with_dep -> transitive

Package::new("pub_dep", "0.0.1")
.file("src/lib.rs", "pub fn pub_dep() {}")
.publish();

Package::new("priv_dep", "0.0.1")
.file("src/lib.rs", "pub fn priv_dep() {}")
.publish();

Package::new("transitive", "0.0.1")
.file("src/lib.rs", "pub fn transitive() {}")
.publish();

Package::new("priv_dep_with_dep", "0.0.1")
.dep("transitive", "0.0.1")
.file("src/lib.rs", "pub fn priv_dep_with_dep() {}")
.publish();

let p = project()
.file(
"Cargo.toml",
r#"
cargo-features = ["public-dependency"]

[package]
name = "foo"
version = "0.0.1"
edition = "2021"

[dependencies]
pub_dep = { version = "0.0.1", public = true }
priv_dep = { version = "0.0.1", public = false }
priv_dep_with_dep = "0.0.1"
"#,
)
.file("src/lib.rs", "pub fn foo() {}")
.build();

p.cargo("doc -Zpublic-dependency")
.masquerade_as_nightly_cargo(&["public-dependency"])
.run();

assert!(p.root().join("target/doc/foo/index.html").is_file());
assert!(p.root().join("target/doc/pub_dep/index.html").is_file());
assert!(p.root().join("target/doc/priv_dep/index.html").is_file());
assert!(
p.root()
.join("target/doc/priv_dep_with_dep/index.html")
.is_file()
);
assert!(!p.root().join("target/doc/transitive/index.html").is_file());
}

#[cargo_test(nightly, reason = "public-dependency feature is unstable")]
fn doc_workspace_member_private_dep() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test was added in the wrong commit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, thanks

// selected and skipped are both workspace members.
// selected has a private dep on skipped.
// skipped has a dep on transitive (a registry crate).
// Running `cargo doc -p selected`:
// - selected is documented (it is user-selected)
// - skipped is documented because it is a direct dep of selected (user-selected
// packages always have all their direct deps documented)
// - transitive is NOT documented: skipped is not user-selected, so the
// public-dependency filter applies to its deps, and transitive is not
// marked public by skipped

Package::new("transitive", "0.0.1")
.file("src/lib.rs", "pub fn transitive() {}")
.publish();

let p = project()
.file(
"Cargo.toml",
r#"
[workspace]
members = ["selected", "skipped"]
"#,
)
.file(
"selected/Cargo.toml",
r#"
cargo-features = ["public-dependency"]

[package]
name = "selected"
version = "0.0.1"
edition = "2021"

[dependencies]
skipped = { path = "../skipped", public = false }
"#,
)
.file("selected/src/lib.rs", "pub fn selected() {}")
.file(
"skipped/Cargo.toml",
r#"
[package]
name = "skipped"
version = "0.0.1"
edition = "2021"

[dependencies]
transitive = "0.0.1"
"#,
)
.file("skipped/src/lib.rs", "pub fn skipped() {}")
.build();

p.cargo("doc -p selected -Zpublic-dependency")
.masquerade_as_nightly_cargo(&["public-dependency"])
.run();

// selected is documented (user-selected)
assert!(p.root().join("target/doc/selected/index.html").is_file());
// skipped is documented because it is a direct dep of the user-selected package
assert!(p.root().join("target/doc/skipped/index.html").is_file());
// transitive is NOT documented: skipped is not user-selected, and it does
// not mark transitive as public
assert!(!p.root().join("target/doc/transitive/index.html").is_file());
}