Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rustc says necessary parentheses are unnecessary #120642

Closed
mysteriouslyseeing opened this issue Feb 4, 2024 · 0 comments · Fixed by #123314
Closed

Rustc says necessary parentheses are unnecessary #120642

mysteriouslyseeing opened this issue Feb 4, 2024 · 0 comments · Fixed by #123314
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` C-bug Category: This is a bug. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@mysteriouslyseeing
Copy link

This code:

trait Foo {
    fn bar();
}

macro_rules! problem {
    ($ty:ident) => {
        impl<$ty: Foo> Foo for ($ty,) {
            fn bar() { <$ty>::bar() }
        }
    };
    ($ty:ident $(, $rest:ident)*) => {
        impl<$ty: Foo, $($rest: Foo),*> Foo for ($ty, $($rest),*) {
            fn bar() {
                <$ty>::bar();
                <($($rest),*)>::bar()
            }
        }

        problem!($($rest),*);
    }
}

problem!(T1, T2);

Playground

Creates this diagnostic:

   Compiling playground v0.0.1 (/home/wschroeder/Projects/playground)
warning: unnecessary parentheses around type
  --> src/lib.rs:15:18
   |
15 |                 <($($rest),*)>::bar()
   |                  ^^^     ^^^^
...
23 | problem!(T1, T2);
   | ---------------- in this macro invocation
   |
   = note: `#[warn(unused_parens)]` on by default
   = note: this warning originates in the macro `problem` (in Nightly builds, run with -Z macro-backtrace for more info)
help: remove these parentheses
   |
15 -                 <($($rest),*)>::bar()
15 +                 <$rest>::bar()
   |

warning: `playground` (lib) generated 1 warning (run `cargo fix --lib -p playground` to apply 1 suggestion)
    Finished dev [unoptimized + debuginfo] target(s) in 0.04s

I ran cargo fix --lib -p playground --allow-dirty (note that I created a local project called playground) which created this output:

    Checking playground v0.0.1 (/home/wschroeder/Projects/playground)
warning: failed to automatically apply fixes suggested by rustc to crate `playground`

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

  * src/lib.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: variable 'rest' is still repeating at this depth
  --> src/lib.rs:15:18
   |
15 |                 <$rest>::bar()
   |                  ^^^^^

error: aborting due to 1 previous error

Original diagnostics will follow.

warning: unnecessary parentheses around type
  --> src/lib.rs:15:18
   |
15 |                 <($($rest),*)>::bar()
   |                  ^^^     ^^^^
...
23 | problem!(T1, T2);
   | ---------------- in this macro invocation
   |
   = note: `#[warn(unused_parens)]` on by default
   = note: this warning originates in the macro `problem` (in Nightly builds, run with -Z macro-backtrace for more info)
help: remove these parentheses
   |
15 -                 <($($rest),*)>::bar()
15 +                 <$rest>::bar()
   |

warning: `playground` (lib) generated 1 warning (run `cargo fix --lib -p playground` to apply 1 suggestion)
    Finished dev [unoptimized + debuginfo] target(s) in 0.07s

This told me to create an issue here.

Going into more depth regarding the issue - the macro expands after one step like so:

impl<T1: Foo, T2: Foo> Foo for (T1, T2) {
    fn bar() {
        <T1>::bar();
        <(T2)>::bar();
    }
}

problem!(T2);

and we can see where the issue is:

<(T2)>::bar()

These parentheses are indeed fully removable, and it points to an issue in the macro definition. By changing

<($($rest),*)>

to

<($($rest,)*)>

the compiler no longer emits the warning, because now it expands to

<(T2,)>::bar()

which is properly recognized as a tuple with one element, rather than a type surrounded by redundant parentheses.

Another way to fix it is to remove the outer parentheses in the macro call:

<$($rest,)*>

which does compile without warnings, but breaks usage of the macro with tuples with more than two elements:

problem!(T1, T2, T3);

Playground
as while its fine to remove the parentheses when there is only one element between the angle brackets (<(T2)> to <T2>), it is not fine when there are more than one element in the tuple (<(T2, T3)> to <T2, T3>).

Importantly, the compiler still emits the unused parentheses warning and suggests their removal when the macro is called with three types when you haven't removed the parentheses (Playground).

Therefore, part of the issue is that rustc detects the problem, and assumes it can fix it by going straight to the macro and correcting it, neglecting previous expansions of the macro which require the currently redundant parentheses.

I say part of the issue, because going back to the original diagnostic:

15 |                 <($($rest),*)>::bar()
   |                  ^^^     ^^^^

rustc marks ($( and ),*) as removable, and it suggests you do so:

15 -                 <($($rest),*)>::bar()
15 +                 <$rest>::bar()

when the diagnostic should really only be suggesting the removal of the outer brackets.

I'm not too sure why this one happens. Using Syn's terminology, my best guess is that it has something to do with the span of the outer parentheses including the inner $( and ),* after expansion.

Meta

rustc --version --verbose:

rustc 1.75.0 (82e1608df 2023-12-21)
binary: rustc
commit-hash: 82e1608dfa6e0b5569232559e3d385fea5a93112
commit-date: 2023-12-21
host: x86_64-unknown-linux-gnu
release: 1.75.0
LLVM version: 17.0.6

Also tried with nightly (had identical results):

rustc 1.78.0-nightly (b11fbfbf3 2024-02-03)
binary: rustc
commit-hash: b11fbfbf351b94c7eecf9e6749a4544a6d4717fa
commit-date: 2024-02-03
host: x86_64-unknown-linux-gnu
release: 1.78.0-nightly
LLVM version: 17.0.6
@mysteriouslyseeing mysteriouslyseeing added the C-bug Category: This is a bug. label Feb 4, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Feb 4, 2024
@clubby789 clubby789 added A-diagnostics Area: Messages for errors, warnings, and lints D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Feb 4, 2024
@fmease fmease added A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. and removed A-diagnostics Area: Messages for errors, warnings, and lints D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. labels Feb 4, 2024
surechen added a commit to surechen/rust that referenced this issue Apr 1, 2024
surechen added a commit to surechen/rust that referenced this issue Apr 10, 2024
fmease added a commit to fmease/rust that referenced this issue Apr 10, 2024
Skip `unused_parens` report for `Paren(Path(..))` in macro.

fixes rust-lang#120642

In following code, `unused_parens` suggest change `<($($rest),*)>::bar()` to `<$rest>::bar()`  which will cause another err: `error: variable 'rest' is still repeating at this depth`:

```rust
trait Foo {
    fn bar();
}

macro_rules! problem {
    ($ty:ident) => {
        impl<$ty: Foo> Foo for ($ty,) {
            fn bar() { <$ty>::bar() }
        }
    };
    ($ty:ident $(, $rest:ident)*) => {
        impl<$ty: Foo, $($rest: Foo),*> Foo for ($ty, $($rest),*) {
            fn bar() {
                <$ty>::bar();
                <($($rest),*)>::bar()
            }
        }
        problem!($($rest),*);
    }
}
```

I think maybe we can handle this by avoid warning for `Paren(Path(..))` in the macro. Is this reasonable approach?
fmease added a commit to fmease/rust that referenced this issue Apr 10, 2024
Skip `unused_parens` report for `Paren(Path(..))` in macro.

fixes rust-lang#120642

In following code, `unused_parens` suggest change `<($($rest),*)>::bar()` to `<$rest>::bar()`  which will cause another err: `error: variable 'rest' is still repeating at this depth`:

```rust
trait Foo {
    fn bar();
}

macro_rules! problem {
    ($ty:ident) => {
        impl<$ty: Foo> Foo for ($ty,) {
            fn bar() { <$ty>::bar() }
        }
    };
    ($ty:ident $(, $rest:ident)*) => {
        impl<$ty: Foo, $($rest: Foo),*> Foo for ($ty, $($rest),*) {
            fn bar() {
                <$ty>::bar();
                <($($rest),*)>::bar()
            }
        }
        problem!($($rest),*);
    }
}
```

I think maybe we can handle this by avoid warning for `Paren(Path(..))` in the macro. Is this reasonable approach?
fmease added a commit to fmease/rust that referenced this issue Apr 10, 2024
Skip `unused_parens` report for `Paren(Path(..))` in macro.

fixes rust-lang#120642

In following code, `unused_parens` suggest change `<($($rest),*)>::bar()` to `<$rest>::bar()`  which will cause another err: `error: variable 'rest' is still repeating at this depth`:

```rust
trait Foo {
    fn bar();
}

macro_rules! problem {
    ($ty:ident) => {
        impl<$ty: Foo> Foo for ($ty,) {
            fn bar() { <$ty>::bar() }
        }
    };
    ($ty:ident $(, $rest:ident)*) => {
        impl<$ty: Foo, $($rest: Foo),*> Foo for ($ty, $($rest),*) {
            fn bar() {
                <$ty>::bar();
                <($($rest),*)>::bar()
            }
        }
        problem!($($rest),*);
    }
}
```

I think maybe we can handle this by avoid warning for `Paren(Path(..))` in the macro. Is this reasonable approach?
@bors bors closed this as completed in c8490a0 Apr 11, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Apr 11, 2024
Rollup merge of rust-lang#123314 - surechen:fix_120642, r=Nadrieril

Skip `unused_parens` report for `Paren(Path(..))` in macro.

fixes rust-lang#120642

In following code, `unused_parens` suggest change `<($($rest),*)>::bar()` to `<$rest>::bar()`  which will cause another err: `error: variable 'rest' is still repeating at this depth`:

```rust
trait Foo {
    fn bar();
}

macro_rules! problem {
    ($ty:ident) => {
        impl<$ty: Foo> Foo for ($ty,) {
            fn bar() { <$ty>::bar() }
        }
    };
    ($ty:ident $(, $rest:ident)*) => {
        impl<$ty: Foo, $($rest: Foo),*> Foo for ($ty, $($rest),*) {
            fn bar() {
                <$ty>::bar();
                <($($rest),*)>::bar()
            }
        }
        problem!($($rest),*);
    }
}
```

I think maybe we can handle this by avoid warning for `Paren(Path(..))` in the macro. Is this reasonable approach?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` C-bug Category: This is a bug. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants