Stop ignoring trailing semicolons in a macro body when a macro is invoked in expression position

[Aaron1011]

Proposal

Summary and problem statement

When a macro_rules! macro is invoked in expression position, a trailing semicolon in the macro body is silently ignored (see issue rust-lang/rust#33953). For example, the following code compiles:

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

fn main() {
    let val = match true {
        true => false,
        _ => foo!()
    };
}

This behavior is inconsistent with how semicolons normally work. In any other context, <expr>; produces a value of (), regardless of the type of <expr>. If the type of <expr> has drop glue, then this could lead to unexpected runtime behavior.

Motivation, use-cases, and solution sketches

I propose to remove this special handling of trailing semicolons. As a result, the following code will stop compiling:

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

fn main() {
    let val = match true {
        true => false,
        _ => foo!() //~ ERROR: unexpected semicolon
    };
	let _ = foo!(); //~ ERROR: unexpected semicolon
	let _ = false || foo!(); //~ ERROR: unexpected semicolon
}

The match arm case is straightforward: _ => true; is a syntax error, since a match arm cannot end with a semicolon.

The two let statements require some explanation. Under the macro expansion proposal described by @petrochenkov, macro expansion works by only reparsing certain tokens after macro expansion. In both let _ = foo!(); and let _ = false || foo!();, the invocation foo!() is used in expression position. As a result, macro expansion will cause us to attempt to parse true; as an expression, which fails.

The alternative would be to reparse the entire let expression - that it, we would try to parse let _ = true;;, resulting in a statement let _ = true; followed by an empty statement ;. In addition to complicating parsing, this would make understanding a macro definition more difficult. After seeing <expr>; as the trailing statement in a macro body, the user now needs to examine the call sites of the macro to determine if the result of <expr> is actually used.

Rolling out this change would take a significant amount of time. As demonstrated in rust-lang/rust#78685, many crates in the ecosystem rely on this behavior, to the point where several upstream fixes are needed for the compiler to even be able to bootstrap. To make matters worse, rustfmt was inserting semicolons into macro arms up until a very recent version (it was fixed by rust-lang/rustfmt#4507). This means that any crates gating CI on stable rustfmt may find it impossible to make the necessary changes until the latest rustfmt rides the release train to stable.

I propose the following strategy for rolling out this change:

1. Add an allow-by-default future-compatibility lint, and deny it for internal rustc crates
2. Do a Crater run to determine the extent of impact
3. When the necessary rustfmt fix makes it way into stable (or earlier, if we determine the impact to be small enough), switch the lint to warn-by-default
4. Mark the lint for inclusion in the cargo future-incompat-report (see Tracking Issue for cargo report future-incompat rust#71249)
5. After some time passes, switch the lint to deny-by-default
6. Make the lint into a hard error (possibly only for macros defined in a crate in a new Rust edition).

Fortunately, this change is very easy on a technical level: we simply need to emit a warning in this code

Prioritization

This doesn't appear to fit into any particular lang-team priority. However, it's part of a larger effort to fix bugs and inconsistencies in Rust's macro system.

Links and related work

• The original issue: Trailing semicolon silently ignored in expr macro rust#33953
• An attempt to do a Crater run (along with other changes): [DO NOT MERGE] Consistent handling of semicolons in macro expansions rust#78685
• @petrochenkov's macro expansion proposal: What is a trailing expression in a block exactly? rust#61733 (comment)
• The macro parsing code that currently accepts trailing semicolons: https://github.com/rust-lang/rust/blob/3d3c8c5e0d534cdd794f1b3359089eba031d492c/compiler/rustc_expand/src/mbe/macro_rules.rs#L151-L156
• The rustfmt PR that prevents automatic insertion of trailing semicolons in macro bodies: Don't insert semicolons inside of a macro_rules! arm body rustfmt#4507

Some PRs to crates removing trailing semicolons:

• Remove trailing semicolons from several macro definitions stdarch#938
• Remove semicolon from internal err macro rust#78449
• Don't emit trailing semicolon from bail! dtolnay/anyhow#120
• Remove trailing semicolon from internal err! macro git2-rs#631
• Remove some trailing semicolons from macro definitions log#422

Initial people involved

I plan to implement this if accepted, with @petrochenkov reviewing the implementation.

What happens now?

This issue is part of the experimental MCP process described in RFC 2936. Once this issue is filed, a Zulip topic will be opened for discussion, and the lang-team will review open MCPs in its weekly triage meetings. You should receive feedback within a week or two.

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

[rustbot]

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

[pnkfelix]

I second this proposal. I'm willing to serve as liason with the lang-team. The work here sounds isolated enough that someone can start directly on a pull request.

[pnkfelix]

(to be clear, the lang team was also generally in support of the change described here. I think we are generally inclined to tie any hard error here to an edition boundary, as suggested above.)

[Aaron1011]

@pnkfelix: Did you mean to second this with rustbot as well?

[nikomatsakis]

@rustbot second

[nikomatsakis]

Really this is @pnkfelix's second here =)

[nikomatsakis]

This was approved in concept. @Aaron1011 (or whomever authors the PR) should I-nominate the actual implementation PR once it's ready for final approval by lang team.

[Aaron1011]

Oh...the lint was already implemented in rust-lang/rust#79819. However, it's currently allow-by-default.