Skip to content

fix: Add semicolon for toggle_macro_delimiter #21522

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.

Sign up for GitHub

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

Jump to bottom
Merged
ChayimFriedman2 merged 1 commit into from
Jan 26, 2026
Merged

fix: Add semicolon for toggle_macro_delimiter #21522

ChayimFriedman2 merged 1 commit into from
Jan 26, 2026

Conversation

@A4-Tacks
Copy link
Member

@A4-Tacks A4-Tacks commented Jan 25, 2026

Example

macro_rules! sth {
    () => {};
}

sth!$0{ }

(old test sth!{}; is a syntax error in item place)

Before this PR

macro_rules! sth {
    () => {};
}

sth![ ]

After this PR

macro_rules! sth {
    () => {};
}

sth![ ];

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 25, 2026
@A4-Tacks A4-Tacks changed the title Fix semicolon for toggle_macro_delimiter fix: Add semicolon for toggle_macro_delimiter Jan 25, 2026
ChayimFriedman2
ChayimFriedman2 reviewed Jan 26, 2026
View reviewed changes
crates/ide-assists/src/handlers/toggle_macro_delimiter.rs Outdated
.parent()
.and_then(ast::MacroExpr::cast)
.and_then(|it| ast::StmtList::cast(it.syntax().parent()?))
.is_some()
Copy link
Contributor

@ChayimFriedman2 ChayimFriedman2 Jan 26, 2026

Choose a reason for hiding this comment

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

I do not understand the logic here. If this is a MacroExpr and the child of a StmtList, it does need semicolon, no?

Copy link
Member Author

@A4-Tacks A4-Tacks Jan 26, 2026

Choose a reason for hiding this comment

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

For {m!{} n!{}}, it ast is StmtList { ExprStmt { MacroExpr }, MacroExpr }

  • m! ExprStmt does not require a value, it requires a semicolon
  • n! MacroExpr requires a value, meaning no semicolons are needed

Copy link
Contributor

@ChayimFriedman2 ChayimFriedman2 Jan 26, 2026

Choose a reason for hiding this comment

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

But what about things like foo!(m! {})? Even if this works, please add a test for this.

Copy link
Member Author

@A4-Tacks A4-Tacks Jan 26, 2026

Choose a reason for hiding this comment

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

It has been tested in test_nested_macros that currently does not support nested toggle delimiters

Copy link
Contributor

@ChayimFriedman2 ChayimFriedman2 Jan 26, 2026

Choose a reason for hiding this comment

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

These are not nested macro calls?

Copy link
Contributor

@ChayimFriedman2 ChayimFriedman2 Jan 26, 2026

Choose a reason for hiding this comment

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

Sorry, I meant foo(m! {}) (foo is not a macro).

@A4-Tacks A4-Tacks force-pushed the tog-macro-delim-semicolon branch from f0f0c2e to 9fbcc94 Compare January 26, 2026 14:31
@A4-Tacks
Fix semicolon for toggle_macro_delimiter
88595fe 
Example
---
```rust
macro_rules! sth {
    () => {};
}

sth!$0{ }
```

(old test `sth!{};` is a syntax error in item place)

**Before this PR**

```rust
macro_rules! sth {
    () => {};
}

sth![ ]
```

**After this PR**

```rust
macro_rules! sth {
    () => {};
}

sth![ ];
```
@A4-Tacks A4-Tacks force-pushed the tog-macro-delim-semicolon branch from 9fbcc94 to 88595fe Compare January 26, 2026 15:10
ChayimFriedman2
ChayimFriedman2 approved these changes Jan 26, 2026
View reviewed changes
Copy link
Contributor

@ChayimFriedman2 ChayimFriedman2 left a comment

Choose a reason for hiding this comment

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

There is also the problem of removing the semicolon in item lists when changing other brackets to {}, but that can be a different PR (not necessarily yours).

Thanks!

@ChayimFriedman2 ChayimFriedman2 added this pull request to the merge queue Jan 26, 2026
Merged via the queue into rust-lang:master with commit e2ab488 Jan 26, 2026
15 checks passed
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 26, 2026
@A4-Tacks A4-Tacks deleted the tog-macro-delim-semicolon branch January 26, 2026 15:39
@A4-Tacks
Copy link
Member Author

A4-Tacks commented Jan 26, 2026

There is also the problem of removing the semicolon in item lists when changing other brackets to {}

Is there a problem with this? If the delimiter is {}, there is no need for a semicolon, and adding a semicolon in item list is a syntax error

@ChayimFriedman2
Copy link
Contributor

ChayimFriedman2 commented Jan 26, 2026

If the delimiter was () or [] it had a semicolon, then you changed it to {} and suddenly the semicolon is a syntax error.

@A4-Tacks
Copy link
Member Author

A4-Tacks commented Jan 26, 2026 •
edited
Loading

But this has already been fixed by #20534

But hasn't this already been implemented? m!(); -> m!{}

@ChayimFriedman2
Copy link
Contributor

ChayimFriedman2 commented Jan 26, 2026

Oops I thought it wasn't 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ChayimFriedman2 ChayimFriedman2 ChayimFriedman2 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@A4-Tacks @ChayimFriedman2 @rustbot