Repository navigation
exported_private_dependencies lint only take effect in innermost dependency #119428
Description
Problem
The exported_private_dependencies lint only take affect in the innermost dependency in a recursively dependent environment.
This inspired by #44663 (comment).
Steps
In order to prove this problem, I purposely contructed a code repository, here
in the repository, the crates folder has three crates,
- grandparent_dep, the init crate provied the pub struct
- parent_dep, the middle crate, add
grandparent_depas public and reexport pub struct from grandparent_dep - pub_dep, the outmost crate, add
parent_depas public and reexport pub struct from parent_dep
(the crates in crates can be treated as download from respority(like github, crates.io))
in src/lib.rs, add pub_dep as dependency but private.
After downloading the resposity,
1、 run cargo build, no lint warning message
2、 change the public = false in crates/pub_dep/Cargo.toml, then run cargo build, no lint warning message
3、 change the public = false in crates/parent_dep/Cargo.toml, run cargo build and a lint warning message comes up.
cargo build
Compiling parent_dep v0.1.0 (/root/workspace/recursive_pub_reexport/crates/parent_dep)
Compiling pub_dep v0.1.0 (/root/workspace/recursive_pub_reexport/crates/pub_dep)
Compiling simple v0.1.0 (/root/workspace/recursive_pub_reexport)
warning: type `FromPriv` from private dependency 'grandparent_dep' in public interface
--> src/lib.rs:3:1
|
3 | pub fn use_pub(_: pub_dep::FromPriv) {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: `#[warn(exported_private_dependencies)]` on by default
Possible Solution(s)
The focus of this issue is to verify whether there is a problem with the current situation, so the solution will not be considered for the time being.
Notes
No response
Version
cargo 1.76.0-nightly (623b78849 2023-12-02)
release: 1.76.0-nightly
commit-hash: 623b788496b3e51dc2f9282373cf0f6971a229b5
commit-date: 2023-12-02
host: x86_64-unknown-linux-gnu
libgit2: 1.7.1 (sys:0.18.1 vendored)
libcurl: 8.4.0-DEV (sys:0.4.68+curl-8.4.0 vendored ssl:OpenSSL/1.1.1u)
ssl: OpenSSL 1.1.1u 30 May 2023
os: Ubuntu 22.04 (jammy) [64-bit]
Activity
epage commentedon Dec 27, 2023
I made a slight change to the repo, renaming
use_pubtouse_privand added the function to each package along the way so you can more easily see what the whole tree was doingepage commentedon Dec 27, 2023
I was curious what is done with diamonds: nothing unexpected.
epage commentedon Dec 27, 2023
Going back to
mainWith line wrapping
epage commentedon Dec 27, 2023
We only pass
--externfor direct dependencies which meansrustcis only told aboutprivfor direct dependencies and rustc then collect that information when recursing through dependencies. I think this should work if everything builds cleanly with--forbid exported_private_dependencies(or--denybut they respect the rules perfectly).In the transitional case where foundational packages have not been updated, users will have to use
#[allow(exported_private_dependencies)]when putting transitive types in their API.In the transitional case where foundation packages have been updated but intermediate packages have not been, no warning will be given even when it should.
So the next questions are
linyihai commentedon Dec 28, 2023
Thank you for dig into this issue.
epage commentedon Dec 28, 2023
Another case I want to test is when a package has a private dependency in its API (so should warn) but it is declared public in a transitive dependency.
epage commentedon Dec 28, 2023
Looks like once a dependency is public, it is always viewed as public.
epage commentedon Dec 28, 2023
Summary: only direct dependencies are marked as public/private; all transitive uses of it respect the highest visibility present in that subtree of the dependency graph.
Example Side effects
This is because we only pass
--externfor direct dependencies and visibility is tied to--extern.Possible solutions (without knowing the compiler side)
externs to see what the effective visibility is for the current crate being compiled--extern@ehuss I think you were involved in the conversations on the cargo/rustc handshake for this. Any thoughts on how to move this forward?
linyihai commentedon Dec 29, 2023
#119428 (comment)
This scenario is more common and worth writing a test case for. I plan to add this in rust-lang/cargo#13183
ehuss commentedon Dec 29, 2023
I believe that is a
rustcissue. It looks like it is checking the wrong thing when it is checking the public-status. It is looking at the DefId of the item, which is where the item is defined. However, I don't think that is correct in the face of re-exports. It should be checking the public status of the crate of theUseitem from where it was re-exported. I don't know much about theDefIdVisitoror the visibility analysis stuff in general, so I can't really suggest a different approach. If you want to work on fixing it yourself, you'll probably want to ask the compiler team for assistance in coming up with a different implementation strategy.I don't think it is necessary to pass more visibility data to rustc. It should be able to compute the effective visibility from the information it has.
It sounds like there needs to be a more principled definition of what it means to "leak" a private item, since it seems like there are some edge cases that the current approach doesn't handle correctly.
epage commentedon Dec 30, 2023
If checking the visibility from where it is re-expoerted from has problems, another idea (without any compiler knowledge) is to compute the effective visibility between the current crate and the DefId, by walking the tree of public dependencies to see if there is a path that can reach it.
apiraino commentedon Jan 17, 2024
Nominating for a compiler meeting discussion
@rustbot label +I-compiler-nominated +T-compiler
11 remaining items
tgross35 commentedon Feb 23, 2025
I have not checked the case in the top post, but some of this has been improved with #135501.
epage commentedon Feb 26, 2025
Thanks @tgross35 !
Stepping through the different cases (with an in-dev
cargo treefeature)Original
Original variant 1
Original variant 2
Diamond
Diamond variant 1
Diamond variant 2
Direct and transitive
Direct and transitive variant 1
Direct and transitive variant 2
epage commentedon Feb 26, 2025
I'm not sure if this should be a blocker for stabilization. The type is public in your API, just the path used to access it isn't.
I worry this will make adoption of this feature more difficult.
Take
cargo -Zminimal-versions. Cargo has been hesitant to stabilize it because someone using it will need to first get their dependency into compliance before they can, rather than being something they can act on all of their own (granted, there is a workaround of no-op deps to force versions higher).Applying that here, adoption of private dependencies will need to happen from the bottom up unless people workaround non-compliant dependencies by adding a direct public dependency on their transitive dependency (leveraging the "Direct and transitive variant 2" case).
If the lint could make the assumption that everything public in a dependency comes from a public transitive dependency (i.e. only warn if I reference something from a item path that is not public), this might smooth out adoption. Whether a dependency intended for a transitive dependency to be public or not won't be noticeable from the noise until it has adopted
public. Once it does, in theory, things should be good. If they do publish with the lint firing, I guess it would help you from accidentally depending on something you shouldn't but that will likely only help in some cases.ShoyuVanilla commentedon Feb 28, 2025
I've been working on this for some days and managed to create a PoC that fixes the remaining part of this issue; the access path things.
I made the following design choices:
However, I'm still unsure about some implementation details, so I’ve asked a question on Zulip: Question about tracking crate provenance in PartialRes.
I'd really appreciate any feedback on this
feat(tree): Add `--depth public` behind `-Zunstable-options` (#15243)
mladedav commentedon Jul 21, 2025
If I understand the discussion correctly, there isn't a consensus yet about what to do with dependencies that are transitively public but are used/reexported from a private dependency.
There's currently a test that summarizes it:
Test case
I would expect it to work in a way where I can reexport anything from a public dependency but cannot reexport anything from a private dependency, regardless of what other dependencies I might depend on directly or indirectly.
For one, if only direct dependencies matter, I have a better visibility into what is my public dependency. I might have something from a transitive dependency in my public API which was reexported through a direct dependency, but for all intents and purposes, it could have just as well be created by the direct dependency. The alternative is that I might be surprised to find out I have an unexpected package in my public API. I think something like the motivating example in the RFC could conceivably happen to some people where they accidentally put e.g.
serde_json::Errorinto their public API but it will not be linted if it is public transitively.Cargo could also add a lint about declaring private dependencies that are actually public if we went with non-path privacy. But this feels like forcing people to have public dependencies they don't really want.
That also ties into the possible issues with adoption. If the only thing a given crate must pay attention to is its direct dependencies, any crate can adopt and benefit from this feature whenever they like without having to wait for their dependencies to adopt it too.
The only relevant thing I found in the RFC was that public transitive dependencies are also considered public dependencies and private dependencies is everything else. But I suspect that is mostly kept there from the original version where these distinctions were made for the resolver logic.
@ehuss do you remember why you thought that doing this by paths might be wrong?
ehuss commentedon Oct 2, 2025
I don't remember exactly, but I suspect I was just waffling on whether or not to allow publicly exposing
bar::Thing. Thinking about it right now, I would probably lean towards allowing it (which would require being path-sensitive). Implementation-wise I don't know if that is feasible 😦 .epage commentedon May 25, 2026
@Mark-Simulacrum This is the public/private dep issue I mentioned.
mladedav commentedon Jun 30, 2026
@Eh2406 pinging you here because of this comment. If we changed the lint to work based on where a name is imported from instead of where the underlying type is defined, the transitive property would not automatically hold anymore.
epage commentedon Jun 30, 2026
While the lint won't be directly enforcing that
public = trueis set through a series a reexports, the lint will fire at each layer of the reexports, helping to ensure that the flag is set correctly through the tree. So while the lint won't enforce transitive correctness, the over all workflow will.mladedav commentedon Jul 1, 2026
Yes, but the main difference I see there is that in a chain
A -(pub)-> B -(pub)-> Cwhere A also privately depends on C, it cannot accidentally make larger part of the C API public than what B already exported.And in
A -(pub)-> B -(pub)-> C -(pub)-> D, if B doesn't re-export anything originating in D, then for all intents and purposes, D isn't a public dependency of A. So at that point, it's not a recursive transitive closure.epage commentedon Jul 2, 2026
I think it is correct that
Acan only use parts ofCin its public API that came throughB;Acannot expand the public scope ofCs API.I suspect there is a communication gap. When @Eh2406 talked about it public deps being recursive, it was specifically in the context of a reply about
Derefthough a chain likeA -(pub)-> B -(pub)-> C. @Eh2406 is also on the Cargo team and answering on a Cargo issue and likely had a Cargo viewpoint in that reply. Cargo does not have any extra visibility into the subsets viewed.cargo tree -e publicand eventuallycargo docwill recursively walk all public dependencies.cargo addwill auto-select versions from public dependencies as part of an effort to align major versions. This is dependent on the intent of the lint but not on the exact implementation (whether it checks where you import from or traced the reexports and it was valid at each level). In my time working on this, we've never considered havingA -(pub)-> B -(pub)-> C; A -> CimplyA -> Cis public.All that said, if narrowing of public APIs happens in ways that an APi is no longer public (like
Din your case), then yes, Cargo will still treatDas public whilerustcs lint shoudn't allowDto be used inAs public API. Depending on what question you are asking,Dis still relevant. For example, "ifDmakes a breaking change, what is the impact", and the answer is "it is a breaking change to upgradeC -> Dwhich is a breaking change toB- > Cwhich is a breaking change toA -> B. Though the breaking APi change isn't transitively relevant, the version number bumps are. Even for the questions whereDisn't relevant (like toAs rust API), the scope and impact of includingDis likely small enough to not be an issue.mladedav commentedon Jul 8, 2026
Sorry for any confusion I might have caused then. I must have misunderstood something and thought this was still undecided and the current lint implementation is still basing the decisions on where the item was defined and not where it was imported from, which does implement this as a transitive closure.
I take it then this is just waiting for someone to redo the lint implementation.
Thanks for the explanation.