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

Better errors with bad/missing identifiers in MBEs #118939

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

EliseZeroTwo
Copy link
Contributor

@EliseZeroTwo EliseZeroTwo commented Dec 14, 2023

This PR implements better error handling around identifiers in MBEs.

It is related to #118295 which is already closed, but it fixes up issues left present by the solution for that issue.

Improved errors:

Parenthesised Ident

Previously with the code:

macro_rules! (meow) {
    () => {}
}

The compiler would output the following, incorrect error:

error: macros that expand to items must be delimited with braces or followed by a semicolon
 --> src/lib.rs:1:14
  |
1 | macro_rules! (meow) {
  |              ^^^^^^
  |
help: change the delimiters to curly braces
  |
1 | macro_rules! {meow} {
  |              ~    ~
help: add a semicolon
  |
1 | macro_rules! (meow); {
  |                    +

error: expected item, found `{`
 --> src/lib.rs:1:21
  |
1 | macro_rules! (meow) {
  |                     ^ expected item
  |
  = note: for a full list of items that can appear in modules, see <https://doc.rust-lang.org/reference/items.html>

Now it outputs:

error: expected identifier, found `(`
 --> src/lib.rs:1:14
  |
1 | macro_rules! (meow) {
  |              ^ expected identifier
  |
note: try removing the parenthesis around the name for this `macro_rules!`
 --> src/lib.rs:1:15
  |
1 | macro_rules! (meow) {
  |               ^^^^
Missing Ident
Previously with the code:
macro_rules! {}

The compiler would output the following, incorrect error:

error: cannot find macro `macro_rules` in this scope
 --> src/lib.rs:1:1
  |
1 | macro_rules! {}
  | ^^^^^^^^^^^

Now it outputs:

error: expected identifier, found `{`
 --> src/lib.rs:1:14
  |
1 | macro_rules! {}
  |              ^ expected identifier
  |
  = note: maybe you have forgotten to define a name for this `macro_rules!`

This will interfere with the stderr of the test case I introduced in #118928 that has not yet been merged, due to that test case requiring a (templated) parenthesised ident to be present.

It includes regression tests.

🖤

@rustbot
Copy link
Collaborator

rustbot commented Dec 14, 2023

r? @TaKO8Ki

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

@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 Dec 14, 2023
@rust-log-analyzer

This comment has been minimized.

@EliseZeroTwo EliseZeroTwo force-pushed the EliseZeroTwo/fix-macrorules-ident-errors branch from aeb58d5 to a51902a Compare December 14, 2023 15:37
@rust-log-analyzer

This comment has been minimized.

@EliseZeroTwo
Copy link
Contributor Author

In #62258 macro_rules was reallowed as an identifier for MBE definitions. This removes the possibility of proper error checking of MBEs at the parser level. Given that in the merge request @eddyb stated it could be potentially reverted in the future, I was wondering what the next step with this should be. As per the specification it appears to me as if you shouldn't be allowed to define MBEs with the macro_rules identifier.

@EliseZeroTwo EliseZeroTwo force-pushed the EliseZeroTwo/fix-macrorules-ident-errors branch from a51902a to 953a1ef Compare December 14, 2023 18:37
@rust-log-analyzer

This comment has been minimized.

@TaKO8Ki
Copy link
Member

TaKO8Ki commented Dec 15, 2023

@EliseZeroTwo A test tests/ui/macros/user-defined-macro-rules.rs has failed on CI. Could you fix it?

@TaKO8Ki
Copy link
Member

TaKO8Ki commented Dec 15, 2023

@rustbot author

@rustbot rustbot 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 Dec 15, 2023
@EliseZeroTwo EliseZeroTwo force-pushed the EliseZeroTwo/fix-macrorules-ident-errors branch 2 times, most recently from ee200da to 3452e01 Compare December 15, 2023 16:14
@EliseZeroTwo
Copy link
Contributor Author

@TaKO8Ki I have re-implemented the special casing of macro_rules not being allowed as an identifier for MBE definitions, and fixed the previously failing test that needed to be adjusted as a result of it (the failing test was also added in #62258 due to the change in it). All tests pass now.

compiler/rustc_parse/src/parser/item.rs Outdated Show resolved Hide resolved
compiler/rustc_parse/src/parser/item.rs Outdated Show resolved Hide resolved
@EliseZeroTwo EliseZeroTwo force-pushed the EliseZeroTwo/fix-macrorules-ident-errors branch from 3452e01 to 661b30a Compare December 20, 2023 14:44
@EliseZeroTwo
Copy link
Contributor Author

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 20, 2023
@rust-log-analyzer

This comment has been minimized.

@EliseZeroTwo EliseZeroTwo force-pushed the EliseZeroTwo/fix-macrorules-ident-errors branch from 661b30a to db53e3f Compare December 20, 2023 16:05
@rust-log-analyzer

This comment has been minimized.

@EliseZeroTwo
Copy link
Contributor Author

@rustbot author

@rustbot rustbot 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 Dec 20, 2023
@EliseZeroTwo EliseZeroTwo force-pushed the EliseZeroTwo/fix-macrorules-ident-errors branch from db53e3f to 4c4b155 Compare December 20, 2023 19:38
@rust-log-analyzer

This comment has been minimized.

@EliseZeroTwo EliseZeroTwo force-pushed the EliseZeroTwo/fix-macrorules-ident-errors branch from 4c4b155 to 7073fb9 Compare December 20, 2023 20:48
@EliseZeroTwo
Copy link
Contributor Author

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 20, 2023
Copy link
Contributor

@cjgillot cjgillot left a comment

Choose a reason for hiding this comment

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

r=me with one nit

compiler/rustc_parse/src/parser/item.rs Outdated Show resolved Hide resolved
@EliseZeroTwo EliseZeroTwo force-pushed the EliseZeroTwo/fix-macrorules-ident-errors branch from 7073fb9 to 3714817 Compare December 24, 2023 22:05
@petrochenkov
Copy link
Contributor

This is a breaking language change, if I understand correctly.

@EliseZeroTwo
Copy link
Contributor Author

EliseZeroTwo commented Dec 24, 2023

This is a breaking language change, if I understand correctly.

As documentation seems to indicate that this is how it is meant to be, I did not think it was. I took a look before submitting this and also could not find published software using this, but maybe testing the change against crates.io would be a good idea anyways?

It does undo the change you implemented in #62258 which reallowed macro_rules as an identifier for macros.

@petrochenkov
Copy link
Contributor

this is how it is meant to be

Well, not by me, at least.
macro_rules is reserved as minimally as possible, because the eventual goal was always retiring it in favor of macro - it isn't strictly necessary to reserve macro_rules in this context, so it's not reserved (that's similar to other weak keywords like union).

In any case, such changes should go through the language team.
(And I'm personally against it going, because the only benefit here is a minor diagnostic improvement.)

@bors
Copy link
Contributor

bors commented Dec 26, 2023

☔ The latest upstream changes (presumably #119146) made this pull request unmergeable. Please resolve the merge conflicts.

@apiraino
Copy link
Contributor

apiraino commented Feb 1, 2024

I'll try to push this conversation forward by nominating for T-lang discussion, keeping in mind some raised concerns.

@rustbot label I-lang-nominated

@rustbot rustbot added the I-lang-nominated Nominated for discussion during a lang team meeting. label Feb 1, 2024
@traviscross traviscross added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Feb 7, 2024
@apiraino apiraino added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-lang-nominated Nominated for discussion during a lang team meeting. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. 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.

9 participants