From f7a201c399e53992301b241a098820d3c79ced60 Mon Sep 17 00:00:00 2001 From: Ed Morley <501702+edmorley@users.noreply.github.com> Date: Sun, 19 Nov 2023 13:58:29 +0000 Subject: [PATCH] Enable additional rustc and Clippy lints Enables some off-by-default lints that we already use in the `libcnb.rs` repository. https://doc.rust-lang.org/rustc/lints/listing/allowed-by-default.html#unreachable-pub https://doc.rust-lang.org/rustc/lints/listing/allowed-by-default.html#unsafe-code https://rust-lang.github.io/rust-clippy/master/index.html#/panic_in_result_fn https://rust-lang.github.io/rust-clippy/master/index.html#/unwrap_used GUS-W-14523799. --- Cargo.toml | 4 ++++ buildpacks/ruby/src/gem_list.rs | 9 ++++++--- .../ruby/src/layers/bundle_download_layer.rs | 8 ++++---- .../ruby/src/layers/bundle_install_layer.rs | 16 ++++++++-------- .../ruby/src/layers/metrics_agent_install.rs | 2 +- buildpacks/ruby/src/layers/ruby_install_layer.rs | 8 ++++---- buildpacks/ruby/src/main.rs | 6 +++--- buildpacks/ruby/src/rake_status.rs | 4 ++-- buildpacks/ruby/src/rake_task_detect.rs | 10 +++++++--- buildpacks/ruby/tests/integration_test.rs | 2 ++ clippy.toml | 1 + commons/bin/print_style_guide.rs | 6 ++++++ commons/src/cache/clean.rs | 4 ++-- commons/src/cache/in_app_dir_cache_layer.rs | 4 ++-- commons/src/output/background_timer.rs | 2 ++ commons/src/output/util.rs | 2 +- 16 files changed, 55 insertions(+), 33 deletions(-) create mode 100644 clippy.toml diff --git a/Cargo.toml b/Cargo.toml index 2fecfbc4..8412867b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -7,8 +7,12 @@ edition = "2021" rust-version = "1.74" [workspace.lints.rust] +unreachable_pub = "warn" +unsafe_code = "warn" unused_crate_dependencies = "warn" [workspace.lints.clippy] +panic_in_result_fn = "warn" pedantic = "warn" +unwrap_used = "warn" module_name_repetitions = "allow" diff --git a/buildpacks/ruby/src/gem_list.rs b/buildpacks/ruby/src/gem_list.rs index 12d3248f..03fed8fd 100644 --- a/buildpacks/ruby/src/gem_list.rs +++ b/buildpacks/ruby/src/gem_list.rs @@ -14,8 +14,8 @@ use std::process::Command; /// /// Requires `ruby` and `bundle` to be installed and on the PATH #[derive(Debug)] -pub struct GemList { - pub gems: HashMap, +pub(crate) struct GemList { + pub(crate) gems: HashMap, } /// Converts the output of `$ gem list` into a data structure that can be inspected and compared @@ -59,7 +59,10 @@ impl GemList { /// # Errors /// /// Errors if the command `bundle list` is unsuccessful. - pub fn from_bundle_list(envs: T, _logger: &dyn SectionLogger) -> Result + pub(crate) fn from_bundle_list( + envs: T, + _logger: &dyn SectionLogger, + ) -> Result where T: IntoIterator, K: AsRef, diff --git a/buildpacks/ruby/src/layers/bundle_download_layer.rs b/buildpacks/ruby/src/layers/bundle_download_layer.rs index e874f2a8..7917c4f8 100644 --- a/buildpacks/ruby/src/layers/bundle_download_layer.rs +++ b/buildpacks/ruby/src/layers/bundle_download_layer.rs @@ -18,7 +18,7 @@ use std::process::Command; #[derive(Deserialize, Serialize, Debug, Clone)] pub(crate) struct BundleDownloadLayerMetadata { - pub version: ResolvedBundlerVersion, + pub(crate) version: ResolvedBundlerVersion, } /// # Install the bundler gem @@ -28,9 +28,9 @@ pub(crate) struct BundleDownloadLayerMetadata { /// Installs a copy of `bundler` to the `` with a bundler executable in /// `/bin`. Must run before [`crate.steps.bundle_install`]. pub(crate) struct BundleDownloadLayer<'a> { - pub env: Env, - pub metadata: BundleDownloadLayerMetadata, - pub _section_logger: &'a dyn SectionLogger, + pub(crate) env: Env, + pub(crate) metadata: BundleDownloadLayerMetadata, + pub(crate) _section_logger: &'a dyn SectionLogger, } impl<'a> Layer for BundleDownloadLayer<'a> { diff --git a/buildpacks/ruby/src/layers/bundle_install_layer.rs b/buildpacks/ruby/src/layers/bundle_install_layer.rs index 9c1da4fe..cdb73818 100644 --- a/buildpacks/ruby/src/layers/bundle_install_layer.rs +++ b/buildpacks/ruby/src/layers/bundle_install_layer.rs @@ -31,17 +31,17 @@ pub(crate) const FORCE_BUNDLE_INSTALL_CACHE_KEY: &str = "v1"; /// `BundleInstallLayer::create` are the same. #[derive(Debug)] pub(crate) struct BundleInstallLayer<'a> { - pub env: Env, - pub without: BundleWithout, - pub _section_log: &'a dyn SectionLogger, - pub metadata: BundleInstallLayerMetadata, + pub(crate) env: Env, + pub(crate) without: BundleWithout, + pub(crate) _section_log: &'a dyn SectionLogger, + pub(crate) metadata: BundleInstallLayerMetadata, } #[derive(Deserialize, Serialize, Debug, Clone, Eq, PartialEq)] pub(crate) struct BundleInstallLayerMetadata { - pub stack: StackId, - pub ruby_version: ResolvedRubyVersion, - pub force_bundle_install_key: String, + pub(crate) stack: StackId, + pub(crate) ruby_version: ResolvedRubyVersion, + pub(crate) force_bundle_install_key: String, /// A struct that holds the cryptographic hash of components that can /// affect the result of `bundle install`. When these values do not @@ -54,7 +54,7 @@ pub(crate) struct BundleInstallLayerMetadata { /// This value is cached with metadata, so changing the struct /// may cause metadata to be invalidated (and the cache cleared). /// - pub digest: MetadataDigest, // Must be last for serde to be happy https://github.com/toml-rs/toml-rs/issues/142 + pub(crate) digest: MetadataDigest, // Must be last for serde to be happy https://github.com/toml-rs/toml-rs/issues/142 } impl<'a> BundleInstallLayer<'a> { diff --git a/buildpacks/ruby/src/layers/metrics_agent_install.rs b/buildpacks/ruby/src/layers/metrics_agent_install.rs index d1400c3f..1c65e72a 100644 --- a/buildpacks/ruby/src/layers/metrics_agent_install.rs +++ b/buildpacks/ruby/src/layers/metrics_agent_install.rs @@ -31,7 +31,7 @@ const DOWNLOAD_SHA: &str = "f9bf9f33c949e15ffed77046ca38f8dae9307b6a0181c6af29a2 #[derive(Debug)] pub(crate) struct MetricsAgentInstall<'a> { - pub _in_section: &'a dyn SectionLogger, // force the layer to be called within a Section logging context, not necessary but it's safer + pub(crate) _in_section: &'a dyn SectionLogger, // force the layer to be called within a Section logging context, not necessary but it's safer } #[derive(Deserialize, Serialize, Debug, Clone)] diff --git a/buildpacks/ruby/src/layers/ruby_install_layer.rs b/buildpacks/ruby/src/layers/ruby_install_layer.rs index d34ac6d1..35f0238d 100644 --- a/buildpacks/ruby/src/layers/ruby_install_layer.rs +++ b/buildpacks/ruby/src/layers/ruby_install_layer.rs @@ -31,14 +31,14 @@ use url::Url; /// When the Ruby version changes, invalidate and re-run. /// pub(crate) struct RubyInstallLayer<'a> { - pub _in_section: &'a dyn SectionLogger, // force the layer to be called within a Section logging context, not necessary but it's safer - pub metadata: RubyInstallLayerMetadata, + pub(crate) _in_section: &'a dyn SectionLogger, // force the layer to be called within a Section logging context, not necessary but it's safer + pub(crate) metadata: RubyInstallLayerMetadata, } #[derive(Deserialize, Serialize, Debug, Clone)] pub(crate) struct RubyInstallLayerMetadata { - pub stack: StackId, - pub version: ResolvedRubyVersion, + pub(crate) stack: StackId, + pub(crate) version: ResolvedRubyVersion, } impl<'a> Layer for RubyInstallLayer<'a> { diff --git a/buildpacks/ruby/src/main.rs b/buildpacks/ruby/src/main.rs index 2424d89a..33e73ae8 100644 --- a/buildpacks/ruby/src/main.rs +++ b/buildpacks/ruby/src/main.rs @@ -35,7 +35,7 @@ use libcnb_test as _; use clap as _; -pub(crate) struct RubyBuildpack; +struct RubyBuildpack; impl Buildpack for RubyBuildpack { type Platform = GenericPlatform; @@ -238,7 +238,7 @@ fn needs_java(gemfile_lock: &str) -> bool { } #[derive(Debug)] -pub(crate) enum RubyBuildpackError { +enum RubyBuildpackError { RakeDetectError(CmdError), GemListGetError(CmdError), RubyInstallError(RubyInstallError), @@ -260,7 +260,7 @@ impl From for libcnb::Error { buildpack_main!(RubyBuildpack); #[derive(serde::Deserialize, serde::Serialize, Debug, Clone, PartialEq, Eq)] -pub(crate) struct BundleWithout(String); +struct BundleWithout(String); impl BundleWithout { fn new(without: impl AsRef) -> Self { diff --git a/buildpacks/ruby/src/rake_status.rs b/buildpacks/ruby/src/rake_status.rs index a1c6d46b..ae42db11 100644 --- a/buildpacks/ruby/src/rake_status.rs +++ b/buildpacks/ruby/src/rake_status.rs @@ -2,7 +2,7 @@ use crate::gem_list::GemList; use std::path::{Path, PathBuf}; /// Determine if an application is ready to run a rake task or not -pub fn check_rake_ready( +pub(crate) fn check_rake_ready( app_path: &Path, gem_list: &GemList, globs: impl IntoIterator>, @@ -15,7 +15,7 @@ pub fn check_rake_ready( } #[derive(Debug, Eq, PartialEq)] -pub enum RakeStatus { +pub(crate) enum RakeStatus { Ready(PathBuf), MissingRakeGem, MissingRakefile, diff --git a/buildpacks/ruby/src/rake_task_detect.rs b/buildpacks/ruby/src/rake_task_detect.rs index a094e90a..cfdedc65 100644 --- a/buildpacks/ruby/src/rake_task_detect.rs +++ b/buildpacks/ruby/src/rake_task_detect.rs @@ -17,7 +17,7 @@ use std::{ffi::OsStr, process::Command}; /// assert!(!rake_detect.has_task("assets:precompile")); /// ``` #[derive(Default)] -pub struct RakeDetect { +pub(crate) struct RakeDetect { output: String, } @@ -25,7 +25,11 @@ impl RakeDetect { /// # Errors /// /// Will return `Err` if `bundle exec rake -p` command cannot be invoked by the operating system. - pub fn from_rake_command, K: AsRef, V: AsRef>( + pub(crate) fn from_rake_command< + T: IntoIterator, + K: AsRef, + V: AsRef, + >( _logger: &dyn SectionLogger, envs: T, error_on_failure: bool, @@ -53,7 +57,7 @@ impl RakeDetect { } #[must_use] - pub fn has_task(&self, string: &str) -> bool { + pub(crate) fn has_task(&self, string: &str) -> bool { let task_re = regex::Regex::new(&format!("\\s{string}")).expect("clippy"); task_re.is_match(&self.output) } diff --git a/buildpacks/ruby/tests/integration_test.rs b/buildpacks/ruby/tests/integration_test.rs index 7d60cc5c..61decac6 100644 --- a/buildpacks/ruby/tests/integration_test.rs +++ b/buildpacks/ruby/tests/integration_test.rs @@ -1,5 +1,7 @@ // Required due to: https://github.com/rust-lang/rust/issues/95513 #![allow(unused_crate_dependencies)] +// Required due to: https://github.com/rust-lang/rust-clippy/issues/11119 +#![allow(clippy::unwrap_used)] use libcnb_test::{ assert_contains, assert_empty, BuildConfig, BuildpackReference, ContainerConfig, diff --git a/clippy.toml b/clippy.toml new file mode 100644 index 00000000..154626ef --- /dev/null +++ b/clippy.toml @@ -0,0 +1 @@ +allow-unwrap-in-tests = true diff --git a/commons/bin/print_style_guide.rs b/commons/bin/print_style_guide.rs index bce8c0fb..b180b350 100644 --- a/commons/bin/print_style_guide.rs +++ b/commons/bin/print_style_guide.rs @@ -63,6 +63,8 @@ fn main() { .step("Output can be streamed. Mostly from commands. Example:") .step_timed_stream(&format!("Running {}", fmt::command(command.name()))); + // TODO: Remove usage of unwrap(): https://github.com/heroku/buildpacks-ruby/issues/238 + #[allow(clippy::unwrap_used)] command.stream_output(stream.io(), stream.io()).unwrap(); log = stream.finish_timed_stream().end_section(); drop(log); @@ -100,6 +102,8 @@ fn main() { // do work here }); log_step_stream("log_step_stream()", |stream| { + // TODO: Remove usage of unwrap(): https://github.com/heroku/buildpacks-ruby/issues/238 + #[allow(clippy::unwrap_used)] Command::new("bash") .args(["-c", "ps aux | grep cargo"]) .stream_output(stream.io(), stream.io()) @@ -113,6 +117,8 @@ fn main() { } { + // TODO: Remove usage of unwrap(): https://github.com/heroku/buildpacks-ruby/issues/238 + #[allow(clippy::unwrap_used)] let cmd_error = Command::new("iDoNotExist").named_output().err().unwrap(); let mut log = BuildLog::new(stdout()).buildpack_name("Error and warnings"); diff --git a/commons/src/cache/clean.rs b/commons/src/cache/clean.rs index bbd4cd60..0891b00d 100644 --- a/commons/src/cache/clean.rs +++ b/commons/src/cache/clean.rs @@ -128,7 +128,7 @@ mod tests { use super::*; #[test] - pub fn test_grabs_files() { + fn test_grabs_files() { // FWIW This action must be done on two lines as the tmpdir gets cleaned // as soon as the variable goes out of scope and a path // reference does not retain it's caller @@ -146,7 +146,7 @@ mod tests { use byte_unit::n_mib_bytes; - pub fn touch_file(path: &PathBuf, f: impl FnOnce(&PathBuf)) { + fn touch_file(path: &PathBuf, f: impl FnOnce(&PathBuf)) { if let Some(parent) = path.parent() { if !parent.exists() { fs_err::create_dir_all(parent).unwrap(); diff --git a/commons/src/cache/in_app_dir_cache_layer.rs b/commons/src/cache/in_app_dir_cache_layer.rs index 19502bf7..be581e0d 100644 --- a/commons/src/cache/in_app_dir_cache_layer.rs +++ b/commons/src/cache/in_app_dir_cache_layer.rs @@ -23,7 +23,7 @@ use std::path::PathBuf; /// asset. #[derive(Deserialize, Serialize, Debug, Clone)] pub(crate) struct InAppDirCacheLayer { - pub app_dir_path: PathBuf, + pub(crate) app_dir_path: PathBuf, buildpack: PhantomData, } @@ -33,7 +33,7 @@ pub(crate) struct InAppDirCacheLayerMetadata { } impl InAppDirCacheLayer { - pub fn new(app_dir_path: PathBuf) -> Self { + pub(crate) fn new(app_dir_path: PathBuf) -> Self { Self { app_dir_path, buildpack: PhantomData, diff --git a/commons/src/output/background_timer.rs b/commons/src/output/background_timer.rs index 5fa7f456..05a02c10 100644 --- a/commons/src/output/background_timer.rs +++ b/commons/src/output/background_timer.rs @@ -37,6 +37,8 @@ where let arc_io = arc_io.clone(); let handle = std::thread::spawn(move || { + // TODO: Remove usage of unwrap(): https://github.com/heroku/buildpacks-ruby/issues/238 + #[allow(clippy::unwrap_used)] let mut io = arc_io.lock().unwrap(); write!(&mut io, "{start}").expect("Internal error"); io.flush().expect("Internal error"); diff --git a/commons/src/output/util.rs b/commons/src/output/util.rs index 8b49fa6b..ee82a7b9 100644 --- a/commons/src/output/util.rs +++ b/commons/src/output/util.rs @@ -116,7 +116,7 @@ pub(crate) struct LinesWithEndings<'a> { } impl<'a> LinesWithEndings<'a> { - pub fn from(input: &'a str) -> LinesWithEndings<'a> { + pub(crate) fn from(input: &'a str) -> LinesWithEndings<'a> { LinesWithEndings { input } } }