More detail when expecting expression but encountering bad macro argument by estebank · Pull Request #114292 · rust-lang/rust

estebank · 2023-07-31T14:56:55Z

On nested macro invocations where the same macro fragment changes fragment type from one to the next, point at the chain of invocations and at the macro fragment definition place, explaining that the change has occurred.

Fix #71039.

error: expected expression, found pattern `1 + 1`
  --> $DIR/trace_faulty_macros.rs:49:37
   |
LL |     (let $p:pat = $e:expr) => {test!(($p,$e))};
   |                   -------                -- this is interpreted as expression, but it is expected to be pattern
   |                   |
   |                   this macro fragment matcher is expression
...
LL |     (($p:pat, $e:pat)) => {let $p = $e;};
   |               ------                ^^ expected expression
   |               |
   |               this macro fragment matcher is pattern
...
LL |     test!(let x = 1+1);
   |     ------------------
   |     |             |
   |     |             this is expected to be expression
   |     in this macro invocation
   |
   = note: when forwarding a matched fragment to another macro-by-example, matchers in the second macro will see an opaque AST of the fragment type, not the underlying tokens
   = note: this error originates in the macro `test` (in Nightly builds, run with -Z macro-backtrace for more info)

rustbot · 2023-07-31T14:57:01Z

r? @compiler-errors

(rustbot has picked a reviewer for you, use r? to override)

compiler-errors · 2023-07-31T14:58:29Z

r? compiler

kinda swamped

compiler-errors · 2023-07-31T15:00:49Z

tests/ui/macros/trace_faulty_macros.stderr

@@ -53,6 +53,9 @@ LL |     my_recursive_macro!();
 error: expected expression, found `A { a: a, b: 0, c: _, .. }`


Could we perhaps just say "found pattern A ..." instead of highlighting a span and having to render out a possibly very long token-stream in an additional note?

Changed to do that

compiler-errors · 2023-07-31T15:02:47Z

tests/ui/macros/trace_faulty_macros.stderr

+error: expected expression, found `1 + 1`
+  --> $DIR/trace_faulty_macros.rs:49:37
+   |
+LL |     (let $p:pat = $e:expr) => {test!(($p,$e))};
+   |                                          -- this is interpreted as pattern `1 + 1` (in expansion #2)
+...
+LL |     (($p:pat, $e:pat)) => {let $p = $e;};
+   |                                     ^^ expected expression
+...
+LL |     test!(let x = 1+1);
+   |     ------------------
+   |     |             |
+   |     |             this is interpreted as expression `1 + 1` (in expansion #1)
+   |     in this macro invocation
+   |
+   = note: this error originates in the macro `test` (in Nightly builds, run with -Z macro-backtrace for more info)


This is a lot of words but it still doesn't clearly explain what the issue is here, imo

I know, ideally we would actually point at $e:pat, but without this context it is even harder to understand that the reason 1 + 1 is understood as a pattern is that there's multiple levels of macro evaluation at play.

#71039 is two problems:

reinterpreting expr non-terminals as patterns leads to confusing errors with multiple layers of nesting,

unrelated to nesting, using pattern non-terminals in expr position itself leads to weird errors.

this is an example of (2.), which would be fixed if we were to point at the :pat kind, and which (afaict) isn't helped by this pr?

macro_rules! m { ($e:pat) => { let x = $e; } //~^ ERROR expected expression, found `1` } fn main() { m!(1); }

which (afaict) isn't helped by this pr?

maybe this is a mischaracterization -- the context added by this pr might help a bit, but ideally the only thing we'd need to add a new label for is the :pat part.

Yeah, I think I figured out a way to add the span for e:pat as the def_site context of the span for $e, but I'll need to talk with petrochenkov to see if that's reasonable. If we had that, it could also be used in a bunch of other errors as well.

compiler-errors · 2023-07-31T15:03:26Z

tests/ui/macros/trace_faulty_macros.stderr

+   |                                          -- this is interpreted as pattern `1 + 1` (in expansion #2)
+...
+LL |     (($p:pat, $e:pat)) => {let $p = $e;};
+   |                                     ^^ expected expression


Specifically, it doesn't explain why 1 + 1 is interpreted as a pattern -- I think the only thing we need to point out is that the non-terminal kind is pat

I'm not sure how familiar users are with the "non-terminal" terminology.

It would be helpful to add a link to https://doc.rust-lang.org/reference/macros-by-example.html#forwarding-a-matched-fragment

Copied the wording from the reference, but would like to expand a little bit on the explanation

fee1-dead

I think it would also be helpful to have a test where an input ident is first accepted as $x: ident and then subsequently accepted as an expr then pattern triggering the error. Would be nice to see how the macro backtracking works in that case

compiler/rustc_parse/src/parser/diagnostics.rs

fee1-dead · 2023-07-31T17:50:13Z

tests/ui/macros/trace_faulty_macros.stderr

+   |                                          -- this is interpreted as pattern `1 + 1` (in expansion #2)
+...
+LL |     (($p:pat, $e:pat)) => {let $p = $e;};
+   |                                     ^^ expected expression


It would be helpful to add a link to https://doc.rust-lang.org/reference/macros-by-example.html#forwarding-a-matched-fragment

tests/ui/macros/trace_faulty_macros.rs

fee1-dead · 2023-08-22T08:16:36Z

cc #115089

r? compiler

estebank · 2023-10-10T22:58:32Z

@bors try @rust-timer queue

bors · 2023-10-10T22:59:42Z

⌛ Trying commit 24c4fa0 with merge 06d28f4...

More detail when expecting expression but encountering bad macro argument Partially address rust-lang#71039. ``` error: expected expression, found `1 + 1` --> $DIR/trace_faulty_macros.rs:49:37 | LL | (let $p:pat = $e:expr) => {test!(($p,$e))}; | -- this is interpreted as pattern `1 + 1` (in expansion #2) ... LL | (($p:pat, $e:pat)) => {let $p = $e;}; | ^^ expected expression ... LL | test!(let x = 1+1); | ------------------ | | | | | this is interpreted as expression `1 + 1` (in expansion #1) | in this macro invocation | = note: this error originates in the macro `test` (in Nightly builds, run with -Z macro-backtrace for more info) ```

bors · 2023-10-11T00:13:03Z

☀️ Try build successful - checks-actions
Build commit: 06d28f4 (06d28f4eea09d6d73f6ccc6419d461ae314db6be)

rust-timer · 2023-10-11T02:26:50Z

Finished benchmarking commit (06d28f4): comparison URL.

Overall result: ❌ regressions - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

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.6%	[0.3%, 1.0%]	15
Regressions ❌ (secondary)	2.7%	[0.6%, 4.9%]	14
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.1%]	2
All ❌✅ (primary)	0.6%	[0.3%, 1.0%]	15

Max RSS (memory usage)

Results

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)	1.2%	[1.2%, 1.2%]	1
Regressions ❌ (secondary)	4.5%	[0.9%, 8.2%]	3
Improvements ✅ (primary)	-2.7%	[-2.9%, -2.4%]	2
Improvements ✅ (secondary)	-4.3%	[-6.4%, -1.4%]	5
All ❌✅ (primary)	-1.4%	[-2.9%, 1.2%]	3

Cycles

Results

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.2%	[2.4%, 6.8%]	15
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 628.751s -> 627.511s (-0.20%)
Artifact size: 270.76 MiB -> 270.80 MiB (0.02%)

estebank · 2023-10-11T15:43:30Z

As expected, the tests affected by the larger type are all macro related, in particular tt-muncher (again, unsurprising). I leave it to the reviewer to determine if such a perf impact is worth it for this tracking. An alternative approach would be to create a new span context for macro arguments and inject those in the macro backtrace as the def_span, but the machinery is not set up to do that easily.

…ment Partially address rust-lang#71039.

estebank · 2023-11-16T16:20:33Z

@bors try @rust-timer queue

bors · 2023-11-16T16:21:43Z

⌛ Trying commit 4e41880 with merge 97a26f5...

More detail when expecting expression but encountering bad macro argument Partially address rust-lang#71039. ``` error: expected expression, found pattern `1 + 1` --> $DIR/trace_faulty_macros.rs:49:37 | LL | (let $p:pat = $e:expr) => {test!(($p,$e))}; | ------- -- this is interpreted as expression, but it is expected to be pattern | | | this macro fragment matcher is expression ... LL | (($p:pat, $e:pat)) => {let $p = $e;}; | ------ ^^ expected expression | | | this macro fragment matcher is pattern ... LL | test!(let x = 1+1); | ------------------ | | | | | this is expected to be expression | in this macro invocation | = note: when forwarding a matched fragment to another macro-by-example, matchers in the second macro will see an opaque AST of the fragment type, not the underlying tokens = note: this error originates in the macro `test` (in Nightly builds, run with -Z macro-backtrace for more info) ```

bors · 2023-11-16T17:48:49Z

☀️ Try build successful - checks-actions
Build commit: 97a26f5 (97a26f57d0ee2215ceb9cf711db4a9d340c3b6e4)

rust-timer · 2023-11-16T19:12:04Z

Finished benchmarking commit (97a26f5): comparison URL.

Overall result: ✅ improvements - 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 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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-0.2%, -0.2%]	1
Improvements ✅ (secondary)	-0.5%	[-0.6%, -0.3%]	6
All ❌✅ (primary)	-0.2%	[-0.2%, -0.2%]	1

Max RSS (memory usage)

Results

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.0%	[0.6%, 4.2%]	3
Regressions ❌ (secondary)	4.8%	[3.0%, 5.8%]	3
Improvements ✅ (primary)	-0.6%	[-0.6%, -0.6%]	2
Improvements ✅ (secondary)	-3.6%	[-5.7%, -2.5%]	6
All ❌✅ (primary)	0.9%	[-0.6%, 4.2%]	5

Cycles

Results

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)	1.7%	[0.9%, 2.6%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 676.569s -> 674.073s (-0.37%)
Artifact size: 313.63 MiB -> 313.62 MiB (-0.00%)

estebank · 2023-11-16T21:37:36Z

@b-naber given the current perf results, I would like to merge this PR without further changes 😄

estebank · 2023-11-17T19:05:30Z

Ah, I see that you had already approved earlier, contingent on perf @b-naber. If the current code looks reasonable, I'd like to merge this sooner rather than later, as it is a bit bitrotty.

b-naber · 2023-11-17T19:16:34Z

@bors r+

bors · 2023-11-17T19:16:37Z

📌 Commit 4e41880 has been approved by b-naber

It is now in the queue for this repository.

bors · 2023-11-17T20:57:16Z

⌛ Testing commit 4e41880 with merge 2831701...

bors · 2023-11-17T22:57:12Z

☀️ Test successful - checks-actions
Approved by: b-naber
Pushing 2831701 to master...

rust-timer · 2023-11-18T00:18:57Z

Finished benchmarking commit (2831701): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

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
Regressions ❌ (secondary)	1.0%	[1.0%, 1.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.4%	[-0.6%, -0.2%]	6
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

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)	2.8%	[1.3%, 5.5%]	3
Improvements ✅ (primary)	-1.5%	[-2.7%, -0.4%]	6
Improvements ✅ (secondary)	-6.8%	[-10.7%, -1.6%]	6
All ❌✅ (primary)	-1.5%	[-2.7%, -0.4%]	6

Cycles

Results

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.4%	[0.4%, 0.4%]	1
Regressions ❌ (secondary)	1.6%	[1.2%, 2.0%]	4
Improvements ✅ (primary)	-0.7%	[-0.7%, -0.7%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.2%	[-0.7%, 0.4%]	2

Binary size

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

Bootstrap: 675.186s -> 675.761s (0.09%)
Artifact size: 313.59 MiB -> 313.69 MiB (0.03%)

rustbot assigned compiler-errors Jul 31, 2023

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 Jul 31, 2023

rustbot assigned fee1-dead and unassigned compiler-errors Jul 31, 2023

compiler-errors reviewed Jul 31, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

compiler-errors reviewed Jul 31, 2023

View reviewed changes

fee1-dead reviewed Jul 31, 2023

View reviewed changes

fee1-dead 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 Aug 7, 2023

rustbot assigned b-naber and unassigned fee1-dead Aug 22, 2023

estebank force-pushed the issue-71039 branch from c9266ef to 51f3d24 Compare October 10, 2023 18:58

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 10, 2023

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Oct 11, 2023

This comment was marked as resolved.

Sign in to view

estebank force-pushed the issue-71039 branch from bb992a2 to 03fde33 Compare October 15, 2023 22:34

This comment was marked as resolved.

Sign in to view

estebank force-pushed the issue-71039 branch from 03fde33 to a9a0cd9 Compare November 16, 2023 16:10

This comment was marked as resolved.

Sign in to view

More detail when expecting expression but encountering bad macro argu…

4e41880

…ment Partially address rust-lang#71039.

estebank force-pushed the issue-71039 branch from a9a0cd9 to 4e41880 Compare November 16, 2023 16:19

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 16, 2023

This comment has been minimized.

Sign in to view

rustbot removed S-waiting-on-perf Status: Waiting on a perf run to be completed. perf-regression Performance regression. labels Nov 16, 2023

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 17, 2023

bors mentioned this pull request Nov 17, 2023

Add some additional warnings for duplicated diagnostic items #117742

Merged

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 17, 2023

bors merged commit 2831701 into rust-lang:master Nov 17, 2023

rustbot added this to the 1.76.0 milestone Nov 17, 2023

		@@ -53,6 +53,9 @@ LL \| my_recursive_macro!();
		error: expected expression, found `A { a: a, b: 0, c: _, .. }`

Uh oh!

Conversation

estebank commented Jul 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jul 31, 2023

Uh oh!

compiler-errors commented Jul 31, 2023

Uh oh!

compiler-errors Jul 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compiler-errors Jul 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fee1-dead left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fee1-dead commented Aug 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

estebank commented Oct 10, 2023

Uh oh!

This comment has been minimized.

bors commented Oct 10, 2023

Uh oh!

bors commented Oct 11, 2023

Uh oh!

This comment has been minimized.

rust-timer commented Oct 11, 2023

Overall result: ❌ regressions - ACTION NEEDED

Uh oh!

estebank commented Oct 11, 2023

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

estebank commented Nov 16, 2023

Uh oh!

This comment has been minimized.

bors commented Nov 16, 2023

Uh oh!

bors commented Nov 16, 2023

Uh oh!

This comment has been minimized.

rust-timer commented Nov 16, 2023

Overall result: ✅ improvements - no action needed

Uh oh!

estebank commented Nov 16, 2023

Uh oh!

estebank commented Nov 17, 2023

Uh oh!

b-naber commented Nov 17, 2023

Uh oh!

estebank commented Jul 31, 2023 •

edited

Loading

compiler-errors Jul 31, 2023 •

edited

Loading

compiler-errors Jul 31, 2023 •

edited

Loading

fee1-dead commented Aug 22, 2023 •

edited

Loading