Enabled the use of clippy for the project. (#144)

* Run clippy in CI
    - Require SAFETY documents in front of every unsafe block.
    - Set MSRV version in clippy.toml to avoid lints that break the MSRV
* Several fixes to make clippy happy
    - Ran cargo clippy --fix
    - Added some clippy exceptions
    - Added many SAFETY comments to unsafe blocks
* Tested MIRAI static analyzer to find "unintentional panics"
    - added some debug-build overflow checks to satsify MIRAI
    - Currently, installing MIRAI takes way too long in CI so it is
      disabled for now. Need to find a way to cache this.

Signed-off-by: Michael Rodler <[email protected]>
Co-authored-by: Michael Rodler <[email protected]>

Author: f0rki

.github/workflows/ci.yml

@@ -18,6 +18,7 @@ jobs:
       - msrv_x64
       - msrv_aarch64
       - miri
+      - clippy_check
     steps:
       - run: exit 0
 
@@ -158,6 +159,13 @@ jobs:
           command: build
           args: --target aarch64-unknown-linux-gnu
 
+  clippy_check:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v3
+      - name: Run clippy
+        run: cargo clippy --all-targets --all-features
+
   miri:
     name: Test with Miri
     runs-on: ubuntu-latest
@@ -176,6 +184,31 @@ jobs:
 
       - name: Test
         run: MIRIFLAGS="-Zmiri-tag-raw-pointers -Zmiri-check-number-validity" cargo miri test
+  #
+  # mirai:
+  #   name: MIRAI static analysis
+  #   runs-on: ubuntu-latest
+  #
+  #   steps:
+  #     - name: Checkout
+  #       uses: actions/checkout@v1
+  #
+  #     - name: Install Rust
+  #       uses: actions-rs/toolchain@v1
+  #       with:
+  #         profile: minimal
+  #         toolchain: nightly-2023-05-09
+  #         components: clippy, rustfmt, rustc-dev, rust-src, rust-std, llvm-tools-preview
+  #         override: true
+  #
+  #     - name: install mirai
+  #       run: cargo install --locked --git https://github.com/facebookexperimental/MIRAI/ mirai
+  #       env:
+  #         # MIRAI_FLAGS: --diag=(default|verify|library|paranoid)
+  #         MIRAI_FLAGS: --diag=default
+  #
+  #     - name: cargo mirai
+  #       run: cargo mirai --lib
 
   aarch64:
     name: Test aarch64 (neon)

benches/parse.rs

@@ -112,9 +112,10 @@ fn header(c: &mut Criterion) {
         c.benchmark_group("header")
         .throughput(Throughput::Bytes(input.len() as u64))
         .bench_function(name, |b| b.iter(|| {
-            black_box({
+            {
                 let _ = httparse::parse_headers(input, &mut headers).unwrap();
-            });
+            };
+            black_box(());
         }));
     }
 

build.rs

@@ -6,7 +6,7 @@ fn main() {
     // We check rustc version to enable features beyond MSRV, such as:
     // - 1.59 => neon_intrinsics
     let rustc = env::var_os("RUSTC").unwrap_or(OsString::from("rustc"));
-    let output = Command::new(&rustc)
+    let output = Command::new(rustc)
         .arg("--version")
         .output()
         .expect("failed to check 'rustc --version'")
@@ -105,7 +105,7 @@ impl Version {
         let s = s.trim_start_matches("rustc ");
 
         let mut iter = s
-            .split(".")
+            .split('.')
             .take(3)
             .map(|s| match s.find(|c: char| !c.is_ascii_digit()) {
                 Some(end) => &s[..end],

clippy.toml

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

src/iter.rs

@@ -5,6 +5,7 @@ use core::convert::TryFrom;
 pub struct Bytes<'a> {
     start: *const u8,
     end: *const u8,
+    /// INVARIANT: start <= cursor && cursor <= end
     cursor: *const u8,
     phantom: core::marker::PhantomData<&'a ()>,
 }
@@ -14,6 +15,7 @@ impl<'a> Bytes<'a> {
     #[inline]
     pub fn new(slice: &'a [u8]) -> Bytes<'a> {
         let start = slice.as_ptr();
+        // SAFETY: obtain pointer to slice end; start points to slice start.
         let end = unsafe { start.add(slice.len()) };
         let cursor = start;
         Bytes {
@@ -32,7 +34,7 @@ impl<'a> Bytes<'a> {
     #[inline]
     pub fn peek(&self) -> Option<u8> {
         if self.cursor < self.end {
-            // SAFETY: bounds checked
+            // SAFETY:  bounds checked
             Some(unsafe { *self.cursor })
         } else {
             None
@@ -41,9 +43,11 @@ impl<'a> Bytes<'a> {
 
     #[inline]
     pub fn peek_ahead(&self, n: usize) -> Option<u8> {
+        // SAFETY: obtain a potentially OOB pointer that is later compared against the `self.end`
+        // pointer.
         let ptr = unsafe { self.cursor.add(n) };
         if ptr < self.end {
-            // SAFETY: bounds checked
+            // SAFETY: bounds checked pointer dereference is safe
             Some(unsafe { *ptr })
         } else {
             None
@@ -58,11 +62,21 @@ impl<'a> Bytes<'a> {
         self.as_ref().get(..n)?.try_into().ok()
     }
 
+    /// Advance by 1, equivalent to calling `advance(1)`.
+    ///
+    /// # Safety
+    /// 
+    /// Caller must ensure that Bytes hasn't been advanced/bumped by more than [`Bytes::len()`].
     #[inline]
     pub unsafe fn bump(&mut self) {
         self.advance(1)
     }
 
+    /// Advance cursor by `n`
+    ///
+    /// # Safety
+    /// 
+    /// Caller must ensure that Bytes hasn't been advanced/bumped by more than [`Bytes::len()`].
     #[inline]
     pub unsafe fn advance(&mut self, n: usize) {
         self.cursor = self.cursor.add(n);
@@ -74,15 +88,25 @@ impl<'a> Bytes<'a> {
         self.end as usize - self.cursor as usize
     }
 
+    #[inline]
+    pub fn is_empty(&self) -> bool {
+        self.len() == 0
+    }
+
     #[inline]
     pub fn slice(&mut self) -> &'a [u8] {
-        // not moving position at all, so it's safe
+        // SAFETY: not moving position at all, so it's safe
         let slice = unsafe { slice_from_ptr_range(self.start, self.cursor) };
         self.commit();
         slice
     }
 
     // TODO: this is an anti-pattern, should be removed
+    /// Deprecated. Do not use!
+    /// # Safety
+    /// 
+    /// Caller must ensure that `skip` is at most the number of advances (i.e., `bytes.advance(3)`
+    /// implies a skip of at most 3).
     #[inline]
     pub unsafe fn slice_skip(&mut self, skip: usize) -> &'a [u8] {
         debug_assert!(self.cursor.sub(skip) >= self.start);
@@ -96,6 +120,9 @@ impl<'a> Bytes<'a> {
         self.start = self.cursor
     }
 
+    /// # Safety
+    /// 
+    /// see [`Bytes::advance`] safety comment.
     #[inline]
     pub unsafe fn advance_and_commit(&mut self, n: usize) {
         self.advance(n);
@@ -117,6 +144,9 @@ impl<'a> Bytes<'a> {
         self.end
     }
 
+    /// # Safety
+    /// 
+    /// Must ensure invariant `bytes.start() <= ptr && ptr <= bytes.end()`.
     #[inline]
     pub unsafe fn set_cursor(&mut self, ptr: *const u8) {
         debug_assert!(ptr >= self.start);
@@ -128,10 +158,14 @@ impl<'a> Bytes<'a> {
 impl<'a> AsRef<[u8]> for Bytes<'a> {
     #[inline]
     fn as_ref(&self) -> &[u8] {
+        // SAFETY: not moving position at all, so it's safe
         unsafe { slice_from_ptr_range(self.cursor, self.end) }
     }
 }
 
+/// # Safety
+///
+/// Must ensure start and end point to the same memory object to uphold memory safety.
 #[inline]
 unsafe fn slice_from_ptr_range<'a>(start: *const u8, end: *const u8) -> &'a [u8] {
     debug_assert!(start <= end);
@@ -144,7 +178,7 @@ impl<'a> Iterator for Bytes<'a> {
     #[inline]
     fn next(&mut self) -> Option<u8> {
         if self.cursor < self.end {
-            // SAFETY: bounds checked
+            // SAFETY: bounds checked dereference
             unsafe {
                 let b = *self.cursor;
                 self.bump();