Skip to content

Loosen shadowing check inside macro contexts (attempt 2). #100453

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

davidv1992
Copy link
Contributor

This allows macro code to use identifiers for local variables which are
also defined as globals outside of the macro, so long as their
definition is not ambiguous. This improves predictability of code
containing macros, and reduces the leakage of macro implementation
details to their surroundings.

Concretely, constructions such as

thread_local!(static FOO: usize = 0);
static init: usize = 0;

no longer generate unexpected error messages on the implementation
detail that thread_local! defines a function with init as a parameter
name.

This fixes #99018

Note that this is a reworked implementation of #99191. Despite the comments of @petrochenkov I am of the opinion that this is worthwhile to merge, and does not really represent a change in the hygiene rules. However, I agree with the statement there that this will need a lang FCP.

My argument for this is that this is not a change in the hygiene rules for consts/statics/etc.., but rather represents a fix for an edge case in local variable and parameter name hygiene. As a developer, the intuition behind macro hygiene for locals is that there is effectively a separate namespace for locals defined fully within the macro. As such, when a variable definition in the macro is not ambiguous, I would expect it to not cause any noticeable side-effects outside of the macro. This patch aimed at ensuring just that.

Secondarily, the possibly confusing behavior that is at the basis of the error message generated outside of macros also is less of a concern within macros, as the macro probably was never written with the idea in mind that there might be a name collision on a variable name in the first place. (As an aside, I'm not even sure why E0530 is a hard error and not a warning, but let's leave that out of scope for now)

A nice side effect to this change is that changes of internal variable names used inside macros should no longer be potential breaking changes for the user of the macro, which should help with stability guarantees.

PS: Apologies that it took me a while to react to the comments made in the previous pull request, I had some personal circumstances that prevented me from putting significant time towards this project.

r? @petrochenkov

also defined as globals outside of the macro, so long as their
definition is not ambiguous. This improves predictability of code
containing macros, and reduces the leakage of macro implementation
details to their surroundings.

Concretely, constructions such as
```rust
thread_local!(static FOO: usize = 0);
static init: usize = 0;
```
no longer generate unexpected error messages on the implementation
detail that thread_local! defines a function with init as a parameter
name.
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 12, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 12, 2022
@petrochenkov
Copy link
Contributor

I'll mark this as waiting on lang team, but I'm still against making this change - it adds one more ad hoc rule to Rust's hygiene system which is already too complex.

@petrochenkov petrochenkov added 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. I-lang-nominated Nominated for discussion during a lang team meeting. and removed 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 Aug 12, 2022
@petrochenkov
Copy link
Contributor

Also see rust-lang/rfcs#3305 for why statics in patterns are reserved.

}

fn main() {
h!();
Copy link
Contributor

@cjgillot cjgillot Aug 17, 2022

Choose a reason for hiding this comment

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

What should this print?

macro_rules! h2 { ($z:ident) => {
  let x @ _ = 3;
  eprintln!("{} {}", x, $z);
}}

const x: u32 = 5;
fn main() {
  h2!(x)
}

@nikomatsakis
Copy link
Contributor

We discussed this in today's @rust-lang/lang meeting. We agreed with the author of the PR that there is a problem here, but also agreed with @petrochenkov that we're reluctant to make 'one-off' changes to hygiene without a more holistic plan in place. Therefore, we are inclined to close the PR -- though if there are folks interested in driving a larger hygiene effort, I'd personally be interested to talk about what that would look like.

@rfcbot fcp close

@rfcbot
Copy link
Collaborator

rfcbot commented Sep 6, 2022

Team member @nikomatsakis has proposed to close this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Sep 6, 2022
@rfcbot
Copy link
Collaborator

rfcbot commented Sep 7, 2022

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Sep 7, 2022
@nikomatsakis nikomatsakis removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Sep 13, 2022
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Sep 17, 2022
@rfcbot
Copy link
Collaborator

rfcbot commented Sep 17, 2022

The final comment period, with a disposition to close, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

@davidv1992
Copy link
Contributor Author

Closed per outcome of the FCP. If there comes a point where I have a bit more time, I might take you up on the offer for discussion on a larger hygiene effor @nikomatsakis, however I have currently not enough time to put towards such a project.

@davidv1992 davidv1992 closed this Sep 22, 2022
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Sep 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. finished-final-comment-period The final comment period is finished for this PR / Issue. 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.

E0530 when using std::thread_local and a static named init
8 participants