From 5c314eeacf69fcae83ba70f9489c6db7fb0c1be6 Mon Sep 17 00:00:00 2001 From: Ed Morley <501702+edmorley@users.noreply.github.com> Date: Thu, 2 Nov 2023 13:04:48 +0000 Subject: [PATCH] Enable lints to prevent unwanted usages of `unwrap()`, `assert!` and `panic!` (#711) The `clippy::unwrap_used` lint prevents usage of `.unwrap()` outside of tests: https://rust-lang.github.io/rust-clippy/master/index.html#/unwrap_used The `clippy::panic_in_result_fn` lint prevents usages of `assert!` or `panic!` in functions that already return a `Result`: https://rust-lang.github.io/rust-clippy/master/index.html#/panic_in_result_fn These lints catch cases like #709, #710 and #712. The lints have not been enabled for the integration test modules (since using `unwrap()` is fine there), or to the example buildpacks, since it's a lot of boilerplate to add to an example. Very soon we will be able to switch to the upcoming `[lints]` table feature, which will mean the duplication of lint definitions can be avoided entirely: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#lints GUS-W-14413286. --- clippy.toml | 1 + libcnb-cargo/src/main.rs | 4 +++- libcnb-common/src/lib.rs | 4 +++- libcnb-data/src/lib.rs | 4 +++- libcnb-package/src/lib.rs | 4 +++- libcnb-proc-macros/src/lib.rs | 4 +++- libcnb-test/src/build.rs | 11 ++++++++++- libcnb-test/src/lib.rs | 2 ++ libcnb/src/layer/tests.rs | 1 + libcnb/src/lib.rs | 4 +++- libherokubuildpack/src/lib.rs | 4 +++- libherokubuildpack/src/log.rs | 12 ++++++++++++ 12 files changed, 47 insertions(+), 8 deletions(-) create mode 100644 clippy.toml 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/libcnb-cargo/src/main.rs b/libcnb-cargo/src/main.rs index f9033190..ff9d5a80 100644 --- a/libcnb-cargo/src/main.rs +++ b/libcnb-cargo/src/main.rs @@ -1,6 +1,8 @@ #![doc = include_str!("../README.md")] -#![warn(clippy::pedantic)] #![warn(unused_crate_dependencies)] +#![warn(clippy::pedantic)] +#![warn(clippy::panic_in_result_fn)] +#![warn(clippy::unwrap_used)] // This lint is too noisy and enforces a style that reduces readability in many cases. #![allow(clippy::module_name_repetitions)] diff --git a/libcnb-common/src/lib.rs b/libcnb-common/src/lib.rs index 6c9ddaa1..acf829c7 100644 --- a/libcnb-common/src/lib.rs +++ b/libcnb-common/src/lib.rs @@ -1,6 +1,8 @@ #![doc = include_str!("../README.md")] -#![warn(clippy::pedantic)] #![warn(unused_crate_dependencies)] +#![warn(clippy::pedantic)] +#![warn(clippy::panic_in_result_fn)] +#![warn(clippy::unwrap_used)] // This lint is too noisy and enforces a style that reduces readability in many cases. #![allow(clippy::module_name_repetitions)] diff --git a/libcnb-data/src/lib.rs b/libcnb-data/src/lib.rs index ad15a58e..0a9fd2ab 100644 --- a/libcnb-data/src/lib.rs +++ b/libcnb-data/src/lib.rs @@ -1,6 +1,8 @@ #![doc = include_str!("../README.md")] -#![warn(clippy::pedantic)] #![warn(unused_crate_dependencies)] +#![warn(clippy::pedantic)] +#![warn(clippy::panic_in_result_fn)] +#![warn(clippy::unwrap_used)] // This lint is too noisy and enforces a style that reduces readability in many cases. #![allow(clippy::module_name_repetitions)] diff --git a/libcnb-package/src/lib.rs b/libcnb-package/src/lib.rs index 96aea209..faf14d38 100644 --- a/libcnb-package/src/lib.rs +++ b/libcnb-package/src/lib.rs @@ -1,6 +1,8 @@ #![doc = include_str!("../README.md")] -#![warn(clippy::pedantic)] #![warn(unused_crate_dependencies)] +#![warn(clippy::pedantic)] +#![warn(clippy::panic_in_result_fn)] +#![warn(clippy::unwrap_used)] // This lint is too noisy and enforces a style that reduces readability in many cases. #![allow(clippy::module_name_repetitions)] diff --git a/libcnb-proc-macros/src/lib.rs b/libcnb-proc-macros/src/lib.rs index 52076e2a..9e83a8e7 100644 --- a/libcnb-proc-macros/src/lib.rs +++ b/libcnb-proc-macros/src/lib.rs @@ -1,6 +1,8 @@ #![doc = include_str!("../README.md")] -#![warn(clippy::pedantic)] #![warn(unused_crate_dependencies)] +#![warn(clippy::pedantic)] +#![warn(clippy::panic_in_result_fn)] +#![warn(clippy::unwrap_used)] use proc_macro::TokenStream; use quote::quote; diff --git a/libcnb-test/src/build.rs b/libcnb-test/src/build.rs index 9114f224..4ad4e61f 100644 --- a/libcnb-test/src/build.rs +++ b/libcnb-test/src/build.rs @@ -11,7 +11,10 @@ use std::collections::BTreeMap; use std::fs; use std::path::{Path, PathBuf}; -/// Packages the current crate as a buildpack into a temporary directory. +/// Packages the current crate as a buildpack into the provided directory. +// TODO: Convert the `assert!` usages to an additional `PackageBuildpackError` variant instead: +// https://github.com/heroku/libcnb.rs/issues/709 +#[allow(clippy::panic_in_result_fn)] pub(crate) fn package_crate_buildpack( cargo_profile: CargoProfile, target_triple: impl AsRef, @@ -38,6 +41,9 @@ pub(crate) fn package_crate_buildpack( ) } +// TODO: Convert the `assert!` usages to an additional `PackageBuildpackError` variant instead: +// https://github.com/heroku/libcnb.rs/issues/709 +#[allow(clippy::panic_in_result_fn)] pub(crate) fn package_buildpack( buildpack_id: &BuildpackId, cargo_profile: CargoProfile, @@ -87,6 +93,9 @@ pub(crate) fn package_buildpack( for node in &build_order { let buildpack_destination_dir = buildpack_dir_resolver(&node.buildpack_id); + // TODO: Convert the `unwrap()` to an additional `PackageBuildpackError` variant instead: + // https://github.com/heroku/libcnb.rs/issues/710 + #[allow(clippy::unwrap_used)] fs::create_dir_all(&buildpack_destination_dir).unwrap(); libcnb_package::package::package_buildpack( diff --git a/libcnb-test/src/lib.rs b/libcnb-test/src/lib.rs index 9ad2cc71..186b1e5e 100644 --- a/libcnb-test/src/lib.rs +++ b/libcnb-test/src/lib.rs @@ -2,6 +2,8 @@ // Enable lints that are disabled by default. #![warn(unused_crate_dependencies)] #![warn(clippy::pedantic)] +#![warn(clippy::panic_in_result_fn)] +#![warn(clippy::unwrap_used)] // This lint is too noisy and enforces a style that reduces readability in many cases. #![allow(clippy::module_name_repetitions)] diff --git a/libcnb/src/layer/tests.rs b/libcnb/src/layer/tests.rs index ad33d12f..cbaa170f 100644 --- a/libcnb/src/layer/tests.rs +++ b/libcnb/src/layer/tests.rs @@ -824,6 +824,7 @@ fn layer_env_read_write() { expected_layer_env: LayerEnv, } + #[allow(clippy::panic_in_result_fn)] impl Layer for LayerDataTestLayer { type Buildpack = TestBuildpack; type Metadata = GenericMetadata; diff --git a/libcnb/src/lib.rs b/libcnb/src/lib.rs index 35bb9ec9..5f15fbf6 100644 --- a/libcnb/src/lib.rs +++ b/libcnb/src/lib.rs @@ -1,6 +1,8 @@ #![doc = include_str!("../README.md")] -#![warn(clippy::pedantic)] #![warn(unused_crate_dependencies)] +#![warn(clippy::pedantic)] +#![warn(clippy::panic_in_result_fn)] +#![warn(clippy::unwrap_used)] // Most of libcnb's public API returns user-provided errors, making error docs redundant. #![allow(clippy::missing_errors_doc)] // This lint is too noisy and enforces a style that reduces readability in many cases. diff --git a/libherokubuildpack/src/lib.rs b/libherokubuildpack/src/lib.rs index 35693e9b..faca25ba 100644 --- a/libherokubuildpack/src/lib.rs +++ b/libherokubuildpack/src/lib.rs @@ -1,7 +1,9 @@ #![doc = include_str!("../README.md")] // Enable lints that are disabled by default. -#![warn(clippy::pedantic)] #![warn(unused_crate_dependencies)] +#![warn(clippy::pedantic)] +#![warn(clippy::panic_in_result_fn)] +#![warn(clippy::unwrap_used)] // In most cases adding error docs provides little value. #![allow(clippy::missing_errors_doc)] // This lint is too noisy and enforces a style that reduces readability in many cases. diff --git a/libherokubuildpack/src/log.rs b/libherokubuildpack/src/log.rs index 21a0c648..9e270e27 100644 --- a/libherokubuildpack/src/log.rs +++ b/libherokubuildpack/src/log.rs @@ -5,6 +5,9 @@ use termcolor::{Color, ColorChoice, ColorSpec, StandardStream, WriteColor}; /// /// Will panic if there was a problem setting the color settings, or all bytes could /// not be written due to either I/O errors or EOF being reached. +// TODO: Replace `.unwrap()` usages with `.expect()` to give a clearer error message: +// https://github.com/heroku/libcnb.rs/issues/712 +#[allow(clippy::unwrap_used)] pub fn log_error(header: impl AsRef, body: impl AsRef) { let mut stream = StandardStream::stderr(ColorChoice::Always); stream @@ -24,6 +27,9 @@ pub fn log_error(header: impl AsRef, body: impl AsRef) { /// /// Will panic if there was a problem setting the color settings, or all bytes could /// not be written due to either I/O errors or EOF being reached. +// TODO: Replace `.unwrap()` usages with `.expect()` to give a clearer error message: +// https://github.com/heroku/libcnb.rs/issues/712 +#[allow(clippy::unwrap_used)] pub fn log_warning(header: impl AsRef, body: impl AsRef) { let mut stream = StandardStream::stderr(ColorChoice::Always); stream @@ -43,6 +49,9 @@ pub fn log_warning(header: impl AsRef, body: impl AsRef) { /// /// Will panic if there was a problem setting the color settings, or all bytes could /// not be written due to either I/O errors or EOF being reached. +// TODO: Replace `.unwrap()` usages with `.expect()` to give a clearer error message: +// https://github.com/heroku/libcnb.rs/issues/712 +#[allow(clippy::unwrap_used)] pub fn log_header(title: impl AsRef) { let mut stream = StandardStream::stdout(ColorChoice::Always); stream @@ -56,6 +65,9 @@ pub fn log_header(title: impl AsRef) { /// # Panics /// /// Will panic if all bytes could not be written due to I/O errors or EOF being reached. +// TODO: Replace `.unwrap()` usages with `.expect()` to give a clearer error message: +// https://github.com/heroku/libcnb.rs/issues/712 +#[allow(clippy::unwrap_used)] pub fn log_info(message: impl AsRef) { println!("{}", message.as_ref()); std::io::stdout().flush().unwrap();