Skip to content
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

Document the order preludes are resolved #1765

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Mar 22, 2025

Also document the derive macro sub-namespace; apparently it was missing before.

cc rust-lang/rust#66079 (comment), rust-lang/rust#138840

@rustbot rustbot added the S-waiting-on-review Status: The marked PR is awaiting review from a maintainer label Mar 22, 2025
Names of items are resolved in the following order:

1. Explicit definitions (including imports)
2. Language and macro prelude. If an item is present in both preludes, an error is emitted.
Copy link
Contributor

@petrochenkov petrochenkov Mar 22, 2025

Choose a reason for hiding this comment

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

This doesn't look correct, language prelude should be considered last.

In the compiler the order can be found in fn visit_scopes (it also describes the motivation behind the scope ordering).

  • Type namespace: module items ("explicit definitions") -> extern prelude -> tool prelude -> stdlib prelude -> language prelude (built-in types).
  • Value namespace: module items -> stdlib prelude.
  • Macro namespace: derive helpers -> legacy derive helpers -> macro_rules -> module items -> macro_use prelude -> stdlib prelude -> language prelude (built-in attributes).

Language and macro prelude. If an item is present in both preludes, an error is emitted.

Built-in attributes conflict with all other scopes (in attr sub-namespace), not just with preludes.

Copy link
Contributor

Choose a reason for hiding this comment

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

(This is not relevant to preludes, but the "macro_rules -> module items" should ideally be intermixed, but it's under-implemented and stubbed by errors, so probably no need to specify it for now.)

Copy link
Member Author

Choose a reason for hiding this comment

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

ok; clearly my testing was wrong and i need to think about exactly why. i have fixed the first commit (about sub-namespaces) in the meantime.

Copy link
Member Author

Choose a reason for hiding this comment

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

actually i have a question for you: visit_scopes says that in the type namespace, the extern prelude takes precedence over tool modules. this makes sense by the reasoning there since tool modules are closed but the extern prelude is not (i guess in source this correponds to the fact that extern crate can be anywhere but register_tool can only be at the crate root). but for tools it's not ideal because it means that extern crate rustfmt; will break rustfmt::skip.

that doesn't matter so much today because the tool namespace is controlled. but if we want to stabilize register tool, it becomes more of a problem. if the precedence went the other way, with tools resolved before the extern prelude, there would be a workaround: use ::rustfmt to refer to the crate. but there is no such workaround to refer to the tool module.

Copy link
Member Author

@jyn514 jyn514 Mar 22, 2025

Choose a reason for hiding this comment

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

oh lol i didn't ask a question. my question is: do you agree that's a problem? and if so, do you have a preferred solution?

my preferred solution is to have tool modules take precedence over the extern prelude, but i'm curious to hear your thoughts.

Copy link
Member Author

Choose a reason for hiding this comment

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

i thought of another corner case with register_tool. say you have the following code:

mod rustfmt {}
#[rustfmt::skip] // error: failed to resolve
fn foo() {}

there is no way here to disambiguate the tool module. and we cannot change the precedence either, it is very important that explicit definitions have the highest precedence.

i am not sure what to do here. just document the conflict and tell people "don't do that"? at least the names are controlled by author, they can rename any of their explicit definitions or imports.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh - we could make this a hard error, and this has the benefit that authors of extern tools never need to do name resolution. either the name is a tool attribute or it's a hard error.

(note that rustfmt doesn't do name resolution today, the above code will get formatted whether or not you comment out the module.)

Copy link
Contributor

Choose a reason for hiding this comment

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

it seems like a corner case that will be disallowed eventually, right?
"module items" refers to "pub macro"? i don't plan to document those either, they're still unstable.

Yes, no need to document legacy helper attributes and unstable macros.

Copy link
Contributor

Choose a reason for hiding this comment

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

just document the conflict and tell people "don't do that"? at least the names are controlled by author, they can rename any of their explicit definitions or imports.

Yes, that's the status quo.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh - we could make this a hard error, and this has the benefit that authors of extern tools never need to do name resolution. either the name is a tool attribute or it's a hard error.

(note that rustfmt doesn't do name resolution today, the above code will get formatted whether or not you comment out the module.)

If a tool works after macro expansion, then we are mostly good already.
The only multi-segment attributes that can survive expansion are tool attributes, if it's something else it will either disappear or produce an error.
Tools working early, like rustfmt, have to accept some false positives in recognizing tool attributes.

I agree that nobody expects tools (or proc macros) to do resolution for their helper attributes, that's why derive helpers have the highest priority (#66529), but with derive helpers we could do that backward compatibly and it didn't interfere with resolution in other positions because macro namespace + attribute sub-namespace make it pretty localized.

In case of tool attributes making them top priority or ambiguous with everything else (like built-in attributes) will probably be too destructive to resolution in other locations.
E.g. in something like type MyType = clippy::Type; the prefix will resolve to a tool module and produce an error.
Unless we do some hacks like in rust-lang/rust#139095 (comment) - "yes, it resolved to a tool module, but they cannot result in anything good in this position, so let's fall back to something else".

I guess we can try starting with the most restrictive rules during stabilization and relax them later if there's breakage or complaints.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 31, 2025
…iser

rustc_resolve: Test the order that preludes are resolved

This test is exhaustive. See attached truth table:
![image](https://github.com/user-attachments/assets/11fe703c-e114-48df-84f8-426b63395784)

Companion PR to rust-lang/reference#1765.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 1, 2025
Rollup merge of rust-lang#138840 - jyn514:precedence-order, r=wesleywiser

rustc_resolve: Test the order that preludes are resolved

This test is exhaustive. See attached truth table:
![image](https://github.com/user-attachments/assets/11fe703c-e114-48df-84f8-426b63395784)

Companion PR to rust-lang/reference#1765.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: The marked PR is awaiting review from a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants