diff --git a/Cargo.toml b/Cargo.toml index 1f191b8db..7a18b16a5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -84,6 +84,11 @@ url = { version = "2.5.2", features = ["serde"] } x509-cert = { version = "0.2.5", features = ["builder"] } zeroize = "1.8.1" +[workspace.lints.clippy] +unwrap_in_result = "deny" +unwrap_used = "deny" +panic = "deny" + # Use O3 in tests to improve the RSA key generation speed in the stackable-certs crate [profile.test.package] stackable-certs.opt-level = 3 diff --git a/clippy.toml b/clippy.toml new file mode 100644 index 000000000..f69b4a67f --- /dev/null +++ b/clippy.toml @@ -0,0 +1,2 @@ +allow-unwrap-in-tests = true +allow-panic-in-tests = true diff --git a/crates/k8s-version/Cargo.toml b/crates/k8s-version/Cargo.toml index 90105a5f4..b031882d1 100644 --- a/crates/k8s-version/Cargo.toml +++ b/crates/k8s-version/Cargo.toml @@ -23,3 +23,6 @@ quote.workspace = true proc-macro2.workspace = true serde_yaml.workspace = true syn.workspace = true + +[lints] +workspace = true diff --git a/crates/k8s-version/src/level/mod.rs b/crates/k8s-version/src/level/mod.rs index 1e10006d9..43bbf3b5c 100644 --- a/crates/k8s-version/src/level/mod.rs +++ b/crates/k8s-version/src/level/mod.rs @@ -47,6 +47,14 @@ pub enum Level { impl FromStr for Level { type Err = ParseLevelError; + // SAFETY: We purposefully allow the `clippy::unwrap_in_result` lint below in this function. + // We can use expect here, because the correct match labels must be used. + // + // FIXME (@Techassi): This attribute can be used on individual unwrap and expect calls since + // Rust 1.91.0. We should move this attribute to not contaminate an unnecessarily large scope + // once we bump the toolchain to 1.91.0. + // See https://github.com/rust-lang/rust-clippy/pull/15445 + #[allow(clippy::unwrap_in_result)] fn from_str(input: &str) -> Result { let captures = LEVEL_REGEX.captures(input).context(InvalidFormatSnafu)?; diff --git a/crates/k8s-version/src/version/mod.rs b/crates/k8s-version/src/version/mod.rs index a145e8a31..f41d6fc74 100644 --- a/crates/k8s-version/src/version/mod.rs +++ b/crates/k8s-version/src/version/mod.rs @@ -53,6 +53,14 @@ pub struct Version { impl FromStr for Version { type Err = ParseVersionError; + // SAFETY: We purposefully allow the `clippy::unwrap_in_result` lint below in this function. + // We can use expect here, because the correct match label must be used. + // + // FIXME (@Techassi): This attribute can be used on individual unwrap and expect calls since + // Rust 1.91.0. We should move this attribute to not contaminate an unnecessarily large scope + // once we bump the toolchain to 1.91.0. + // See https://github.com/rust-lang/rust-clippy/pull/15445 + #[allow(clippy::unwrap_in_result)] fn from_str(input: &str) -> Result { let captures = VERSION_REGEX.captures(input).context(InvalidFormatSnafu)?; @@ -141,6 +149,7 @@ mod test { #[case("v1gamma12", ParseVersionError::ParseLevel { source: ParseLevelError::UnknownIdentifier })] #[case("v1betä1", ParseVersionError::InvalidFormat)] #[case("1beta1", ParseVersionError::InvalidFormat)] + #[case("v", ParseVersionError::InvalidFormat)] #[case("", ParseVersionError::InvalidFormat)] fn invalid_version(#[case] input: &str, #[case] error: ParseVersionError) { let err = Version::from_str(input).expect_err("invalid Kubernetes version"); diff --git a/crates/stackable-certs/Cargo.toml b/crates/stackable-certs/Cargo.toml index 421a5e952..e74941974 100644 --- a/crates/stackable-certs/Cargo.toml +++ b/crates/stackable-certs/Cargo.toml @@ -29,3 +29,6 @@ tokio.workspace = true tracing.workspace = true x509-cert.workspace = true zeroize.workspace = true + +[lints] +workspace = true diff --git a/crates/stackable-certs/src/ca/mod.rs b/crates/stackable-certs/src/ca/mod.rs index 79f0c5b3c..c3bb57099 100644 --- a/crates/stackable-certs/src/ca/mod.rs +++ b/crates/stackable-certs/src/ca/mod.rs @@ -110,7 +110,7 @@ impl PartialEq for Error { x509_cert::builder::Error::Signature(_), x509_cert::builder::Error::Signature(_), ) => panic!( - "it is impossible to compare the opaque Error contained witin signature::error::Error" + "it is impossible to compare the opaque Error contained within signature::error::Error" ), _ => false, }, @@ -205,6 +205,16 @@ where /// validity, this function offers complete control over these parameters. /// If this level of control is not needed, use [`CertificateAuthority::new`] /// instead. + // + // SAFETY: We purposefully allow the `clippy::unwrap_in_result` lint below in this function. + // We can use expect here, because the subject name is defined as a constant which must be able + // to be parsed. + // + // FIXME (@Techassi): This attribute can be used on individual unwrap and expect calls since + // Rust 1.91.0. We should move this attribute to not contaminate an unnecessarily large scope + // once we bump the toolchain to 1.91.0. + // See https://github.com/rust-lang/rust-clippy/pull/15445 + #[allow(clippy::unwrap_in_result)] #[instrument(name = "create_certificate_authority_with", skip(signing_key_pair))] pub fn new_with(signing_key_pair: S, serial_number: u64, validity: Duration) -> Result { let serial_number = SerialNumber::from(serial_number); @@ -214,7 +224,7 @@ where // created by us should contain the same subject consisting a common set // of distinguished names (DNs). let subject = Name::from_str(SDP_ROOT_CA_SUBJECT) - .expect("the SDP_ROOT_CA_SUBJECT must be a valid subject"); + .expect("the constant SDP_ROOT_CA_SUBJECT must be a valid subject"); let spki_pem = signing_key_pair .verifying_key() @@ -511,7 +521,7 @@ mod tests { #[tokio::test] async fn rsa_key_generation() { - let mut ca = CertificateAuthority::new_rsa().unwrap(); + let mut ca = CertificateAuthority::new_rsa().expect("must be able to create RSA-based CA"); let cert = ca .generate_rsa_leaf_certificate("Product", "pod", [TEST_SAN], TEST_CERT_LIFETIME) .expect( @@ -523,7 +533,9 @@ mod tests { #[tokio::test] async fn ecdsa_key_generation() { - let mut ca = CertificateAuthority::new_ecdsa().unwrap(); + let mut ca = + CertificateAuthority::new_ecdsa().expect("must be able to create ECDSA-based CA"); + let cert = ca .generate_ecdsa_leaf_certificate("Product", "pod", [TEST_SAN], TEST_CERT_LIFETIME) .expect( @@ -535,11 +547,11 @@ mod tests { fn assert_cert_attributes(cert: &Certificate) { let cert = &cert.tbs_certificate; + let expected_subject = Name::from_str("CN=Product Certificate for pod") + .expect("constant subject must be valid"); + // Test subject - assert_eq!( - cert.subject, - Name::from_str("CN=Product Certificate for pod").unwrap() - ); + assert_eq!(cert.subject, expected_subject); // Test SAN extension is present let extensions = cert.extensions.as_ref().expect("cert must have extensions"); diff --git a/crates/stackable-operator-derive/Cargo.toml b/crates/stackable-operator-derive/Cargo.toml index fe39f6861..7b9c85faf 100644 --- a/crates/stackable-operator-derive/Cargo.toml +++ b/crates/stackable-operator-derive/Cargo.toml @@ -21,3 +21,6 @@ syn.workspace = true [dev-dependencies] stackable-operator = { path = "../stackable-operator" } + +[lints] +workspace = true diff --git a/crates/stackable-operator/Cargo.toml b/crates/stackable-operator/Cargo.toml index c6787094d..4c77a3279 100644 --- a/crates/stackable-operator/Cargo.toml +++ b/crates/stackable-operator/Cargo.toml @@ -58,3 +58,6 @@ url.workspace = true indoc.workspace = true rstest.workspace = true tempfile.workspace = true + +[lints] +workspace = true diff --git a/crates/stackable-operator/src/logging/k8s_events.rs b/crates/stackable-operator/src/logging/k8s_events.rs index 874998df5..edb9d2ed9 100644 --- a/crates/stackable-operator/src/logging/k8s_events.rs +++ b/crates/stackable-operator/src/logging/k8s_events.rs @@ -1,6 +1,6 @@ //! Utilities for publishing Kubernetes events -use std::error::Error; +use std::{error::Error, fmt::Write}; use kube::runtime::{ controller, @@ -12,25 +12,20 @@ use super::controller::ReconcilerError; /// Converts an [`Error`] into a publishable Kubernetes [`Event`] fn error_to_event(err: &E) -> Event { // Walk the whole error chain, so that we get all the full reason for the error - let mut full_msg = { - use std::fmt::Write; - let mut buf = err.to_string(); - let mut err: &dyn Error = err; - loop { - err = match err.source() { - Some(err) => { - write!(buf, ": {err}").unwrap(); - err - } - None => break buf, - } - } - }; - message::truncate_with_ellipsis(&mut full_msg, 1024); + let mut error = err.to_string(); + let mut source = err.source(); + + while let Some(err) = source { + write!(error, ": {err}").expect("must be able to concat errors"); + source = err.source(); + } + + message::truncate_with_ellipsis(&mut error, 1024); + Event { type_: EventType::Warning, reason: err.category().to_string(), - note: Some(full_msg), + note: Some(error), action: "Reconcile".to_string(), secondary: err.secondary_object().map(|secondary| secondary.into()), } @@ -70,8 +65,7 @@ mod message { pub fn truncate_with_ellipsis(msg: &mut String, max_len: usize) { const ELLIPSIS: char = '…'; const ELLIPSIS_LEN: usize = ELLIPSIS.len_utf8(); - let len = msg.len(); - if len > max_len { + if msg.len() > max_len { let start_of_trunc_char = find_start_of_char(msg, max_len.saturating_sub(ELLIPSIS_LEN)); msg.truncate(start_of_trunc_char); if ELLIPSIS_LEN <= max_len { diff --git a/crates/stackable-operator/src/product_config_utils.rs b/crates/stackable-operator/src/product_config_utils.rs index 1eaed1382..f78d580c9 100644 --- a/crates/stackable-operator/src/product_config_utils.rs +++ b/crates/stackable-operator/src/product_config_utils.rs @@ -218,11 +218,12 @@ pub fn validate_all_roles_and_groups_config( ignore_err: bool, ) -> Result { let mut result = HashMap::new(); + for (role, role_group) in role_config { - result.insert(role.to_string(), HashMap::new()); + let role_entry = result.entry(role.to_string()).or_insert(HashMap::new()); for (group, properties_by_kind) in role_group { - result.get_mut(role).unwrap().insert( + role_entry.insert( group.clone(), validate_role_and_group_config( version, diff --git a/crates/stackable-shared/Cargo.toml b/crates/stackable-shared/Cargo.toml index 00ad06869..4925a11cc 100644 --- a/crates/stackable-shared/Cargo.toml +++ b/crates/stackable-shared/Cargo.toml @@ -28,3 +28,6 @@ time = { workspace = true, optional = true } [dev-dependencies] k8s-openapi.workspace = true rstest.workspace = true + +[lints] +workspace = true diff --git a/crates/stackable-shared/src/time/duration.rs b/crates/stackable-shared/src/time/duration.rs index a5b2dcd15..2f7b10c3f 100644 --- a/crates/stackable-shared/src/time/duration.rs +++ b/crates/stackable-shared/src/time/duration.rs @@ -472,7 +472,7 @@ mod test { #[case("15d2m2s1000ms", 1296123)] #[case("213503982334d", 18446744073657600)] fn parse_as_secs(#[case] input: &str, #[case] output: u64) { - let dur: Duration = input.parse().unwrap(); + let dur: Duration = input.parse().expect("valid duration input must parse"); assert_eq!(dur.as_secs(), output); } @@ -484,7 +484,7 @@ mod test { #[case("", DurationParseError::EmptyInput)] #[case("213503982335d", DurationParseError::Overflow { input: "213503982335d".to_string(), value: 213503982335_u128, unit: DurationUnit::Days })] fn parse_invalid(#[case] input: &str, #[case] expected_err: DurationParseError) { - let err = Duration::from_str(input).unwrap_err(); + let err = Duration::from_str(input).expect_err("invalid duration input must not parse"); assert_eq!(err, expected_err) } @@ -495,7 +495,7 @@ mod test { #[case] input: &str, #[case] expected_err: DurationParseError, ) { - let err = Duration::from_str(input).unwrap_err(); + let err = Duration::from_str(input).expect_err("invalid duration input must produce error"); assert_eq!(err, expected_err) } @@ -506,7 +506,7 @@ mod test { #[case("1m", None)] #[case("1s", None)] fn to_string(#[case] input: &str, #[case] expected: Option<&str>) { - let dur: Duration = input.parse().unwrap(); + let dur: Duration = input.parse().expect("valid duration input must parse"); match expected { Some(e) => assert_eq!(dur.to_string(), e), None => assert_eq!(dur.to_string(), input), @@ -520,7 +520,7 @@ mod test { dur: Duration, } - let s: S = serde_yaml::from_str("dur: 15d2m2s").unwrap(); + let s: S = serde_yaml::from_str("dur: 15d2m2s").expect("valid duration must deserialize"); assert_eq!(s.dur.as_secs(), 1296122); } @@ -532,14 +532,20 @@ mod test { } let s = S { - dur: "15d2m2s".parse().unwrap(), + dur: "15d2m2s" + .parse() + .expect("static string must be valid duration"), }; - assert_eq!(serde_yaml::to_string(&s).unwrap(), "dur: 15d2m2s\n"); + + assert_eq!( + serde_yaml::to_string(&s).expect("valid duration must serialize"), + "dur: 15d2m2s\n" + ); } #[test] fn add_ops() { - let mut dur1 = Duration::from_str("20s").unwrap(); + let mut dur1 = Duration::from_str("20s").expect("static string must be valid duration"); let dur2 = Duration::from_secs(10); let dur = dur1 + dur2; @@ -551,7 +557,7 @@ mod test { #[test] fn sub_ops() { - let mut dur1 = Duration::from_str("20s").unwrap(); + let mut dur1 = Duration::from_str("20s").expect("static string must be valid duration"); let dur2 = Duration::from_secs(10); let dur = dur1 - dur2; diff --git a/crates/stackable-telemetry/Cargo.toml b/crates/stackable-telemetry/Cargo.toml index be4308367..562ff4e2c 100644 --- a/crates/stackable-telemetry/Cargo.toml +++ b/crates/stackable-telemetry/Cargo.toml @@ -34,6 +34,9 @@ tracing-opentelemetry.workspace = true rstest.workspace = true stackable-webhook = { path = "../stackable-webhook" } +[lints] +workspace = true + [package.metadata.cargo-udeps.ignore] # Required for doc tests in stackable-telemetry development = ["stackable-webhook"] diff --git a/crates/stackable-telemetry/src/tracing/mod.rs b/crates/stackable-telemetry/src/tracing/mod.rs index 3d3878903..2e36e699f 100644 --- a/crates/stackable-telemetry/src/tracing/mod.rs +++ b/crates/stackable-telemetry/src/tracing/mod.rs @@ -408,6 +408,16 @@ impl Tracing { /// Name the guard variable appropriately, do not just use let _ =, as that will drop /// immediately. /// + // + // SAFETY: We purposefully allow the `clippy::unwrap_in_result` lint below in this function. + // We can use expect here, because the directives are defined as a constant value which must be + // able to be parsed. + // + // FIXME (@Techassi): This attribute can be used on individual unwrap and expect calls since + // Rust 1.91.0. We should move this attribute to not contaminate an unnecessarily large scope + // once we bump the toolchain to 1.91.0. + // See https://github.com/rust-lang/rust-clippy/pull/15445 + #[allow(clippy::unwrap_in_result)] pub fn init(mut self) -> Result { let mut layers: Vec + Sync + Send>> = Vec::new(); diff --git a/crates/stackable-versioned-macros/Cargo.toml b/crates/stackable-versioned-macros/Cargo.toml index 038681010..4e3b74afd 100644 --- a/crates/stackable-versioned-macros/Cargo.toml +++ b/crates/stackable-versioned-macros/Cargo.toml @@ -55,3 +55,6 @@ serde_yaml.workspace = true snafu.workspace = true tracing.workspace = true trybuild.workspace = true + +[lints] +workspace = true diff --git a/crates/stackable-versioned-macros/src/attrs/item/field.rs b/crates/stackable-versioned-macros/src/attrs/item/field.rs index 359854744..2fd1e1340 100644 --- a/crates/stackable-versioned-macros/src/attrs/item/field.rs +++ b/crates/stackable-versioned-macros/src/attrs/item/field.rs @@ -1,5 +1,5 @@ use darling::{Error, FromField, FromMeta, Result, util::Flag}; -use syn::{Attribute, Ident}; +use syn::{Attribute, Ident, spanned::Spanned}; use crate::{ attrs::item::CommonItemAttributes, @@ -58,11 +58,12 @@ impl FieldAttributes { /// /// Internally, it calls out to other specialized validation functions. fn validate(self) -> Result { - let ident = self - .ident - .as_ref() - .expect("internal error: field must have an ident") - .clone(); + let field_span = self.ident.span(); + + let ident = self.ident.clone().ok_or_else(|| { + darling::Error::custom("internal error: field must have an ident") + .with_span(&field_span) + })?; self.common .validate(FieldIdents::from(ident), &self.attrs)?; diff --git a/crates/stackable-versioned-macros/src/codegen/item/field.rs b/crates/stackable-versioned-macros/src/codegen/item/field.rs index 0134b1b6f..9c51b196d 100644 --- a/crates/stackable-versioned-macros/src/codegen/item/field.rs +++ b/crates/stackable-versioned-macros/src/codegen/item/field.rs @@ -4,7 +4,7 @@ use darling::{FromField, Result, util::IdentString}; use k8s_version::Version; use proc_macro2::TokenStream; use quote::quote; -use syn::{Attribute, Field, Ident, Path, Type}; +use syn::{Attribute, Field, Ident, Path, Type, spanned::Spanned}; use crate::{ attrs::item::{FieldAttributes, Hint}, @@ -36,9 +36,12 @@ impl VersionedField { field_attributes.validate_versions(versions)?; field_attributes.validate_nested_flag(experimental_conversion_tracking)?; - let ident = field - .ident - .expect("internal error: field must have an ident"); + let field_span = field.span(); + + let ident = field.ident.ok_or_else(|| { + darling::Error::custom("internal error: field must have an ident") + .with_span(&field_span) + })?; let idents = FieldIdents::from(ident); let changes = field_attributes @@ -89,7 +92,11 @@ impl VersionedField { // then depends on the relation to other versions (with actions). let field_type = &self.ty; - // NOTE (@Techassi): https://rust-lang.github.io/rust-clippy/master/index.html#/expect_fun_call + // NOTE (@Techassi): `unwrap_or_else` used instead of `expect`. + // See: https://rust-lang.github.io/rust-clippy/master/index.html#expect_fun_call + // We could use expect here, but we would lose the version in the panic message. We need to allow + // a lint in either case anyway. + #[allow(clippy::panic)] match changes.get(&version.inner).unwrap_or_else(|| { panic!( "internal error: chain must contain container version {}", diff --git a/crates/stackable-versioned-macros/src/codegen/item/variant.rs b/crates/stackable-versioned-macros/src/codegen/item/variant.rs index f56e75aeb..2055fed19 100644 --- a/crates/stackable-versioned-macros/src/codegen/item/variant.rs +++ b/crates/stackable-versioned-macros/src/codegen/item/variant.rs @@ -66,7 +66,10 @@ impl VersionedVariant { match &self.changes { // NOTE (@Techassi): `unwrap_or_else` used instead of `expect`. - // See: https://rust-lang.github.io/rust-clippy/master/index.html#/expect_fun_call + // See: https://rust-lang.github.io/rust-clippy/master/index.html#expect_fun_call + // We could use expect here, but we would lose the version in the panic message. We need to allow + // a lint in either case anyway. + #[allow(clippy::panic)] Some(changes) => match changes.get(&version.inner).unwrap_or_else(|| { panic!( "internal error: chain must contain container version {}", diff --git a/crates/stackable-versioned-macros/src/codegen/module.rs b/crates/stackable-versioned-macros/src/codegen/module.rs index a7a15aaf7..7aeb8bc35 100644 --- a/crates/stackable-versioned-macros/src/codegen/module.rs +++ b/crates/stackable-versioned-macros/src/codegen/module.rs @@ -192,9 +192,11 @@ impl Module { for container in &self.containers { let versioned = inner_and_between_tokens .get_mut(container.get_original_ident()) - .unwrap(); - let VersionedContainerTokens { inner, between } = - versioned.remove(&version.inner).unwrap(); + .expect("inner_and_between_tokens map must contain versioned container tokens"); + + let VersionedContainerTokens { inner, between } = versioned + .remove(&version.inner) + .expect("versioned container tokens map must contain tokens for version"); inner_tokens.extend(inner); between_tokens.extend(between); diff --git a/crates/stackable-versioned/Cargo.toml b/crates/stackable-versioned/Cargo.toml index 801252599..2bb8d43bd 100644 --- a/crates/stackable-versioned/Cargo.toml +++ b/crates/stackable-versioned/Cargo.toml @@ -23,3 +23,6 @@ snafu.workspace = true insta.workspace = true k8s-openapi.workspace = true kube.workspace = true + +[lints] +workspace = true diff --git a/crates/stackable-versioned/tests/person.rs b/crates/stackable-versioned/tests/person.rs index 79dc89319..fbbe930b4 100644 --- a/crates/stackable-versioned/tests/person.rs +++ b/crates/stackable-versioned/tests/person.rs @@ -26,7 +26,10 @@ pub fn roundtrip_conversion_review( response_review: ConversionReview, desired_api_version: PersonVersion, ) -> ConversionReview { - let response = response_review.response.unwrap(); + let response = response_review + .response + .expect("conversion review must have response"); + ConversionReview { types: response_review.types, request: Some(ConversionRequest { diff --git a/crates/stackable-webhook/Cargo.toml b/crates/stackable-webhook/Cargo.toml index 3dc40e036..6190e7d2a 100644 --- a/crates/stackable-webhook/Cargo.toml +++ b/crates/stackable-webhook/Cargo.toml @@ -34,9 +34,11 @@ x509-cert.workspace = true [dev-dependencies] # Only needed for doc tests stackable-operator = { path = "../stackable-operator" } - clap.workspace = true +[lints] +workspace = true + # Only needed for tests, this is a false positive of "cargo udeps" [package.metadata.cargo-udeps.ignore] development = ["stackable-operator"] diff --git a/crates/xtask/Cargo.toml b/crates/xtask/Cargo.toml index dd2ceba68..b095a7313 100644 --- a/crates/xtask/Cargo.toml +++ b/crates/xtask/Cargo.toml @@ -12,3 +12,6 @@ serde.workspace = true serde_json.workspace = true snafu.workspace = true strum.workspace = true + +[lints] +workspace = true