Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dogfood and CI fixes #6849

Merged
merged 6 commits into from
Mar 5, 2021
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions .github/workflows/clippy.yml
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ env:

jobs:
base:
# NOTE: If you modify this job, make sure you copy the changes to clippy_bors.yml
runs-on: ubuntu-latest

steps:
Expand All @@ -50,9 +51,6 @@ jobs:
- name: Build
run: cargo build --features deny-warnings,internal-lints

- name: Test "--fix -Zunstable-options"
run: cargo run --features deny-warnings,internal-lints --bin cargo-clippy -- clippy --fix -Zunstable-options

- name: Test
run: cargo test --features deny-warnings,internal-lints

Expand All @@ -72,6 +70,10 @@ jobs:
run: ../target/debug/cargo-clippy
working-directory: clippy_workspace_tests

- name: Test cargo-clippy --fix
run: ../target/debug/cargo-clippy clippy --fix -Zunstable-options
working-directory: clippy_workspace_tests

- name: Test clippy-driver
run: bash .github/driver.sh
env:
Expand Down
12 changes: 12 additions & 0 deletions .github/workflows/clippy_bors.yml
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ jobs:

runs-on: ${{ matrix.os }}

# NOTE: If you modify this job, make sure you copy the changes to clippy.yml
steps:
# Setup
- uses: rust-lang/simpleinfra/github-actions/cancel-outdated-builds@master
Expand Down Expand Up @@ -131,11 +132,22 @@ jobs:
run: ../target/debug/cargo-clippy
working-directory: clippy_workspace_tests

- name: Test cargo-clippy --fix
run: ../target/debug/cargo-clippy clippy --fix -Zunstable-options
working-directory: clippy_workspace_tests

- name: Test clippy-driver
run: bash .github/driver.sh
env:
OS: ${{ runner.os }}

- name: Test cargo dev new lint
run: |
cargo dev new_lint --name new_early_pass --pass early
cargo dev new_lint --name new_late_pass --pass late
cargo check
git reset --hard HEAD

integration_build:
needs: changelog
runs-on: ubuntu-latest
Expand Down
2 changes: 1 addition & 1 deletion clippy_dev/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -530,7 +530,7 @@ fn test_gen_deprecated() {
#[should_panic]
fn test_gen_deprecated_fail() {
let lints = vec![Lint::new("should_assert_eq2", "group2", "abc", None, "module_name")];
let _ = gen_deprecated(lints.iter());
let _deprecated_lints = gen_deprecated(lints.iter());
}

#[test]
Expand Down
68 changes: 40 additions & 28 deletions clippy_dev/src/lintcheck.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,12 @@

#![cfg(feature = "lintcheck")]
#![allow(clippy::filter_map, clippy::collapsible_else_if)]
#![allow(clippy::blocks_in_if_conditions)] // FP on `if x.iter().any(|x| ...)`

use crate::clippy_project_root;

use std::collections::HashMap;
use std::process::Command;
use std::sync::atomic::{AtomicUsize, Ordering};
use std::{collections::HashMap, io::ErrorKind};
use std::{
env, fmt,
fs::write,
Expand Down Expand Up @@ -116,9 +115,21 @@ impl CrateSource {
// url to download the crate from crates.io
let url = format!("https://crates.io/api/v1/crates/{}/{}/download", name, version);
println!("Downloading and extracting {} {} from {}", name, version, url);
let _ = std::fs::create_dir("target/lintcheck/");
let _ = std::fs::create_dir(&krate_download_dir);
let _ = std::fs::create_dir(&extract_dir);
std::fs::create_dir("target/lintcheck/").unwrap_or_else(|err| {
if err.kind() != ErrorKind::AlreadyExists {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it to only allow the dir to exist. I think panicking for other things should be fine?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, seems fine. :)

panic!("cannot create lintcheck target dir");
}
});
std::fs::create_dir(&krate_download_dir).unwrap_or_else(|err| {
if err.kind() != ErrorKind::AlreadyExists {
panic!("cannot create crate download dir");
}
});
std::fs::create_dir(&extract_dir).unwrap_or_else(|err| {
if err.kind() != ErrorKind::AlreadyExists {
panic!("cannot create crate extraction dir");
}
});

let krate_file_path = krate_download_dir.join(format!("{}-{}.crate.tar.gz", name, version));
// don't download/extract if we already have done so
Expand Down Expand Up @@ -198,18 +209,18 @@ impl CrateSource {
// the source path of the crate we copied, ${copy_dest}/crate_name
let crate_root = copy_dest.join(name); // .../crates/local_crate

if !crate_root.exists() {
println!("Copying {} to {}", path.display(), copy_dest.display());

dir::copy(path, &copy_dest, &dir::CopyOptions::new()).unwrap_or_else(|_| {
panic!("Failed to copy from {}, to {}", path.display(), crate_root.display())
});
} else {
if crate_root.exists() {
println!(
"Not copying {} to {}, destination already exists",
path.display(),
crate_root.display()
);
} else {
println!("Copying {} to {}", path.display(), copy_dest.display());

dir::copy(path, &copy_dest, &dir::CopyOptions::new()).unwrap_or_else(|_| {
panic!("Failed to copy from {}, to {}", path.display(), crate_root.display())
});
}

Crate {
Expand All @@ -236,8 +247,8 @@ impl Crate {
// advance the atomic index by one
let index = target_dir_index.fetch_add(1, Ordering::SeqCst);
// "loop" the index within 0..thread_limit
let target_dir_index = index % thread_limit;
let perc = ((index * 100) as f32 / total_crates_to_lint as f32) as u8;
let thread_index = index % thread_limit;
let perc = (index * 100) / total_crates_to_lint;

if thread_limit == 1 {
println!(
Expand All @@ -247,7 +258,7 @@ impl Crate {
} else {
println!(
"{}/{} {}% Linting {} {} in target dir {:?}",
index, total_crates_to_lint, perc, &self.name, &self.version, target_dir_index
index, total_crates_to_lint, perc, &self.name, &self.version, thread_index
);
}

Expand All @@ -269,7 +280,7 @@ impl Crate {
// use the looping index to create individual target dirs
.env(
"CARGO_TARGET_DIR",
shared_target_dir.join(format!("_{:?}", target_dir_index)),
shared_target_dir.join(format!("_{:?}", thread_index)),
)
// lint warnings will look like this:
// src/cargo/ops/cargo_compile.rs:127:35: warning: usage of `FromIterator::from_iter`
Expand Down Expand Up @@ -529,6 +540,10 @@ fn lintcheck_needs_rerun(lintcheck_logs_path: &Path) -> bool {
}

/// lintchecks `main()` function
///
/// # Panics
///
/// This function panics if the clippy binaries don't exist.
pub fn run(clap_config: &ArgMatches) {
let config = LintcheckConfig::from_clap(clap_config);

Expand Down Expand Up @@ -579,9 +594,9 @@ pub fn run(clap_config: &ArgMatches) {
// if we don't have the specified crate in the .toml, throw an error
if !crates.iter().any(|krate| {
let name = match krate {
CrateSource::CratesIo { name, .. } => name,
CrateSource::Git { name, .. } => name,
CrateSource::Path { name, .. } => name,
CrateSource::CratesIo { name, .. } | CrateSource::Git { name, .. } | CrateSource::Path { name, .. } => {
name
},
};
Comment on lines 582 to 586
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's super neat, I'll have to remember that! :)

Copy link
Member Author

@flip1995 flip1995 Mar 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an auto-fixable Clippy lint 😉

name == only_one_crate
}) {
Expand All @@ -597,8 +612,7 @@ pub fn run(clap_config: &ArgMatches) {
.into_iter()
.map(|krate| krate.download_and_extract())
.filter(|krate| krate.name == only_one_crate)
.map(|krate| krate.run_clippy_lints(&cargo_clippy_path, &AtomicUsize::new(0), 1, 1))
.flatten()
.flat_map(|krate| krate.run_clippy_lints(&cargo_clippy_path, &AtomicUsize::new(0), 1, 1))
.collect()
} else {
if config.max_jobs > 1 {
Expand All @@ -621,17 +635,15 @@ pub fn run(clap_config: &ArgMatches) {
crates
.into_par_iter()
.map(|krate| krate.download_and_extract())
.map(|krate| krate.run_clippy_lints(&cargo_clippy_path, &counter, num_cpus, num_crates))
.flatten()
.flat_map(|krate| krate.run_clippy_lints(&cargo_clippy_path, &counter, num_cpus, num_crates))
.collect()
} else {
// run sequential
let num_crates = crates.len();
crates
.into_iter()
.map(|krate| krate.download_and_extract())
.map(|krate| krate.run_clippy_lints(&cargo_clippy_path, &counter, 1, num_crates))
.flatten()
.flat_map(|krate| krate.run_clippy_lints(&cargo_clippy_path, &counter, 1, num_crates))
.collect()
}
};
Expand All @@ -646,7 +658,7 @@ pub fn run(clap_config: &ArgMatches) {
.map(|w| (&w.crate_name, &w.message))
.collect();

let mut all_msgs: Vec<String> = clippy_warnings.iter().map(|warning| warning.to_string()).collect();
let mut all_msgs: Vec<String> = clippy_warnings.iter().map(ToString::to_string).collect();
all_msgs.sort();
all_msgs.push("\n\n\n\nStats:\n".into());
all_msgs.push(stats_formatted);
Expand All @@ -673,13 +685,13 @@ fn read_stats_from_file(file_path: &Path) -> HashMap<String, usize> {
},
};

let lines: Vec<String> = file_content.lines().map(|l| l.to_string()).collect();
let lines: Vec<String> = file_content.lines().map(ToString::to_string).collect();

// search for the beginning "Stats:" and the end "ICEs:" of the section we want
let start = lines.iter().position(|line| line == "Stats:").unwrap();
let end = lines.iter().position(|line| line == "ICEs:").unwrap();

let stats_lines = &lines[start + 1..=end - 1];
let stats_lines = &lines[start + 1..end];

stats_lines
.iter()
Expand Down
42 changes: 20 additions & 22 deletions clippy_lints/src/transmute/transmute_int_to_char.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,28 +17,26 @@ pub(super) fn check<'tcx>(
) -> bool {
match (&from_ty.kind(), &to_ty.kind()) {
(ty::Int(ty::IntTy::I32) | ty::Uint(ty::UintTy::U32), &ty::Char) => {
{
span_lint_and_then(
cx,
TRANSMUTE_INT_TO_CHAR,
e.span,
&format!("transmute from a `{}` to a `char`", from_ty),
|diag| {
let arg = sugg::Sugg::hir(cx, &args[0], "..");
let arg = if let ty::Int(_) = from_ty.kind() {
arg.as_ty(ast::UintTy::U32.name_str())
} else {
arg
};
diag.span_suggestion(
e.span,
"consider using",
format!("std::char::from_u32({}).unwrap()", arg.to_string()),
Applicability::Unspecified,
);
},
)
};
span_lint_and_then(
cx,
TRANSMUTE_INT_TO_CHAR,
e.span,
&format!("transmute from a `{}` to a `char`", from_ty),
|diag| {
let arg = sugg::Sugg::hir(cx, &args[0], "..");
let arg = if let ty::Int(_) = from_ty.kind() {
arg.as_ty(ast::UintTy::U32.name_str())
} else {
arg
};
diag.span_suggestion(
e.span,
"consider using",
format!("std::char::from_u32({}).unwrap()", arg.to_string()),
Applicability::Unspecified,
);
},
);
true
},
_ => false,
Expand Down
4 changes: 2 additions & 2 deletions clippy_lints/src/transmute/transmute_ptr_to_ref.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ pub(super) fn check<'tcx>(
qpath: &'tcx QPath<'_>,
) -> bool {
match (&from_ty.kind(), &to_ty.kind()) {
(ty::RawPtr(from_pty), ty::Ref(_, to_ref_ty, mutbl)) => {
(ty::RawPtr(from_ptr_ty), ty::Ref(_, to_ref_ty, mutbl)) => {
span_lint_and_then(
cx,
TRANSMUTE_PTR_TO_REF,
Expand All @@ -34,7 +34,7 @@ pub(super) fn check<'tcx>(
("&*", "*const")
};

let arg = if from_pty.ty == *to_ref_ty {
let arg = if from_ptr_ty.ty == *to_ref_ty {
arg
} else {
arg.as_ty(&format!("{} {}", cast, get_type_snippet(cx, qpath, to_ref_ty)))
Expand Down
Loading