`exported_private_dependencies` lint only take effect in innermost dependency

linyihai commented 9 months ago

Problem

The exported_private_dependencies lint only take affect in the innermost dependency in a recursively dependent environment.

This inspired by https://github.com/rust-lang/rust/issues/44663#issuecomment-1851227083.

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_dep as public and reexport pub struct from grandparent_dep
pub_dep, the outmost crate, add parent_dep as 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]

epage commented 8 months ago

I made a slight change to the repo, renaming use_pub to use_priv and added the function to each package along the way so you can more easily see what the whole tree was doing

epage commented 8 months ago

I was curious what is done with diamonds: nothing unexpected.

epage commented 8 months ago

Going back to main

$ cargo +nightly clean && cargo +nightly check --all -vvv
     Removed 44 files, 183.6KiB total
    Checking grandparent_dep v0.1.0 (/home/epage/src/personal/dump/recursive_pub_reexport/crates/grandparent_dep)
     Running `CARGO=/home/epage/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo CARGO_CRATE_NAME=grandpa
rent_dep CARGO_MANIFEST_DIR=/home/epage/src/personal/dump/recursive_pub_reexport/crates/grandparent_dep CARGO_PKG_AUTH
ORS='' CARGO_PKG_DESCRIPTION='' CARGO_PKG_HOMEPAGE='' CARGO_PKG_LICENSE='' CARGO_PKG_LICENSE_FILE='' CARGO_PKG_NAME=gr
andparent_dep CARGO_PKG_README='' CARGO_PKG_REPOSITORY='' CARGO_PKG_RUST_VERSION='' CARGO_PKG_VERSION=0.1.0 CARGO_PKG_
VERSION_MAJOR=0 CARGO_PKG_VERSION_MINOR=1 CARGO_PKG_VERSION_PATCH=0 CARGO_PKG_VERSION_PRE='' CARGO_RUSTC_CURRENT_DIR=/
home/epage/src/personal/dump/recursive_pub_reexport LD_LIBRARY_PATH='/home/epage/src/personal/dump/recursive_pub_reexp
ort/target/debug/deps:/home/epage/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib:/home/epage/.rustup/toolchai
ns/nightly-x86_64-unknown-linux-gnu/lib' /home/epage/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/rustc --c
rate-name grandparent_dep --edition=2021 crates/grandparent_dep/src/lib.rs --error-format=json --json=diagnostic-rende
red-ansi,artifacts,future-incompat --diagnostic-width=118 --crate-type lib --emit=dep-info,metadata -C embed-bitcode=n
o -C debuginfo=2 -C metadata=ec231377d3a2b1c4 -C extra-filename=-ec231377d3a2b1c4 --out-dir /home/epage/src/personal/d
ump/recursive_pub_reexport/target/debug/deps -C incremental=/home/epage/src/personal/dump/recursive_pub_reexport/targe
t/debug/incremental -L dependency=/home/epage/src/personal/dump/recursive_pub_reexport/target/debug/deps`
    Checking parent_dep v0.1.0 (/home/epage/src/personal/dump/recursive_pub_reexport/crates/parent_dep)
     Running `CARGO=/home/epage/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo CARGO_CRATE_NAME=parent_
dep CARGO_MANIFEST_DIR=/home/epage/src/personal/dump/recursive_pub_reexport/crates/parent_dep CARGO_PKG_AUTHORS='' CAR
GO_PKG_DESCRIPTION='' CARGO_PKG_HOMEPAGE='' CARGO_PKG_LICENSE='' CARGO_PKG_LICENSE_FILE='' CARGO_PKG_NAME=parent_dep C
ARGO_PKG_README='' CARGO_PKG_REPOSITORY='' CARGO_PKG_RUST_VERSION='' CARGO_PKG_VERSION=0.1.0 CARGO_PKG_VERSION_MAJOR=0
 CARGO_PKG_VERSION_MINOR=1 CARGO_PKG_VERSION_PATCH=0 CARGO_PKG_VERSION_PRE='' CARGO_RUSTC_CURRENT_DIR=/home/epage/src/
personal/dump/recursive_pub_reexport LD_LIBRARY_PATH='/home/epage/src/personal/dump/recursive_pub_reexport/target/debu
g/deps:/home/epage/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib:/home/epage/.rustup/toolchains/nightly-x86_
64-unknown-linux-gnu/lib' /home/epage/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/rustc --crate-name paren
t_dep --edition=2021 crates/parent_dep/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future
-incompat --diagnostic-width=118 --crate-type lib --emit=dep-info,metadata -C embed-bitcode=no -C debuginfo=2 -C metad
ata=b0e93a3b309a3856 -C extra-filename=-b0e93a3b309a3856 --out-dir /home/epage/src/personal/dump/recursive_pub_reexpor
t/target/debug/deps -C incremental=/home/epage/src/personal/dump/recursive_pub_reexport/target/debug/incremental -L de
pendency=/home/epage/src/personal/dump/recursive_pub_reexport/target/debug/deps --extern grandparent_dep=/home/epage/s
rc/personal/dump/recursive_pub_reexport/target/debug/deps/libgrandparent_dep-ec231377d3a2b1c4.rmeta`
    Checking pub_dep v0.1.0 (/home/epage/src/personal/dump/recursive_pub_reexport/crates/pub_dep)
     Running `CARGO=/home/epage/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo CARGO_CRATE_NAME=pub_dep
 CARGO_MANIFEST_DIR=/home/epage/src/personal/dump/recursive_pub_reexport/crates/pub_dep CARGO_PKG_AUTHORS='' CARGO_PKG
_DESCRIPTION='' CARGO_PKG_HOMEPAGE='' CARGO_PKG_LICENSE='' CARGO_PKG_LICENSE_FILE='' CARGO_PKG_NAME=pub_dep CARGO_PKG_
README='' CARGO_PKG_REPOSITORY='' CARGO_PKG_RUST_VERSION='' CARGO_PKG_VERSION=0.1.0 CARGO_PKG_VERSION_MAJOR=0 CARGO_PK
G_VERSION_MINOR=1 CARGO_PKG_VERSION_PATCH=0 CARGO_PKG_VERSION_PRE='' CARGO_RUSTC_CURRENT_DIR=/home/epage/src/personal/
dump/recursive_pub_reexport LD_LIBRARY_PATH='/home/epage/src/personal/dump/recursive_pub_reexport/target/debug/deps:/h
ome/epage/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib:/home/epage/.rustup/toolchains/nightly-x86_64-unknow
n-linux-gnu/lib' /home/epage/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/rustc --crate-name pub_dep --edit
ion=2021 crates/pub_dep/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --dia
gnostic-width=118 --crate-type lib --emit=dep-info,metadata -C embed-bitcode=no -C debuginfo=2 -C metadata=523c770971b
0c118 -C extra-filename=-523c770971b0c118 --out-dir /home/epage/src/personal/dump/recursive_pub_reexport/target/debug/
deps -C incremental=/home/epage/src/personal/dump/recursive_pub_reexport/target/debug/incremental -L dependency=/home/
epage/src/personal/dump/recursive_pub_reexport/target/debug/deps --extern parent_dep=/home/epage/src/personal/dump/rec
ursive_pub_reexport/target/debug/deps/libparent_dep-b0e93a3b309a3856.rmeta`
    Checking simple v0.1.0 (/home/epage/src/personal/dump/recursive_pub_reexport)
     Running `CARGO=/home/epage/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo CARGO_CRATE_NAME=simple
CARGO_MANIFEST_DIR=/home/epage/src/personal/dump/recursive_pub_reexport CARGO_PKG_AUTHORS='' CARGO_PKG_DESCRIPTION=''
CARGO_PKG_HOMEPAGE='' CARGO_PKG_LICENSE='' CARGO_PKG_LICENSE_FILE='' CARGO_PKG_NAME=simple CARGO_PKG_README='' CARGO_P
KG_REPOSITORY='' CARGO_PKG_RUST_VERSION='' CARGO_PKG_VERSION=0.1.0 CARGO_PKG_VERSION_MAJOR=0 CARGO_PKG_VERSION_MINOR=1
 CARGO_PKG_VERSION_PATCH=0 CARGO_PKG_VERSION_PRE='' CARGO_PRIMARY_PACKAGE=1 CARGO_RUSTC_CURRENT_DIR=/home/epage/src/pe
rsonal/dump/recursive_pub_reexport LD_LIBRARY_PATH='/home/epage/src/personal/dump/recursive_pub_reexport/target/debug/
deps:/home/epage/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib:/home/epage/.rustup/toolchains/nightly-x86_64
-unknown-linux-gnu/lib' /home/epage/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/rustc --crate-name simple
--edition=2021 src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --diagnostic-w
idth=118 --crate-type lib --emit=dep-info,metadata -C embed-bitcode=no -C debuginfo=2 -C metadata=2b1b07927f979b6c -C
extra-filename=-2b1b07927f979b6c --out-dir /home/epage/src/personal/dump/recursive_pub_reexport/target/debug/deps -C i
ncremental=/home/epage/src/personal/dump/recursive_pub_reexport/target/debug/incremental -L dependency=/home/epage/src
/personal/dump/recursive_pub_reexport/target/debug/deps --extern 'priv:pub_dep=/home/epage/src/personal/dump/recursive
_pub_reexport/target/debug/deps/libpub_dep-523c770971b0c118.rmeta' -Z unstable-options`
    Finished dev [unoptimized + debuginfo] target(s) in 0.08s

With line wrapping

epage commented 8 months ago

We only pass --extern for direct dependencies which means rustc is only told about priv for 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 --deny but 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

Are there cases I'm missing?
What are our options for fixing this?
How much do we care?

linyihai commented 8 months ago

Thank you for dig into this issue.

epage commented 8 months ago

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 commented 8 months ago

Looks like once a dependency is public, it is always viewed as public.

epage commented 8 months ago

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

If you directly depend on a transitive dependency that is marked as public, it will be considered public for you, not emitting any warnings
If a grandparent is re-exported by a parent (and declared it public) and you use it in your API, you won't get a warning about needing to mark the parent as public (or grandparent despite not directly depending on it) because the parent declaring the grandparent public makes the compiler think its also public for you

This is because we only pass --extern for direct dependencies and visibility is tied to --extern.

Possible solutions (without knowing the compiler side)

The compiler check the graph of externs to see what the effective visibility is for the current crate being compiled
Cargo pass visibility for the entire dependency graph, separate from --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 commented 8 months ago

https://github.com/rust-lang/rust/issues/119428#issuecomment-1872438243

This scenario is more common and worth writing a test case for. I plan to add this in https://github.com/rust-lang/cargo/pull/13183

ehuss commented 8 months ago

I believe that is a rustc issue. 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 the Use item from where it was re-exported. I don't know much about the DefIdVisitor or 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 commented 8 months ago

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 commented 8 months ago

Nominating for a compiler meeting discussion

@rustbot label +I-compiler-nominated +T-compiler

wesleywiser commented 8 months ago

I believe that is a rustc issue. 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 the Use item from where it was re-exported. I don't know much about the DefIdVisitor or 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.

@petrochenkov do you have expertise with the visibility analysis? If so, do you agree with the assessment here or is there a better way to perform the exported_private_dependencies analysis?

petrochenkov commented 8 months ago

@wesleywiser This specific issue is not about visibility analysis, but about private_dep: bool flags set by rustc on all crates in the crate dependency graph.

The current implementation just set these flags to true... sometimes, without much of a consistent scheme. I was talking about this soon after the initial implementation in 2019 (https://github.com/rust-lang/rust/issues/44663#issuecomment-542324722), but it was never fixed.

Someone need to decide how these flags should propagate through the dependency graph in general case (with all kinds of chain and diamond cases), after that it could be properly implemented in rustc_metadata.

petrochenkov commented 8 months ago

Let's say A is a graph with N nodes with privacy flags PA₁..PA_N, and B is a graph with M nodes with privacy flags PB₁..PB_N. Then we add Head(B) as a dependency to some node of A and get graph C with K ∈ [N; N + M] nodes (the node sets of A and B may intersect). For privacies we then need to define some function PC(PA, PB) to generate the new privacy flags PC₁..PC_K.

The final flags will then be the result of a repeated application of this function for adding all direct dependencies of the current crate.

ehuss commented 8 months ago

@petrochenkov To clarify, are you suggesting there should be some kind of precedence rules to deal with conflicts? For example:

foo → (public) bar → (public) baz foo → (private) baz

Here, bar publicly re-exports Thing from baz. There are two ways foo could expose the item Thing from baz, either from baz::Thing, or bar::Thing. They refer to the same item, just through different paths. Are you suggesting there should be some rule that says foo exposing either baz::Thing or bar::Thing should result in the same behavior (either warning or not)?

I was originally thinking it would be path-sensitive, so that they would be different (bar::Thing would be OK, but baz::Thing would not). Then, rustc would only care about the visibility of direct dependencies. However, thinking about that more, I'm feeling like I might be wrong. Either way, I can imagine this has the potential to be confusing.

Do people have an intuition of which approach would be easier to grasp? If it is based on where items are defined (as opposed to path-based), does anyone want to suggest what rules would be used to break conflicts? (I started writing some down, but they seemed horribly complicated.)

I'm trying to think about what the intentions of the user might be in such a case, and it isn't obvious to me. I also can't decide whether or not it would be better to err on the side of warning.

epage commented 7 months ago

FYI @matthewjasper

rust-lang / rust