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

Use edition of macro_rules when compiling the macro #133274

Merged
merged 2 commits into from
Nov 27, 2024

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Nov 21, 2024

This changes the edition assigned to a macro_rules macro when it is compiled to use the edition of where the macro came from instead of the local crate's edition.

This fixes a problem when a macro_rules macro is created by a proc-macro. Previously that macro would be tagged with the local edition, which would cause problems with using the correct edition behavior inside the macro. For example, the check for unsafe attributes would cause errors in 2024 when using proc-macros from older editions.

This is partially related to #132906. Unfortunately this is only a half fix for that issue. It fixes the error that happens in 2024, but does not fix the lint firing in 2021. I'm still trying to think of some way to fix that, but I'm running low on ideas.

@rustbot
Copy link
Collaborator

rustbot commented Nov 21, 2024

r? @compiler-errors

rustbot has assigned @compiler-errors.
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 Nov 21, 2024
@rust-log-analyzer

This comment has been minimized.

@ehuss ehuss force-pushed the macro_rules-edition-from-pm branch from 94c2a92 to 993e084 Compare November 21, 2024 01:29
@compiler-errors
Copy link
Member

Do you expect this to have any effect on preexisting code? In that case, should we crater this change?

@petrochenkov
Copy link
Contributor

It would be better to run crater on this.

@compiler-errors
Copy link
Member

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 21, 2024
…=<try>

Use edition of `macro_rules` when compiling the macro

This changes the edition assigned to a macro_rules macro when it is compiled to use the edition of where the macro came from instead of the local crate's edition.

This fixes a problem when a macro_rules macro is created by a proc-macro. Previously that macro would be tagged with the local edition, which would cause problems with using the correct edition behavior inside the macro. For example, the check for unsafe attributes would cause errors in 2024 when using proc-macros from older editions.

This is partially related to rust-lang#132906. Unfortunately this is only a half fix for that issue. It fixes the error that happens in 2024, but does not fix the lint firing in 2021. I'm still trying to think of some way to fix that, but I'm running low on ideas.
@bors
Copy link
Contributor

bors commented Nov 21, 2024

⌛ Trying commit 993e084 with merge 9a6ce9b...

@bors
Copy link
Contributor

bors commented Nov 21, 2024

☀️ Try build successful - checks-actions
Build commit: 9a6ce9b (9a6ce9bcee4fff9f2010ea88aaa7654cc4d95132)

@compiler-errors
Copy link
Member

@craterbot check p=10

@craterbot
Copy link
Collaborator

👌 Experiment pr-133274 created and queued.
🤖 Automatically detected try build 9a6ce9b
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 21, 2024
@compiler-errors
Copy link
Member

@rust-timer build 9a6ce9b

@rust-timer

This comment has been minimized.

@craterbot
Copy link
Collaborator

🚧 Experiment pr-133274 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@compiler-errors
Copy link
Member

Actually why did I start rust-timer on this? Anyways, ignore that.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (9a6ce9b): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 795.947s -> 794.767s (-0.15%)
Artifact size: 336.01 MiB -> 336.00 MiB (-0.00%)

@craterbot
Copy link
Collaborator

🎉 Experiment pr-133274 is completed!
📊 15 regressed and 4 fixed (541620 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Nov 24, 2024
@compiler-errors
Copy link
Member

From a cursory glance, looks like they're all just spurious crater failures. It does not seem like any of these crates are affected by this change.

@compiler-errors
Copy link
Member

@bors r+ rollup=never (for bisection, just in case)

@bors
Copy link
Contributor

bors commented Nov 27, 2024

📌 Commit 993e084 has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 27, 2024
@bors
Copy link
Contributor

bors commented Nov 27, 2024

⌛ Testing commit 993e084 with merge 83965ef...

@bors
Copy link
Contributor

bors commented Nov 27, 2024

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 83965ef to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 27, 2024
@bors bors merged commit 83965ef into rust-lang:master Nov 27, 2024
7 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Nov 27, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (83965ef): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary -3.5%, secondary -2.9%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-3.5% [-3.5%, -3.5%] 1
Improvements ✅
(secondary)
-2.9% [-2.9%, -2.9%] 1
All ❌✅ (primary) -3.5% [-3.5%, -3.5%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 793.27s -> 793.555s (0.04%)
Artifact size: 336.19 MiB -> 336.20 MiB (0.00%)

ehuss added a commit to ehuss/rust that referenced this pull request Feb 3, 2025
This adds tests to check the behavior of how nested macro_rules
definitions work across edition boundaries. This covers a change in
behavior due to rust-lang#133274.

See rust-lang#135669
ehuss added a commit to ehuss/rust that referenced this pull request Feb 3, 2025
This adds tests to check the behavior of how nested macro_rules
definitions work across edition boundaries. This covers a change in
behavior due to rust-lang#133274.

See rust-lang#135669
ehuss added a commit to ehuss/rust that referenced this pull request Feb 3, 2025
This adds tests to check the behavior of how nested macro_rules
definitions work across edition boundaries. This covers a change in
behavior due to rust-lang#133274.

See rust-lang#135669
ehuss added a commit to ehuss/rust that referenced this pull request Feb 3, 2025
This adds tests to check the behavior of how nested macro_rules
definitions work across edition boundaries. This covers a change in
behavior due to rust-lang#133274.

See rust-lang#135669
jhpratt added a commit to jhpratt/rust that referenced this pull request Feb 4, 2025
…r=jieyouxu

Add tests for nested macro_rules edition behavior

This adds tests to check the behavior of how nested macro_rules definitions work across edition boundaries. This covers a change in behavior due to rust-lang#133274.

See rust-lang#135669
fmease added a commit to fmease/rust that referenced this pull request Feb 5, 2025
…r=jieyouxu

Add tests for nested macro_rules edition behavior

This adds tests to check the behavior of how nested macro_rules definitions work across edition boundaries. This covers a change in behavior due to rust-lang#133274.

See rust-lang#135669
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 5, 2025
Rollup merge of rust-lang#136509 - ehuss:nested-macro-rules-edition, r=jieyouxu

Add tests for nested macro_rules edition behavior

This adds tests to check the behavior of how nested macro_rules definitions work across edition boundaries. This covers a change in behavior due to rust-lang#133274.

See rust-lang#135669
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Feb 6, 2025
Add tests for nested macro_rules edition behavior

This adds tests to check the behavior of how nested macro_rules definitions work across edition boundaries. This covers a change in behavior due to rust-lang/rust#133274.

See rust-lang/rust#135669
@apiraino
Copy link
Contributor

apiraino commented Feb 6, 2025

This caused a regression in #135669. T-lang decided to keep this behaviour, at least for now (comment). For this reason I'm adding this to be mentioned in the next releases notes (hoping it's the correct procedure):

@rustbot label +relnotes

@rustbot rustbot added the relnotes Marks issues that should be documented in the release notes of the next release. label Feb 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

10 participants