unused_parens triggers on macro by example code.

[weiznich]

The latest beta contains a new? lunused_parens lint that triggers on macro by example code. This does not necessarily make sense because macros could be used to construct nearly arbitrary custom syntax for example something requiring to wrap a single element into a parenthesis.
The strange thing here is that seems to only happen on beta, not on nightly, not on current stable.

Found via the diesel CI

Error message:

    --> diesel/src/macros/mod.rs:274:27
     |
274  |               primary_key = (id),
     |                             ^^^^ help: remove these parentheses
…     |                             ^^^^ help: remove these parentheses
...
1085 | /     table! {
1086 | |         use sql_types::*;
1087 | |         use macros::tests::my_types::*;
1088 | |
...    |
1092 | |         }
1093 | |     }
     | |_____- in this macro invocation

rustc version: rustc 1.40.0-beta.1 (76b40532a 2019-11-05)

[hellow554]

nearly arbitrary custom syntax for example something requiring to wrap a single element into a parenthesis.

My head can't come up with an example that requires parentheses. macro_rules! macros are hygenic, so there should be no problem in omitting the parentheses and just writing primary_key = id will be fine.

Can you give me a counter example?

[weiznich]

@hellow554 A real world example is the diesel table! macro. Somewhere in the inner it does some clever things to support plain types and tuples by just saying ($($some_ty),+) where $some_ty is some repeated type variable. Depending on the number of actual types there this will result in ($some_ty) which is a single type or ($some_ty1, $some_ty2, …) which is a tuple. It's probably possible to rewrite that code doing the same stuff but that would involve a lot of duplication (Because than you would need to have a special case for a single type everywhere).

[hellow554]

So somehing like this:

macro_rules! foo {
    ($($foo:expr),+) => {
        ($($foo),*)
    }
}

fn main() {
    let _a = foo!(3, 4);
    let _b = foo!(3);
}

You're right. The question is: should we remove the lint for all macros? Only for macros that contain repetitive expansions? (Is that even possible to detect?)

[weiznich]

Would be great to get that fixed till it is part of the next stable release.

[weiznich]

This seems to occur first on nightly-2019-11-02

[pnkfelix]

nominating for discussing at T-compiler + T-lang meeting, namely to take peoples' temperature with respect to the idea of just not linting this case when the flagged code appears as part of a macro definition.

[pnkfelix]

also, marking as P-high w.r.t. answer the question(s) posed here. (I personally don't think the severity of the actual impact is likely to be worth a P-high rating; the priority is likely just temporary until the discussion happens.)

[Centril]

The language team discussed this last week. The general consensus was that we can stop linting when the redundant parens are inside the macro (as opposed to e.g. mac!( ( 1 + 2) ) where the spans are from the outside). We believe this is fine as the lint is mostly a style thing which does not protect from anything correctness or performance related.

[weiznich]

I've played a bit around with this issue today. Seems like those conditions need to be adjusted to also check if we are in macro expanded code. I've tried something like value.span.from_expansion() but that does remove more warnings that we want.

[pnkfelix]

I think the questions have been basically answered in terms of what path we see best going forward. Self-assigning, removing I-nominated.