Compile error after applying `clippy::identity_op`

[dhedey]

Summary

I was writing tests for preinterpet on this draft PR and encountered the following error after running clippy:

after fixes were automatically applied the compiler reported errors
...
If you could open an issue at
https://github.com/rust-lang/rust-clippy/issues
quoting the full output of this command we'd be very appreciative!
...
clippy::identity_op

The code

The code which tripped up clippy was a little artificial, as it was a test case for building partial expressions from token streams inside macros. In particular, it was the following code:

    assert_preinterpret_eq!({
        0
        [!if! true { + 1 }]
    }, 1);

The full error

warning: failed to automatically apply fixes suggested by rustc to crate `control_flow`

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

  * tests/control_flow.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-clippy/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: mismatched closing delimiter: `]`
  --> tests/control_flow.rs:21:28
   |
21 |     assert_preinterpret_eq!({
   |                            ^ unclosed delimiter
22 |         1 }]
   |            ^ mismatched closing delimiter

error: unexpected closing delimiter: `)`
  --> tests/control_flow.rs:23:9
   |
22 |         1 }]
   |            - missing open `[` for this delimiter
23 |     }, 1);
   |         ^ unexpected closing delimiter

error: aborting due to 2 previous errors

Original diagnostics will follow.

warning: this operation has no effect
  --> tests/control_flow.rs:22:9
   |
22 | /         0
23 | |         [!if! true { + 1 }]
   | |________________________^ help: consider reducing it to: `1`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#identity_op
   = note: `#[warn(clippy::identity_op)]` on by default

warning: `preinterpret` (test "control_flow") generated 1 warning (run `cargo clippy --fix --test "control_flow"` to apply 1 suggestion)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 1.43s

Reproducer

I tried this code:

    assert_preinterpret_eq!({
        0
        [!if! true { + 1 }]
    }, 1);

Where the assert_preinterpret_eq! macro was defined as:

macro_rules! assert_preinterpret_eq {
    ($input:tt, $($output:tt)*) => {
        assert_eq!(preinterpret!($input), $($output)*);
    };
}

In this WIP branch in this repo.

You can probably substitute the following to reproduce the issue, without using preinterpret:

macro_rules! assert_preinterpret_eq {
    ($($output:tt)*) => {};
}

I expected to see this happen: Clippy ignored it

Instead, this happened: Clippy attempted to fix it and got a compile error.

Version

rustc 1.81.0 (eeb90cda1 2024-09-04)
binary: rustc
commit-hash: eeb90cda1969383f56a2637cbd3037bdf598841c
commit-date: 2024-09-04
host: aarch64-apple-darwin
release: 1.81.0
LLVM version: 18.1.7

Additional Labels

@rustbot label +l-suggestion-causes-error

[rustbot]

Error: Label l-suggestion-causes-error can only be set by Rust team members

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

[samueltardieu]

I understand that some macros have been rewritten. Does this still happen? If it does, could you provide us with a standalone example that reproduces the problem? If you don't succeed in producing a standalone example, could you put the failing code in a branch so that running Clippy on the repository with this branch out triggers the problem?

[dhedey]

@samueltardieu Ahh sorry, my ticket wasn't super helpful as-is!

A hopefully more usable minimal example is as follows:

macro_rules! append_streams {
    ([$($stream1:tt)*] [$($stream2:tt)*]) => {
        $($stream1)* $($stream2)*
    };
}

#[test]
fn example_test() {
    let one = append_streams!([0] [+ 1]);
    assert_eq!(1, one);
}

And then run cargo clippy --fix --tests --allow-dirty --allow-staged; gives the following error:

warning: failed to automatically apply fixes suggested by rustc to crate `temp`

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

  * tests/temp.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-clippy/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: unexpected end of macro invocation
  --> tests/temp.rs:15:34
   |
1  | macro_rules! append_streams {
   | --------------------------- when calling this macro
...
15 |     let one = append_streams!([1]);
   |                                  ^ missing tokens in macro arguments
   |
note: while trying to match `[`
  --> tests/temp.rs:2:24
   |
2  |     ([$($stream1:tt)*] [$($stream2:tt)*]) => {

Whilst this appears a little artificial (and I accept it's not a high priority), I can imagine this can could up in real cases where macros are calculating a length with an operation like 0 (+ L_i)* for each sub-length L_i.

My hunch is that the bug is to do with spans and not quite enough checking that the fix is doing the right thing:

• It appears that to trigger the bug, both the spans of the identity (0) and the sum (+ 1) need to originate from roughly the same place
• The fix attempts to use spans to remove the 0 + but possibly it actually removes 0] [+ and breaks the code.

Perhaps the nicest fix would be that the lint checks that the source code and resultant code both look the same before triggering the lint / proposing a fix.

[dhedey]

Note that the following code does not result in a clippy panic:

macro_rules! combine_streams {
    ($([$($substream:tt)*])*) => {
        $($($substream)*)*
    };
}

#[test]
fn temp_test() {
    let one = combine_streams!([0] [+ 1]);
    assert_eq!(1, one);
}

Clippy replaces combine_streams!([0] [+ 1]) with combine_streams!([1]) (likely by cutting out the string 0] [+) which just happens to not panic in this case, because combine_streams!(...) accepts any number of substreams [...], whereas append_streams!(...) required exactly two.