fix nightly clippy warnings #991

jyn514 · 2020-09-26T23:19:38Z

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

jyn514 · 2020-09-26T23:52:52Z

The CI failure looks like it's running this against the master version of workflows/CI.yml, which doesn't have the version bump. Not sure how to fix it.

hawkw · 2020-09-27T16:31:06Z

The CI failure looks like it's running this against the master version of workflows/CI.yml, which doesn't have the version bump. Not sure how to fix it.

There's not really an easy way to fix this; changes to the CI configs aren't reflected until those changes merge. I think in this case I'll have to administratively approve merging this PR and ignore the CI failure.

hawkw · 2020-09-27T16:34:30Z

.github/workflows/CI.yml

@@ -12,7 +12,7 @@ jobs:
    runs-on: ubuntu-latest
    strategy:
      matrix:
-        rust: [stable, 1.40.0]
+        rust: [stable, 1.42.0]


If we need to bump the MSRV, we'll also need to update the listed minimum supported version throughout the documentation (it's in the lib.rs RustDoc and the readme for each crate).

(sorry that's a big pile of work, but IMO, it's really important to ensure that the MSRV is documented and visible for users)

Don't worry, it didn't take very long :) tracing uses the same 'minimum supported version is X' wording everywhere so I automated it:

rg 'version is 1\.40' -l | xargs sed -i 's/version is 1\.40/version is 1.42/'

There's also a note at the top of the lib.rs docs and READMEs at the end of the "Overview" section ("Compiler support: requires rustc ..."). Mind rging those as well? Thanks!

Ok, I think I got them all this time :)

fd '.(md|rs)' | xargs rg '1\.40' -l | xargs sed -i 's/1\.40/1.42/'

jyn514 · 2020-09-27T18:23:54Z

The 'test macOS' job failed on 693890e but not on db267ae. I don't think running cargo fmt could have fixed it, maybe there's an intermittent failure going on? https://github.com/tokio-rs/tracing/runs/1173169110

hawkw · 2020-09-27T18:28:24Z

The 'test macOS' job failed on 693890e but not on db267ae. I don't think running cargo fmt could have fixed it, maybe there's an intermittent failure going on? https://github.com/tokio-rs/tracing/runs/1173169110

That test is occasionally flaky, I restarted it --- really need to get that fixed.

Currently, there are some minor issues with `Ord` and `PartialOrd` impls in `tracing_subscriber::filter::env`: - The `Directive` and `StaticDirective` types implement `PartialOrd` with an implementation that never returns `None`, and then have `Ord` implementations that call `partial_cmp` and `expect` that the returned value is `Some`. This isn't necessary. - `field::Match` implements `PartialOrd` manually but derives `Ord`. Since these traits must agree, using the derived implementation for one but manually implementing the other is potentially incorrect (see #991). This branch fixes these issues. I've moved actual comparison code from `PartialOrd` impls for `Directive` and `StaticDirective` to their `Ord` impls, and changed `PartialOrd::partial_cmp` for those types to call `Ord::cmp` and wrap it in a `Some`, rather than having `Ord::cmp` call `PartialOrd::partial_cmp` and unwrap it. This way, the fact that these comparison impls can never return `None` is encoded at the type level, rather than having an `expect` that will always succeed. Additionally, I've added a manual impl of `Ord` for `field::Match` and removed the derived impl. This should make Clippy happier. Signed-off-by: Eliza Weisman <eliza@buoyant.io>

## Motivation Currently, there are some minor issues with `Ord` and `PartialOrd` impls in `tracing_subscriber::filter::env`: - The `Directive` and `StaticDirective` types implement `PartialOrd` with an implementation that never returns `None`, and then have `Ord` implementations that call `partial_cmp` and `expect` that the returned value is `Some`. This isn't necessary. - `field::Match` implements `PartialOrd` manually but derives `Ord`. Since these traits must agree, using the derived implementation for one but manually implementing the other is potentially incorrect (see #991). ## Solution This branch fixes these issues. I've moved actual comparison code from `PartialOrd` impls for `Directive` and `StaticDirective` to their `Ord` impls, and changed `PartialOrd::partial_cmp` for those types to call `Ord::cmp` and wrap it in a `Some`, rather than having `Ord::cmp` call `PartialOrd::partial_cmp` and unwrap it. This way, the fact that these comparison impls can never return `None` is encoded at the type level, rather than having an `expect` that will always succeed. Additionally, I've added a manual impl of `Ord` for `field::Match` and removed the derived impl. This should make Clippy happier. Signed-off-by: Eliza Weisman <eliza@buoyant.io>

hawkw · 2020-09-28T16:52:18Z

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

I've merged #995, which should fix this warning. Everything looks good to me besides updating the "Compiler support ..." line in the docs!

jyn514 · 2020-09-28T21:22:50Z

I don't understand this error either :/ I can't reproduce locally.

error[E0061]: this function takes 1 argument but 0 arguments were supplied
Error:  --> tracing-macros/examples/factorial.rs:7:8
  |
7 |     if dbg!(n <= 1) {
  |        ^^^^^^^^^^^^
  |        |
  |        supplied 0 arguments
  |        expected 1 argument

hawkw · 2020-09-29T17:58:27Z

The latest CI failure is unrelated to this change and should be fixed on master, if you don't mind merging or rebasing the latest master?

This will avoid breaking CI on new releases of clippy. It also makes the code a little easier to read. - 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 error 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 ```

For the next person that comes along, the way to do this automatically is ```sh rg 'version is 1\.40' -l | xargs sed -i 's/version is 1\.40/version is 1.42/' ```

``` fd '.(md|rs)' | xargs rg '1\.40' -l | xargs sed -i 's/1\.40/1.42/' ```

jyn514 · 2020-09-29T19:18:55Z

I rebased over master.

hawkw · 2020-09-30T16:26:00Z

Oh, I guess the CI checks are never going to turn green because it still expects the Rust 1.40.0 CI jobs to run, and they won't on this branch. Everything else is passing, so I'm going to go ahead and merge this.

* upstream/master: subscriber: warn if trying to enable a statically disabled level (tokio-rs#990) subscriber: use macros for module declarations (tokio-rs#1009) chore: remove `stdlib.rs` (tokio-rs#1008) core: fix linked list tests reusing `Registration`s (tokio-rs#1016) subscriber: support dash in target names (tokio-rs#1012) docs: switch to intra-doc links in tracing-core (tokio-rs#1010) tracing-opentelemetry: implement additional record types (bool, i64, u64) (tokio-rs#1007) core: add intrusive linked list for callsite registry (tokio-rs#988) serde: allow tracing-serde to work on no_std. (tokio-rs#960) tracing: remove `Into<Option<Id>>` impl for `Span` (tokio-rs#1003) tracing: make `Entered` `!Send` (tokio-rs#1001) chore: fix nightly clippy warnings (tokio-rs#991) chore: bump all crate versions (tokio-rs#998) macros: fix the `tracing-macros` crate not compiling (tokio-rs#1000) tracing: prepare to release 0.1.21 (tokio-rs#997) core: prepare to release 0.1.17 (tokio-rs#996) subscriber: make `PartialOrd` & `Ord` impls more correct (tokio-rs#995) core, tracing: don't inline dispatcher::get_default (tokio-rs#994) core: make `Level` and `LevelFilter` `Copy` (tokio-rs#992)

## 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

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

jyn514 requested review from hawkw and a team as code owners September 26, 2020 23:19

jyn514 force-pushed the clippy branch 2 times, most recently from 5545cae to f1e3e00 Compare September 26, 2020 23:50

hawkw reviewed Sep 27, 2020

View reviewed changes

jyn514 requested review from jtescher and yaahc as code owners September 27, 2020 16:58

jyn514 mentioned this pull request Sep 27, 2020

field_filter_spans intermittently fails #993

Closed

hawkw mentioned this pull request Sep 28, 2020

subscriber: make PartialOrd & Ord impls more correct #995

Merged

jyn514 added 4 commits September 29, 2020 15:17

Note that the new MSRV is 1.42

f0f0a76

For the next person that comes along, the way to do this automatically is ```sh rg 'version is 1\.40' -l | xargs sed -i 's/version is 1\.40/version is 1.42/' ```

cargo fmt

fadafee

Update lib.rs and README files too

07b81bb

``` fd '.(md|rs)' | xargs rg '1\.40' -l | xargs sed -i 's/1\.40/1.42/' ```

jyn514 force-pushed the clippy branch from 4f2b1e0 to 07b81bb Compare September 29, 2020 19:17

hawkw merged commit 2f59b32 into tokio-rs:master Sep 30, 2020

jyn514 deleted the clippy branch September 30, 2020 16:39

hawkw mentioned this pull request Oct 7, 2020

chore: fix nightly clippy warnings (#991) #1025

Merged

davidbarsky mentioned this pull request Sep 26, 2023

chore: backport roughly a year's worth of changes #2728

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix nightly clippy warnings #991

fix nightly clippy warnings #991

jyn514 commented Sep 26, 2020 •

edited

Loading

jyn514 commented Sep 26, 2020

hawkw commented Sep 27, 2020

hawkw Sep 27, 2020 •

edited

Loading

hawkw Sep 27, 2020

jyn514 Sep 27, 2020

hawkw Sep 28, 2020

jyn514 Sep 28, 2020

jyn514 commented Sep 27, 2020

hawkw commented Sep 27, 2020

hawkw commented Sep 28, 2020

jyn514 commented Sep 28, 2020

hawkw commented Sep 29, 2020

jyn514 commented Sep 29, 2020

hawkw commented Sep 30, 2020

fix nightly clippy warnings #991

fix nightly clippy warnings #991

Conversation

jyn514 commented Sep 26, 2020 • edited Loading

Motivation

Solution

jyn514 commented Sep 26, 2020

hawkw commented Sep 27, 2020

hawkw Sep 27, 2020 • edited Loading

Choose a reason for hiding this comment

hawkw Sep 27, 2020

Choose a reason for hiding this comment

jyn514 Sep 27, 2020

Choose a reason for hiding this comment

hawkw Sep 28, 2020

Choose a reason for hiding this comment

jyn514 Sep 28, 2020

Choose a reason for hiding this comment

jyn514 commented Sep 27, 2020

hawkw commented Sep 27, 2020

hawkw commented Sep 28, 2020

jyn514 commented Sep 28, 2020

hawkw commented Sep 29, 2020

jyn514 commented Sep 29, 2020

hawkw commented Sep 30, 2020

jyn514 commented Sep 26, 2020 •

edited

Loading

hawkw Sep 27, 2020 •

edited

Loading