From bd7f83dc3773ee1a0ce6ebb5bf7c3a8cd04bb968 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 4 Jun 2022 11:48:46 -0400 Subject: [PATCH 1/3] run clippy on CI --- .github/workflows/ci.yml | 55 ++++++++++++++++++++-------------------- rustup-toolchain | 5 +++- 2 files changed, 31 insertions(+), 29 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f34e92571ff..41cf159e0c8 100644 --- a/.github/workflows/ci.yml +++ b/.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 - with: - toolchain: nightly - components: rustfmt - default: true - - name: Check formatting (miri) - uses: actions-rs/cargo@v1 - with: - command: fmt - args: --all -- --check - - name: Check formatting (cargo-miri) - uses: actions-rs/cargo@v1 - with: - command: fmt - args: --manifest-path cargo-miri/Cargo.toml --all -- --check + run: | + rustup toolchain install nightly --component rustfmt + rustup override set nightly + - name: Formatting (miri, ui_test) + run: cargo fmt --all --check + - name: Formatting (cargo-miri) + run: cargo fmt --manifest-path cargo-miri/Cargo.toml --all --check + + clippy: + name: clippy (ignored by bors) + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + - name: Install required toolchain + # 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 diff --git a/rustup-toolchain b/rustup-toolchain index 59ce6f85a08..7e5d57349b9 100755 --- a/rustup-toolchain +++ b/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. From 151b6b13e0766c23833038db0a5b99a09562af53 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 4 Jun 2022 11:55:36 -0400 Subject: [PATCH 2/3] clippy: main crate --- benches/helpers/fibonacci_helper_iterative.rs | 2 +- src/lib.rs | 7 ++----- src/machine.rs | 6 +++--- src/shims/intrinsics.rs | 2 +- src/shims/posix/sync.rs | 7 ++++--- 5 files changed, 11 insertions(+), 13 deletions(-) diff --git a/benches/helpers/fibonacci_helper_iterative.rs b/benches/helpers/fibonacci_helper_iterative.rs index 59283be4820..0c273282896 100644 --- a/benches/helpers/fibonacci_helper_iterative.rs +++ b/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 } diff --git a/src/lib.rs b/src/lib.rs index e571c8a0010..f7c256656a7 100644 --- a/src/lib.rs +++ b/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; diff --git a/src/machine.rs b/src/machine.rs index 9a358bb6d53..6a7cbe58711 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -89,10 +89,10 @@ pub enum MiriMemoryKind { Tls, } -impl Into> for MiriMemoryKind { +impl From for MemoryKind { #[inline(always)] - fn into(self) -> MemoryKind { - MemoryKind::Machine(self) + fn from(kind: MiriMemoryKind) -> MemoryKind { + MemoryKind::Machine(kind) } } diff --git a/src/shims/intrinsics.rs b/src/shims/intrinsics.rs index 1f06971a3e7..8d4da31fd0d 100644 --- a/src/shims/intrinsics.rs +++ b/src/shims/intrinsics.rs @@ -1389,7 +1389,7 @@ fn bool_to_simd_element(b: bool, size: Size) -> Scalar { 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, diff --git a/src/shims/posix/sync.rs b/src/shims/posix/sync.rs index f56a309bf07..373996312ea 100644 --- a/src/shims/posix/sync.rs +++ b/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")? { - this.eval_libc_i32("EPERM") - } else if kind == this.eval_libc("PTHREAD_MUTEX_RECURSIVE")? { + } else if kind == this.eval_libc("PTHREAD_MUTEX_ERRORCHECK")? + || 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) { From 3d30aece836c5f524b49eb308ee516a09dd9e0a9 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 4 Jun 2022 12:11:23 -0400 Subject: [PATCH 3/3] clippy: cargo-miri --- cargo-miri/bin.rs | 49 ++++++++++++++++++++++++----------------------- 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/cargo-miri/bin.rs b/cargo-miri/bin.rs index 34904279e94..ba885d307a8 100644 --- a/cargo-miri/bin.rs +++ b/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> Iterator for ArgSplitFlagValue<'_, I> { fn next(&mut self) -> Option { 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[1..].to_owned())); + return Some(Ok(suffix.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 { - cmd.arg(config_flag).arg(config); - } + ) + // Only look at `Ok` + .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 = name.replace("-", "_"); + let name = member.split(' ').next().unwrap(); + 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()..]; - assert!(val.starts_with("="), "`cargo` should pass `--emit=X` as one argument"); - let val = &val[1..]; + let val = + val.strip_prefix('=').expect("`cargo` should pass `--emit=X` as one argument"); 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) { - let suffix = &arg[error_format_flag.len()..]; + } else if let Some(suffix) = arg.strip_prefix(error_format_flag) { assert!(suffix.starts_with('=')); // Drop this argument. - } else if arg.starts_with(json_flag) { - let suffix = &arg[json_flag.len()..]; + } else if let Some(suffix) = arg.strip_prefix(json_flag) { assert!(suffix.starts_with('=')); // Drop this argument. } else {