Skip to content

fix: Calculate the privacy of dependencies wrt current local crate #137441

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

ShoyuVanilla
Copy link
Member

Partially mitigates #119428

This PR addresses most aspects of the issue, except for the following cases:

  • When the same crate is exported through multiple routes in the dependency graph, requiring us to determine its pub-priv status based on the import path.

For example, consider the following cases:

// FIXME: Should this trigger?
//
// One could make an argument that I said I want "reexport" to be public, and
// since "reexport" says "shared_direct_private" is public, then it should
// transitively be public for me. However, as written, this is explicitly
// referring to a dependency that is marked "private", which I think is
// confusing.
pub fn leaks_priv() -> shared::Shared {

// FIXME: This should trigger.
pub fn leaks_priv() -> diamond_priv_dep::Shared {

Resolving the remaining issue likely requires modifying the path lowering logic. I plan to work on that in a follow-up PR—assuming, of course, that this one gets merged first! 😅

P.S. I'd also appreciate a review on #134176 😊

@rustbot
Copy link
Collaborator

rustbot commented Feb 22, 2025

r? @fee1-dead

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 22, 2025
@@ -1,7 +1,6 @@
//@ aux-crate:priv:shared=shared.rs
//@ aux-crate:reexport=reexport.rs
//@ aux-crate:priv:reexport=reexport.rs
Copy link
Member Author

Choose a reason for hiding this comment

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

According to the diagram commented below, I think that this is the original intention of this test

@ShoyuVanilla ShoyuVanilla changed the title fix: Calculate privacy of dependents wrt current local crate fix: Calculate the privacy of dependencies wrt current local crate Feb 22, 2025
@bors
Copy link
Collaborator

bors commented Feb 23, 2025

☔ The latest upstream changes (presumably #137446) made this pull request unmergeable. Please resolve the merge conflicts.

@ShoyuVanilla
Copy link
Member Author

Oh, this overlaps a bit with #135501. I'll resolve the conflicts.

@ShoyuVanilla
Copy link
Member Author

Well, the test case I've edited in this PR also works in #135501 and the extra complexities in this PR is not justfied so far - at the moment I was working on this PR, #135501 wasn't merged yet and tihs was solving more of the cases 😅 - closing for now and will revisit this for fixing #119428 entirely.

@tgross35
Copy link
Contributor

Ah, sorry for racing!

Well, the test case I've edited in this PR also works in #135501

Do you mean shared_both_private.rs now passes with aux-crate:priv? If so, that's probably still worth updating on its own.

@tgross35
Copy link
Contributor

P.S. I'd also appreciate a review on #134176 😊

That one has been open for a while so you can just r? compiler to reroll

@ShoyuVanilla
Copy link
Member Author

Ah, sorry for racing!

Well, the test case I've edited in this PR also works in #135501

Do you mean shared_both_private.rs now passes with aux-crate:priv? If so, that's probably still worth updating on its own.

It's not really a race—your PR was already open when I started working on this, and I just wasn’t aware of it. 😅

And yes, shared_both_private.rs now passes since your PR. I'm planning to include it in my new PR if it is still untouched by then, which addresses the path-lowering thing I mentioned in this PR's description. (Though I’m not sure if it will be merged, as it might cause some performance regressions. 🤔)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants