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

Clippy doesn't know how to fix arbitrary_self_type in some async functions #6089

Closed
jyn514 opened this issue Sep 26, 2020 · 1 comment · Fixed by #6093
Closed

Clippy doesn't know how to fix arbitrary_self_type in some async functions #6089

jyn514 opened this issue Sep 26, 2020 · 1 comment · Fixed by #6093
Assignees
Labels
C-bug Category: Clippy is not doing the correct thing

Comments

@jyn514
Copy link
Member

jyn514 commented Sep 26, 2020

I tried the code from https://github.com/tokio-rs/tracing/blob/9ae4676054067dcf99b55cb76fcee3997207de6d/tracing-attributes/tests/async_fn.rs. Here is my attempt at an MCVE, but it doesn't compile:

use async_trait::async_trait;
use tracing_attributes::instrument;

#[async_trait]
pub trait Test {
    async fn call();
    async fn call_with_self(&self);
    async fn call_with_mut_self(&mut self);
}

#[derive(Clone, Debug)]
struct TestImpl;

#[async_trait]
impl Test for TestImpl {
    // instrumenting this is currently not possible, see https://github.com/tokio-rs/tracing/issues/864#issuecomment-667508801
    //#[instrument(fields(Self=std::any::type_name::<Self>()))]
    async fn call() {}

    #[instrument(fields(Self=std::any::type_name::<Self>()))]
    async fn call_with_self(&self) {}

    #[instrument(fields(Self=std::any::type_name::<Self>()))]
    async fn call_with_mut_self(self: &mut Self) {}
}

I expected to see this happen: cargo +nightly clippy --fix -Zunstable-options turns the code into async fn call_with_mut_self(&mut self) {}

Instead, this happened: Clippy replaces it with async fn call_with_mut_self(&'life0 mut self) {} which fails to compile.

error[E0261]: use of undeclared lifetime name `'life0`
   --> tracing-attributes/tests/async_fn.rs:226:38
    |
226 |         async fn call_with_mut_self(&'life0 mut self) {}
    |                                      ^^^^^^ undeclared lifetime
    |
    = help: if you want to experiment with in-band lifetime bindings, add `#![feature(in_band_lifetimes)]` to the crate attributes
help: consider introducing lifetime `'life0` here
    |
217 |     impl<'life0> Test for TestImpl {
    |         ^^^^^^^^
help: consider introducing lifetime `'life0` here
    |
216 |     'life0, #[async_trait]
    |     ^^^^^^^

error[E0261]: use of undeclared lifetime name `'life0`
   --> tracing-attributes/tests/async_fn.rs:226:38
    |
226 |         async fn call_with_mut_self(&'life0 mut self) {}
    |                                    - ^^^^^^ undeclared lifetime
    |                                    |
    |                                    help: consider introducing lifetime `'life0` here: `<'life0>`
    |
    = help: if you want to experiment with in-band lifetime bindings, add `#![feature(in_band_lifetimes)]` to the crate attributes

error[E0195]: lifetime parameters or bounds on method `call_with_mut_self` do not match the trait declaration
   --> tracing-attributes/tests/async_fn.rs:216:5
    |
206 |     #[async_trait]
    |     -------------- lifetimes in impl do not match this method in trait
...
216 |     #[async_trait]
    |     ^^^^^^^^^^^^^^ lifetimes do not match method in trait
    |
    = note: this error originates in an attribute macro (in Nightly builds, run with -Z macro-backtrace for more info)

Meta

  • cargo clippy -V: clippy 0.0.212 (043f6d7 2020-09-25)
  • rustc -V: rustc 1.46.0 (04488afe3 2020-08-24)
@jyn514 jyn514 added the C-bug Category: Clippy is not doing the correct thing label Sep 26, 2020
@ebroto
Copy link
Member

ebroto commented Sep 28, 2020

I'm working on this one, should have a PR soon

@ebroto ebroto self-assigned this Sep 28, 2020
bors added a commit that referenced this issue Sep 28, 2020
needless arbitrary self: handle macros

This fixes two cases related to macros:

* If the parameter comes from expansion, do not lint as the user has no possibility of changing it. This is not directly related to the fixed issue, but we should probably do that.
* If *only* the lifetime name comes from expansion, lint, but allow the user decide the name of the lifetime. In the related issue, the lifetime was unnamed and then renamed by `async_trait`, so just removing the name in the suggestion would work, but in the general case a macro can rename a lifetime that was named differently, and we can't reliably know that name anymore.

As a hint for the reviewer, the expanded code for the test can be checked with this command (from the root dir of the repo):
```sh
rustc -L target/debug/test_build_base/needless_arbitrary_self_type_unfixable.stage-id.aux -Zunpretty=expanded tests/ui/needless_arbitrary_self_type_unfixable.rs
```

changelog: [`needless_arbitrary_self_type`]: handle macros

Fixes #6089
@bors bors closed this as completed in 99483be Sep 29, 2020
hawkw pushed a commit to tokio-rs/tracing that referenced this issue Sep 30, 2020
## Motivation

This will avoid breaking CI on new releases of clippy. It also makes the
code a little easier to read.

## Solution

- Convert `match val { pat => true, _ => false }` to `matches!(val, pat)`
- Remove unnecessary closures
- Convert `self: &mut Self` to `&mut self`

This bumps the MSRV to 1.42.0 for `matches!`.
The latest version of rust is 1.46.0, so as per
https://github.com/tokio-rs/tracing#supported-rust-versions this is not
considered a breaking change.

I didn't fix the following warning because the fix was not trivial/needed
a decision:

```
warning: you are deriving `Ord` but have implemented `PartialOrd` explicitly
   --> tracing-subscriber/src/filter/env/field.rs:16:32
    |
16  | #[derive(Debug, Eq, PartialEq, Ord)]
    |                                ^^^
    |
    = note: `#[warn(clippy::derive_ord_xor_partial_ord)]` on by default
note: `PartialOrd` implemented here
   --> tracing-subscriber/src/filter/env/field.rs:98:1
    |
98  | / impl PartialOrd for Match {
99  | |     fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
100 | |         // Ordering for `Match` directives is based first on _whether_ a value
101 | |         // is matched or not. This is semantically meaningful --- we would
...   |
121 | |     }
122 | | }
    | |_^
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#derive_ord_xor_partial_ord
```

As a side note, this found a bug in clippy 😆 rust-lang/rust-clippy#6089
hawkw pushed a commit to tokio-rs/tracing that referenced this issue Oct 7, 2020
## Motivation

This will avoid breaking CI on new releases of clippy. It also makes the
code a little easier to read.

## Solution

- Convert `match val { pat => true, _ => false }` to `matches!(val, pat)`
- Remove unnecessary closures
- Convert `self: &mut Self` to `&mut self`

This bumps the MSRV to 1.42.0 for `matches!`.
The latest version of rust is 1.46.0, so as per
https://github.com/tokio-rs/tracing#supported-rust-versions this is not
considered a breaking change.

I didn't fix the following warning because the fix was not trivial/needed
a decision:

```
warning: you are deriving `Ord` but have implemented `PartialOrd` explicitly
   --> tracing-subscriber/src/filter/env/field.rs:16:32
    |
16  | #[derive(Debug, Eq, PartialEq, Ord)]
    |                                ^^^
    |
    = note: `#[warn(clippy::derive_ord_xor_partial_ord)]` on by default
note: `PartialOrd` implemented here
   --> tracing-subscriber/src/filter/env/field.rs:98:1
    |
98  | / impl PartialOrd for Match {
99  | |     fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
100 | |         // Ordering for `Match` directives is based first on _whether_ a value
101 | |         // is matched or not. This is semantically meaningful --- we would
...   |
121 | |     }
122 | | }
    | |_^
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#derive_ord_xor_partial_ord
```

As a side note, this found a bug in clippy 😆 rust-lang/rust-clippy#6089
hawkw added a commit to tokio-rs/tracing that referenced this issue Oct 7, 2020
This backports PR #991 to v0.1.x. This is primarily necessary for the MSRV
bump, since some dependencies no longer compile on Rust 1.40.0.

This has already been approved on `master`, in PR #991, so it should be 
fine to ship.

## Motivation

This will avoid breaking CI on new releases of clippy. It also makes the
code a little easier to read.

## Solution

- Convert `match val { pat => true, _ => false }` to `matches!(val, pat)`
- Remove unnecessary closures
- Convert `self: &mut Self` to `&mut self`

This bumps the MSRV to 1.42.0 for `matches!`.
The latest version of rust is 1.46.0, so as per
https://github.com/tokio-rs/tracing#supported-rust-versions this is not
considered a breaking change.

I didn't fix the following warning because the fix was not trivial/needed
a decision:

```
warning: you are deriving `Ord` but have implemented `PartialOrd` explicitly
   --> tracing-subscriber/src/filter/env/field.rs:16:32
    |
16  | #[derive(Debug, Eq, PartialEq, Ord)]
    |                                ^^^
    |
    = note: `#[warn(clippy::derive_ord_xor_partial_ord)]` on by default
note: `PartialOrd` implemented here
   --> tracing-subscriber/src/filter/env/field.rs:98:1
    |
98  | / impl PartialOrd for Match {
99  | |     fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
100 | |         // Ordering for `Match` directives is based first on _whether_ a value
101 | |         // is matched or not. This is semantically meaningful --- we would
...   |
121 | |     }
122 | | }
    | |_^
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#derive_ord_xor_partial_ord
```

As a side note, this found a bug in clippy 😆 rust-lang/rust-clippy#6089
davidbarsky pushed a commit to tokio-rs/tracing-opentelemetry that referenced this issue Mar 21, 2023
This backports PR #991 to v0.1.x. This is primarily necessary for the MSRV
bump, since some dependencies no longer compile on Rust 1.40.0.

This has already been approved on `master`, in PR #991, so it should be 
fine to ship.

## Motivation

This will avoid breaking CI on new releases of clippy. It also makes the
code a little easier to read.

## Solution

- Convert `match val { pat => true, _ => false }` to `matches!(val, pat)`
- Remove unnecessary closures
- Convert `self: &mut Self` to `&mut self`

This bumps the MSRV to 1.42.0 for `matches!`.
The latest version of rust is 1.46.0, so as per
https://github.com/tokio-rs/tracing#supported-rust-versions this is not
considered a breaking change.

I didn't fix the following warning because the fix was not trivial/needed
a decision:

```
warning: you are deriving `Ord` but have implemented `PartialOrd` explicitly
   --> tracing-subscriber/src/filter/env/field.rs:16:32
    |
16  | #[derive(Debug, Eq, PartialEq, Ord)]
    |                                ^^^
    |
    = note: `#[warn(clippy::derive_ord_xor_partial_ord)]` on by default
note: `PartialOrd` implemented here
   --> tracing-subscriber/src/filter/env/field.rs:98:1
    |
98  | / impl PartialOrd for Match {
99  | |     fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
100 | |         // Ordering for `Match` directives is based first on _whether_ a value
101 | |         // is matched or not. This is semantically meaningful --- we would
...   |
121 | |     }
122 | | }
    | |_^
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#derive_ord_xor_partial_ord
```

As a side note, this found a bug in clippy 😆 rust-lang/rust-clippy#6089
davidbarsky pushed a commit to tokio-rs/tracing-opentelemetry that referenced this issue Mar 21, 2023
## Motivation

This will avoid breaking CI on new releases of clippy. It also makes the
code a little easier to read.

## Solution

- Convert `match val { pat => true, _ => false }` to `matches!(val, pat)`
- Remove unnecessary closures
- Convert `self: &mut Self` to `&mut self`

This bumps the MSRV to 1.42.0 for `matches!`.
The latest version of rust is 1.46.0, so as per
https://github.com/tokio-rs/tracing#supported-rust-versions this is not
considered a breaking change.

I didn't fix the following warning because the fix was not trivial/needed
a decision:

```
warning: you are deriving `Ord` but have implemented `PartialOrd` explicitly
   --> tracing-subscriber/src/filter/env/field.rs:16:32
    |
16  | #[derive(Debug, Eq, PartialEq, Ord)]
    |                                ^^^
    |
    = note: `#[warn(clippy::derive_ord_xor_partial_ord)]` on by default
note: `PartialOrd` implemented here
   --> tracing-subscriber/src/filter/env/field.rs:98:1
    |
98  | / impl PartialOrd for Match {
99  | |     fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
100 | |         // Ordering for `Match` directives is based first on _whether_ a value
101 | |         // is matched or not. This is semantically meaningful --- we would
...   |
121 | |     }
122 | | }
    | |_^
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#derive_ord_xor_partial_ord
```

As a side note, this found a bug in clippy 😆 rust-lang/rust-clippy#6089
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants