Auto merge of #2189 - RalfJung:clippy, r=RalfJung

run Clippy on CI

and fix some things it complains about. Also use `rustup-toolchain` script on CI (reduces code duplication, and good thing to make sure it keeps working, since we recommend it in the docs).

I left `ui_test` out for now; I'll leave those nits to `@oli-obk.` ;)

.github/workflows/ci.yml

@@ -72,20 +72,10 @@ jobs:
         shell: bash
         run: |
           if [[ ${{ github.event_name }} == 'schedule' ]]; then
-            RUSTC_HASH=$(git ls-remote https://github.com/rust-lang/rust.git master | awk '{print $1}')
+            ./rustup-toolchain HEAD --host ${{ matrix.host_target }}
           else
-            RUSTC_HASH=$(< rust-version)
+            ./rustup-toolchain "" --host ${{ matrix.host_target }}
           fi
-          # We need a nightly cargo for parts of the cargo miri test suite.
-          rustup-toolchain-install-master \
-            -f \
-            -n master "$RUSTC_HASH" \
-            -c cargo \
-            -c rust-src \
-            -c rustc-dev \
-            -c llvm-tools \
-            --host ${{ matrix.host_target }}
-          rustup default master
 
       - name: Show Rust version
         run: |
@@ -97,26 +87,35 @@ jobs:
         run: bash ./ci.sh
 
   fmt:
-    name: check formatting (ignored by bors)
+    name: formatting (ignored by bors)
     runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@v3
       - name: Install latest nightly
-        uses: actions-rs/toolchain@v1
+        run: |
-        with:
+          rustup toolchain install nightly --component rustfmt
-            toolchain: nightly
+          rustup override set nightly
-            components: rustfmt
+      - name: Formatting (miri, ui_test)
-            default: true
+        run: cargo fmt --all --check
-      - name: Check formatting (miri)
+      - name: Formatting (cargo-miri)
-        uses: actions-rs/cargo@v1
+        run: cargo fmt --manifest-path cargo-miri/Cargo.toml --all --check
-        with:
+
-          command: fmt
+  clippy:
-          args: --all -- --check
+    name: clippy (ignored by bors)
-      - name: Check formatting (cargo-miri)
+    runs-on: ubuntu-latest
-        uses: actions-rs/cargo@v1
+    steps:
-        with:
+      - uses: actions/checkout@v3
-          command: fmt
+      - name: Install required toolchain
-          args: --manifest-path cargo-miri/Cargo.toml --all -- --check
+        # We need a toolchain that can actually build Miri, just a nightly won't do.
+        run: |
+          cargo install rustup-toolchain-install-master # TODO: cache this?
+          ./rustup-toolchain "" -c clippy
+      - name: Clippy (miri)
+        run: cargo clippy --all-targets -- -D warnings
+      #- name: Clippy (ui_test)
+      #  run: cargo clippy --manifest-path ui_test/Cargo.toml --all-targets -- -D warnings
+      - name: Clippy (cargo-miri)
+        run: cargo clippy --manifest-path cargo-miri/Cargo.toml --all-targets -- -D warnings
 
   # These jobs doesn't actually test anything, but they're only used to tell
   # bors the build completed, as there is no practical way to detect when a

benches/helpers/fibonacci_helper_iterative.rs

@@ -9,7 +9,7 @@ fn fib(n: usize) -> usize {
     for _ in 0..n {
         let c = a;
         a = b;
-        b = c + b;
+        b += c;
     }
     a
 }

cargo-miri/bin.rs

@@ -1,3 +1,5 @@
+#![allow(clippy::useless_format, clippy::derive_partial_eq_without_eq)]
+
 mod version;
 
 use std::env;
@@ -96,6 +98,9 @@ fn show_version() {
     // Only use `option_env` on vergen variables to ensure the build succeeds
     // when vergen failed to find the git info.
     if let Some(sha) = option_env!("VERGEN_GIT_SHA_SHORT") {
+        // This `unwrap` can never fail because if VERGEN_GIT_SHA_SHORT exists, then so does
+        // VERGEN_GIT_COMMIT_DATE.
+        #[allow(clippy::option_env_unwrap)]
         write!(&mut version, " ({} {})", sha, option_env!("VERGEN_GIT_COMMIT_DATE").unwrap())
             .unwrap();
     }
@@ -135,16 +140,14 @@ impl<I: Iterator<Item = String>> Iterator for ArgSplitFlagValue<'_, I> {
 
     fn next(&mut self) -> Option<Self::Item> {
         let arg = self.args.next()?;
-        if arg.starts_with(self.name) {
+        if let Some(suffix) = arg.strip_prefix(self.name) {
             // Strip leading `name`.
-            let suffix = &arg[self.name.len()..];
             if suffix.is_empty() {
                 // This argument is exactly `name`; the next one is the value.
                 return self.args.next().map(Ok);
-            } else if suffix.starts_with('=') {
+            } else if let Some(suffix) = suffix.strip_prefix('=') {
                 // This argument is `name=value`; get the value.
-                // Strip leading `=`.
+                return Some(Ok(suffix.to_owned()));
-                return Some(Ok(suffix[1..].to_owned()));
             }
         }
         Some(Err(arg))
@@ -255,7 +258,7 @@ fn xargo_version() -> Option<(u32, u32, u32)> {
     let line = out
         .stderr
         .lines()
-        .nth(0)
+        .next()
         .expect("malformed `xargo --version` output: not at least one line")
         .expect("malformed `xargo --version` output: error reading first line");
     let (name, version) = {
@@ -285,7 +288,7 @@ fn xargo_version() -> Option<(u32, u32, u32)> {
         .expect("malformed `xargo --version` output: not a patch version piece")
         .parse()
         .expect("malformed `xargo --version` output: patch version is not an integer");
-    if !version_pieces.next().is_none() {
+    if version_pieces.next().is_some() {
         panic!("malformed `xargo --version` output: more than three pieces in version");
     }
     Some((major, minor, patch))
@@ -311,7 +314,7 @@ fn ask_to_run(mut cmd: Command, ask: bool, text: &str) {
         println!("Running `{:?}` to {}.", cmd, text);
     }
 
-    if cmd.status().expect(&format!("failed to execute {:?}", cmd)).success().not() {
+    if cmd.status().unwrap_or_else(|_| panic!("failed to execute {:?}", cmd)).success().not() {
         show_error(format!("failed to {}", text));
     }
 }
@@ -499,10 +502,11 @@ fn get_cargo_metadata() -> Metadata {
     for arg in ArgSplitFlagValue::new(
         env::args().skip(3), // skip the program name, "miri" and "run" / "test"
         config_flag,
-    ) {
+    )
-        if let Ok(config) = arg {
+    // Only look at `Ok`
-            cmd.arg(config_flag).arg(config);
+    .flatten()
-        }
+    {
+        cmd.arg(config_flag).arg(arg);
     }
     let mut child = cmd
         .stdin(process::Stdio::null())
@@ -524,11 +528,11 @@ fn get_cargo_metadata() -> Metadata {
 /// Additionally, somewhere between cargo metadata and TyCtxt, '-' gets replaced with '_' so we
 /// make that same transformation here.
 fn local_crates(metadata: &Metadata) -> String {
-    assert!(metadata.workspace_members.len() > 0);
+    assert!(!metadata.workspace_members.is_empty());
     let mut local_crates = String::new();
     for member in &metadata.workspace_members {
-        let name = member.split(" ").nth(0).unwrap();
+        let name = member.split(' ').next().unwrap();
-        let name = name.replace("-", "_");
+        let name = name.replace('-', "_");
         local_crates.push_str(&name);
         local_crates.push(',');
     }
@@ -708,7 +712,7 @@ fn out_filename(prefix: &str, suffix: &str) -> PathBuf {
                 get_arg_flag_value("--crate-name").unwrap(),
                 // This is technically a `-C` flag but the prefix seems unique enough...
                 // (and cargo passes this before the filename so it should be unique)
-                get_arg_flag_value("extra-filename").unwrap_or(String::new()),
+                get_arg_flag_value("extra-filename").unwrap_or_default(),
                 suffix,
             ));
             path
@@ -808,11 +812,10 @@ fn out_filename(prefix: &str, suffix: &str) -> PathBuf {
         // Forward arguments, but remove "link" from "--emit" to make this a check-only build.
         let emit_flag = "--emit";
         while let Some(arg) = args.next() {
-            if arg.starts_with(emit_flag) {
+            if let Some(val) = arg.strip_prefix(emit_flag) {
                 // Patch this argument. First, extract its value.
-                let val = &arg[emit_flag.len()..];
+                let val =
-                assert!(val.starts_with("="), "`cargo` should pass `--emit=X` as one argument");
+                    val.strip_prefix('=').expect("`cargo` should pass `--emit=X` as one argument");
-                let val = &val[1..];
                 let mut val: Vec<_> = val.split(',').collect();
                 // Now make sure "link" is not in there, but "metadata" is.
                 if let Some(i) = val.iter().position(|&s| s == "link") {
@@ -937,12 +940,10 @@ fn phase_runner(binary: &Path, binary_args: env::Args, phase: RunnerPhase) {
     while let Some(arg) = args.next() {
         if arg == "--extern" {
             forward_patched_extern_arg(&mut args, &mut cmd);
-        } else if arg.starts_with(error_format_flag) {
+        } else if let Some(suffix) = arg.strip_prefix(error_format_flag) {
-            let suffix = &arg[error_format_flag.len()..];
             assert!(suffix.starts_with('='));
             // Drop this argument.
-        } else if arg.starts_with(json_flag) {
+        } else if let Some(suffix) = arg.strip_prefix(json_flag) {
-            let suffix = &arg[json_flag.len()..];
             assert!(suffix.starts_with('='));
             // Drop this argument.
         } else {

rustup-toolchain

@@ -12,6 +12,8 @@ set -e
 # ./rustup-toolchain HEAD: Update "miri" toolchain and `rust-version` file to latest rustc HEAD.
 #
 # ./rustup-toolchain $COMMIT: Update "miri" toolchain and `rust-version` file to match that commit.
+#
+# Any extra parameters are passed to `rustup-toolchain-install-master`.
 
 # Make sure rustup-toolchain-install-master is installed.
 if ! which rustup-toolchain-install-master >/dev/null; then
@@ -28,6 +30,7 @@ else
     NEW_COMMIT="$1"
 fi
 echo "$NEW_COMMIT" > rust-version
+shift
 
 # Check if we already are at that commit.
 CUR_COMMIT=$(rustc +miri --version -v 2>/dev/null | egrep "^commit-hash: " | cut -d " " -f 2)
@@ -39,7 +42,7 @@ fi
 
 # Install and setup new toolchain.
 rustup toolchain uninstall miri
-rustup-toolchain-install-master -n miri -c cargo -c rust-src -c rustc-dev -c llvm-tools -- "$NEW_COMMIT"
+rustup-toolchain-install-master -n miri -c cargo -c rust-src -c rustc-dev -c llvm-tools "$@" -- "$NEW_COMMIT"
 rustup override set miri
 
 # Cleanup.

src/lib.rs

@@ -7,19 +7,16 @@
 #![feature(io_error_more)]
 #![warn(rust_2018_idioms)]
 #![allow(
-    clippy::cast_lossless,
     clippy::collapsible_else_if,
     clippy::collapsible_if,
     clippy::comparison_chain,
     clippy::enum_variant_names,
     clippy::field_reassign_with_default,
-    clippy::from_over_into,
-    clippy::if_same_then_else,
     clippy::manual_map,
-    clippy::needless_lifetimes,
     clippy::new_without_default,
     clippy::single_match,
-    clippy::useless_format
+    clippy::useless_format,
+    clippy::derive_partial_eq_without_eq
 )]
 
 extern crate rustc_apfloat;

src/machine.rs

@@ -89,10 +89,10 @@ pub enum MiriMemoryKind {
     Tls,
 }
 
-impl Into<MemoryKind<MiriMemoryKind>> for MiriMemoryKind {
+impl From<MiriMemoryKind> for MemoryKind<MiriMemoryKind> {
     #[inline(always)]
-    fn into(self) -> MemoryKind<MiriMemoryKind> {
+    fn from(kind: MiriMemoryKind) -> MemoryKind<MiriMemoryKind> {
-        MemoryKind::Machine(self)
+        MemoryKind::Machine(kind)
     }
 }
 

src/shims/intrinsics.rs

@@ -1389,7 +1389,7 @@ fn bool_to_simd_element(b: bool, size: Size) -> Scalar<Tag> {
     Scalar::from_int(val, size)
 }
 
-fn simd_element_to_bool<'tcx>(elem: ImmTy<'tcx, Tag>) -> InterpResult<'tcx, bool> {
+fn simd_element_to_bool(elem: ImmTy<'_, Tag>) -> InterpResult<'_, bool> {
     let val = elem.to_scalar()?.to_int(elem.layout.size)?;
     Ok(match val {
         0 => false,

src/shims/posix/sync.rs

@@ -535,9 +535,9 @@ fn pthread_mutex_unlock(&mut self, mutex_op: &OpTy<'tcx, Tag>) -> InterpResult<'
                 throw_ub_format!(
                     "unlocked a PTHREAD_MUTEX_NORMAL mutex that was not locked by the current thread"
                 );
-            } else if kind == this.eval_libc("PTHREAD_MUTEX_ERRORCHECK")? {
+            } else if kind == this.eval_libc("PTHREAD_MUTEX_ERRORCHECK")?
-                this.eval_libc_i32("EPERM")
+                || kind == this.eval_libc("PTHREAD_MUTEX_RECURSIVE")?
-            } else if kind == this.eval_libc("PTHREAD_MUTEX_RECURSIVE")? {
+            {
                 this.eval_libc_i32("EPERM")
             } else {
                 throw_unsup_format!("called pthread_mutex_unlock on an unsupported type of mutex");
@@ -642,6 +642,7 @@ fn pthread_rwlock_unlock(&mut self, rwlock_op: &OpTy<'tcx, Tag>) -> InterpResult
         let id = rwlock_get_or_create_id(this, rwlock_op)?;
         let active_thread = this.get_active_thread();
 
+        #[allow(clippy::if_same_then_else)]
         if this.rwlock_reader_unlock(id, active_thread) {
             Ok(0)
         } else if this.rwlock_writer_unlock(id, active_thread) {