Improve CI, requring all feature combos to succeed along with clippy

.github/workflows/rustfmt.yml

@@ -14,17 +14,10 @@ jobs:
     # Only run on PRs if the source branch is on someone else's repo
     if: ${{ github.event_name != 'pull_request' || github.repository != github.event.pull_request.head.repo.full_name }}
     runs-on: ubuntu-latest
-    strategy:
-      matrix:
-        rust:
-          - stable
     steps:
-      - uses: actions/checkout@v2
-      - uses: actions-rs/toolchain@v1
+      - uses: actions/checkout@v4
+      - uses: dtolnay/rust-toolchain@stable
         with:
-          profile: minimal
-          toolchain: ${{ matrix.rust }}
-          override: true
           components: rustfmt
       - shell: bash
         run: |

.github/workflows/test.yml

@@ -26,38 +26,109 @@ jobs:
     strategy:
       fail-fast: false # Even if one job fails we still want to see the other ones
       matrix:
-        # 1.49 is MSRV
-        rust: [1.49, stable, nightly]
+        rust:
+          # Minimum Supported Rust Version: 1.49
+          #
+          # This is hardcoded and needs to be in sync with Cargo.toml and the README
+          #
+          # Right now we have to explicitly list the combination of features & msrv,
+          # because some feature combinations are unsupported (currently nested-values).
+          #
+          # TODO: Once the MSRV supports all feature combos, add this back
+          # - << put msrv here >>
+
+          # Per the MSRV policy (discussed in Wiki/docs), this places an upper bound on the MSRV
+          #
+          # In other words, we can never raise the MSRV past this, and must always support it.
+          - "stable minus 15 releases"
+          # Major release 1.56: The first version to support `edition="2021"`
+          #
+          # This appears to be the earliest version that `erased-serde` supports,
+          # so is the earliest one we can enable the 'nested-values' feature.
+          - 1.56
+          # A recent version of stable rust that is hardcoded.
+          #
+          # This should be kept as up to date as possible.
+          #
+          # This is used so that clippy & tests are run on a reliable reference point.
+          # If clippy has any warnings, this will fail the build (we run with --deny warnings)
+          - 1.72
+          # The most recent version of stable rust (automatically updated)
+          #
+          # Sometimes, this is exactly the same as the hardcoded right above.
+          # However sometimes it will be automatically updated to something a little newer.
+          #
+          # If there are new clippy lints in the automatic update that aren't
+          # in the hardcoded versions, they will _NOT_ fail the build.
+          # This is true even if they are set to deny by default (clippy does this for some 'correctness' lints).
+          # They will simply be regular warnings.
+          - stable
+          - nightly
         # NOTE: Features to test must be specified manually. They are applied to all versions separately.
         #
         # This has the advantage of being more flexibile and thorough
         # This has the disadvantage of being more vebrose
         #
         # Specific feature combos can be overridden per-version with 'include' and 'ecclude'
-        features: ["", "nested-values", "dynamic-keys", "nothreads", "nested-values dynamic-keys", "nested-values dynamic-keys nothreads"]
+        features:
+          - ""
+          - "nested-values"
+          - "dynamic-keys"
+          - "nothreads"
+          - "nested-values dynamic-keys"
+          - "nested-values dynamic-keys nothreads"
+        include:
+          # Minimum Supported Rust Version (1.49) doesn't support nested-values feature
+          #
+          # This is because the erased-serde dependency currently requires edition="2021", which was added in Rust 1.56.
+          # As a workaround, we explicitly include the combinations of features that we want to test on MSRV.
+          #
+          # TODO: Go back to testing all features (see above)
+          - rust: 1.49
+            features: "" # Default features
+          - rust: 1.49
+            features: "dynamic-keys"
+          - rust: 1.49
+            features: "nothreads"
+          - rust: 1.49
+            features: "nothreads dynamic-keys" # All (supported) features
 
     steps:
-      - uses: actions/checkout@v2
-      - uses: actions-rs/toolchain@v1
+      - uses: actions/checkout@v4
+      - uses: dtolnay/rust-toolchain@master
         with:
           toolchain: ${{ matrix.rust }}
-          override: true
+          components: clippy
       - name: Check
         # A failing `cargo check` always ends the build
         run: |
-          cargo check --verbose --features "${{ matrix.features }}"
+          cargo check --all-targets --verbose --features "${{ matrix.features }}"
         # A failing `cargo check` always fails the build
         continue-on-error: false
       - name: Test
         run: |
-          cargo test --verbose --features "${{ matrix.features }}"
+          cargo test --all-targets --verbose --features "${{ matrix.features }}"
 
-        # By default, we require tests to succeed if either
-        # 1. It has default features (`features == ""`)
+        # We require tests to succeed on all the feature combinations.
+        #
+        # However, we can grant special exceptions for the Minimum Supported Rust Version
+        # if there is a really good reason (like a dependency that requires a newer version).
+        continue-on-error: false
+
+      - name: Clippy
+        # With the exception of nightly, we use --deny warnings to treat warnings on errors.
+        run: |
+          cargo clippy --all-targets --verbose --features "${{ matrix.features }}" -- --deny "${{ matrix.rust != 'nightly' && 'warnings' || 'clippy::correctness' }}"
+        # Clippy is required to succeed on hardcoded versions, and may not give any warnings.
         #
-        # Otherwise, we allow tests to fail.
+        # However, on automatically updated versions of rust (both stable & nightly) we allow clippy to fail.
+        # This is in case automatic updates have introduced new lints that would give warnings/errors
+        # about code that was previously allowed.
         #
-        # This is necessary because most feature combos currently break the tests :(
+        # This is the main reason that we have a 'hardcoded recent stable' version.
+        # We want as many lints from recent stable possible
+        # but don't want the surprises of automatic updates to our stable rust.
         #
-        # TODO: Add some sort of option to get around this (or fix the build)
-        continue-on-error: ${{ matrix.features == '' }} 
+        # Also, include an explicit exception for Rust 1.56.
+        # We don't want to deal with the fact clippy changed the names of some lints.
+        continue-on-error: ${{ !contains(matrix.rust, '1.') || matrix.rust == '1.56' }}

CHANGELOG.md

@@ -15,8 +15,13 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
   * Internal refactoring to make different feature combos much easier (PR #301)
 * Switch from Travis CI to Github Actions (fixes #294)
   * `rustfmt --check` now run by default
+  * Require `clippy` to succeed without warnings
+  * Require all feature combinations to pass tests.
+    * Make exception for `nested-values` feature on MSRV (1.49),
+      because `erased-serde` dependency requires `edition="2021"` (needs 1.56)
 * Fix `#` format when not used as a last argument.
 * Implement `Value` for `std::borrow::Cow`
+* Fix duplicate `AsRef<str>` implementations, fixing support for `dynamic-keys`
 
 ## 2.7.0 - 2020-11-29
 

Cargo.toml

@@ -25,6 +25,11 @@ edition = "2018"
 #
 # The first version of Cargo that supports this field was in Rust 1.56.0.
 # In older releases, the field will be ignored, and Cargo will display a warning.
+#
+# This must be kept in sync with the following places:
+# - .github/workflows/test.yml (Github Actions)
+# - README.md
+# - clippy.toml (Clippy config)
 rust-version = "1.49.0"
 
 [profile.release]

README.md

@@ -6,7 +6,7 @@
   <br>
 
   <a href="https://github.com/slog-rs/slog/actions/workflows/test.yml">
-      <img src="https://img.shields.io/github/workflow/status/slog-rs/slog/Cargo%20Test" alt="GH Actions (Cargo Test)">
+      <img src="https://img.shields.io/github/actions/workflow/status/slog-rs/slog/test.yml?branch=master" alt="GH Actions (Cargo Test)">
   </a>
 
   <a href="https://crates.io/crates/slog">
@@ -20,8 +20,9 @@
   <a href="https://docs.rs/releases/search?query=slog-">
       <img src="https://docs.rs/slog/badge.svg" alt="docs-rs: release versions documentation">
   </a>
-  <a href="https://blog.rust-lang.org/2018/12/06/Rust-1.31-and-rust-2018.html">
-      <img src="https://img.shields.io/badge/rust-1.31%2B-orange.svg">
+  <!-- Badge showing our Minimum Supported Rust Version, along with a link to the release announcement on the official blog -->
+  <a href="https://blog.rust-lang.org/2020/12/31/Rust-1.49.0.html">
+      <img src="https://img.shields.io/badge/rust-1.49%2B-orange.svg">
   </a>
   <br>
     <strong><a href="https://github.com/slog-rs/slog/wiki/Getting-started">Getting started</a></strong>

clippy.toml

@@ -0,0 +1,6 @@
+msrv = "1.49.0"
+# TODO: Consider adding this?
+#
+# I would much prefer supressing lints that break
+# API compatibility on a case-by-case basis.
+# avoid-breaking-exported-api = true

examples/common/mod.rs

@@ -29,7 +29,7 @@ impl Drain for PrintlnDrain {
             .unwrap();
         values.serialize(record, &mut PrintlnSerializer).unwrap();
 
-        println!("");
+        println!();
         Ok(())
     }
 }

examples/struct-log-self.rs

@@ -11,10 +11,7 @@ struct Peer {
 
 impl Peer {
     fn new(host: String, port: u32) -> Self {
-        Peer {
-            host: host,
-            port: port,
-        }
+        Peer { host, port }
     }
 }
 
@@ -47,7 +44,7 @@ impl Server {
         Server {
             _host: host,
             _port: port,
-            log: log,
+            log,
         }
     }
 
@@ -66,7 +63,7 @@ struct PeerCounter {
 
 impl PeerCounter {
     fn new(log: Logger) -> Self {
-        PeerCounter { count: 0, log: log }
+        PeerCounter { count: 0, log }
     }
 
     // A hybrid approach with `Logger` with parent logging-context embedded into

src/key/dynamic.rs

@@ -56,10 +56,11 @@ impl Key {
     pub fn as_str(&self) -> &str {
         self.data.as_ref()
     }
-
-    /// Take as a reference
-    pub fn as_ref(&self) -> &str {
-        self.as_str()
+}
+impl AsRef<str> for Key {
+    #[inline]
+    fn as_ref(&self) -> &str {
+        self.data.as_ref()
     }
 }
 
@@ -177,12 +178,6 @@ impl PartialEq<Self> for Key {
     }
 }
 
-impl AsRef<str> for Key {
-    fn as_ref<'a>(&'a self) -> &'a str {
-        self.data.as_ref()
-    }
-}
-
 impl fmt::Display for Key {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self.data {