Wrong "unnecessary parentheses around function argument"-warning

[SWW13]

Recent nightly versions give warnings when using the macro do_parse!() of nom:

I tried this code:

named!(pub parse_string <String>, do_parse!(
    value: map_res!(take_until!("\x00"), |value: &[u8]| String::from_utf8(value.to_vec())) >>
    tag!(b"\x00") >>
    (value)
));

I expected to see this happen:
No warning.

Instead, this happened:

warning: unnecessary parentheses around function argument
   |
13 | / named!(pub parse_string <String>, do_parse!(
14 | |     value: map_res!(take_until!("\x00"), |value: &[u8]| String::from_utf8(value.to_vec())) >>
15 | |     tag!(b"\x00") >>
16 | |     (value)
17 | | ));
   | |___^ help: remove these parentheses
   |
   = note: #[warn(unused_parens)] on by default
   = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

After investigating this further I found the source of the warning (see SWW13/nom@f1a04eb#diff-06e70f2a187023418eedf26d22d3aa5d).
But these parentheses are needed for the additional tuple, removing them breaks the nom tests.

CI logs show last working version rustc 1.25.0-nightly (da569fa9d 2018-01-16) and first broken version rustc 1.25.0-nightly (97520ccb1 2018-01-21).

Meta

Related Issue: rust-bakery/nom#668

rustc --version --verbose:

rustc 1.25.0-nightly (9fd7da904 2018-01-25)
binary: rustc
commit-hash: 9fd7da904b46ff7aa78c2e2cc1986c4975aeccc6
commit-date: 2018-01-25
host: x86_64-unknown-linux-gnu
release: 1.25.0-nightly
LLVM version: 4.0

[zackmdavis]

Introduced in #46980. 😰

I'm having trouble fully understanding the nom example, but prior work suggests it's possible to not lint macro-generated code, which I think is reasonable in this case.

[zackmdavis]

I haven't been able to construct a minimal standalone example (which we would want in order to attempt a more fundamental fix before resorting to the lazy answer of just not linting macro expansions). The macro backtrace from nom is somewhat opaque:

zmd@ReflectiveCoherence:~/Code/Misc/repro_paren/src$ RUSTFLAGS="-Z unstable-options -Z external-macro-backtrace -F warnings" cargo +nightly run 
   Compiling libc v0.2.36
   Compiling memchr v1.0.2
   Compiling nom v3.2.1
   Compiling repro_paren v0.1.0 (file:///home/zmd/Code/Misc/repro_paren)
error: unnecessary parentheses around function argument
   --> <do_parse macros>:2:36
    |
1   |         ( __impl $ i : expr , $ consumed : expr , ( $ ( $ rest : expr ) , * ) ) => (
    |       __-
    |      |__|
    |     ||__|
    |    |||__|
    |   ||||
2   |   ||||  $ crate :: IResult :: Done ( $ i , ( $ ( $ rest ) , * ) ) ) ; (
    |   ||||                                     ^^^^^^^^^^^^^^^^^^^^ help: remove these parentheses
3   |   ||||  __impl $ i : expr , $ consumed : expr , $ field : ident : $ submac : ident ! (
4   |   ||||  $ ( $ args : tt ) * ) ) => (
...     ||||
49  |   ||||  let i_ = i . clone (  ) ; do_parse ! (
    |  _||||____________________________-
50  | | ||||  __impl i_ , $ consumed + (
51  | | ||||  $ crate :: InputLength :: input_len ( & ( $ i ) ) - $ crate :: InputLength ::
52  | | ||||  input_len ( & i ) ) , $ ( $ rest ) * ) } , } } ) ; (
    | |_||||_______________________________________- in this macro invocation (#5)
...     ||||
71  |   ||||  let $ field = o ; let i_ = i . clone (  ) ; do_parse ! (
    |  _||||______________________________________________-
72  | | ||||  __impl i_ , $ consumed + (
73  | | ||||  $ crate :: InputLength :: input_len ( & ( $ i ) ) - $ crate :: InputLength ::
74  | | ||||  input_len ( & i ) ) , $ ( $ rest ) * ) } , } } ) ; (
    | |_||||_______________________________________- in this macro invocation (#4)
...     ||||
112 |   ||||  { do_parse ! ( __impl $ i , 0usize , $ ( $ rest ) * ) } ) ; (
    |   ||||    --------------------------------------------------- in this macro invocation (#3)
...     ||||
123 |   ||||  ) ; ) ; ( $ e : ident ! >> $ ( $ rest : tt ) * ) => (
124 |   ||||  do_parse ! ( call ! ( $ e ) >> $ ( $ rest ) * ) ; ) ;
    |   ||||                                                      -
    |   ||||______________________________________________________|
    |   |||_______________________________________________________in this expansion of `do_parse!` (#2)
    |   ||________________________________________________________in this expansion of `do_parse!` (#3)
    |   |_________________________________________________________in this expansion of `do_parse!` (#4)
    |                                                             in this expansion of `do_parse!` (#5)
    | 
   ::: <named macros>
    |
1   |       / ( # $ ( $ args : tt ) * ) => ( named_attr ! ( # $ ( $ args ) * ) ; ) ; (
2   |       | $ name : ident ( $ i : ty ) -> $ o : ty , $ submac : ident ! (
3   |       | $ ( $ args : tt ) * ) ) => (
4   |       | # [ allow ( unused_variables ) ] fn $ name ( i : $ i ) -> $ crate :: IResult <
...         |
41  |       | crate :: IResult < & [ u8 ] , & [ u8 ] , u32 > {
42  |       | $ submac ! ( i , $ ( $ args ) * ) } ) ;
    |       |_______________________________________- in this expansion of `named!` (#1)
    | 
   ::: src/main.rs
    |
4   |             named!(pub parse_string <String>, do_parse!(
    |  ___________-_________________________________-
    | | __________|
    | ||
5   | ||              value: map_res!(take_until!("\x00"), |value: &[u8]| String::from_utf8(value.to_vec())) >>
6   | ||                  tag!(b"\x00") >>
7   | ||                  (value)
8   | ||          ));
    | ||____________- in this macro invocation (#1)
...   |
    |
    = note: `-F unused-parens` implied by `-F warnings`

error: aborting due to previous error

error: Could not compile `repro_paren`.

To learn more, run the command again with --verbose.

[LegNeato]

I believe we are seeing this in juniper with this PR: graphql-rust/juniper#135 (specifically graphql-rust/juniper@438d56a#diff-46914ac7413539fd200e08e90af18e09). Putting the parens in causes the warning, taking them out causes a build failure.

[zackmdavis]

@LegNeato thanks. Minimization of the Juniper example:

macro_rules! name_maybe_fn_from_ident {
    ( @as_expr, $e:expr) => { $e };
    ( @generate_fn, $name:tt) => {
        fn name_maybe<'a>() -> Option<&'a str> {
            Some(name_maybe_fn_from_ident!( @as_expr, $name ))
        }
    };
    ( $name:ident ) => { name_maybe_fn_from_ident!( @generate_fn, (stringify!($name))); }
}

name_maybe_fn_from_ident!(rah);

fn main() {}

Here, the lint is "legitimately" firing when one macro expansion's single-element token-tree gets interpreted as another macro expansion's expression. (The generated code here is fn name_maybe<'a>() -> Option<&'a str> { Some(("rah")) }, with extra parens around the Some constructor's argument.)

Hopefully there's some way for the compiler to not lint this case, which macro writers shouldn't need to care about, while still linting literal unused parens in macro expansions? If not, not-linting unused parens in macro expansions at all, while an unprincipled hack, is better for users than endorsing nightly's current behavior towards cases like those of Nom and Juniper.

[zackmdavis]

not-linting unused parens in macro expansions at all

Or less bad, nested macro expansions specifically: #47896

[nikomatsakis]

triage: P-high

The behavior of the compiler while technically correct is super annoying. Thanks @zackmdavis for taking initiative.

[nikomatsakis]

I believe this is now fixed, right? cc @zackmdavis

[zackmdavis]

@nikomatsakis yes