Public in private manifest errors #16002

jneem · 2025-09-23T17:09:33Z

What does this PR try to resolve?

This is a stab at #13096, adding Cargo.toml context to public-in-private errors. It intercepts public-in-private warnings from rustc and modifies them by adding a little context.

I had a bit of difficulty with renamed crates (like, foo_renamed = { version = "0.1.0", package = "foo" }). Surprisingly for me, rustc's errors mention the original package name (foo) and not the renamed one (foo_renamed), even though the renamed one is that one that's guaranteed unique. This PR does its best to figure out which package is the one being referred to, but if there's non-uniqueness involved then it just gives up and doesn't provide context.

rustbot · 2025-09-23T17:09:40Z

r? @epage

rustbot has assigned @epage.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Muscraft

Thanks for working on this!

src/cargo/core/compiler/mod.rs

Muscraft · 2025-09-25T00:22:46Z

src/cargo/core/compiler/mod.rs

+        static PRIV_DEP_REGEX: LazyLock<Regex> = LazyLock::new(|| {
+            Regex::new("from private dependency '([A-Za-z0-9-_]+)' in public interface").unwrap()
+        });
+        if let Some(crate_name) = PRIV_DEP_REGEX.captures(diag).and_then(|m| m.get(1)) {


Instead of using a regex, it would probably be better to get the UnitDeps for a Unit¹, and iterate over them until one of their names or dep_name matches what's in the string. Using the UnitDep would also allow us to always find the correct dep in the manifest.

Footnotes

build_runner.unit_deps(unit) ↩

I didn't quite follow the "Instead of using a regex" part, because don't we still need to parse the compiler diagnostic (either with regex or some other string manipulation) to figure out the dependency's name?

I'm also having trouble with the "always" in "would also allow us to always find the correct dep in the manifest", because in my testing if you have a duplicate renamed crate like

dep1 = { version = "0.1.0", package = "foo" } dep2 = { path = "../something", package = "foo" }

then rustc will just complain about "foo" with no way I can see to disambiguate.

@Muscraft is your expectation that we would do dep_names.any(|dep_name| diag.contains(dep_name))? If so, then that can lead to false positives if a dep name is a generic word that is also contained in the error message.

I suspect we'll need to parse the string either way which is unfortunate. We do parse some rustc error messages but we need to be careful because it does cause update problems between the repos. We should probably make sure the team is ok with this before moving forward.

src/cargo/core/compiler/mod.rs

Muscraft · 2025-09-25T00:32:56Z

src/cargo/core/compiler/mod.rs

+fn find_crate_span<'doc>(
+    unrenamed: &str,
+    manifest_spans: &'doc toml::Spanned<toml::de::DeTable<'static>>,
+) -> Option<Range<usize>> {
+    // Easy case: there's a dependency whose key matches the name, and it didn't get renamed.
+    if let Some((k, v)) = get_key_value(manifest_spans, &["dependencies", unrenamed]) {
+        let Some(table) = v.get_ref().as_table() else {
+            return Some(k.span());
+        };
+        if !table.contains_key("package") {
+            return Some(k.span());
+        }
+    }
+
+    let Some(deps) =
+        get_key_value(manifest_spans, &["dependencies"]).and_then(|(_k, v)| v.get_ref().as_table())
+    else {
+        return None;
+    };
+
+    let mut unique_crate = None;
+    for v in deps.values() {
+        if let Some(package) = v.as_ref().as_table().and_then(|t| t.get("package")) {
+            if package.get_ref().as_str() == Some(unrenamed) {
+                if unique_crate.is_some() {
+                    // There were two dependencies with the same un-renamed name.
+                    return None;
+                } else {
+                    unique_crate = Some(package.span());
+                }
+            }
+        }
+    }
+
+    unique_crate
+}


This needs to be modified to check all dependency tables, as well as handle inherited dependencies.

I think I have them all now. I'm checking all the [target.<something>.dependencies] tables. For workspace dependencies, I'm pointing the the location in the crate manifest, not the workspace manifest (while using UnitDeps to handle workspace-level renames correctly)

src/cargo/core/compiler/mod.rs

epage · 2025-09-25T18:22:24Z

src/cargo/core/compiler/mod.rs

+    // Returns `true` if the diagnostic was modified.
+    let extra_diag_message = |diag: &mut String| -> bool {
+        static PRIV_DEP_REGEX: LazyLock<Regex> = LazyLock::new(|| {
+            Regex::new("from private dependency '([A-Za-z0-9-_]+)' in public interface").unwrap()


This won't match the case I was just trying out

"rendered": "warning: struct `SetMatchesIntoIter` from private dependency 'regex' is re-exported\n --> regex-wrapper/src/lib.rs:1:9\n |\n1 | pub use regex::*;\n | ^^^^^^^^\n\n",

Ok, this case is now also handled. I grepped through rustc, and there are currently only two error messages that they issue for exported_private_dependencies.

epage · 2025-09-25T18:26:36Z

src/cargo/core/compiler/mod.rs

+            if let Some(span) = find_crate_span(crate_name, &manifest.spans) {
+                let rel_path = pathdiff::diff_paths(&manifest.path, &manifest.cwd)
+                    .unwrap_or_else(|| manifest.path.clone())
+                    .display()
+                    .to_string();
+                let report = [Group::with_title(
+                    Level::NOTE
+                        .secondary_title(format!("dependency `{crate_name}` declared here",)),
+                )
+                .element(
+                    Snippet::source(&manifest.contents)
+                        .path(rel_path)
+                        .annotation(AnnotationKind::Context.span(span)),
+                )];
+
+                let rendered = Renderer::styled().render(&report);
+                diag.push('\n');
+                diag.push_str(&rendered);


@Muscraft had talked about the idea of re-rendering

One of the concerns with doing so is race conditions, especially if we go after the replay log as we might need to read the file

It looks like there might be enough information for a rudimentary message without reading the file as we get the text for the line of the diagnostic report.

{ "reason": "compiler-message", "package_id": "path+file:///home/epage/src/personal/dump/cargo-public-private/regex-wrapper#0.1.0", "manifest_path": "/home/epage/src/personal/dump/cargo-public-private/regex-wrapper/Cargo.toml", "target": { "kind": [ "lib" ], "crate_types": [ "lib" ], "name": "regex_wrapper", "src_path": "/home/epage/src/personal/dump/cargo-public-private/regex-wrapper/src/lib.rs", "edition": "2024", "doc": true, "doctest": true, "test": true }, "message": { "rendered": "warning: struct `Regex` from private dependency 'regex' is re-exported\n --> regex-wrapper/src/lib.rs:4:9\n |\n4 | pub use regex::Regex; // Comment\n | ^^^^^^^^^^^^\n |\n = note: `#[warn(exported_private_dependencies)]` on by default\n\n", "$message_type": "diagnostic", "children": [ { "children": [], "code": null, "level": "note", "message": "`#[warn(exported_private_dependencies)]` on by default", "rendered": null, "spans": [] } ], "code": { "code": "exported_private_dependencies", "explanation": null }, "level": "warning", "message": "struct `Regex` from private dependency 'regex' is re-exported", "spans": [ { "byte_end": 46, "byte_start": 34, "column_end": 21, "column_start": 9, "expansion": null, "file_name": "regex-wrapper/src/lib.rs", "is_primary": true, "label": null, "line_end": 4, "line_start": 4, "suggested_replacement": null, "suggestion_applicability": null, "text": [ { "highlight_end": 21, "highlight_start": 9, "text": "pub use regex::Regex; // Comment" } ] } ] } }

epage · 2025-09-25T20:23:10Z

For the ambiguous dependency name in error case, I started a zulip thread at #t-compiler > Confusing diagnostics with renamed dependencies @ 💬

epage · 2025-09-29T16:46:14Z

r? @Muscraft

src/cargo/core/compiler/mod.rs

Muscraft · 2025-09-30T22:22:19Z

src/cargo/core/compiler/mod.rs

+            requested_kinds: bcx.target_data.requested_kinds().to_owned(),
+            host_name: bcx.rustc().host,
+            rename_table,
+            cwd: bcx.gctx.cwd().to_owned(),


This cwd is the user's actual cwd, which can be different from the cwd we pass to rustc (explanation here). The path_args function will give us the correct one (I think it's the second PathBuf).

Suggested change

cwd: bcx.gctx.cwd().to_owned(),

cwd: path_args(build_runner.bcx.ws, unit).1,

Test case

#[cargo_test(nightly, reason = "exported_private_dependencies lint is unstable")] fn relative_display_path() { Package::new("priv_dep", "0.1.0") .file("src/lib.rs", "pub struct FromPriv;") .publish(); let p = project() .file( "Cargo.toml", r#" [workspace] members = ["foo"] "#, ) .file( "foo/Cargo.toml", r#" [package] name = "foo" version = "0.0.1" edition = "2015" [dependencies] priv_dep = "0.1.0" "#, ) .file( "foo/src/lib.rs", " extern crate priv_dep; pub use priv_dep::FromPriv; ", ) .build(); p.cargo("check -Zpublic-dependency") .cwd("foo") .masquerade_as_nightly_cargo(&["public-dependency"]) .with_stderr_data(str![[r#" [UPDATING] `dummy-registry` index [LOCKING] 1 package to latest compatible version [DOWNLOADING] crates ... [DOWNLOADED] priv_dep v0.1.0 (registry `dummy-registry`) [CHECKING] priv_dep v0.1.0 [CHECKING] foo v0.0.1 ([ROOT]/foo/foo) [WARNING] struct `FromPriv` from private dependency 'priv_dep' is re-exported --> foo/src/lib.rs:3:21 | 3 | pub use priv_dep::FromPriv; | ^^^^^^^^^^^^^^^^^^ | = [NOTE] `#[warn(exported_private_dependencies)]` on by default [NOTE] dependency `priv_dep` declared here --> Cargo.toml:8:17 | 8 | priv_dep = "0.1.0" | -------- [WARNING] `foo` (lib) generated 1 warning [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s "#]]) .run();

Isn't the user's cwd the better one for the error message? In the test case, the error message says that the problem is in "Cargo.toml" but from the user's point of view isn't it in "foo/Cargo.toml"? I originally got this behavior from rel_cwd_manifest_path, which is used in other lints.

Isn't the user's cwd the better one for the error message?

In this case, no, since it wouldn't match rustc's output

In the test case, the error message says that the problem is in "Cargo.toml" but from the user's point of view isn't it in "foo/Cargo.toml"?

I should have specified that the test case is what the current output is, because you are correct, from the user's point of view it is in foo/Cargo.toml. If you use the PathBuf from path_args and update the test it should show foo/Cargo.toml.

I originally got this behavior from rel_cwd_manifest_path, which is used in other lints.

The linting system has the same problem, unfortunately (which I did not realize until I today 😅).

Got it, thanks for the explanation!

Add a workspace test

Co-authored-by: Scott Schafer <scott@scottschafer.dev>

Update cargo submodule 24 commits in f2932725b045d361ff5f18ba02b1409dd1f44e71..2394ea6cea8b26d717aa67eb1663a2dbf2d26078 2025-09-24 11:31:26 +0000 to 2025-10-03 14:13:01 +0000 - Recommend `package.rust-version` in the Rust version section of `reference/semver.md`. (rust-lang/cargo#15806) - fix(toml): Prevent non-script fields in Cargo scripts (rust-lang/cargo#16026) - chore(ci): unpin libc (rust-lang/cargo#16044) - chore: Update dependencies (rust-lang/cargo#16034) - Fix FileLock path tracking after rename in package operation (rust-lang/cargo#16036) - Lockfile schemas error cleanup (rust-lang/cargo#16039) - Convert a multi-part diagnostic to a report (rust-lang/cargo#16035) - fix(run): Override arg0 for cargo scripts (rust-lang/cargo#16027) - Public in private manifest errors (rust-lang/cargo#16002) - chore(deps): update actions/checkout action to v5 (rust-lang/cargo#16031) - fix: remove FIXME comment that's no longer a problem (rust-lang/cargo#16025) - Add retry for `git fetch` failures in `CARGO_NET_GIT_FETCH_WITH_CLI` path (rust-lang/cargo#16016) - Added better filesystem layout testing harness (rust-lang/cargo#15874) - Small cleanup to normalize_dependencies (rust-lang/cargo#16022) - fix: better error message for rust version incompatibility (rust-lang/cargo#16021) - fix(shell): Use a distinct style for transient status (rust-lang/cargo#16019) - chore(deps): Depend on `serde_core` in `cargo-platform` (rust-lang/cargo#15992) - Remove package-workspace from unstable doc index (rust-lang/cargo#16014) - fix(shell): Switch to annotate snippets for notes (rust-lang/cargo#15945) - docs: update changelog (rust-lang/cargo#15986) - chore(ci): add rustfmt for docs job (rust-lang/cargo#16013) - chore: bump to 0.93.0 (rust-lang/cargo#16009) - fix(config): combine key error context into one (rust-lang/cargo#16004) - test(docker): openssh requires a newer libcrypto3 (rust-lang/cargo#16010) r? ghost

rustbot added A-build-execution Area: anything dealing with executing the compiler S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 23, 2025

rustbot assigned epage Sep 23, 2025

Muscraft reviewed Sep 25, 2025

View reviewed changes

epage reviewed Sep 25, 2025

View reviewed changes

src/cargo/core/compiler/mod.rs Show resolved Hide resolved

epage reviewed Sep 25, 2025

View reviewed changes

jneem force-pushed the public-in-private-manifest-errors branch from 493c981 to f3eb177 Compare September 29, 2025 16:19

rustbot added the A-cfg-expr Area: Platform cfg expressions label Sep 29, 2025

jneem force-pushed the public-in-private-manifest-errors branch from f3eb177 to eb77a74 Compare September 29, 2025 16:25

jneem requested review from Muscraft and epage September 29, 2025 16:26

rustbot assigned Muscraft and unassigned epage Sep 29, 2025

Muscraft reviewed Sep 29, 2025

View reviewed changes

src/cargo/core/compiler/mod.rs Outdated Show resolved Hide resolved

src/cargo/core/compiler/mod.rs Outdated Show resolved Hide resolved

src/cargo/core/compiler/mod.rs Outdated Show resolved Hide resolved

jneem force-pushed the public-in-private-manifest-errors branch from cf44f7b to 0dd82b3 Compare September 30, 2025 15:07

jneem requested a review from Muscraft September 30, 2025 15:40

Muscraft reviewed Sep 30, 2025

View reviewed changes

jneem and others added 2 commits September 30, 2025 22:19

Add new tests

db6b2e4

Add a workspace test

Add a manifest location to public-in-private errors

ea98c6b

Co-authored-by: Scott Schafer <scott@scottschafer.dev>

jneem force-pushed the public-in-private-manifest-errors branch from 0dd82b3 to ea98c6b Compare October 1, 2025 03:26

jneem requested a review from Muscraft October 1, 2025 03:26

Muscraft approved these changes Oct 1, 2025

View reviewed changes

Muscraft added this pull request to the merge queue Oct 1, 2025

Merged via the queue into rust-lang:master with commit da81ca3 Oct 1, 2025
25 checks passed

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 1, 2025

weihanglo mentioned this pull request Oct 3, 2025

Update cargo submodule rust-lang/rust#147301

Merged

rustbot added this to the 1.92.0 milestone Oct 3, 2025

	cwd: bcx.gctx.cwd().to_owned(),
	cwd: path_args(build_runner.bcx.ws, unit).1,

Public in private manifest errors #16002

Public in private manifest errors #16002

Conversation

jneem commented Sep 23, 2025

What does this PR try to resolve?

Uh oh!

rustbot commented Sep 23, 2025

Uh oh!

Muscraft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epage commented Sep 25, 2025

Uh oh!

epage commented Sep 29, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!