From ccd2540bf1adfeafb3303124c9a57f05dadb67b8 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 9 Jun 2023 08:11:44 -0500 Subject: [PATCH 1/2] chore(ci): Ensure bors is blocked on clippy --- .github/workflows/main.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 282c50462c3..3deae63555f 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -218,6 +218,7 @@ jobs: name: bors build finished needs: - build_std + - clippy - docs - lockfile - resolver @@ -234,6 +235,7 @@ jobs: name: bors build finished needs: - build_std + - clippy - docs - lockfile - resolver From 9df5e79dfb3e2f01d48f1156abfa6f2e5227f06d Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 8 Jun 2023 21:39:43 -0500 Subject: [PATCH 2/2] chore: Migrate print-ban from test to clippy This should be more resilient to false positives like in #12245 where a string contains `println`. --- clippy.toml | 2 + src/cargo/lib.rs | 3 ++ tests/internal.rs | 107 ---------------------------------------------- 3 files changed, 5 insertions(+), 107 deletions(-) delete mode 100644 tests/internal.rs diff --git a/clippy.toml b/clippy.toml index 4f9be8f9b08..050cc871638 100644 --- a/clippy.toml +++ b/clippy.toml @@ -1,3 +1,5 @@ +allow-print-in-tests = true +allow-dbg-in-tests = true disallowed-methods = [ { path = "std::env::var", reason = "Use `Config::get_env` instead. See rust-lang/cargo#11588" }, { path = "std::env::var_os", reason = "Use `Config::get_env_os` instead. See rust-lang/cargo#11588" }, diff --git a/src/cargo/lib.rs b/src/cargo/lib.rs index 31d03ad259c..a03d5119978 100644 --- a/src/cargo/lib.rs +++ b/src/cargo/lib.rs @@ -6,6 +6,9 @@ #![allow(clippy::all)] #![warn(clippy::disallowed_methods)] #![warn(clippy::self_named_module_files)] +#![warn(clippy::print_stdout)] +#![warn(clippy::print_stderr)] +#![warn(clippy::dbg_macro)] #![allow(rustdoc::private_intra_doc_links)] //! # Cargo as a library diff --git a/tests/internal.rs b/tests/internal.rs deleted file mode 100644 index c42cfa8f07b..00000000000 --- a/tests/internal.rs +++ /dev/null @@ -1,107 +0,0 @@ -//! Tests for internal code checks. - -#![allow(clippy::all)] - -use std::fs; - -#[test] -fn check_forbidden_code() { - // Do not use certain macros, functions, etc. - if !cargo_util::is_ci() { - // Only check these on CI, otherwise it could be annoying. - use std::io::Write; - writeln!( - std::io::stderr(), - "\nSkipping check_forbidden_code test, set CI=1 to enable" - ) - .unwrap(); - return; - } - let root_path = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("src"); - for entry in walkdir::WalkDir::new(&root_path) - .into_iter() - .filter_entry(|e| e.path() != root_path.join("doc")) - .filter_map(|e| e.ok()) - { - let path = entry.path(); - if !entry - .file_name() - .to_str() - .map(|s| s.ends_with(".rs")) - .unwrap_or(false) - { - continue; - } - eprintln!("checking {}", path.display()); - let c = fs::read_to_string(path).unwrap(); - for (line_index, line) in c.lines().enumerate() { - if line.trim().starts_with("//") { - continue; - } - if line_has_print(line) { - if entry.file_name().to_str().unwrap() == "cargo_new.rs" && line.contains("Hello") { - // An exception. - continue; - } - panic!( - "found print macro in {}:{}\n\n{}\n\n\ - print! macros should not be used in Cargo because they can panic.\n\ - Use one of the drop_print macros instead.\n\ - ", - path.display(), - line_index, - line - ); - } - if line_has_macro(line, "dbg") { - panic!( - "found dbg! macro in {}:{}\n\n{}\n\n\ - dbg! should not be used outside of debugging.", - path.display(), - line_index, - line - ); - } - } - } -} - -fn line_has_print(line: &str) -> bool { - line_has_macro(line, "print") - || line_has_macro(line, "eprint") - || line_has_macro(line, "println") - || line_has_macro(line, "eprintln") -} - -#[test] -fn line_has_print_works() { - assert!(line_has_print("print!")); - assert!(line_has_print("println!")); - assert!(line_has_print("eprint!")); - assert!(line_has_print("eprintln!")); - assert!(line_has_print("(print!(\"hi!\"))")); - assert!(!line_has_print("print")); - assert!(!line_has_print("i like to print things")); - assert!(!line_has_print("drop_print!")); - assert!(!line_has_print("drop_println!")); - assert!(!line_has_print("drop_eprint!")); - assert!(!line_has_print("drop_eprintln!")); -} - -fn line_has_macro(line: &str, mac: &str) -> bool { - for (i, _) in line.match_indices(mac) { - if line.get(i + mac.len()..i + mac.len() + 1) != Some("!") { - continue; - } - if i == 0 { - return true; - } - // Check for identifier boundary start. - let prev1 = line.get(i - 1..i).unwrap().chars().next().unwrap(); - if prev1.is_alphanumeric() || prev1 == '_' { - continue; - } - return true; - } - false -}