Enabled clippy in CI; added some safety comments; fixed some clippy warnings;

Author: Michael Rodler

.github/workflows/ci.yml

@@ -41,6 +41,17 @@ jobs:
         if: matrix.benches
         run: cargo test --benches ${{ matrix.features }}
 
+          
+  clippy_check:
+    runs-on: ubuntu-latest
+    # Make sure CI fails on all warnings, including Clippy lints
+    env:
+      RUSTFLAGS: "-Dwarnings"
+    steps:
+      - uses: actions/checkout@v3
+      - name: Run Clippy
+        run: cargo clippy --all-targets --all-features
+
   msrv:
     name: Test MSRV
     runs-on: ubuntu-latest

Cargo.toml

@@ -20,7 +20,7 @@ A set of types for representing HTTP requests and responses.
 keywords = ["http"]
 categories = ["web-programming"]
 edition = "2018"
-# When updating this value, don't forget to also adjust the GitHub Actions config.
+# When updating this value, don't forget to also adjust the clippy config.
 rust-version = "1.49.0"
 
 [workspace]

clippy.toml

@@ -0,0 +1 @@
+msrv="1.49"

src/extensions.rs

@@ -60,6 +60,7 @@ impl Extensions {
     /// assert_eq!(ext.insert(9i32), Some(5i32));
     /// ```
     pub fn insert<T: Send + Sync + 'static>(&mut self, val: T) -> Option<T> {
+        #[allow(clippy::box_default)]
         self.map
             .get_or_insert_with(|| Box::new(HashMap::default()))
             .insert(TypeId::of::<T>(), Box::new(val))

src/header/map.rs

@@ -961,6 +961,7 @@ impl<T> HeaderMap<T> {
         let entries = &mut self.entries[..] as *mut _;
         let extra_values = &mut self.extra_values as *mut _;
         let len = self.entries.len();
+        // SAFETY: see comment above
         unsafe { self.entries.set_len(0); }
 
         Drain {
@@ -2070,6 +2071,7 @@ impl<'a, T> Iterator for Iter<'a, T> {
     fn next(&mut self) -> Option<Self::Item> {
         self.inner
             .next_unsafe()
+            // SAFETY: Iter invariant: only valid pointers
             .map(|(key, ptr)| (key, unsafe { &*ptr }))
     }
 
@@ -2090,6 +2092,7 @@ impl<'a, T> IterMut<'a, T> {
         use self::Cursor::*;
 
         if self.cursor.is_none() {
+            // SAFETY: invariant dereferencing the self.map pointer is always safe
             if (self.entry + 1) >= unsafe { &*self.map }.entries.len() {
                 return None;
             }
@@ -2098,6 +2101,7 @@ impl<'a, T> IterMut<'a, T> {
             self.cursor = Some(Cursor::Head);
         }
 
+        // SAFETY: invariant dereferencing the self.map pointer is always safe
         let entry = unsafe { &(*self.map).entries[self.entry] };
 
         match self.cursor.unwrap() {
@@ -2106,6 +2110,7 @@ impl<'a, T> IterMut<'a, T> {
                 Some((&entry.key, &entry.value as *const _ as *mut _))
             }
             Values(idx) => {
+                // SAFETY: invariant dereferencing the self.map pointer is always safe
                 let extra = unsafe { &(*self.map).extra_values[idx] };
 
                 match extra.next {
@@ -2128,6 +2133,7 @@ impl<'a, T> Iterator for IterMut<'a, T> {
     }
 
     fn size_hint(&self) -> (usize, Option<usize>) {
+        // SAFETY: invariant dereferencing the self.map pointer is always safe
         let map = unsafe { &*self.map };
         debug_assert!(map.entries.len() >= self.entry);
 
@@ -2204,9 +2210,8 @@ impl<'a, T> Iterator for Drain<'a, T> {
             // Remove the extra value
 
             let raw_links = RawLinks(self.entries);
-            let extra = unsafe {
-                remove_extra_value(raw_links, &mut *self.extra_values, next)
-            };
+            // SAFETY: dereferencing self.extra_values valid as long as self is alive.
+            let extra = remove_extra_value(raw_links, unsafe { &mut *self.extra_values } , next);
 
             match extra.next {
                 Link::Extra(idx) => self.next = Some(idx),
@@ -2224,6 +2229,8 @@ impl<'a, T> Iterator for Drain<'a, T> {
 
         self.idx += 1;
 
+        // SAFETY: pointer operation always valid, as `self` cannot outlive the HeaderMap it is
+        // referencing.
         unsafe {
             let entry = &(*self.entries)[idx];
 
@@ -2243,6 +2250,7 @@ impl<'a, T> Iterator for Drain<'a, T> {
         // For instance, extending a new `HeaderMap` wouldn't need to
         // reserve the upper-bound in `entries`, only the lower-bound.
         let lower = self.len - self.idx;
+        // SAFETY: dereferencing self.extra_values valid as long as self is alive.
         let upper = unsafe { (*self.extra_values).len() } + lower;
         (lower, Some(upper))
     }
@@ -2606,6 +2614,7 @@ impl<'a, T: 'a> Iterator for ValueIterMut<'a, T> {
     fn next(&mut self) -> Option<Self::Item> {
         use self::Cursor::*;
 
+        // SAFETY: dereferencing self.map valid as long as self is alive.
         let entry = unsafe { &mut (*self.map).entries[self.index] };
 
         match self.front {
@@ -2626,6 +2635,7 @@ impl<'a, T: 'a> Iterator for ValueIterMut<'a, T> {
                 Some(&mut entry.value)
             }
             Some(Values(idx)) => {
+                // SAFETY: dereferencing self.map valid as long as self is alive.
                 let extra = unsafe { &mut (*self.map).extra_values[idx] };
 
                 if self.front == self.back {
@@ -2649,6 +2659,7 @@ impl<'a, T: 'a> DoubleEndedIterator for ValueIterMut<'a, T> {
     fn next_back(&mut self) -> Option<Self::Item> {
         use self::Cursor::*;
 
+        // SAFETY: dereferencing self.map valid as long as self is alive.
         let entry = unsafe { &mut (*self.map).entries[self.index] };
 
         match self.back {
@@ -2658,6 +2669,7 @@ impl<'a, T: 'a> DoubleEndedIterator for ValueIterMut<'a, T> {
                 Some(&mut entry.value)
             }
             Some(Values(idx)) => {
+                // SAFETY: dereferencing self.map valid as long as self is alive.
                 let extra = unsafe { &mut (*self.map).extra_values[idx] };
 
                 if self.front == self.back {
@@ -2726,7 +2738,7 @@ impl<T> Drop for IntoIter<T> {
         // Ensure the iterator is consumed
         for _ in self.by_ref() {}
 
-        // All the values have already been yielded out.
+        // SAFETY: All the values have already been yielded out, once dropped.
         unsafe {
             self.extra_values.set_len(0);
         }

src/header/name.rs

@@ -87,8 +87,8 @@ macro_rules! standard_headers {
             #[inline]
             fn as_str(&self) -> &'static str {
                 match *self {
-                    // Safety: test_parse_standard_headers ensures these &[u8]s are &str-safe.
                     $(
+                    // SAFETY: test_parse_standard_headers ensures these &[u8]s are &str-safe.
                     StandardHeader::$konst => unsafe { std::str::from_utf8_unchecked( $name_bytes ) },
                     )+
                 }
@@ -1267,6 +1267,7 @@ impl HeaderName {
                 i += 1;
             }
         } {
+            #[allow(clippy::no_effect)]
             ([] as [u8; 0])[0]; // Invalid header name
         }
 

src/header/value.rs

@@ -85,6 +85,7 @@ impl HeaderValue {
         let mut i = 0;
         while i < bytes.len() {
             if !is_visible_ascii(bytes[i]) {
+                #[allow(clippy::no_effect)]
                 ([] as [u8; 0])[0]; // Invalid header value
             }
             i += 1;
@@ -191,6 +192,10 @@ impl HeaderValue {
     ///
     /// This function does NOT validate that illegal bytes are not contained
     /// within the buffer.
+    ///
+    /// ## Safety
+    ///
+    /// The caller must ensure that `src` contains only legal utf-8.
     pub unsafe fn from_maybe_shared_unchecked<T>(src: T) -> HeaderValue
     where
         T: AsRef<[u8]> + 'static,

src/lib.rs

@@ -159,6 +159,10 @@
 //! ```
 
 #![deny(warnings, missing_docs, missing_debug_implementations)]
+#![deny(
+    clippy::missing_safety_doc,
+    clippy::undocumented_unsafe_blocks
+)]
 
 #[cfg(test)]
 #[macro_use]

src/status.rs

@@ -143,6 +143,8 @@ impl StatusCode {
         { &CODE_DIGITS[offset..offset+3] }
 
         #[cfg(not(debug_assertions))]
+        // SAFETY: we assume `StatusCode` is constructed in a way that self.0 (the numerical status
+        // code is >= 100 and also <= 1000, which makes any possible offset here valid.
         unsafe { CODE_DIGITS.get_unchecked(offset..offset+3) }
     }
 
@@ -304,7 +306,9 @@ macro_rules! status_codes {
         impl StatusCode {
         $(
             $(#[$docs])*
-            pub const $konst: StatusCode = StatusCode(unsafe { NonZeroU16::new_unchecked($num) });
+            pub const $konst: StatusCode = StatusCode(
+                // SAFETY: only called with constants
+                unsafe { NonZeroU16::new_unchecked($num) });
         )+
 
         }

src/uri/mod.rs

@@ -837,6 +837,7 @@ fn parse_full(mut s: Bytes) -> Result<Uri, InvalidUri> {
             let _ = scheme.split_off(n);
 
             // Allocate the ByteStr
+            // SAFETY: previously verified by `Scheme2::parse`
             let val = unsafe { ByteStr::from_utf8_unchecked(scheme) };
 
             Scheme2::Other(Box::new(val))
@@ -853,6 +854,7 @@ fn parse_full(mut s: Bytes) -> Result<Uri, InvalidUri> {
         }
 
         let authority = Authority {
+            // SAFETY: previously verified by `Authority::parse`
             data: unsafe { ByteStr::from_utf8_unchecked(s) },
         };
 
@@ -870,6 +872,7 @@ fn parse_full(mut s: Bytes) -> Result<Uri, InvalidUri> {
 
     let authority = s.split_to(authority_end);
     let authority = Authority {
+        // SAFETY: previously verified by `Authority::parse`
         data: unsafe { ByteStr::from_utf8_unchecked(authority) },
     };