Skip to content

Add compiler support for namespaced crates #140271

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

b-naber
Copy link
Contributor

@b-naber b-naber commented Apr 24, 2025

Attempt to implement the remaining changes for RFC 3243. This is joint work with @eholk.

Opening a draft PR to get feedback from @petrochenkov on the viability of the approach.

r? @petrochenkov

@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 Apr 24, 2025
@b-naber b-naber marked this pull request as draft April 24, 2025 21:52
@@ -118,6 +119,13 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
return module.copied();
}

//if def_id.is_crate_root() && !def_id.is_local() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Expected this to hold, but it fails when compiling libc. Likely here: https://github.com/rust-lang/libc/blob/e8b01525c30545041af1ad7ba24c05ee3251016b/src/lib.rs#L33-L37

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@petrochenkov
Copy link
Contributor

I've reviewed the implementation, perhaps it can be made a bit differently, but and I don't think it can be made significantly less hacky, so I'm against the direction in general - #122349 (comment).

@petrochenkov petrochenkov added T-lang Relevant to the language team, which will review and decide on the PR/issue. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 26, 2025
@b-naber
Copy link
Contributor Author

b-naber commented Apr 27, 2025

I've reviewed the implementation, perhaps it can be made a bit differently, but and I don't think it can be made significantly less hacky, so I'm against the direction in general - #122349 (comment).

Thanks for looking at the PR. I disagree with your conclusion though.

The changes that this PR introduces are fairly isolated from the existing name resolution functionality. We only ever try to resolve to a namespaced crate when try_resolve_ident_in_module fails. If there's a conflict in the sense that my_api exists as a dependency, has a module named utils and a namespaced crate my_api::utils exists at the same time, then the existing conflict detection functionality handles this.

If we do resolve to a namespaced crate it's simply treated as a Module. The current path resolution logic isn't backwards looking; it doesn't matter whether the module path was my_api or my_api::utils when resolving Idents in that Module. So there's essentially no impact here too.

As far as I can tell there's very little maintainability burden introduced by this solution. If there's a large change in the future in terms of how name resolution is supposed to work, I do agree that this is an additional road block that could pose a problem. I kind of imagine that to be manageable, even though it's impossible to tell really without a concrete change in mind. So I can understand you in your wish that this feature wouldn't touch the compiler at all, the fact is that this RFC has already been accepted.

@petrochenkov
Copy link
Contributor

This is a bit similar to cfg(accessible), which seems simple until you actually try to implement it correctly in presence of macro expansion and glob imports, because the definition of "fallback" in name resolution becomes more vague.
I'm going to be skeptical about any proposal involving fallback, unless it can be modeled in terms of the currently used sequences of Scopes.
What this PR does is almost certainly incorrect in general case and not the way to go long term.

the fact is that this RFC has already been accepted.

Until it's stable it can be unaccepted.

While it's unstable I think I'm fine with gradually merging parts that do fit into the current model, and then thinking how to model the remaining parts.
Some parts, e.g. when you cannot have a crate and an organization with the same name, are certainly fine.
(I can think what can be split and merged first, but probably just not right now.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants