Update `expr` matcher for Edition 2024 and add `expr_2021` nonterminal #123865

eholk · 2024-04-12T17:34:08Z

This commit adds a new nonterminal expr_2021 in macro patterns, and expr_fragment_specifier_2024 feature flag.

This change also updates expr so that on Edition 2024 it will also match const { ... } blocks, while expr_2021 preserves the current behavior of expr, matching expressions without const blocks.

Joint work with @vincenzopalazzo.

Issue #123742

rustbot · 2024-04-12T17:34:15Z

r? @wesleywiser

rustbot has assigned @wesleywiser.
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

compiler/rustc_ast/src/token.rs

tests/ui/macros/expr_2021_old_edition.stderr

compiler/rustc_expand/src/mbe/quoted.rs

compiler/rustc_parse/src/parser/nonterminal.rs

tests/ui/macros/expr_2021_inline_const.rs

fmease · 2024-04-24T03:07:12Z

I think the biggest question left is what exactly should be included in Rust 2024's expr as you've started to discuss in the tracking issue.

eholk · 2024-05-01T18:46:22Z

@fmease - I addressed your simpler comments, so I think this should be ready for another look.

vincenzopalazzo · 2024-05-09T08:14:02Z

Looks like we should update the tests

  failures:
  
  ---- [ui] tests/ui/macros/expr_2021_inline_const.rs#edi2021 stdout ----
  diff of stderr:
  
  28	LL |     ($e:expr) => {
  29	   |      ^^^^^^^
  30	
  -	error: aborting due to 2 previous errors
  +	warning: the feature `inline_const` has been stable since 1.80.0-nightly and no longer requires an attribute to enable
  +	  --> $DIR/expr_2021_inline_const.rs:7:12
  +	   |
  +	LL | #![feature(inline_const)]
  +	   |            ^^^^^^^^^^^^
  +	   |
  +	   = note: `#[warn(stable_features)]` on by default
  +	
  +	error: aborting due to 2 previous errors; 1 warning emitted
  32	
  33

This should be the diff that it is needed

diff --git a/tests/ui/macros/expr_2021_inline_const.rs b/tests/ui/macros/expr_2021_inline_const.rs
index ffce1f2b908..ebc5ea36421 100644
--- a/tests/ui/macros/expr_2021_inline_const.rs
+++ b/tests/ui/macros/expr_2021_inline_const.rs
@@ -4,7 +4,6 @@
 
 // This test ensures that the inline const match only on edition 2024
 #![feature(expr_fragment_specifier_2024)]
-#![feature(inline_const)]
 #![allow(incomplete_features)]
 
 macro_rules! m2021 {

and then --bless should make the CI again happy.

This commit adds a new nonterminal `expr_2021` in macro patterns, and `expr_fragment_specifier_2024` feature flag. For now, `expr` and `expr_2021` are treated the same, but in future PRs we will update `expr` to match to new grammar. Co-authored-by: Vincezo Palazzo <vincenzopalazzodev@gmail.com>

Co-authored-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>

Co-authored-by: Eric Holk <eric@theincredibleholk.org> Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>

- use feature_err to report unstable expr_2021 - Update downlevel expr_2021 diagnostics Co-authored-by: León Orell Valerian Liehr <me@fmease.dev>

fmease

Thanks! My suggestion https://github.com/rust-lang/rust/pull/123865/files#r1577167279 wasn't applied, I assume that's an oversight because you've applied suggestions of mine that are similar to this.

Should be good to go after that, work like double-checking the behavior of weird nested macro decls can be done in follow-ups as suggested by you.

r=me then.

fmease · 2024-05-15T20:18:35Z

tests/ui/macros/expr_2021_old_edition.stderr

+ = help: fragment specifier `expr_2021` requires Rust 2021 or later
+ valid fragment specifiers are `ident`, `block`, `stmt`, `expr`, `pat`, `ty`, `lifetime`, `literal`, `path`, `meta`, `tt`, `item` and `vis`


I would probably split this into two separate help messages but er, doesn't matter

Yeah, I think that'd be better. Let me poke around at it some more, I'm still getting the hang of the diagnostics system.

Actually, I'd rather save this for another PR so we can wrap this one up. I feel like otherwise I'll sit on this for another week or two, and I'd like to get this in as a foundation for similar edition-related changes.

fmease · 2024-05-15T20:20:21Z

tests/ui/macros/expr_2021_inline_const.edi2021.stderr

This (intentional) behavior is a bit unfortunate but it makes sense. Well, it makes the most sense when edition-dependent keywords are involved.

compiler/rustc_parse/src/parser/nonterminal.rs

Co-authored-by: León Orell Valerian Liehr <me@fmease.dev>

bors · 2024-05-18T00:02:54Z

☀️ Test successful - checks-actions
Approved by: fmease
Pushing 9b75a43 to master...

rust-timer · 2024-05-18T01:18:31Z

Finished benchmarking commit (9b75a43): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.2%, 0.3%]	5
Regressions ❌ (secondary)	0.5%	[0.2%, 1.7%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.2%	[0.2%, 0.3%]	5

Max RSS (memory usage)

Results (primary 0.0%, secondary -4.7%)

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)	2.6%	[2.6%, 2.6%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.3%	[-2.3%, -0.2%]	2
Improvements ✅ (secondary)	-4.7%	[-4.7%, -4.7%]	1
All ❌✅ (primary)	0.0%	[-2.3%, 2.6%]	3

Cycles

Results (primary -3.1%, secondary 2.5%)

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)	4.1%	[2.0%, 8.1%]	8
Improvements ✅ (primary)	-3.1%	[-3.1%, -3.1%]	1
Improvements ✅ (secondary)	-1.7%	[-2.1%, -1.3%]	3
All ❌✅ (primary)	-3.1%	[-3.1%, -3.1%]	1

Binary size

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

Bootstrap: 668.756s -> 668.325s (-0.06%)
Artifact size: 316.02 MiB -> 316.15 MiB (0.04%)

rylev · 2024-05-21T12:16:44Z

@eholk @vincenzopalazzo @fmease the regression seems legit. It seems the primary benchmark affected by this html5ever seems to spend lots of time in expand_crate which could be impacted by this change. Thoughts on whether this needs further investigation or if we should just treat that particular benchmark as a quasi-stress-test and move on?

vincenzopalazzo · 2024-05-21T13:43:55Z

@rylev I was planning to discuss with @eholk and take a look at the code, idk if @eholk already gave a look at the code, but I need a couple of more days

[perf-only] Revert "Update `expr` matcher for Edition 2024 and add `expr_2021` nonterminal" CC rust-lang#123865 r? ghost

fmease · 2024-05-23T02:37:11Z

Update: In #125389 I've checked the perf of a full revert. Doing a naive inversion / deduction, this PR "might've only had" 8 counts of secondary regressions (mean: +0.3%, range: [+0.5%, +0.3%]). Anyways, if I find the time I will look deeper into this. Otherwise, I'll just wait for #123865 (comment).

vincenzopalazzo

I finally had a look at the code and I post my analysis, I am not sure how to solve this BTW :/

vincenzopalazzo · 2024-05-25T11:17:29Z

compiler/rustc_parse/src/parser/nonterminal.rs

+ token.can_begin_expr()
+ // This exception is here for backwards compatibility.
+ && !token.is_keyword(kw::Let)
+ && (token.span.edition().at_least_rust_2024() || !token.is_keyword(kw::Const))


I am not a performance expert, so I might be wrong here, but I think the only thing that could cause a performance issue is the token.span.edition().

If we follow the call stack, the edition() will resolve in the following call: https://github.com/rust-lang/rust/blob/master/compiler/rustc_span/src/hygiene.rs#L877-L879.

If we repeat this call for every expression token, we could see a performance decrease.

…egression, r=<try> [perf-only] rustc_parser: avoid checking the edition as much as possible CC rust-lang#123865 r? ghost

@eholk

Inside rust-lang#123865, we are adding support for the new semantics for expr2024, but we have noted a performance issue. We realized there is a redundant check for each token regarding an edition. This commit moves the edition check to the end, avoiding some extra checks that can slow down compilation time. Link: rust-lang#123865 Co-Developed-by: @eholk Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>

@eholk

Inside rust-lang#123865, we are adding support for the new semantics for expr2024, but we have noted a performance issue. We realized there is a redundant check for each token regarding an edition. This commit moves the edition check to the end, avoiding some extra checks that can slow down compilation time. Link: rust-lang#123865 Co-Developed-by: @eholk Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>

@eholk

Inside rust-lang#123865, we are adding support for the new semantics for expr2024, but we have noted a performance issue. We realized there is a redundant check for each token regarding an edition. This commit moves the edition check to the end, avoiding some extra checks that can slow down compilation time. Link: rust-lang#123865 Co-Developed-by: @eholk Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>

…egression, r=fmease Avoid checking the edition as much as possible Inside rust-lang#123865, we are adding support for the new semantics for expr2024, but we have noted a performance issue. While talking with `@eholk,` we realized there is a redundant check for each token regarding an edition. This commit moves the edition check to the end, avoiding some extra checks that can slow down compilation time. However, we should keep this issue under observation because we may want to improve the edition check if we are unable to significantly improve compiler performance. r? ghost

…, r=fmease Avoid checking the edition as much as possible Inside rust-lang/rust#123865, we are adding support for the new semantics for expr2024, but we have noted a performance issue. While talking with `@eholk,` we realized there is a redundant check for each token regarding an edition. This commit moves the edition check to the end, avoiding some extra checks that can slow down compilation time. However, we should keep this issue under observation because we may want to improve the edition check if we are unable to significantly improve compiler performance. r? ghost

Make edition dependent `:expr` macro fragment act like the edition-dependent `:pat` fragment does Parse the `:expr` fragment as `:expr_2021` in editions <=2021, and as `:expr` in edition 2024. This is similar to how we parse `:pat` as `:pat_param` in edition <=2018 and `:pat_with_or` in >=2021, and means we can get rid of a span dependency from `nonterminal_may_begin_with`. Specifically, this fixes a theoretical regression since the `expr_2021` macro fragment previously would allow `const {}` if the *caller* is edition 2024. This is inconsistent with the way that the `pat` macro fragment was upgraded, and also leads to surprising behavior when a macro *caller* crate upgrades to edtion 2024, since they may have parsing changes that they never asked for (with no way of opting out of it). This PR also allows using `expr_2021` in all editions. Why was this was disallowed in the first place? It's purely additive, and also it's still feature gated? r? `@fmease` `@eholk` cc `@vincenzopalazzo` cc rust-lang#123865 Tracking: - rust-lang#123742

Make edition dependent `:expr` macro fragment act like the edition-dependent `:pat` fragment does Parse the `:expr` fragment as `:expr_2021` in editions <=2021, and as `:expr` in edition 2024. This is similar to how we parse `:pat` as `:pat_param` in edition <=2018 and `:pat_with_or` in >=2021, and means we can get rid of a span dependency from `nonterminal_may_begin_with`. Specifically, this fixes a theoretical regression since the `expr_2021` macro fragment previously would allow `const {}` if the *caller* is edition 2024. This is inconsistent with the way that the `pat` macro fragment was upgraded, and also leads to surprising behavior when a macro *caller* crate upgrades to edtion 2024, since they may have parsing changes that they never asked for (with no way of opting out of it). This PR also allows using `expr_2021` in all editions. Why was this was disallowed in the first place? It's purely additive, and also it's still feature gated? r? ``@fmease`` ``@eholk`` cc ``@vincenzopalazzo`` cc rust-lang#123865 Tracking: - rust-lang#123742

Make edition dependent `:expr` macro fragment act like the edition-dependent `:pat` fragment does Parse the `:expr` fragment as `:expr_2021` in editions <=2021, and as `:expr` in edition 2024. This is similar to how we parse `:pat` as `:pat_param` in edition <=2018 and `:pat_with_or` in >=2021, and means we can get rid of a span dependency from `nonterminal_may_begin_with`. Specifically, this fixes a theoretical regression since the `expr_2021` macro fragment previously would allow `const {}` if the *caller* is edition 2024. This is inconsistent with the way that the `pat` macro fragment was upgraded, and also leads to surprising behavior when a macro *caller* crate upgrades to edtion 2024, since they may have parsing changes that they never asked for (with no way of opting out of it). This PR also allows using `expr_2021` in all editions. Why was this was disallowed in the first place? It's purely additive, and also it's still feature gated? r? ```@fmease``` ```@eholk``` cc ```@vincenzopalazzo``` cc rust-lang#123865 Tracking: - rust-lang#123742

Rollup merge of rust-lang#126700 - compiler-errors:fragment, r=fmease Make edition dependent `:expr` macro fragment act like the edition-dependent `:pat` fragment does Parse the `:expr` fragment as `:expr_2021` in editions <=2021, and as `:expr` in edition 2024. This is similar to how we parse `:pat` as `:pat_param` in edition <=2018 and `:pat_with_or` in >=2021, and means we can get rid of a span dependency from `nonterminal_may_begin_with`. Specifically, this fixes a theoretical regression since the `expr_2021` macro fragment previously would allow `const {}` if the *caller* is edition 2024. This is inconsistent with the way that the `pat` macro fragment was upgraded, and also leads to surprising behavior when a macro *caller* crate upgrades to edtion 2024, since they may have parsing changes that they never asked for (with no way of opting out of it). This PR also allows using `expr_2021` in all editions. Why was this was disallowed in the first place? It's purely additive, and also it's still feature gated? r? ```@fmease``` ```@eholk``` cc ```@vincenzopalazzo``` cc rust-lang#123865 Tracking: - rust-lang#123742

…acros-between-edition, r=compiler-errors test: cross-edition metavar fragment specifiers There's a subtle interaction between macros with metavar expressions and the edition-dependent fragment matching behavior. This test illustrates the current behavior when using macro-generating-macros across crate boundaries with different editions. See the original suggestion rust-lang#123865 (comment) Tracking: - rust-lang#123742

…ros-between-edition, r=compiler-errors test: cross-edition metavar fragment specifiers There's a subtle interaction between macros with metavar expressions and the edition-dependent fragment matching behavior. This test illustrates the current behavior when using macro-generating-macros across crate boundaries with different editions. See the original suggestion rust-lang#123865 (comment) Tracking: - rust-lang#123742

rustbot assigned wesleywiser Apr 12, 2024

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 12, 2024

eholk force-pushed the expr_2021 branch 2 times, most recently from 512458e to 0777912 Compare April 12, 2024 17:39

traviscross added the A-edition-2024 Area: The 2024 edition label Apr 12, 2024

eholk commented Apr 12, 2024

View reviewed changes

compiler/rustc_ast/src/token.rs Outdated Show resolved Hide resolved

eholk changed the title ~~Add expr_2021 nonterminal and feature flag~~ Update expr matcher for Edition 2024 and add expr_2021 nonterminal Apr 16, 2024

eholk mentioned this pull request Apr 17, 2024

Non-terminals, e.g. expr, have diverged between parser and macro matcher #86730

Open

fmease reviewed Apr 24, 2024

View reviewed changes

traviscross mentioned this pull request Apr 30, 2024

Tracking Issue for updating expr macro fragment specifier for Rust 2024 #123742

Open

10 tasks

This comment has been minimized.

Sign in to view

fmease self-assigned this May 10, 2024

eholk and others added 5 commits May 13, 2024 11:27

Add test that expr_2021 only works on 2024 edition

65da4ad

Co-authored-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>

expr_2021 should be allowed on edition 2021 and later

73303c3

Macros: match const { ... } with expr nonterminal in edition 2024

a55d063

Co-authored-by: Eric Holk <eric@theincredibleholk.org> Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>

Apply code review suggestions

f364011

- use feature_err to report unstable expr_2021 - Update downlevel expr_2021 diagnostics Co-authored-by: León Orell Valerian Liehr <me@fmease.dev>

eholk force-pushed the expr_2021 branch from acbc58c to f364011 Compare May 13, 2024 18:56

fmease approved these changes May 15, 2024

View reviewed changes

fmease unassigned wesleywiser May 15, 2024

fmease added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 16, 2024

This comment has been minimized.

Sign in to view

Update compiler/rustc_parse/src/parser/nonterminal.rs

3986ea0

Co-authored-by: León Orell Valerian Liehr <me@fmease.dev>

bors added the merged-by-bors This PR was explicitly merged by bors. label May 18, 2024

bors merged commit 9b75a43 into rust-lang:master May 18, 2024
7 checks passed

rustbot added this to the 1.80.0 milestone May 18, 2024

This was referenced May 18, 2024

Stabilize extended_varargs_abi_support #116161

Open

Remove Nonterminal and TokenKind::Interpolated #124141

Open

rustbot added the perf-regression Performance regression. label May 18, 2024

fmease mentioned this pull request May 21, 2024

[perf-only] Revert "Update expr matcher for Edition 2024 and add expr_2021 nonterminal" #125389

Closed

bors added a commit to rust-lang-ci/rust that referenced this pull request May 21, 2024

Auto merge of rust-lang#125389 - fmease:perf-expr_2021, r=<try>

428af23

[perf-only] Revert "Update `expr` matcher for Edition 2024 and add `expr_2021` nonterminal" CC rust-lang#123865 r? ghost

vincenzopalazzo reviewed May 25, 2024

View reviewed changes

vincenzopalazzo mentioned this pull request May 31, 2024

Avoid checking the edition as much as possible #125828

Merged

bors added a commit to rust-lang-ci/rust that referenced this pull request May 31, 2024

Auto merge of rust-lang#125828 - vincenzopalazzo:macros/performance-r…

d693eba

…egression, r=<try> [perf-only] rustc_parser: avoid checking the edition as much as possible CC rust-lang#123865 r? ghost

This was referenced Jun 19, 2024

[RFC] mbe: consider the _ in 2024 an expression #126697

Merged

Make edition dependent :expr macro fragment act like the edition-dependent :pat fragment does #126700

Merged

vincenzopalazzo mentioned this pull request Aug 29, 2024

test: cross-edition metavar fragment specifiers #129755

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update `expr` matcher for Edition 2024 and add `expr_2021` nonterminal #123865

Update `expr` matcher for Edition 2024 and add `expr_2021` nonterminal #123865

eholk commented Apr 12, 2024 •

edited

Loading

rustbot commented Apr 12, 2024

fmease commented Apr 24, 2024

This comment has been minimized.

eholk commented May 1, 2024

This comment has been minimized.

vincenzopalazzo commented May 9, 2024 •

edited

Loading

fmease left a comment

fmease May 15, 2024

eholk May 17, 2024

eholk May 17, 2024

fmease May 15, 2024

This comment has been minimized.

bors commented May 18, 2024

rust-timer commented May 18, 2024

rylev commented May 21, 2024

vincenzopalazzo commented May 21, 2024

fmease commented May 23, 2024

vincenzopalazzo left a comment

vincenzopalazzo May 25, 2024

		= help: fragment specifier `expr_2021` requires Rust 2021 or later
		valid fragment specifiers are `ident`, `block`, `stmt`, `expr`, `pat`, `ty`, `lifetime`, `literal`, `path`, `meta`, `tt`, `item` and `vis`

Update expr matcher for Edition 2024 and add expr_2021 nonterminal #123865

Update expr matcher for Edition 2024 and add expr_2021 nonterminal #123865

Conversation

eholk commented Apr 12, 2024 • edited Loading

rustbot commented Apr 12, 2024

fmease commented Apr 24, 2024

This comment has been minimized.

eholk commented May 1, 2024

This comment has been minimized.

vincenzopalazzo commented May 9, 2024 • edited Loading

fmease left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

bors commented May 18, 2024

rust-timer commented May 18, 2024

Overall result: ❌ regressions - ACTION NEEDED

rylev commented May 21, 2024

vincenzopalazzo commented May 21, 2024

fmease commented May 23, 2024

vincenzopalazzo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Update `expr` matcher for Edition 2024 and add `expr_2021` nonterminal #123865

Update `expr` matcher for Edition 2024 and add `expr_2021` nonterminal #123865

eholk commented Apr 12, 2024 •

edited

Loading

vincenzopalazzo commented May 9, 2024 •

edited

Loading