Add compiler support for namespaced crates by b-naber · Pull Request #140271 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation10 Commits3 Checks6 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Co-authored-by: Eric Holk eric.holk@gmail.com
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.
labels
| return module.copied(); |
|---|
| } |
| //if def_id.is_crate_root() && !def_id.is_local() { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment has been minimized.
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).
Relevant to the language team
DEPRECATED: Use the team-based variants `S-waiting-on-t-lang`, `S-waiting-on-t-compiler`, ...
and removed T-compiler
Relevant to the compiler team, which will review and decide on the PR/issue.
Status: Awaiting review from the assignee but also interested parties.
labels
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.
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.)
6 tasks
This comment has been minimized.
This comment has been minimized.
There's some discussion about this PR and RFC 3243 going on at #t-compiler > RFC 3243. (I'm mostly posting this here to increase my chances of finding the link in the future.)
| #[test] |
| fn test_bar() { |
| assert_eq!(bar::BAR, "bar"); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test seems unrelated to namespaced crates, right?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, no idea what this test is, seems to be incomplete.