Skip to content

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