Private dependencies in public interface lint rework

[mladedav]

View all comments

• I did not use an LLM to create a change in this PR.
• I used an LLM to create a change in this PR, and I have explained below how it was used.

Tracking issue: #44663
Closes #119428

This reworks the lint so that it does not fire based on where an item is defined but based on how it is imported. I think this is best illustrated in the following test:

tests/ui/privacy/pub-priv-dep/diamond_deps.rs

The test here is edited for brevity. Note that Shared is the same type and the lint fires only when imported through the private dependency.

// A diamond dependency:
//
//           diamond_reepxort
//                  /\
//        (public) /  \ (PRIVATE)
//                /    \
//   diamond_pub_dep  diamond_priv_dep
//                \    /
//        (public) \  /  (public)
//                  \/
//                shared

#![deny(exported_private_dependencies)]

extern crate diamond_priv_dep;
extern crate diamond_pub_dep;

pub fn leaks_priv() -> diamond_priv_dep::Shared {
    //~^ ERROR type `Shared` from private dependency 'diamond_priv_dep' in public interface
    diamond_priv_dep::Shared
}

// This does NOT produce a lint because the dependency is public
pub fn leaks_pub() -> diamond_pub_dep::Shared {
    diamond_pub_dep::Shared
}

This is my first contribution of this scope and as such, I'm not sure about several things I did:

• is the new query necessary? The current is_private_dep is not enough since it handles transitively public dependencies as public
• is it necessary to save this separately into crate metadata? I've modeled it after private_dep so I just did the same things I've seen there.
• should I use tcx.hir_module_items(mod_id).par_items(..) instead of tcx.hir_visit_all_item_likes_in_crate
  • I think so, but I'd appreciate some guidance here
• can the old PrivateItemsInPublicInterfacesChecker be trimmed down more so that there are not two passes for this? It still emits hard errors for types in associated types and I'm not sure how to check for this with the visitor.

@epage I'd appreciate if you could check the test changes in the first commit. The state now is how I understand the desired state based on the discussion in #119428 but I'd rather get a final ok on this.

[rustbot]

r? @JohnTitor

rustbot has assigned @JohnTitor.
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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 75 candidates
• Random selection from 19 candidates

[rustbot]

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

[JohnTitor]

@rustbot reroll
sorry, I'm not familiar with this

[mati865]

I'm not familiar either. @rustbot reroll

[petrochenkov]

Purely from the description, checking for how it is imported and rustc_privacy is generally not a good combination.
Some stuff will probably need to be moved to rustc_resolve, like in #143856.
I'll look at this, but only next week, at best.

[mladedav]

Right, I've found some cases where this rework doesn't work, it just happened to work for the tests that were here but using fully qualified paths vs imports has effect on whether the lint fires so this is broken.

I'll try to rework it so that the needed information is collected I guess in the late pass of resolve.

@rustbot label +S-waiting-on-author -S-waiting-on-review

[mladedav]

I've reworked it so that the information on which crate a path imported items from in resolve (last hop, not definition crate), but I still consume it in privacy where I emit the lint. From what I understand, effective visibilities are only partly constructed in resolve so I have to do the check later than that and since it was previously emitted in privacy, I put it at the same place.

@rustbot label -S-waiting-on-author +S-waiting-on-review

[Enselic]

r? petrochenkov

[petrochenkov]

Isn't this flag obsoleted by is_extern_private_dep?
It would be pretty bad to have the both systems in place.

View changes since the review

[mladedav]

It was still used in the other privacy visitor and I wasn't sure so since it has slightly different semantics, I thought it safer to just keep it.

I'll try to see what actually uses it and if it can be safely replaced.

[petrochenkov]

Can entry_crate ever refer to an indirect dependency?
If it cannot, then we don't need to encode the dep privacy flags into metadata.

What is the behavior in this case

pub fn my_interface() -> other_crate::module::SomeType { ... }

if other_crate is a public dependency, but module is actually a reexport to a module in some third crate, which is not a direct dependency?
Is that third crate considered a private or public dependency? Or it depends on the value of is_extern_private_dep encoded into the other_crate's metadata?

View changes since the review

[petrochenkov]

And what if other_crate is a private dependency in the same situation?

[mladedav]

Can entry_crate ever refer to an indirect dependency?
If it cannot, then we don't need to encode the dep privacy flags into metadata.

It cannot, I'll get rid of that then.

What is the behavior in this case

pub fn my_interface() -> other_crate::module::SomeType { ... }

if other_crate is a public dependency, but module is actually a reexport to a module in some third crate, which is not a direct dependency?

Then it is considered ok. Anything other_crate exposes publicly is public regardless of what crate defined it. I think that should include the foreign module and anything accessible in there.

Assuming that other_crate (and all other transitive deps) also manages its public dependencies correctly, anything it exported publicly is from its own (transitive) public dependency so in that sense, we still require types in our public API to be from a transitive public dependency. But we additionally require that the import is through our own public dependency if there are more paths.

We don't care if any crate in the re-export chain (including our direct public dependencies) actually doesn't use public dependencies, we just trust that whatever our public dependencies have in their public API won't break in a minor release.

At least that was the design I had in mind, I don't think any test uses a type from a re-exported module so I might be describing how I tried to make it work but the implementation in this PR might do something different. So I'll add a test with that and report back if I was wrong.

Is that third crate considered a private or public dependency? Or it depends on the value of is_extern_private_dep encoded into the other_crate's metadata?

It's not considered at all in the context of the lint. We handle types imported from other_crate the same way regardless of whether they were defined or just re-exported there.

From the point of view of the new query, transitive dependencies are not passed at all with the extern flag so there is no extern priv so in that sense they're considered public (or rather they're not considered private). But that information shouldn't be ever retrieved for transitive dependencies. I think I ended with that shape instead of direct_pub_dep because it simplified some edge-cases, I think with core and std which are now considered public by that query because of that.

And what if other_crate is a private dependency in the same situation?

Then that example should lint regardless of where SomeType is actually defined.

[petrochenkov]

I was asking because in the actual implementation decl.parent_module for SomeType will point to the indirect dependency crate.

Also, what happens in cases like this?

use other_crate::module::SomeType;
pub fn my_interface() -> SomeType { ... }

[petrochenkov]

As I understand the reexport checking wasn't migrated to the new model yet.

So this is still based on where the definition of Type is located and not on the dep-privateness of dep.

pub use dep::Type;

[petrochenkov]

having a resolved definitions (Res) for a path is not enough now.

Regular resolutions are kept in paths inline after AST lowering, and not in side tables.
I wonder if it makes sense to keep the new parts of the path resolution inline as well.

So far the paths are checked in TyKind::Paths and TraitRefs, do you think we'll need to check them somewhere else?
Like the constants from #161664, since the logic is now entirely different from the private-in-public lints and doesn't need to be consistent with it.

Could you rebase this PR so I could run benchmarks?

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[petrochenkov]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 56aa046 (56aa046262221498455b66399b55a75a8394b646)
Base parent: 420ed2a (420ed2a0c3d7225b1744266fd884d431b4d8cfe0)

[mladedav]

I've rebased it onto current main.

I didn't change reexports, I'll do that on top of this. I somehow convinced myself that lint already works but yeah, that would be inconsistent.

So far the paths are checked in TyKind::Paths and TraitRefs, do you think we'll need to check them somewhere else?

Maybe constants and static variables, otherwise I don't think so. But that's based more on it not being needed for the tests currently and all other cases I could think of being covered by those two.

Regular resolutions are kept in paths inline after AST lowering, and not in side tables.
I wonder if it makes sense to keep the new parts of the path resolution inline as well.

Do you mean putting the importing CrateName into rustc_hir::Path? I can try that.

---

For the record, I've since found some ways to confuse the lint by having a private type alias to a foreign type from private dependency and then implementations on that alias are considered ok. There is also a similar way to implement public traits on associated types (e.g. impl MyPubTrait for <() as MyPrivateTrait>::Assoc {}) effectively implementing traits on any type without directly naming it in the impl.

This might end up requiring more bookkeeping regarding which types were used for aliases or associative types and how were they named.

[rust-timer]

Finished benchmarking commit (56aa046): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never rustc-perf
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.3%	[0.1%, 0.6%]	57
Regressions ❌ (secondary)	0.4%	[0.2%, 0.9%]	44
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.3%	[-0.3%, -0.2%]	9
All ❌✅ (primary)	0.3%	[0.1%, 0.6%]	57

Max RSS (memory usage)

Results (primary 2.1%, secondary -3.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.1%	[0.9%, 3.8%]	6
Regressions ❌ (secondary)	2.7%	[2.6%, 2.7%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.3%	[-8.3%, -2.1%]	12
All ❌✅ (primary)	2.1%	[0.9%, 3.8%]	6

Cycles

Results (primary -2.5%, secondary -0.8%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.0%	[4.0%, 4.0%]	1
Improvements ✅ (primary)	-2.5%	[-2.5%, -2.5%]	1
Improvements ✅ (secondary)	-3.1%	[-3.3%, -3.0%]	2
All ❌✅ (primary)	-2.5%	[-2.5%, -2.5%]	1

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 498.333s -> 498.806s (0.09%)
Artifact size: 408.93 MiB -> 409.01 MiB (0.02%)

[mladedav]

This comment should be updated (first implementation used a direct public dep query, now it's extern priv query)

View changes since the review

[mladedav]

I'm trying to understand better the perf output better and I'll see if I can replicate it locally. I tried to run the tool before the rebase and back then it showed as empty which I understood meant no perf improvements or regressions. I don't have other experience with it so I can't guarantee I just didn't use it wrong.

I'll try to delete the new unnecessary crate metadata and double check if I didn't do something wrong in the rebase which included changes on how lowering state is handled.