Fix bugs in macro-expanded statement parsing

[jseyfried]

Fixes #34543.

This is a [breaking-change].
The following would break be a warning (c.f. #34660 (comment)):

macro_rules! m { () => {
    println!("") println!("")
    //^ Semicolons are now required on macro-expanded non-braced macro invocations
    //| in statement positions.
    let x = 0
    //^ Semicolons are now required on macro-expanded `let` statements
    //| that are followed by more statements, so this would break.
    let y = 0 //< (this would still be allowed to reduce breakage in the wild)
}
fn main() { m!() }

r? @eddyb

[eddyb]

Starting a crater run.

[jseyfried]

@eddyb Thanks!

[jseyfried]

cc @nrc

[eddyb]

Crater report claims 42 root regressions (87 total).
However, most of those seem to be caused by a single nom macro.

@jseyfried Apparently this change causes type-checking to find a mismatch between some type and ().
I wonder if it accidentally produces StmtSemi where it shouldn't.

[jseyfried]

@eddyb
That regression was caused by f639fc3.
The underlying issue is that trailing semicolons are ignored after expansions in expression positions:

macro_rules! m { () => { 2; } }
fn f() -> i32 {
    let _: i32 = m!(); // The semicolon after `2` is ignored, so this is typechecks.
    m!() // Before f639fc3, this invocation is considered to be in an expression position,
         // so the semicolon after the `2` is ignored and this typechecks.
         //
         // After f639fc3, the invocation is considered to be in a statement position, so
         // the semicolon is not ignored and it expands into a `StmtKind::Semi` statement,
         // which doesn't typecheck.
}

I removed f639fc3, which fixed the breakage. However, removing f639fc3 also means that

macro_rules! n { () => {
    let _ = ();
    m!() // this is no longer considered to be in a statement position like it was
         // before this PR, which could potentially lead to more breakage.
}
fn main() { n!(); }

If this causes breakage in practice, I could treat only macro-expanded macros in trailing expression positions as statements.

[eddyb]

@jseyfried Sheesh, that's awful. I'll start a second crater run.

[nrc]

Hmm, this seems like a bug fix and therefore something we should land even if it does break code. Would it be possible to turn this into a warning for a cycle? (Doesn't seem easy to me). Otherwise we should submit a PR to nom and fix that bustage (if possible) before landing this.

cc @rust-lang/compiler

[jseyfried]

@nrc

Would it be possible to turn this into a warning for a cycle?

I think the only change we can't phase in with a warning cycle is making macros in trailing expression statement positions expand into statements, not expressions (i.e. f639fc3). While I prefer this change, it's not super important and I had only included it here to reduce breakage (which backfired).

I'd like to land as much as we can in this PR without causing breakage in practice. After that, we can do a PR that adds a warning cycle for the rest. If @eddyb's crater run comes back clean, all that will remain is to forbid trailing macro-expanded let statements from lacking semicolons:

macro_rules m! { () => {
    let x = 0 //< This PR will not affect trailing let statements, i.e. this will still be allowed.
}}
fn main() { m!(); }

If we want to, it would be easy to start a warning cycle for this (we haven't cratered it yet, but based on the fallout in rustc it seems likely to cause significant breakage in practice).

[eddyb]

Crater report has 7 confirmed root regressions (13 total), most of which are:

expected expression, found ``

[jseyfried]

The above commit should fix the remaining breakage.

[nrc]

even if crater comes back clean, there might still be breakage out there in projects or platforms not covered by crater. It might be worth having a warning cycle for any errors one might reasonably expect to occur.

[jseyfried]

@nrc
Ok, I can do a warning cycle for macro-expanded let statements that lack semicolons.
Should this warning apply to final let statements? E.x.

macro_rules! m { () => { let x = 0 } }
fn main() {
    m!(); // Is this OK? I think not, but it might be too common in the wild
          // to include in the warning cycle -- we haven't cratered it yet.
}

The rest of the potential breakage comes from non-braced macro invocation expression statements requiring trailing semicolons (except at the end of a block). Warning for this would be significantly more involved, but is doable.

[jseyfried]

@nrc Actually, not all remaining potential breakage is warnable.
For example, this compiles today but doesn't compile after this PR:

macro_rules! m { () => { () } }
macro_rules! n {
    () => { m!() - 1 } // This is treated like "m!(); -1" today
}
fn main() { n!(); }

and this doesn't compile today but does compile after this PR:

macro_rules! m { () => { 2 } } //< The only difference is that the `()` is changed to a `2`
macro_rules! n {
    () => { m!() - 1 }
}
fn main() { n!(); }

Since we can't distinguish these cases until type-checking, a complete warning cycle is infeasible.

[jseyfried]

@nrc @eddyb
After my most recent commit, this PR can only break macros that expand into multiple statements.

All the breakage so far has been due to macros that expand into one statement / expression.
Macros that expand into multiple statements appear to be far less common in practice, so I believe this PR is highly unlikely to cause breakage in the wild (consistent with the lack crater-detected breakage).

[nrc]

So, regarding what to warn on, I think we should do the right thing and warn wherever possible. We shouldn't preserve incorrect behaviour for the sake of preventing breakage, unless there is an overwhelming number of uses. So, for example, the let without a semi-colon example should be an error.

@jseyfried I'm not sure I understand the second more complex examples - could you explain the rules that the macro system will be using with this PR?

cc @rust-lang/compiler @rust-lang/lang

[nrc]

Nominating for discussion due to potential breakage.

[nrc]

un-nominating since this PR is turning into more of a bug fix and will avoid breakage after all.

[nrc]

@eddyb: could you do a Crater run with this version of the PR please?

[nrc]

Could you comment on what this closure does (i.e., why you don't just use parser.parse_stmt)?

Could it be a function rather than a closure?

[jseyfried]

Good point, I'll replace the method finish_parsing_statement with something like parse_full_stmt and use that here instead of defining a closure.

[jseyfried]

Done.

[nrc]

Should "expressions" be "statements" here?

[jseyfried]

Could be either, I'll change it to "expression statements" to be most precise.

[nrc]

Discussed at the compiler meeting, if it is possible, we would like to do a warning cycle here. During which time, we could also submit PRs to the broken crates.

@jseyfried how hard would it be to do a warning cycle for the current PR?

cc @Manishearth - Clippy is one of the crates with breakage here, I expect it is an easy fix, it should just need adding a semicolon or two to a macro somewhere.

[jseyfried]

@nrc
The cargo-clippy breakage appears to be spurious. The breakage has nothing to do with semicolons or macros, I couldn't reproduce it locally, and it doesn't break on the second crater run, which should have caused strictly more breakage.

The clocked-dispatch breakage is caused by this (let statements lacking semicolons). This is easy to warn.

The Inflector breakage is caused by this (non-braced macro invocations lacking semicolons). With the exception of the corner case I described here, this is also feasible to warn.
We would warn when a non-braced macro invocation is followed by something that can start an expression but can't continue an expression. The corner case is a problem because - can start an expression or continue an expression.

[bors]

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

[jseyfried]

@nrc I rebased and started a best-effort warning cycle.

[nrc]

@bors: r+

[bors]

📌 Commit 57fac56 has been approved by nrc

[bors]

⌛️ Testing commit 57fac56 with merge 2ab18ce...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-cargotest, auto-linux-64-cross-freebsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-mir, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-mir, auto-win-msvc-64-opt-rustbuild
Approved by: nrc
Pushing 2ab18ce to master...