Skip to content

Commit

Permalink
Enable lints to prevent unwanted usages of unwrap(), assert! and …
Browse files Browse the repository at this point in the history
…`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.
  • Loading branch information
edmorley authored Nov 2, 2023
1 parent 2698a71 commit 5c314ee
Show file tree
Hide file tree
Showing 12 changed files with 47 additions and 8 deletions.
1 change: 1 addition & 0 deletions clippy.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
allow-unwrap-in-tests = true
4 changes: 3 additions & 1 deletion libcnb-cargo/src/main.rs
Original file line number Diff line number Diff line change
@@ -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)]

Expand Down
4 changes: 3 additions & 1 deletion libcnb-common/src/lib.rs
Original file line number Diff line number Diff line change
@@ -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)]

Expand Down
4 changes: 3 additions & 1 deletion libcnb-data/src/lib.rs
Original file line number Diff line number Diff line change
@@ -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)]

Expand Down
4 changes: 3 additions & 1 deletion libcnb-package/src/lib.rs
Original file line number Diff line number Diff line change
@@ -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)]

Expand Down
4 changes: 3 additions & 1 deletion libcnb-proc-macros/src/lib.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down
11 changes: 10 additions & 1 deletion libcnb-test/src/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<str>,
Expand All @@ -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,
Expand Down Expand Up @@ -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(
Expand Down
2 changes: 2 additions & 0 deletions libcnb-test/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]

Expand Down
1 change: 1 addition & 0 deletions libcnb/src/layer/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
4 changes: 3 additions & 1 deletion libcnb/src/lib.rs
Original file line number Diff line number Diff line change
@@ -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.
Expand Down
4 changes: 3 additions & 1 deletion libherokubuildpack/src/lib.rs
Original file line number Diff line number Diff line change
@@ -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.
Expand Down
12 changes: 12 additions & 0 deletions libherokubuildpack/src/log.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<str>, body: impl AsRef<str>) {
let mut stream = StandardStream::stderr(ColorChoice::Always);
stream
Expand All @@ -24,6 +27,9 @@ pub fn log_error(header: impl AsRef<str>, body: impl AsRef<str>) {
///
/// 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<str>, body: impl AsRef<str>) {
let mut stream = StandardStream::stderr(ColorChoice::Always);
stream
Expand All @@ -43,6 +49,9 @@ pub fn log_warning(header: impl AsRef<str>, body: impl AsRef<str>) {
///
/// 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<str>) {
let mut stream = StandardStream::stdout(ColorChoice::Always);
stream
Expand All @@ -56,6 +65,9 @@ pub fn log_header(title: impl AsRef<str>) {
/// # 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<str>) {
println!("{}", message.as_ref());
std::io::stdout().flush().unwrap();
Expand Down

0 comments on commit 5c314ee

Please sign in to comment.