Skip to content

Fix duplicate semicolon suggestion for macros in expression position - #164286

Open
tehforsch wants to merge 5 commits into
rust-lang:mainfrom
tehforsch:duplicate-semi
Open

tehforsch wants to merge 5 commits into
rust-lang:mainfrom
tehforsch:duplicate-semi

Conversation

@tehforsch

@tehforsch tehforsch commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Closes #164087

@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 Oct 13, 2026
@rustbot

rustbot commented Oct 13, 2026

Copy link
Copy Markdown
Collaborator

r? @davidtwco

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

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: compiler
  • compiler expanded to 77 candidates
  • Random selection from 20 candidates


fn main() {
let a = a!();
a!()

@tehforsch tehforsch Oct 13, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The trailing a!() is sort of unnecessary but my first fix didn't handle this case properly, so I added it to the same test. I can remove this or move it to a separate test if necessary

View changes since the review

);

if parser.token == token::Semi {
if !is_trailing_mac {

@xizheyin xizheyin Oct 16, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

expansion_data: ExpansionData {
id: expn_id,
depth: self.cx.current_expansion.depth + 1,
..self.cx.current_expansion.clone()
},

if there is nest macro, the trailing_mac may be wrong. for example, inner will inherit the outer's is_trailing_mac(true), and make it add ;, it may be a regression. We should make is_trailing_mac correct for nested macro when constructing expansion data.🤔

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

macro_rules! outer {
    () => {{ let _ = inner!(); }};
}

fn main() {
    outer!()
}

View changes since the review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, this also fails for the other diagnostic that is_trailing_mac is responsible for, like in

macro_rules! inner {
    () => { true; };
}

macro_rules! outer {
    () => {{ let _ = inner!(); }};
}

fn main() {
    outer!()
}

which will result in the wrong note

note: macro invocations at the end of a block are treated as expressions
note: to ignore the value produced by the macro, add a semicolon after the invocation of `inner`

@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 Oct 16, 2026
@rustbot

rustbot commented Oct 16, 2026

Copy link
Copy Markdown
Collaborator

Reminder, once the PR becomes ready for a review, use @rustbot ready.

Comment on lines +572 to +577
self.cx.current_expansion.is_trailing_mac &= match &fragment {
AstFragment::Expr(expr) | AstFragment::OptExpr(Some(expr)) => {
matches!(expr.kind, ExprKind::MacCall(..))
}
_ => false,
};

@tehforsch tehforsch Oct 19, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't know if this is the right way to fix this. The reason we can't just set is_trailing_mac to false is that in some cases, we have to inherit the is_trailing_mac from the parent so that a directly nested macro like

macro_rules! inner {
    () => { true; };
}

macro_rules! outer {
    () => { inner!() };
}

fn foo() -> bool {
    outer!()
}

is still treated as a trailing macro.
(I added a test for this, although its results are the same on main and on this branch, see tests/ui/lint/semicolon-in-expressions-from-macros/semicolon-in-expression-from-macros-nested-2.rs)

However, in the current logic, is_trailing_mac was basically always true for nested macros where the outer one is trailing, so that wasn't right either.

This fix is sort of the dumbest logic that solves the problem ...

View changes since the review

@tehforsch

tehforsch commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor Author

r? @xizheyin
@rustbot ready

@rustbot rustbot assigned xizheyin and unassigned davidtwco Oct 19, 2026
@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 Oct 19, 2026
@rustbot

This comment has been minimized.

@rust-bors

This comment has been minimized.

@xizheyin

xizheyin commented Nov 4, 2026

Copy link
Copy Markdown
Member

I dont have the right to review. r? compiler

@rustbot rustbot assigned jieyouxu and unassigned xizheyin Nov 4, 2026
@rustbot

rustbot commented Nov 4, 2026

Copy link
Copy Markdown
Collaborator

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@tehforsch

Copy link
Copy Markdown
Contributor Author

I dont have the right to review. r? compiler

Oh sorry, didn't realize. I rebased this after the changes in #161035

@rust-log-analyzer

This comment has been minimized.

This was previously `true` for any macro called from within a trailing
outer macro. This fix checks explicitly if the outer macro expands
directly to the inner one, i.e. `macro_rules! outer => { { inner!() } }`
in which case the `is_trailing_mac` state is preserved, otherwise it is
reset.

This branch has not been deployed

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

Labels

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

semicolon suggested after a macro invocation even if there already is one

6 participants