Skip to content
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.

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

Build failing after update to Rust 1.47.0 #77789

Closed
Tamschi opened this issue Oct 10, 2020 · 11 comments
Closed

Build failing after update to Rust 1.47.0 #77789

Tamschi opened this issue Oct 10, 2020 · 11 comments
Labels
A-proc-macros Area: Procedural macros C-bug Category: This is a bug. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@Tamschi
Copy link

Tamschi commented Oct 10, 2020

I have a build that works fine on Rust 1.46.0 but fails on 1.47.0.
(I didn't have this issue on any 1.46 version, but I only run CI for 1.46.0.)

To reproduce, you can run:

git clone https://github.com/Tamschi/taml.git -b misc/rustc-bug-report
cd taml
cargo build

(Apologies for the large sample code. I failed to narrow this down on my own.)

On 1.46.0, this and cargo test complete successfully.
On 1.47.0, I get errors of this form:

error: expected `,`
   --> src\parsing.rs:541:12
    |
541 |                       Token {
    |  ___________________________^
542 | |                         span: _1,
543 | |                         token: lexerToken::Identifier(_0),
544 | |                     } = iter.next().unwrap()
    | |_____________________^
    |
    = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
Context

	loop {
		match iter.peek().map(|t| &t.token) {
			None => break,
			Some(lexerToken::Identifier(_)) => {
				let (key, key_span) = try_match!(
					Token {
						span: _1,
						token: lexerToken::Identifier(_0),
					} = iter.next().unwrap()
				)
				.debugless_unwrap();
				base.push(BasicPathElement {

Meta

The issue also appears on beta and nightly (and also on Linux, judging by the Travis run).

rustc --version --verbose:

  • stable:
    rustc 1.47.0 (18bf6b4f0 2020-10-07)
    binary: rustc
    commit-hash: 18bf6b4f01a6feaf7259ba7cdae58031af1b7b39
    commit-date: 2020-10-07
    host: x86_64-pc-windows-msvc
    release: 1.47.0
    LLVM version: 11.0
    
  • beta:
    rustc 1.48.0-beta.2 (212e76c31 2020-10-08)
    binary: rustc
    commit-hash: 212e76c31d461462544b5d516d3bdc99fc8f016e
    commit-date: 2020-10-08
    host: x86_64-pc-windows-msvc
    release: 1.48.0-beta.2
    LLVM version: 11.0
    
  • nightly:
    rustc 1.49.0-nightly (38d911dfc 2020-10-09)
    binary: rustc
    commit-hash: 38d911dfc55a7a1eea1c80139113ed2ff0151087
    commit-date: 2020-10-09
    host: x86_64-pc-windows-msvc
    release: 1.49.0-nightly
    LLVM version: 11.0
    
Macro Backtrace

RUSTFLAGS="-Z macro-backtrace" cargo +nightly build (excerpt):

   Compiling taml v0.0.3 (C:\Users\Tamme\Documents\Clones\taml)
error: expected `,`
   --> src\parsing.rs:541:12
    |
540 |                   let (key, key_span) = try_match!(
    |  _______________________________________-
541 | |                     Token {
    | |                           ^
542 | |                         span: _1,
543 | |                         token: lexerToken::Identifier(_0),
544 | |                     } = iter.next().unwrap()
545 | |                 )
    | |_________________- in this macro invocation (#1)
    |
   ::: C:\Users\Tamme\.cargo\registry\src\git.luolix.top-1ecc6299db9ec823\try_match-0.2.2\src\lib.rs:106:1
    |
106 | / macro_rules! try_match {
107 | |     ($p:pat = $in:expr => $out:expr) => {
108 | |         match $in {
109 | |             $p => ::core::result::Result::Ok($out),
...   |
120 | |         $crate::implicit_try_match!(($p) = $($in)*)
    | |         ------------------------------------------- in this macro invocation (#2)
121 | |     };
122 | | }
    | |_- in this expansion of `try_match!` (#1)
...
127 | / macro_rules! implicit_try_match {
128 | |     ($p:pat = $($in:tt)*) => {
129 | |         // `$p` needs to be parenthesized for it to work on nightly-2020-05-30
130 | |         // and syn 1.0.29
131 | |         $crate::implicit_try_match_inner!(std($p) = $($in)*)
    | |         ---------------------------------------------------- in this macro invocation (#3)
132 | |     };
133 | | }
    | |_- in this expansion of `$crate::implicit_try_match!` (#2)
...
170 |   #[proc_macro_hack::proc_macro_hack]
    |   -----------------------------------
    |   |
    |   in this expansion of `$crate::implicit_try_match_inner!` (#3)
    |  _in this macro invocation (#4)
    | |
171 | | #[doc(hidden)]
172 | | pub use try_match_inner::implicit_try_match_inner;
    | |_________________________________________________- in this expansion of `proc_macro_call!` (#4)
error: expected `,`
   --> src\parsing.rs:589:14
[...]

@Tamschi Tamschi added the C-bug Category: This is a bug. label Oct 10, 2020
@dodomorandi
Copy link

dodomorandi commented Oct 10, 2020

I tried to dig into the issue as well. As a first thing, I modified try_match in order to don't use proc_macro_hack. Then I added the extra-traits feature to the syn crate inside try_match_inner.

Take the following code extracted from the tests of try_match:

#[cfg(feature = "implicit_map")]
#[test]
fn unwrap_option() {
    assert_eq!(try_match!(Some(a) = Some(42)), Ok(42));
}

The implicit_try_match_inner is called with std(Some(a)) = Some(42) in this particular case. The tokenstream in 1.46 is the following:

TokenStream [
    Ident {
        ident: "std",
        span: #6 bytes(6504400..6504403),
    },
    Group {
        delimiter: Parenthesis,
        stream: TokenStream [
            Group {
                delimiter: None,
                stream: TokenStream [
                    Group {
                        delimiter: Parenthesis,
                        stream: TokenStream [
                            Ident {
                                ident: "Some",
                                span: #6 bytes(6504404..6504406),
                            },
                            Group {
                                delimiter: Parenthesis,
                                stream: TokenStream [
                                    Ident {
                                        ident: "a",
                                        span: #6 bytes(6504404..6504406),
                                    },
                                ],
                                span: #6 bytes(6504404..6504406),
                            },
                        ],
                        span: #6 bytes(6504404..6504406),
                    },
                ],
                span: #6 bytes(6504404..6504406),
            },
        ],
        span: #6 bytes(6504403..6504407),
    },
    Punct {
        ch: '=',
        spacing: Alone,
        span: #6 bytes(6504408..6504409),
    },
    Ident {
        ident: "Some",
        span: #0 bytes(124..128),
    },
    Group {
        delimiter: Parenthesis,
        stream: TokenStream [
            Literal {
                kind: Integer,
                symbol: "42",
                suffix: None,
                span: #0 bytes(129..131),
            },
        ],
        span: #0 bytes(128..132),
    },
]

The tokenstream in 1.47.0:

TokenStream [
    Ident {
        ident: "std",
        span: #9 bytes(6811810..6811813),
    },
    Group {
        delimiter: Parenthesis,
        stream: TokenStream [
            Group {
                delimiter: None,
                stream: TokenStream [
                    Group {
                        delimiter: Parenthesis,
                        stream: TokenStream [
                            Group {
                                delimiter: None,
                                stream: TokenStream [
                                    Ident {
                                        ident: "Some",
                                        span: #0 bytes(114..118),
                                    },
                                    Group {
                                        delimiter: Parenthesis,
                                        stream: TokenStream [
                                            Ident {
                                                ident: "a",
                                                span: #0 bytes(119..120),
                                            },
                                        ],
                                        span: #0 bytes(118..121),
                                    },
                                ],
                                span: #8 bytes(6811487..6811489),
                            },
                        ],
                        span: #8 bytes(6811486..6811490),
                    },
                ],
                span: #9 bytes(6811814..6811816),
            },
        ],
        span: #9 bytes(6811813..6811817),
    },
    Punct {
        ch: '=',
        spacing: Alone,
        span: #9 bytes(6811818..6811819),
    },
    Ident {
        ident: "Some",
        span: #0 bytes(124..128),
    },
    Group {
        delimiter: Parenthesis,
        stream: TokenStream [
            Literal {
                kind: Integer,
                symbol: "42",
                suffix: None,
                span: #0 bytes(129..131),
            },
        ],
        span: #0 bytes(128..132),
    },
]

If you diff the two tokenstreams, you will discover that in 1.46.0 the identifier Some is inside the leaf Group contained inside a TokenStream, in 1.47.0 it is inside the TokenStream, just before the Group. What happens is that syn is not able to convert the token stream to a syn::Pat, breaking the compilation.

Unfortunately, I think that the issue could involve many other projects that use proc macros.

@cuviper
Copy link
Member

cuviper commented Oct 10, 2020

This is probably due to #73084, which AIUI is considered a necessary bug fix even though some code is no longer accepted.

@jyn514 jyn514 added A-proc-macros Area: Procedural macros T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Oct 10, 2020
@jyn514
Copy link
Member

jyn514 commented Oct 10, 2020

Possible duplicate of #76480

@Tamschi
Copy link
Author

Tamschi commented Oct 10, 2020

Interesting, that means I likely ran into this before myself on another project.
I have a fairly complicated proc macro crate I'm building, so I'll need to see if I have to add support for these Groups in a few places.

@Aaron1011
Copy link
Member

@Tamschi: As @cuviper mentioned, this is an unfortunate consequence of #73084, which is needed to make progress on #43081. Previously, these None-delimited groups were getting removed from the TokenStream - now that they're preserved, proc macros may need additional code to handle them properly.

@Aaron1011
Copy link
Member

We did a Crater run in #73084 to try to detect this kind of breakage ahead of time. Unfortunately, it looks like taml didn't get tested due to rust-lang/crater#548

@Tamschi
Copy link
Author

Tamschi commented Oct 10, 2020

@Aaron1011 Yeah, I can definitely see why they need to be preserved.
I only published this crate and the repository in late August, in any case, so the crater run wouldn't have picked it up either way.

I'm pretty sure that the only (edit: potential, it turns out) user of taml right now is a friend building a cursed payphone, so I have some time to see how I'm going to repair my crate.

@dodomorandi
Copy link

@Aaron1011 there is something I don't get however: try_matches v0.2.2 fails to compile tests (which is the real reason behind the issue with taml), but crater did not discover any issue because it did not run any test. Is this behavior expected? I thought that crater could run tests in order to discover regressions, am I wrong?

Surely running all the tests for all crates.io is takes a tremendous amount of resources, maybe compiling the tests without running them could be a good compromise? In this case it would have helped detecting the issue.

@Aaron1011
Copy link
Member

@dodomorandi I've opened rust-lang/crater#549 to track checking tests on Crater.

@dodomorandi
Copy link

The problem has been fixed in syn (thanks @dtolnay!), both try_match and taml now compile without problems with 1.47.

Tamschi added a commit to Tamschi/taml that referenced this issue Oct 11, 2020
@Tamschi
Copy link
Author

Tamschi commented Oct 11, 2020

This (of course) works on my end too now: https://travis-ci.com/github/Tamschi/taml/builds/189299403
Thanks for taking care of this so quickly @dtolnay and thanks @dodomorandi for narrowing it down and filing the issue!

I hope it's alright if I close this issue here, since the build in question would now complete if I restarted it.

@Tamschi Tamschi closed this as completed Oct 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-proc-macros Area: Procedural macros C-bug Category: This is a bug. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants