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

Add a note to the ArbitraryExpressionInPattern error #124488

Merged
merged 2 commits into from Apr 30, 2024

Conversation

est31
Copy link
Member

@est31 est31 commented Apr 28, 2024

The current "arbitrary expressions aren't allowed in patterns" error is confusing, as it fires for code where it looks like a pattern but the compiler still treats it as an expression. That this is due to the :expr fragment specifier forcing the expression-ness property on the code.

In the test suite, the "arbitrary expressions aren't allowed in patterns" error can only be found in combination with macro_rules macros that force expression-ness of their content, namely via :expr metavariables. I also can't come up with cases where there would be an expression instead of a pattern, so I think it's always coming from an :expr.

In order to make the error less confusing, this adds a note explaining the weird :expr fragment behaviour.

Fixes #99380

@rustbot
Copy link
Collaborator

rustbot commented Apr 28, 2024

r? @pnkfelix

rustbot has assigned @pnkfelix.
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 Apr 28, 2024
@est31 est31 force-pushed the arbitrary_expressions_error branch from 7974b09 to d339d1a Compare April 28, 2024 18:47
@rust-log-analyzer

This comment has been minimized.

@est31 est31 force-pushed the arbitrary_expressions_error branch from d339d1a to 4284bca Compare April 28, 2024 19:27
@est31
Copy link
Member Author

est31 commented Apr 29, 2024

I also can't come up with cases where there would be an expression instead of a pattern

Note that should in the future anyone discover cases where there was no involvement of :expr fragment specifiers, we can still change the wording or make it dependent on fragment specifiers being present.

@pnkfelix
Copy link
Member

#99380 is interesting. It seems like the intent was to signal that the macro definition itself is ill-formed, but the error reporting machinery is only firing after the macro itself is invoked, and only highlights the macro invocation site, rather than saying anything at all about the macro definition.

Does that sound like a reasonable synopsis of what problem the error message itself was trying to address, @est31 ?

@pnkfelix
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Apr 29, 2024

📌 Commit c6e946d has been approved by pnkfelix

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 Apr 29, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 29, 2024
…iaskrgr

Rollup of 3 pull requests

Successful merges:

 - rust-lang#124185 (Remove optionality from MoveData::base_local)
 - rust-lang#124488 (Add a note to the ArbitraryExpressionInPattern error)
 - rust-lang#124530 (Fix Fuchsia build broken by rust-lang#124210)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 42ab090 into rust-lang:master Apr 30, 2024
10 checks passed
@rustbot rustbot added this to the 1.80.0 milestone Apr 30, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 30, 2024
Rollup merge of rust-lang#124488 - est31:arbitrary_expressions_error, r=pnkfelix

Add a note to the ArbitraryExpressionInPattern error

The current "arbitrary expressions aren't allowed in patterns" error is confusing, as it fires for code where it *looks* like a pattern but the compiler still treats it as an expression. That this is due to the `:expr` fragment specifier forcing the expression-ness property on the code.

In the test suite, the "arbitrary expressions aren't allowed in patterns" error can only be found in combination with macro_rules macros that force expression-ness of their content, namely via `:expr` metavariables. I also can't come up with cases where there would be an expression instead of a pattern, so I think it's always coming from an `:expr`.

In order to make the error less confusing, this adds a note explaining the weird `:expr` fragment behaviour.

Fixes rust-lang#99380
@est31
Copy link
Member Author

est31 commented Apr 30, 2024

@pnkfelix

It seems like the intent was to signal that the macro definition itself is ill-formed, but the error reporting machinery is only firing after the macro itself is invoked, and only highlights the macro invocation site, rather than saying anything at all about the macro definition.

I don't know what the original intent of the error was but in many cases, the usual way to resolve is indeed to change the fragment specifier from expr to something more generic like tt. Sometimes the expr property is intended, like for the vec![] macro used in patterns for example.

Before this patch, the error message was extremely terse and would cause a lot of confusion as to why it's an expression and not a pattern. Now, it at least mentions fragment specifiers.

I could have made it point to the fragment specifier but the only way I could find would involve changing both the token and AST format to include the additional data: not sure it's worth that. I have it in this branch.

error: arbitrary expressions aren't allowed in patterns
  --> $DIR/issue-99380.rs:10:10
   |
LL |     foo!(Some(3));
   |          ^^^^^^^
   |
note: the expression looks like a pattern but it comes from an `:expr` meta variable
  --> $DIR/issue-99380.rs:2:7
   |
LL |     ($p:expr) => {
   |       ^

error: aborting due to 1 previous error

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Confusing "arbitrary expressions aren't allowed in patterns" error
6 participants