Rust: Regression: spurious "error: expected `,`" in serde::Serialize on macro_rules-variable path enum discriminant

Created on 26 May 2020  路  11Comments  路  Source: rust-lang/rust

Somewhat minimized test case: (still involves serde_derive and syn)

[dependencies]
serde = {version = "1", features = ["derive"]}
syn = { version = "1", features = ["full"] }

```rust
mod gl {
pub const DEPTH_COMPONENT: isize = 0;
}

macro_rules! gl_enums {
($mod:ident) => {
#[derive(serde::Serialize)]
pub enum TexFormat {
DepthComponent = $mod::DEPTH_COMPONENT
}
}
}

gl_enums!(gl);


Compiles in `nightly-2020-05-24`, errors in `nightly-2020-05-25`:

```rust
$ cargo +nightly-2020-05-25 rustc -- -Z macro-backtrace
   Compiling proc-macro2 v1.0.17
   Compiling unicode-xid v0.2.0
   Compiling syn v1.0.25
   Compiling serde v1.0.110
   Compiling quote v1.0.6
   Compiling serde_derive v1.0.110
   Compiling aa v0.1.0 (/tmp/aa)
error: expected `,`
  --> src/lib.rs:9:34
   |
5  | / macro_rules! gl_enums {
6  | |     ($mod:ident) => {
7  | |         #[derive(serde::Serialize)]
8  | |         pub enum TexFormat {
9  | |             DepthComponent = $mod::DEPTH_COMPONENT
   | |                                  ^^
10 | |         }
11 | |     }
12 | | }
   | |_- in this expansion of `gl_enums!`
13 | 
14 |   gl_enums!(gl);
   |   -------------- in this macro invocation

error: aborting due to previous error

Any of these changes makes compilation succeed:

  • Removing use of serde::Serialize
  • Disabling the syn/full feature
  • Replacing $mod with a non-variable module name
  • Replacing the path expression with a { use $mod::DEPTH_COMPONENT; DEPTH_COMPONENT } block

Regression range: 8970e8bcf...46e85b432. Merges:

Excluding those that seem unrelated to me leaves:

A-macros C-bug E-needs-mcve P-critical T-compiler regression-from-stable-to-nightly
Source
picture SimonSapin

Most helpful comment

I released a fix to parse $mod::DEPTH_COMPONENT in syn 1.0.26.

picture dtolnay on 26 May 2020
❤3 👍1

All 11 comments

Let鈥檚 see if we can minimize the example and bisect further to find the culprit PR?

LeSeulArtichaut picture LeSeulArtichaut on 26 May 2020

https://blog.rust-lang.org/inside-rust/2019/12/18/bisecting-rust-compiler.html worked pretty well:

searched nightlies: from nightly-2020-05-24 to nightly-2020-05-26
regressed nightly: nightly-2020-05-25
searched commits: from https://github.com/rust-lang/rust/commit/8970e8bcf6153d1ead2283f1a0ed7b192230eca6 to https://github.com/rust-lang/rust/commit/46e85b4328fe18492894093c1092dfe509df4370
regressed commit: https://github.com/rust-lang/rust/commit/7726070fa755f660b5da3f82f46e07d9c6866f69


bisected with cargo-bisect-rustc v0.5.1

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc

The regression commit is a rollup merge of https://github.com/rust-lang/rust/pull/72388 and https://github.com/rust-lang/rust/pull/72517 .聽The former looks more related. CC @Aaron1011

SimonSapin picture SimonSapin on 26 May 2020

Replacing serde_derive with this proc macro still reproduces the same error:

use proc_macro::TokenStream;
use syn::{DeriveInput, parse_macro_input};

#[proc_macro_derive(Serialize)]
pub fn derive_serialize(input: TokenStream) -> TokenStream {
    let input = parse_macro_input!(input as DeriveInput);
    unimplemented!()
}
SimonSapin picture SimonSapin on 26 May 2020 
#[proc_macro]
pub fn foo(input: proc_macro::TokenStream) -> proc_macro::TokenStream {
    syn::parse_macro_input!(input as syn::Expr);
    unimplemented!()
}

macro_rules! gl_enums {
    ($mod:ident) => {
        not_serde::foo! {
            $mod::DEPTH_COMPONENT
        }
    }
}
gl_enums!(gl);
SimonSapin picture SimonSapin on 26 May 2020

Output of dbg!(&input):

TokenStream [
    Group {
        delimiter: None,
        stream: TokenStream [
            Ident {
                ident: "gl",
                span: #0 bytes(136..138),
            },
        ],
        span: #3 bytes(84..88),
    },
    Punct {
        ch: ':',
        spacing: Joint,
        span: #3 bytes(88..90),
    },
    Punct {
        ch: ':',
        spacing: Alone,
        span: #3 bytes(88..90),
    },
    Ident {
        ident: "DEPTH_COMPONENT",
        span: #3 bytes(90..105),
    },
]

And with $mod replaced by a hard-coded gl path:

TokenStream [
    Ident {
        ident: "gl",
        span: #3 bytes(84..86),
    },
    Punct {
        ch: ':',
        spacing: Joint,
        span: #3 bytes(86..88),
    },
    Punct {
        ch: ':',
        spacing: Alone,
        span: #3 bytes(86..88),
    },
    Ident {
        ident: "DEPTH_COMPONENT",
        span: #3 bytes(88..103),
    },
]
SimonSapin picture SimonSapin on 26 May 2020

Replacing serde_derive with this proc macro still reproduces the same error:

This might be a false lead? The error is identical but it also happen with older rustc where serde succeeds. So I don鈥檛 understand what鈥檚 going on here. syn::parse_macro_input!(input as syn::DeriveInput) is the first thing that serde::Serialize does: https://github.com/serde-rs/serde/blob/v1.0.110/serde_derive/src/lib.rs#L78

SimonSapin picture SimonSapin on 26 May 2020

Could this be a duplicate of or a bug related to #72545? cc @dtolnay

LeSeulArtichaut picture LeSeulArtichaut on 26 May 2020

It does look very similar. I suspect that a change in syn similar to the one discussed at https://github.com/rust-lang/rust/issues/72545#issuecomment-633312939 in proc-macro-hack could solve this. I tried to find the relevant source code in syn but https://github.com/dtolnay/syn/blob/6d5c90ddf790342947cad5737051f34e715c58af/src/path.rs#L35 only has a definition that a PathSegment contains an Ident. Is the parsing code macro-generated? CC @dtolnay

SimonSapin picture SimonSapin on 26 May 2020

I released a fix to parse $mod::DEPTH_COMPONENT in syn 1.0.26.

dtolnay picture dtolnay on 26 May 2020
❤3 👍1

Assigning P-critical as discussed as part of the Prioritization Working Group process and removing I-prioritize.

This seems to be the same issue as #72545 ?. And the culprit being #72388 or am I wrong?

spastorino picture spastorino on 27 May 2020

Yeah I think we can close in favor of #72545, it's the same underlying change.

dtolnay picture dtolnay on 27 May 2020
Was this page helpful?
0 / 5 - 0 ratings

Related issues

dnsl48 picture dnsl48  路  3Comments
overvenus picture overvenus  路  3Comments
defuz picture defuz  路  3Comments
jmegaffin picture jmegaffin  路  3Comments
modsec picture modsec  路  3Comments