Forbid usage of some complex Option:: methods.

Author: fulmicoton-dd

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",

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.

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(),

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;

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
     }

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;

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,16 +83,16 @@ 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 {
     fn from_split_meta_and_request(
         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,28 +110,30 @@ 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<i64>,
 }
 
-impl Range {
-    /// Create a Range from bounds.
-    fn from_bounds(range: impl std::ops::RangeBounds<i64>) -> 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<i64>) -> Self {
         let start = match range.start_bound() {
             Bound::Included(start) => *start,
             Bound::Excluded(start) => {
                 // if we exclude i64::MAX from the start bound, the range is necessarily empty
                 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<i64> for Range {
+impl RangeBounds<i64> 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
+        }
     }
 }
 

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);
     }
 }