Merge pull request #290 from posborne/add-clippy-to-ci

Add clippy to ci (and fixes)

Author: fitzgen

.github/workflows/sightglass.yml

@@ -14,15 +14,16 @@ env:
   REVISION: "v38.0.3"
 
 jobs:
-  format:
+  format-and-clippy:
     runs-on: ubuntu-latest
     steps:
     - uses: actions/checkout@v4
       with:
         submodules: true
     - run: rustup update
-    - run: rustup component add rustfmt
+    - run: rustup component add rustfmt clippy
     - run: cargo fmt --all -- --check
+    - run: cargo clippy --all-targets --all-features -- -D warnings
 
   test:
     runs-on: ${{ matrix.os }}

crates/analysis/src/effect_size.rs

@@ -18,7 +18,7 @@ pub fn calculate<'a>(
     measurements: &[Measurement<'a>],
 ) -> Result<Vec<EffectSize<'a>>> {
     anyhow::ensure!(
-        0.0 <= significance_level && significance_level <= 1.0,
+        (0.0..=1.0).contains(&significance_level),
         "The significance_level must be between 0.0 and 1.0. \
              Typical values are 0.05 and 0.01 (i.e. 95% and 99% confidence). \
              Found {}.",

crates/analysis/src/keys.rs

@@ -94,11 +94,11 @@ pub struct Key<'a> {
 impl Key<'_> {
     /// Does the given measurement match this key?
     pub fn matches(&self, m: &Measurement) -> bool {
-        self.arch.as_ref().map_or(true, |x| *x == m.arch)
-            && self.engine.as_ref().map_or(true, |x| *x == m.engine)
-            && self.wasm.as_ref().map_or(true, |x| *x == m.wasm)
-            && self.phase.as_ref().map_or(true, |x| *x == m.phase)
-            && self.event.as_ref().map_or(true, |x| *x == m.event)
+        self.arch.as_ref().is_none_or(|x| *x == m.arch)
+            && self.engine.as_ref().is_none_or(|x| *x == m.engine)
+            && self.wasm.as_ref().is_none_or(|x| *x == m.wasm)
+            && self.phase.as_ref().is_none_or(|x| *x == m.phase)
+            && self.event.as_ref().is_none_or(|x| *x == m.event)
     }
 }
 

crates/analysis/src/summarize.rs

@@ -6,7 +6,7 @@ use std::io::Write;
 /// Summarize measurements grouped by: architecture, engine, benchmark file, phase and event.
 pub fn calculate<'a>(measurements: &[Measurement<'a>]) -> Vec<Summary<'a>> {
     let mut summaries = Vec::new();
-    for k in KeyBuilder::all().keys(&measurements) {
+    for k in KeyBuilder::all().keys(measurements) {
         let mut grouped_counts: Vec<_> = measurements
             .iter()
             .filter(|m| k.matches(m))

crates/build/src/wasm.rs

@@ -12,7 +12,6 @@ use std::{
 };
 use thiserror::Error;
 use wasmparser::{Import, Payload, TypeRef};
-use wasmprinter;
 
 pub struct WasmBenchmark(PathBuf);
 
@@ -30,31 +29,33 @@ impl WasmBenchmark {
     pub fn is_valid(&self) -> Result<(), ValidationError> {
         // Check that the file actually exists.
         if !self.0.exists() {
-            return ValidationErrorKind::DoesNotExist.with(&self);
+            return ValidationErrorKind::DoesNotExist.with(self);
         }
 
         // Check that the contents are readable.
         let bytes = match fs::read(&self.0) {
             Ok(b) => b,
             Err(_) => {
-                return ValidationErrorKind::Unreadable.with(&self);
+                return ValidationErrorKind::Unreadable.with(self);
             }
         };
 
         // Check that it contains valid Wasm.
-        let mut features = wasmparser::WasmFeatures::default();
-        features.simd = true;
+        let features = wasmparser::WasmFeatures {
+            simd: true,
+            ..Default::default()
+        };
         let mut validator = wasmparser::Validator::new_with_features(features);
-        if let Err(_) = validator.validate_all(&bytes) {
-            return ValidationErrorKind::InvalidWasm.with(&self);
+        if validator.validate_all(&bytes).is_err() {
+            return ValidationErrorKind::InvalidWasm.with(self);
         }
 
         // Check that it has the expected imports/exports.
         if !has_import_function(&bytes, "bench", "start").unwrap() {
-            return ValidationErrorKind::MissingImport("bench.end").with(&self);
+            return ValidationErrorKind::MissingImport("bench.start").with(self);
         }
         if !has_import_function(&bytes, "bench", "end").unwrap() {
-            return ValidationErrorKind::MissingImport("bench.end").with(&self);
+            return ValidationErrorKind::MissingImport("bench.end").with(self);
         }
 
         Ok(())
@@ -80,7 +81,7 @@ impl WasmBenchmark {
         ));
         let mut file = File::create(&wat)?;
         file.write_all(wasmprinter::print_file(&self.0)?.as_bytes())?;
-        file.write(&['\n' as u8])?; // Append a newline on the end.
+        file.write_all(b"\n")?; // Append a newline on the end.
         Ok(wat)
     }
 }
@@ -91,9 +92,9 @@ impl AsRef<Path> for WasmBenchmark {
     }
 }
 
-impl Into<PathBuf> for WasmBenchmark {
-    fn into(self) -> PathBuf {
-        self.0
+impl From<WasmBenchmark> for PathBuf {
+    fn from(val: WasmBenchmark) -> Self {
+        val.0
     }
 }
 
@@ -134,25 +135,20 @@ impl Display for WasmBenchmark {
 
 fn has_import_function(bytes: &[u8], expected_module: &str, expected_field: &str) -> Result<bool> {
     let parser = wasmparser::Parser::new(0);
-    for payload in parser.parse_all(&bytes) {
-        match payload? {
-            Payload::ImportSection(imports) => {
-                for import in imports {
-                    match import? {
-                        Import {
-                            module,
-                            name: field,
-                            ty: TypeRef::Func(_),
-                        } => {
-                            if module == expected_module && field == expected_field {
-                                return Ok(true);
-                            }
-                        }
-                        _ => {}
+    for payload in parser.parse_all(bytes) {
+        if let Payload::ImportSection(imports) = payload? {
+            for import in imports {
+                if let Import {
+                    module,
+                    name: field,
+                    ty: TypeRef::Func(_),
+                } = import?
+                {
+                    if module == expected_module && field == expected_field {
+                        return Ok(true);
                     }
                 }
             }
-            _ => {}
         }
     }
     Ok(false)

crates/cli/src/benchmark.rs

@@ -25,7 +25,7 @@ pub struct BenchmarkCommand {
     /// The path to the file(s) to benchmark. This accepts one or more:
     ///
     /// - `*.wasm` files: individual benchmarks that meet the requirements
-    /// outlined in `benchmarks/README.md`
+    ///   outlined in `benchmarks/README.md`
     ///
     /// - `*.suite` files: a file containing a newline-delimited list of
     ///   benchmarks to execute. A `*.suite` file may contain `#`-prefixed line
@@ -180,7 +180,7 @@ impl BenchmarkCommand {
                 log::info!("Using working directory: {}", working_dir.display());
 
                 // Read the Wasm bytes.
-                let bytes = fs::read(&wasm_file).context("Attempting to read Wasm bytes")?;
+                let bytes = fs::read(wasm_file).context("Attempting to read Wasm bytes")?;
                 log::debug!("Wasm benchmark size: {} bytes", bytes.len());
 
                 let wasm_hash = {
@@ -296,11 +296,11 @@ impl BenchmarkCommand {
             .with_context(|| "expected the benchmark file to have an extension")?
             .to_str()
             .with_context(|| "expected the benchmark file to have a printable name")?;
-        let mut stdout_expected = wasm_file_dir.join(format!("{}.stdout.expected", benchmark_name));
+        let mut stdout_expected = wasm_file_dir.join(format!("{benchmark_name}.stdout.expected"));
         if !stdout_expected.exists() {
             stdout_expected = wasm_file_dir.join("default.stdout.expected");
         }
-        let mut stderr_expected = wasm_file_dir.join(format!("{}.stderr.expected", benchmark_name));
+        let mut stderr_expected = wasm_file_dir.join(format!("{benchmark_name}.stderr.expected"));
         if !stderr_expected.exists() {
             stderr_expected = wasm_file_dir.join("default.stderr.expected");
         }
@@ -365,8 +365,7 @@ impl BenchmarkCommand {
                 .args(
                     self.measures
                         .iter()
-                        .map(|m| ["--measure".to_string(), m.to_string()])
-                        .flatten(),
+                        .flat_map(|m| ["--measure".to_string(), m.to_string()]),
                 )
                 .arg("--raw")
                 .arg("--output-format")
@@ -387,7 +386,7 @@ impl BenchmarkCommand {
             }
 
             if let Some(flags) = &self.engine_flags {
-                command.arg(format!("--engine-flags={}", flags));
+                command.arg(format!("--engine-flags={flags}"));
             }
 
             command.arg("--").arg(&wasm);
@@ -500,7 +499,7 @@ pub fn check_engine_path(engine: &str) -> Result<PathBuf> {
 /// `stderr`) is the same as the `expected` output.
 fn compare_output_file(wasm: &Path, actual: &Path, expected: &Path) -> Result<()> {
     if expected.exists() {
-        let expected_data = std::fs::read_to_string(&expected)
+        let expected_data = std::fs::read_to_string(expected)
             .with_context(|| format!("failed to read `{}`", expected.display()))?;
         let stdout_actual_data = std::fs::read_to_string(actual)
             .with_context(|| format!("failed to read `{}`", actual.display()))?;
@@ -540,7 +539,7 @@ mod tests {
         display_summaries(&measurements, &mut output)?;
 
         let actual = String::from_utf8(output)?;
-        eprintln!("=== Actual ===\n{}", actual);
+        eprintln!("=== Actual ===\n{actual}");
 
         let expected = r#"
 compilation
@@ -574,7 +573,7 @@ execution
       [3556483 3729210.70 4349778] /tmp/old_backend_2.so
       [3639688 4025470.30 5782529] /tmp/old_backend_3.so
 "#;
-        eprintln!("=== Expected ===\n{}", expected);
+        eprintln!("=== Expected ===\n{expected}");
 
         assert_eq!(actual.trim(), expected.trim());
         Ok(())
@@ -589,7 +588,7 @@ execution
         display_effect_size(&measurements, 0.05, &mut output)?;
 
         let actual = String::from_utf8(output)?;
-        eprintln!("=== Actual ===\n{}", actual);
+        eprintln!("=== Actual ===\n{actual}");
 
         let expected = r#"
 compilation :: cycles :: benchmarks/pulldown-cmark/benchmark.wasm
@@ -642,7 +641,7 @@ instantiation :: nanoseconds :: benchmarks/pulldown-cmark/benchmark.wasm
   [65929 71540.70 112190] new_backend.so
   [61849 69023.59 115015] old_backend.so
 "#;
-        eprintln!("=== Expected ===\n{}", expected);
+        eprintln!("=== Expected ===\n{expected}");
 
         assert_eq!(actual.trim(), expected.trim());
         Ok(())

crates/cli/tests/all/benchmark.rs

@@ -81,7 +81,7 @@ fn benchmark_json() {
         .assert();
 
     let stdout = std::str::from_utf8(&assert.get_output().stdout).unwrap();
-    eprintln!("=== stdout ===\n{}\n===========", stdout);
+    eprintln!("=== stdout ===\n{stdout}\n===========");
     assert!(serde_json::from_str::<serde_json::Value>(stdout).is_ok());
 
     assert
@@ -109,7 +109,7 @@ fn benchmark_csv() {
         .assert();
 
     let stdout = std::str::from_utf8(&assert.get_output().stdout).unwrap();
-    eprintln!("=== stdout ===\n{}\n===========", stdout);
+    eprintln!("=== stdout ===\n{stdout}\n===========");
     let mut reader = csv::Reader::from_reader(stdout.as_bytes());
     for measurement in reader.deserialize::<Measurement<'_>>() {
         drop(measurement.unwrap());
@@ -174,12 +174,12 @@ fn benchmark_effect_size() -> anyhow::Result<()> {
         .assert()
         .success()
         .stdout(
-            predicate::str::contains(&format!("compilation :: cycles :: {}", benchmark("noop")))
-                .and(predicate::str::contains(&format!(
+            predicate::str::contains(format!("compilation :: cycles :: {}", benchmark("noop")))
+                .and(predicate::str::contains(format!(
                     "instantiation :: cycles :: {}",
                     benchmark("noop")
                 )))
-                .and(predicate::str::contains(&format!(
+                .and(predicate::str::contains(format!(
                     "execution :: cycles :: {}",
                     benchmark("noop")
                 )))

crates/cli/tests/all/fingerprint.rs

@@ -14,7 +14,7 @@ fn fingerprint_machine() {
         .assert();
 
     let stdout = std::str::from_utf8(&assert.get_output().stdout).unwrap();
-    eprintln!("=== stdout ===\n{}\n===========", stdout);
+    eprintln!("=== stdout ===\n{stdout}\n===========");
     assert!(serde_json::from_str::<Machine>(stdout).is_ok());
 }
 
@@ -30,7 +30,7 @@ fn fingerprint_benchmark() {
         .assert();
 
     let stdout = std::str::from_utf8(&assert.get_output().stdout).unwrap();
-    eprintln!("=== stdout ===\n{}\n===========", stdout);
+    eprintln!("=== stdout ===\n{stdout}\n===========");
     let mut reader = csv::Reader::from_reader(stdout.as_bytes());
     for measurement in reader.deserialize::<Benchmark>() {
         drop(measurement.unwrap());
@@ -58,7 +58,7 @@ fn fingerprint_engine() {
         .assert();
 
     let stdout = std::str::from_utf8(&assert.get_output().stdout).unwrap();
-    eprintln!("=== stdout ===\n{}\n===========", stdout);
+    eprintln!("=== stdout ===\n{stdout}\n===========");
     let mut reader = csv::Reader::from_reader(stdout.as_bytes());
     for measurement in reader.deserialize::<Benchmark>() {
         drop(measurement.unwrap());
@@ -73,7 +73,7 @@ fn fingerprint_engine() {
                 .and(contains(r#""id":"wasmtime-"#))
                 .and(contains(r#""name":"wasmtime""#))
                 .and(contains(r#""datetime":"20"#))
-                .and(contains(format!(r#""path":"{}""#, escaped_engine_path)))
+                .and(contains(format!(r#""path":"{escaped_engine_path}""#)))
                 .and(contains(r#""buildinfo":"NAME=wasmtime"#))
                 .and(ends_with("}")),
         )

crates/cli/tests/all/upload.rs

@@ -22,7 +22,7 @@ fn upload_dryrun() {
 
     // Gather up the logged output from stderr.
     let stderr = std::str::from_utf8(&assert.get_output().stderr).unwrap();
-    eprintln!("=== stderr ===\n{}\n===========", stderr);
+    eprintln!("=== stderr ===\n{stderr}\n===========");
 
     // Gather the fingerprints of the system under test.
     let engine = sightglass_fingerprint::Engine::fingerprint(test_engine()).unwrap();

crates/cli/tests/all/util.rs

@@ -24,7 +24,6 @@ pub fn test_engine() -> PathBuf {
         BUILD_WASMTIME.call_once(|| {
             if engine_path.is_file() {
                 // Use the already built engine library.
-                return;
             } else {
                 // Use this instead of `eprintln!` to avoid `cargo test`'s stdio
                 // capturing.
@@ -67,5 +66,5 @@ pub fn sightglass_cli_benchmark() -> Command {
 
 /// Get the benchmark path for the benchmark with the given name.
 pub fn benchmark(benchmark_name: &str) -> String {
-    format!("../../benchmarks/{}/benchmark.wasm", benchmark_name).into()
+    format!("../../benchmarks/{benchmark_name}/benchmark.wasm")
 }