Fix bugs in macro-expanded statement parsing #34660

jseyfried · 2016-07-05T09:45:11Z

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 · 2016-07-05T09:46:57Z

Starting a crater run.

jseyfried · 2016-07-05T09:48:47Z

@eddyb Thanks!

jseyfried · 2016-07-05T09:50:49Z

cc @nrc

eddyb · 2016-07-05T17:50:14Z

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 · 2016-07-05T20:13:51Z

@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 · 2016-07-05T20:15:35Z

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

nrc · 2016-07-05T22:40:05Z

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 · 2016-07-05T23:13:58Z

@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 · 2016-07-05T23:28:27Z

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

expected expression, found ``

jseyfried · 2016-07-05T23:52:55Z

The above commit should fix the remaining breakage.

nrc · 2016-07-06T03:46:00Z

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 · 2016-07-06T04:05:13Z

@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 · 2016-07-06T07:37:22Z

@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 · 2016-07-06T20:28:27Z

@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 · 2016-07-07T02:54:07Z

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 · 2016-07-07T02:54:31Z

Nominating for discussion due to potential breakage.

nrc · 2016-07-07T03:23:56Z

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

nrc · 2016-07-07T03:25:06Z

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

nrc · 2016-07-07T03:30:34Z

src/libsyntax/ext/tt/macro_rules.rs

@@ -117,12 +117,18 @@ impl<'a> MacResult for ParserAnyMacro<'a> {

    fn make_stmts(self: Box<ParserAnyMacro<'a>>)
                 -> Option<SmallVector<ast::Stmt>> {
+        let parse_stmt = |parser: &mut Parser<'a>| -> ::parse::PResult<'a, _> {


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?

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.

nrc · 2016-07-07T20:52:18Z

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 · 2016-07-07T21:24:08Z

@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 · 2016-07-08T15:38:05Z

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

…tmt`.

jseyfried · 2016-07-12T04:53:41Z

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

…d into statements: ```rust macro_rules! m { () => { let x = 1; x } } macro_rules! n { () => { m!() //< This can now expand into statements }} fn main() { n!(); } ``` and revert needless fallout fixes.

…full_stmt`.

nrc · 2016-07-13T06:09:07Z

@bors: r+

bors · 2016-07-13T06:09:08Z

📌 Commit 57fac56 has been approved by nrc

bors · 2016-07-13T08:37:07Z

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

@eddyb

Fix bugs in macro-expanded statement parsing Fixes #34543. This is a [breaking-change]. For example, the following would break: ```rust 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

bors · 2016-07-13T11:31:29Z

This is only a point release because of the minor change. Also updated regex dependency which is also only a point change.

rust-highfive assigned eddyb Jul 5, 2016

jseyfried mentioned this pull request Jul 5, 2016

Semicolons are not required on some statements in macros #34543

Closed

jseyfried force-pushed the fix_parse_stmt branch from 9dbe21b to e94e014 Compare July 5, 2016 20:12

jseyfried force-pushed the fix_parse_stmt branch from 30ea84e to b2b4dac Compare July 5, 2016 23:52

nrc added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 7, 2016

nrc removed I-nominated 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. labels Jul 7, 2016

nrc reviewed Jul 7, 2016
View reviewed changes

jseyfried added 3 commits July 12, 2016 04:31

Clean up statement parsing without changing the semantics of `parse_s…

6189e6c

…tmt`.

Parse macro-expanded statements like ordinary statements.

57c56dd

Fix fallout.

3372368

jseyfried force-pushed the fix_parse_stmt branch from 1ed1085 to bf3cb4b Compare July 12, 2016 04:53

jseyfried force-pushed the fix_parse_stmt branch from bf3cb4b to f789bc0 Compare July 12, 2016 20:20

jseyfried added 4 commits July 13, 2016 04:49

Fix bug in the pretty printer.

52d485f

Allow macro-expanded macros in trailing expression positions to expan…

759b8a8

…d into statements: ```rust macro_rules! m { () => { let x = 1; x } } macro_rules! n { () => { m!() //< This can now expand into statements }} fn main() { n!(); } ``` and revert needless fallout fixes.

cleanup: Refactor parser method finish_parsing_statement -> `parse_…

c1b850d

…full_stmt`.

Start a best-effort warning cycle.

57fac56

jseyfried force-pushed the fix_parse_stmt branch from f789bc0 to 57fac56 Compare July 13, 2016 04:51

bors merged commit 57fac56 into rust-lang:master Jul 13, 2016

jseyfried mentioned this pull request Jul 17, 2016

macros: fix bug in stmt matchers #34886

Merged

whatisinternet mentioned this pull request Jul 27, 2016

Chore/stable fixes whatisinternet/Inflector#24

Merged

jseyfried mentioned this pull request Aug 31, 2016

Adds a ProcMacro form of syntax extension #36154

Merged

jseyfried deleted the fix_parse_stmt branch September 12, 2016 09:04

jseyfried mentioned this pull request Sep 13, 2016

Add a trailing semicolon to avoid breakage. durch/rust-s3#1

Merged

TimNN mentioned this pull request Oct 16, 2016

semicolon check in macro needs improving #37217

Closed

jseyfried mentioned this pull request Apr 13, 2017

Macro with empty expansion in expression position causes an "unexpected <eof>" error #10951

Closed

petrochenkov mentioned this pull request Jun 18, 2018

RFC: Accept semicolons as item-like rust-lang/rfcs#2479

Closed

ehuss mentioned this pull request Jul 7, 2020

Hygiene Example Typo #74118

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bugs in macro-expanded statement parsing #34660

Fix bugs in macro-expanded statement parsing #34660

jseyfried commented Jul 5, 2016 •

edited

Loading

eddyb commented Jul 5, 2016

jseyfried commented Jul 5, 2016

jseyfried commented Jul 5, 2016

eddyb commented Jul 5, 2016 •

edited

Loading

jseyfried commented Jul 5, 2016 •

edited

Loading

eddyb commented Jul 5, 2016

nrc commented Jul 5, 2016

jseyfried commented Jul 5, 2016 •

edited

Loading

eddyb commented Jul 5, 2016

jseyfried commented Jul 5, 2016

nrc commented Jul 6, 2016

jseyfried commented Jul 6, 2016

jseyfried commented Jul 6, 2016

jseyfried commented Jul 6, 2016 •

edited

Loading

nrc commented Jul 7, 2016

nrc commented Jul 7, 2016

nrc commented Jul 7, 2016

nrc commented Jul 7, 2016

nrc Jul 7, 2016

jseyfried Jul 7, 2016 •

edited

Loading

jseyfried Jul 7, 2016

nrc commented Jul 7, 2016

jseyfried commented Jul 7, 2016

bors commented Jul 8, 2016

jseyfried commented Jul 12, 2016

nrc commented Jul 13, 2016

bors commented Jul 13, 2016

bors commented Jul 13, 2016

bors commented Jul 13, 2016

Fix bugs in macro-expanded statement parsing #34660

Fix bugs in macro-expanded statement parsing #34660

Conversation

jseyfried commented Jul 5, 2016 • edited Loading

eddyb commented Jul 5, 2016

jseyfried commented Jul 5, 2016

jseyfried commented Jul 5, 2016

eddyb commented Jul 5, 2016 • edited Loading

jseyfried commented Jul 5, 2016 • edited Loading

eddyb commented Jul 5, 2016

nrc commented Jul 5, 2016

jseyfried commented Jul 5, 2016 • edited Loading

eddyb commented Jul 5, 2016

jseyfried commented Jul 5, 2016

nrc commented Jul 6, 2016

jseyfried commented Jul 6, 2016

jseyfried commented Jul 6, 2016

jseyfried commented Jul 6, 2016 • edited Loading

nrc commented Jul 7, 2016

nrc commented Jul 7, 2016

nrc commented Jul 7, 2016

nrc commented Jul 7, 2016

nrc Jul 7, 2016

Choose a reason for hiding this comment

jseyfried Jul 7, 2016 • edited Loading

Choose a reason for hiding this comment

jseyfried Jul 7, 2016

Choose a reason for hiding this comment

nrc commented Jul 7, 2016

jseyfried commented Jul 7, 2016

bors commented Jul 8, 2016

jseyfried commented Jul 12, 2016

nrc commented Jul 13, 2016

bors commented Jul 13, 2016

bors commented Jul 13, 2016

bors commented Jul 13, 2016

jseyfried commented Jul 5, 2016 •

edited

Loading

eddyb commented Jul 5, 2016 •

edited

Loading

jseyfried commented Jul 5, 2016 •

edited

Loading

jseyfried commented Jul 5, 2016 •

edited

Loading

jseyfried commented Jul 6, 2016 •

edited

Loading

jseyfried Jul 7, 2016 •

edited

Loading