unnested_or_patterns mishandles macros

[matthiaskrgr]

Summary

.

Lint Name

unnested_or_patterns

Reproducer

I tried this code:

It looks like in the suggestion, we replace macros with actual code and thus produce invalid suggestions...?

// check-pass

// Test various exhaustive matches for `X..`, `..=X` and `..X` ranges.

#![feature(exclusive_range_pattern)]

fn main() {}

macro_rules! m {
    ($s:expr, $($t:tt)+) => {
        match $s { $($t)+ => {} }
    }
}

macro_rules! test_int {
    ($s:expr, $min:path, $max:path) => {
        m!($s, $min..);
        m!($s, $min..5 | 5..);
        m!($s, ..5 | 5..);
        m!($s, ..=4 | 5..);
        m!($s, ..=$max);
        m!($s, ..$max | $max);
        m!(($s, true), (..5 | 5.., true) | (u8::MIN.., false));
    }
}

fn unsigned_int() {
    test_int!(0u8, u8::MIN, u8::MAX);

}

I saw this happen:
cargo clippy --fix -- -Aclippy::all -Wclippy::unnested_or_patterns

    Checking clpy v0.1.0 (/tmp/clpy)
warning: failed to automatically apply fixes suggested by rustc to crate `clpy`

after fixes were automatically applied the compiler reported errors within these files:

  * src/main.rs

This likely indicates a bug in either rustc or cargo itself,
and we would appreciate a bug report! You're likely to see
a number of compiler warnings after this message which cargo
attempted to fix but failed. If you could open an issue at
https://github.com/rust-lang/rust/issues
quoting the full output of this command we'd be very appreciative!
Note that you may be able to make some more progress in the near-term
fixing code with the `--broken-code` flag

The following errors were reported:
error[E0308]: mismatched types
  --> src/main.rs:23:45
   |
23 |         m!(($s, true), (..5 | 5.., true) | (u128::MIN.., false));
   |            ----------                       ^^^^^^^^^ expected `u8`, found `u128`
   |            |
   |            this expression has type `(u8, bool)`
...
28 |     test_int!(0u8, u8::MIN, u8::MAX);
   |     -------------------------------- in this macro invocation
   |
   = note: this error originates in the macro `test_int` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0308]: mismatched types
  --> src/main.rs:23:45
   |
23 |         m!(($s, true), (..5 | 5.., true) | (u128::MIN.., false));
   |            ----------                       ^^^^^^^^^ expected `u16`, found `u128`
   |            |
   |            this expression has type `(u16, bool)`
...
29 |     test_int!(0u16, u16::MIN, u16::MAX);
   |     ----------------------------------- in this macro invocation
   |
   = note: this error originates in the macro `test_int` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0308]: mismatched types
  --> src/main.rs:23:45
   |
23 |         m!(($s, true), (..5 | 5.., true) | (u128::MIN.., false));
   |            ----------                       ^^^^^^^^^ expected `u32`, found `u128`
   |            |
   |            this expression has type `(u32, bool)`
...
30 |     test_int!(0u32, u32::MIN, u32::MAX);
   |     ----------------------------------- in this macro invocation
   |
   = note: this error originates in the macro `test_int` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0308]: mismatched types
  --> src/main.rs:23:45
   |
23 |         m!(($s, true), (..5 | 5.., true) | (u128::MIN.., false));
   |            ----------                       ^^^^^^^^^ expected `u64`, found `u128`
   |            |
   |            this expression has type `(u64, bool)`
...
31 |     test_int!(0u64, u64::MIN, u64::MAX);
   |     ----------------------------------- in this macro invocation
   |
   = note: this error originates in the macro `test_int` (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to 4 previous errors

For more information about this error, try `rustc --explain E0308`.
Original diagnostics will follow.

warning: unnested or-patterns
  --> src/main.rs:23:24
   |
23 |         m!(($s, true), (..5, true) | (5.., true) | ($min.., false));
   |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
28 |     test_int!(0u8, u8::MIN, u8::MAX);
   |     -------------------------------- in this macro invocation
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnested_or_patterns
   = note: requested on the command line with `-W clippy::unnested-or-patterns`
   = note: this warning originates in the macro `test_int` (in Nightly builds, run with -Z macro-backtrace for more info)
help: nest the patterns
   |
23 |         m!(($s, true), (..5 | 5.., true) | (u8::MIN.., false));
   |                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

warning: unnested or-patterns
  --> src/main.rs:23:24
   |
23 |         m!(($s, true), (..5, true) | (5.., true) | ($min.., false));
   |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
29 |     test_int!(0u16, u16::MIN, u16::MAX);
   |     ----------------------------------- in this macro invocation
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnested_or_patterns
   = note: this warning originates in the macro `test_int` (in Nightly builds, run with -Z macro-backtrace for more info)
help: nest the patterns
   |
23 |         m!(($s, true), (..5 | 5.., true) | (u16::MIN.., false));
   |                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

warning: unnested or-patterns
  --> src/main.rs:23:24
   |
23 |         m!(($s, true), (..5, true) | (5.., true) | ($min.., false));
   |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
30 |     test_int!(0u32, u32::MIN, u32::MAX);
   |     ----------------------------------- in this macro invocation
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnested_or_patterns
   = note: this warning originates in the macro `test_int` (in Nightly builds, run with -Z macro-backtrace for more info)
help: nest the patterns
   |
23 |         m!(($s, true), (..5 | 5.., true) | (u32::MIN.., false));
   |                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

warning: unnested or-patterns
  --> src/main.rs:23:24
   |
23 |         m!(($s, true), (..5, true) | (5.., true) | ($min.., false));
   |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
31 |     test_int!(0u64, u64::MIN, u64::MAX);
   |     ----------------------------------- in this macro invocation
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnested_or_patterns
   = note: this warning originates in the macro `test_int` (in Nightly builds, run with -Z macro-backtrace for more info)
help: nest the patterns
   |
23 |         m!(($s, true), (..5 | 5.., true) | (u64::MIN.., false));
   |                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

warning: unnested or-patterns
  --> src/main.rs:23:24
   |
23 |         m!(($s, true), (..5, true) | (5.., true) | ($min.., false));
   |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
32 |     test_int!(0u128, u128::MIN, u128::MAX);
   |     -------------------------------------- in this macro invocation
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnested_or_patterns
   = note: this warning originates in the macro `test_int` (in Nightly builds, run with -Z macro-backtrace for more info)
help: nest the patterns
   |
23 |         m!(($s, true), (..5 | 5.., true) | (u128::MIN.., false));
   |                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

warning: function `unsigned_int` is never used
  --> src/main.rs:27:4
   |
27 | fn unsigned_int() {
   |    ^^^^^^^^^^^^
   |
   = note: `#[warn(dead_code)]` on by default

warning: `clpy` (bin "clpy") generated 6 warnings (6 duplicates)
warning: `clpy` (bin "clpy" test) generated 6 warnings (run `cargo fix --bin "clpy" --tests` to apply 5 suggestions)
    Finished dev [unoptimized + debuginfo] target(s) in 0.60s

I expected to see this happen:

Version

rustc 1.67.0-nightly (c5d82ed7a 2022-11-19)
binary: rustc
commit-hash: c5d82ed7a4ad94a538bb87e5016e7d5ce0bd434b
commit-date: 2022-11-19
host: x86_64-unknown-linux-gnu
release: 1.67.0-nightly
LLVM version: 15.0.4

Additional Labels

No response

[fmease]

While the issue description mostly focuses on the I-suggestion-causes-error, I'm facing a very similar issue with clippy::unnested_or_patterns for which I don't think it's worth opening a separate issue.

I am using macros and or-patterns to define pattern synonyms basically and Clippy claims that the use sites need to be unnested suggesting me to inline my beautiful pattern synonyms which is of course not at all what I want. I don't want all the use sites to have to manually allow the lint. I can't place #[allow(clippy::unnested_or_patterns)] on or inside of the macro since that'd lead to syntax errors.

#![deny(clippy::unnested_or_patterns)]

use Token::*;

macro_rules! Identifier {
    () => { Word | Symbol };
}

pub fn handle(token: Token) {
    match token {
        Identifier!() | TextLiteral => 0,
        NumberLiteral => 1,
    };
}

pub enum Token {
    Word,
    Symbol,
    NumberLiteral,
    TextLiteral,
}

error: unnested or-patterns
  --> src/lib.rs:11:9
   |
11 |         Identifier!() | TextLiteral => 0,
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnested_or_patterns
note: the lint level is defined here
  --> src/lib.rs:1:9
   |
1  | #![deny(clippy::unnested_or_patterns)]
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: nest the patterns
   |
11 |         Word | Symbol | TextLiteral => 0,
   |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~

[ada4a]

It looks like the original issue is no longer reproducible, but @fmease's still is. I'll look into it

[ada4a]

Hm, I could swear I followed up on this.

Anyway, @fmease's issue is ultimately caused by the or-pattern inside the macro being expanded into a PatKind::Paren, and so the whole pattern in the example becoming (Word | Symbol) | TextLiteral.

Two solutions that come to mind are:

1. Suppress the lint completely on any pattern containing macros. Unfortunately, that would also cause false-negatives in patterns where the macro isn't a part of the unnested-or pattern at all.
2. Teach the lint to nest everything but the macro-generated part of the pattern. That would be pretty difficult, because:
   • the nesting logic relies on the pattern having been fully flattened beforehand
   • the pretty-printer used to output the final pattern is oblivious to macros, and so would print Identifier!() in its expanded form anyway

[fmease]

[the] issue is ultimately caused by the or-pattern inside the macro being expanded into a PatKind::Paren

I'm not sure how you came to that conclusion, rustc doesn't wrap the result of the expansion in PatKind::Paren, the AST alone dictates the structure in this case; instead, it expands M!() | C (where M expands to A | B) to Or(Or(A, B), C).

You can verify that yourself by passing -Zunpretty=ast-tree,expanded to rustc. Take this smaller example to reduce the amount of data you have to sift through:

macro_rules! X { () => { 0 | 1 } }

fn main() {
    match x { X!() | 2 => {} }
}

[fmease]

I'm relatively sure that without macros a user cannot write code that leads to an AST like Or(Or(_, _), _) since writing A | B | C leads to a node like Or(A, B, C) and writing (A | B) | C leads to Or(Paren(Or(A, B)), C). So that might be a way to differentiate these cases (if you'd like to avoid looking at the Spans to check whether two nodes come from different SyntaxContexts)?

I just skimmed unnested_or_patterns's impl and it seems it's currently stripping all parentheses beforehand.

---

Meta: With Matthias's issue fixed but not mine, should I open a separate issue for it?

[ada4a]

I'm not sure how you came to that conclusion

I wrote the comment from memory, and apparently forgot some details, sorry about that^^

I just skimmed unnested_or_patterns's impl and it seems it's currently stripping all parentheses beforehand.

Yes, but right after that it also unnests all the or-patterns (in unnest_or_patterns), so the macro and non-macro case become no longer distinguishable. But maybe one could tweak that process..

With Matthias's issue fixed but not mine, should I open a separate issue for it?

I think that'd make sense yeah