From ca6263ed020ceef0bbdf26d31821be3e7697e449 Mon Sep 17 00:00:00 2001 From: fulmicoton Date: Thu, 17 Apr 2025 11:33:09 +0200 Subject: [PATCH] Forbid usage of some complex Option:: methods. --- quickwit/clippy.toml | 13 +++- quickwit/quickwit-common/src/uri.rs | 3 +- .../src/ingest_v2/replication.rs | 3 +- quickwit/quickwit-ingest/src/lib.rs | 1 + .../quickwit-janitor/src/janitor_service.rs | 10 +-- quickwit/quickwit-proto/src/lib.rs | 2 +- quickwit/quickwit-search/src/leaf_cache.rs | 61 ++++++++++--------- quickwit/quickwit-search/src/root.rs | 13 ++-- 8 files changed, 60 insertions(+), 46 deletions(-) diff --git a/quickwit/clippy.toml b/quickwit/clippy.toml index 06bfa7456e3..c9ab656382f 100644 --- a/quickwit/clippy.toml +++ b/quickwit/clippy.toml @@ -1,6 +1,17 @@ disallowed-methods = [ - "std::path::Path::exists" + # This function is not sound because it does not return a Result + "std::path::Path::exists", + # These functions hurt readability (according to Paul) + "std::option::Option::is_some_and", + "std::option::Option::is_none_or", + "std::option::Option::xor", + # "std::option::Option::and_then", + # .map(..).unwrap_or(..) or let Some(..) else {..} + "std::option::Option::map_or", + # .map(..).unwrap_or_else(..) or let Some(..) else {..} + "std::option::Option::map_or_else", ] + ignore-interior-mutability = [ "bytes::Bytes", "bytestring::ByteString", diff --git a/quickwit/quickwit-common/src/uri.rs b/quickwit/quickwit-common/src/uri.rs index 3d52a3a92e0..92a702267bf 100644 --- a/quickwit/quickwit-common/src/uri.rs +++ b/quickwit/quickwit-common/src/uri.rs @@ -14,7 +14,6 @@ use std::borrow::Cow; use std::env; -use std::ffi::OsStr; use std::fmt::{Debug, Display}; use std::hash::Hash; use std::path::{Component, Path, PathBuf}; @@ -126,7 +125,7 @@ impl Uri { /// Returns the extension of the URI. pub fn extension(&self) -> Option<&str> { - Path::new(&self.uri).extension().and_then(OsStr::to_str) + Path::new(&self.uri).extension()?.to_str() } /// Returns the URI as a string slice. diff --git a/quickwit/quickwit-ingest/src/ingest_v2/replication.rs b/quickwit/quickwit-ingest/src/ingest_v2/replication.rs index 342434d5658..5e286ec5b84 100644 --- a/quickwit/quickwit-ingest/src/ingest_v2/replication.rs +++ b/quickwit/quickwit-ingest/src/ingest_v2/replication.rs @@ -893,7 +893,8 @@ mod tests { let replication_position_inclusive = subrequest .from_position_exclusive() .as_usize() - .map_or(batch_len - 1, |pos| pos + batch_len); + .map(|pos| pos + batch_len) + .unwrap_or(batch_len - 1); ReplicateSuccess { subrequest_id: subrequest.subrequest_id, index_uid: subrequest.index_uid.clone(), diff --git a/quickwit/quickwit-ingest/src/lib.rs b/quickwit/quickwit-ingest/src/lib.rs index 5383c241c73..ec84af7cfe4 100644 --- a/quickwit/quickwit-ingest/src/lib.rs +++ b/quickwit/quickwit-ingest/src/lib.rs @@ -18,6 +18,7 @@ mod doc_batch; pub mod error; mod ingest_api_service; #[path = "codegen/ingest_service.rs"] +#[allow(clippy::disallowed_methods)] mod ingest_service; mod ingest_v2; mod memory_capacity; diff --git a/quickwit/quickwit-janitor/src/janitor_service.rs b/quickwit/quickwit-janitor/src/janitor_service.rs index 626f7624720..b8bfe5e54b0 100644 --- a/quickwit/quickwit-janitor/src/janitor_service.rs +++ b/quickwit/quickwit-janitor/src/janitor_service.rs @@ -40,11 +40,13 @@ impl JanitorService { } fn is_healthy(&self) -> bool { - self.delete_task_service_handle - .as_ref() - .is_none_or(|delete_task_service_handle| { + let delete_task_is_not_failure: bool = + if let Some(delete_task_service_handle) = &self.delete_task_service_handle { delete_task_service_handle.state() != ActorState::Failure - }) + } else { + true + }; + delete_task_is_not_failure && self.garbage_collector_handle.state() != ActorState::Failure && self.retention_policy_executor_handle.state() != ActorState::Failure } diff --git a/quickwit/quickwit-proto/src/lib.rs b/quickwit/quickwit-proto/src/lib.rs index 1c7787e4a14..c61fa799491 100644 --- a/quickwit/quickwit-proto/src/lib.rs +++ b/quickwit/quickwit-proto/src/lib.rs @@ -13,7 +13,7 @@ // limitations under the License. #![allow(clippy::derive_partial_eq_without_eq)] -#![deny(clippy::disallowed_methods)] +#![allow(clippy::disallowed_methods)] #![allow(rustdoc::invalid_html_tags)] use std::cmp::Ordering; diff --git a/quickwit/quickwit-search/src/leaf_cache.rs b/quickwit/quickwit-search/src/leaf_cache.rs index e8f36cbcb30..e1f284214f7 100644 --- a/quickwit/quickwit-search/src/leaf_cache.rs +++ b/quickwit/quickwit-search/src/leaf_cache.rs @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::ops::Bound; +use std::ops::{Bound, RangeBounds}; use prost::Message; use quickwit_proto::search::{ @@ -83,7 +83,7 @@ struct CacheKey { request: SearchRequest, /// The effective time range of the request, that is, the intersection of the timerange /// requested, and the timerange covered by the split. - merged_time_range: Range, + merged_time_range: HalfOpenRange, } impl CacheKey { @@ -91,8 +91,8 @@ impl CacheKey { split_info: SplitIdAndFooterOffsets, mut search_request: SearchRequest, ) -> Self { - let split_time_range = Range::from_bounds(split_info.time_range()); - let request_time_range = Range::from_bounds(search_request.time_range()); + let split_time_range = HalfOpenRange::from_bounds(split_info.time_range()); + let request_time_range = HalfOpenRange::from_bounds(search_request.time_range()); let merged_time_range = request_time_range.intersect(&split_time_range); search_request.start_timestamp = None; @@ -110,20 +110,22 @@ impl CacheKey { } /// A (half-open) range bounded inclusively below and exclusively above [start..end). -#[derive(Debug, Clone, PartialEq, Eq, Hash)] -struct Range { +#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] +struct HalfOpenRange { start: i64, end: Option, } -impl Range { - /// Create a Range from bounds. - fn from_bounds(range: impl std::ops::RangeBounds) -> Self { - let empty_range = Range { +impl HalfOpenRange { + fn empty_range() -> HalfOpenRange { + HalfOpenRange { start: 0, end: Some(0), - }; + } + } + /// Create a Range from bounds. + fn from_bounds(range: impl RangeBounds) -> Self { let start = match range.start_bound() { Bound::Included(start) => *start, Bound::Excluded(start) => { @@ -131,7 +133,7 @@ impl Range { if let Some(start) = start.checked_add(1) { start } else { - return empty_range; + return Self::empty_range(); } } Bound::Unbounded => i64::MIN, @@ -143,44 +145,45 @@ impl Range { Bound::Unbounded => None, }; - Range { start, end } + HalfOpenRange { start, end }.normalize() + } + + fn is_empty(self) -> bool { + !self.contains(&self.start) } /// Normalize empty ranges to be 0..0 - fn normalize(self) -> Range { - let empty_range = Range { - start: 0, - end: Some(0), - }; - match self { - Range { - start, - end: Some(end), - } if start >= end => empty_range, - any => any, + fn normalize(self) -> HalfOpenRange { + if self.is_empty() { + Self::empty_range() + } else { + self } } /// Return the intersection of self and other. - fn intersect(&self, other: &Range) -> Range { + fn intersect(&self, other: &HalfOpenRange) -> HalfOpenRange { let start = self.start.max(other.start); - let end = match (self.end, other.end) { (Some(this), Some(other)) => Some(this.min(other)), (Some(this), None) => Some(this), (None, other) => other, }; - Range { start, end }.normalize() + HalfOpenRange { start, end }.normalize() } } -impl std::ops::RangeBounds for Range { +impl RangeBounds for HalfOpenRange { fn start_bound(&self) -> Bound<&i64> { Bound::Included(&self.start) } fn end_bound(&self) -> Bound<&i64> { - self.end.as_ref().map_or(Bound::Unbounded, Bound::Excluded) + if let Some(end_bound) = &self.end { + Bound::Excluded(end_bound) + } else { + Bound::Unbounded + } } } diff --git a/quickwit/quickwit-search/src/root.rs b/quickwit/quickwit-search/src/root.rs index bcfc1160519..74de8e91ff5 100644 --- a/quickwit/quickwit-search/src/root.rs +++ b/quickwit/quickwit-search/src/root.rs @@ -1519,10 +1519,8 @@ impl ExtractTimestampRange<'_> { // a match_none, but the visitor doesn't allow mutation. lower_bound = lower_bound.saturating_add(1); } - self.start_timestamp = Some( - self.start_timestamp - .map_or(lower_bound, |current| current.max(lower_bound)), - ); + + self.start_timestamp = self.start_timestamp.max(Some(lower_bound)); } fn update_end_timestamp(&mut self, upper_bound: &quickwit_query::JsonLiteral, included: bool) { @@ -1537,10 +1535,9 @@ impl ExtractTimestampRange<'_> { // a match_none, but the visitor doesn't allow mutation. upper_bound = upper_bound.saturating_add(1); } - self.end_timestamp = Some( - self.end_timestamp - .map_or(upper_bound, |current| current.min(upper_bound)), - ); + + let new_end_timestamp = self.end_timestamp.unwrap_or(upper_bound).min(upper_bound); + self.end_timestamp = Some(new_end_timestamp); } }