Turn on pedantic clippy lints where possible (#886)

* Deny clippy::all lint group

* clippy::drop_non_drop

These drop calls seem not to be necessary.

* clippy::cast_lossless

Casts, i.e., the `as` syntax, should generally be avoided where
possible. They truncate by default, which is dangerous if it isn't what
you want.

This lint complains about `as` when the cast is lossless, which means
you can use `T::from` or `.into()`. I tried to use `.into()` where
possible due to type inference, becuase it's slightly nicer, and avoids
having to name the types involved.

* clippy::checked_conversions

This avoids an assert, because on the next line we do the conversion with
an unwrap, which has the same effect.

* clippy::cloned_instead_of_copied

Copied should generally be used where possible, because all types which
implement copy implement clone, but not all types which implement copy,
so using `.copied()` gives the additional information that the copy is
cheap.

* clippy::default_trait_access

This one is just kind of annoying, forcing you to write `T::default()`
instead of `Default::default()`, so just ignore it.

* clippy::doc_markdown

Clippy wants items in markdown documentation to be inside backticks. Why
not, since it makes them formatted nicely.

* Ignore benchmark and test temporary files

* clippy::explicit_iter_loop

`for x in foo.iter()` is equivalent to `for x in &foo`, which is shorter
and more idiomatic

* Allow clippy::if_not_else

This complains if you do `if !x { … } else { … }`, on the basis that you
could avoid the `!` and just switch the bodies of the if and else. I
like this one, but it's subjective and creates a lot of code churn, so
I allowed it. If you like it, think about removing it from the allow
block.

* Allow clippy::inline_always

Clippy doesn't like this and suggests always profiling first, but I have
no idea if these instances are correct or not, so allow them.

* Allow clippy::iter_not_returning_iterator

Clippy complains if the return type from a function called `iter` does
not implement `Iterator`. This is an API break or a new implementation
of `Iterator`, so just allow it.

* Allow clippy::let_underscore_drop

Clippy doesn't like if you use `let _ = x` when `x` implements `Drop`,
since it's not necessarily clear that `_` will invoke `Drop`. (Unlike an
`_`-prefixed identifier, which will not drop.) This is kind of
subjective, so allow it.

* Deny clippy::map_unwrap_or

Clippy prefers `map_or` to `map + unwrap_or`. Why not.

* Allow clippy::missing_errors_doc and clippy::missing_panics_doc

Clippy wants you to write docs about panics and errors. Probably a good
idea, but who has time for that. Allow it.

* Deny clippy::match_wildcard_for_single_variants

Replace a `_` match for the single variant it matches. Seems reasonable.

* Allow clippy::module_name_repetitions

Clippy doesn't like repitition of module names inside types. Who cares.
Allow it.

* Allow clippy::needless_pass_by_value

Clippy complains if you pass a type by value when it isn't consumed, so
it could be passed by reference. I think this should be evaluated on a
case-by-case basis, depending on ergonomics, so allow it.

* Allow clippy::option_option

`Option<Option<T>>` is clearly an insane type, but I dare not touch it,
because I have no idea what it's supposed to mean, so allow it.

* Deny clippy::range_plus_one

Using `a..=b` is clearer and shorter than `a..(b + 1)`.

* Deny clippy::type_repetition_in_bounds

In these `where` clauses, `'b` outlives `'a`, so it's enough to declare
that `Self: 'b`.

* Deny clippy::uninlined_format_args

This is nice, since it reminds you where you can inline things in format
strings.

* Allow clippy::unnecessary_wraps

This should probably be denied, but in some cases, a function returns
`Result` or `Option` for API compatibility reasons, and I didn't want to
wade into that.

* Deny clippy::semicolon_if_nothing_returned

You can omit a semicolon after a `()` expression, but seems fine to
deny it for consistent formatting. Add an exception for this lunacy:

    fn from_bytes<'a>(_data: &'a [u8]) -> ()
    where
        Self: 'a,
    {
        ()
    }

* Allow clippy::too_many_lines

You're not the boss of me.

* Allow clippy::similar_names

This is just silly. I'll make similar names if I want to.

* Allow clippy::wildcard_imports

More nanny-state bullshit.

* Allow clippy::unreadable_literal

I do actually think that most of these could use some `_` separators,
the decimal literals should be in groups of three, and the hex should be
maybe every 4 bytes, or something like that, but this is subjective.

* Deny clippy::redundant_else

These seem reasonable to remove. These else blocks are after an if block
which either returns or breaks the loop, so these get rid of some nesting.

* Deny clippy::unused_self

It seems clearer to avoid taking `&self` if it isn't used, and these
aren't public APIs, so it's backwards compatible.

* Allow clippy::must_use_candidate

This is a reasonable lint, but it's subjective and would be a larger
change, so I was too lazy to do it.

* Deny clippy::match_same_arms

Match arms with duplicate captures and bodies can be deduplicated. Seems
reasonable to me.

* Deny clippy::trivially_copy_pass_by_ref

This seems reasonable, small copy values should be passed by value.

* Allow clippy::redundant_closure_for_method_calls

I think this is actually a good one, but fixing it was annoying, so
allow it.

* Deny clippy::transmute_ptr_to_ptr

Clippy doesn't like mem::transmute for transmuting pointers to pointers.
I think the logic is that `mem::transmute` will transmute absolutely any
type into any other type, as long as they're the same size. A pointer
cast is much more limited, so it seems reasonable to use it instead.

* Sort and clean up allow and deny blocks

* Use pointer::cast

* Revert changes to xxh3 and allow lints

.gitignore

@@ -20,3 +20,6 @@ Cargo.lock
 # Profiling
 perf.data*
 flamegraph.svg
+
+# benchmark and test temporary files
+/.tmp*

src/complex_types.rs

@@ -7,12 +7,11 @@ fn encode_varint_len(len: usize, output: &mut Vec<u8>) {
     } else if len <= u16::MAX.into() {
         let u16_len: u16 = len.try_into().unwrap();
         output.push(254);
-        output.extend_from_slice(&u16_len.to_le_bytes())
+        output.extend_from_slice(&u16_len.to_le_bytes());
     } else {
-        assert!(len <= u32::MAX as usize);
         let u32_len: u32 = len.try_into().unwrap();
         output.push(255);
-        output.extend_from_slice(&u32_len.to_le_bytes())
+        output.extend_from_slice(&u32_len.to_le_bytes());
     }
 }
 
@@ -67,7 +66,6 @@ impl<T: Value> Value for Vec<T> {
 
     fn as_bytes<'a, 'b: 'a>(value: &'a Vec<T::SelfType<'b>>) -> Vec<u8>
     where
-        Self: 'a,
         Self: 'b,
     {
         let mut result = if let Some(width) = T::fixed_width() {

src/db.rs

@@ -453,9 +453,9 @@ impl Database {
 
             if !progress {
                 break;
-            } else {
-                compacted = true;
             }
+
+            compacted = true;
         }
 
         Ok(compacted)
@@ -804,7 +804,7 @@ impl RepairSession {
         self.aborted
     }
 
-    /// Abort the repair process. The coorresponding call to [Builder::open] or [Builder::create] will return an error
+    /// Abort the repair process. The coorresponding call to [`Builder::open`] or [`Builder::create`] will return an error
     pub fn abort(&mut self) {
         self.aborted = true;
     }
@@ -852,7 +852,7 @@ impl Builder {
     /// Set a callback which will be invoked periodically in the event that the database file needs
     /// to be repaired.
     ///
-    /// The [RepairSession] argument can be used to control the repair process.
+    /// The [`RepairSession`] argument can be used to control the repair process.
     ///
     /// If the database file needs repair, the callback will be invoked at least once.
     /// There is no upper limit on the number of times it may be called.

src/error.rs

@@ -109,7 +109,7 @@ impl TableError {
             | TableError::TypeDefinitionChanged { .. }
             | TableError::TableDoesNotExist(_)
             | TableError::TableAlreadyOpen(_, _) => {
-                StorageError::Corrupted(format!("{}: {}", msg, &self))
+                StorageError::Corrupted(format!("{msg}: {self}"))
             }
             TableError::Storage(storage) => storage,
         }

src/lib.rs

@@ -1,10 +1,22 @@
-#![allow(clippy::drop_non_drop)]
-#![deny(
-    clippy::cast_possible_truncation,
-    clippy::cast_possible_wrap,
-    clippy::cast_precision_loss,
-    clippy::cast_sign_loss,
-    clippy::disallowed_methods
+#![deny(clippy::all, clippy::pedantic, clippy::disallowed_methods)]
+#![allow(
+    clippy::default_trait_access,
+    clippy::if_not_else,
+    clippy::inline_always,
+    clippy::iter_not_returning_iterator,
+    clippy::let_underscore_drop,
+    clippy::missing_errors_doc,
+    clippy::missing_panics_doc,
+    clippy::module_name_repetitions,
+    clippy::must_use_candidate,
+    clippy::needless_pass_by_value,
+    clippy::option_option,
+    clippy::redundant_closure_for_method_calls,
+    clippy::similar_names,
+    clippy::too_many_lines,
+    clippy::unnecessary_wraps,
+    clippy::unreadable_literal,
+    clippy::wildcard_imports
 )]
 // TODO remove this once wasi no longer requires nightly
 #![cfg_attr(target_os = "wasi", feature(wasi_ext))]

src/multimap_table.rs

@@ -313,7 +313,6 @@ pub(crate) fn finalize_tree_and_subtree_checksums(
                 }
             }
         }
-        drop(accessor);
         // TODO: maybe there's a better abstraction, so that we don't need to call into this low-level method?
         let mut mutator = LeafMutator::new(
             &mut leaf_page,
@@ -525,7 +524,7 @@ impl Into<u8> for DynamicCollectionType {
 /// (when type = 2) root (8 bytes): sub tree root page number
 /// (when type = 2) checksum (16 bytes): sub tree checksum
 ///
-/// NOTE: Even though the [PhantomData] is zero-sized, the inner data DST must be placed last.
+/// NOTE: Even though the [`PhantomData`] is zero-sized, the inner data DST must be placed last.
 /// See [Exotically Sized Types](https://doc.rust-lang.org/nomicon/exotic-sizes.html#dynamically-sized-types-dsts)
 /// section of the Rustonomicon for more details.
 #[repr(transparent)]
@@ -563,7 +562,6 @@ impl<V: Key> Value for &DynamicCollection<V> {
 
     fn as_bytes<'a, 'b: 'a>(value: &'a Self::SelfType<'b>) -> &'a [u8]
     where
-        Self: 'a,
         Self: 'b,
     {
         &value.data
@@ -576,7 +574,7 @@ impl<V: Key> Value for &DynamicCollection<V> {
 
 impl<V: Key> DynamicCollection<V> {
     fn new(data: &[u8]) -> &Self {
-        unsafe { mem::transmute(data) }
+        unsafe { &*(data as *const [u8] as *const DynamicCollection<V>) }
     }
 
     fn collection_type(&self) -> DynamicCollectionType {
@@ -591,7 +589,7 @@ impl<V: Key> DynamicCollection<V> {
     fn as_subtree(&self) -> BtreeHeader {
         assert!(matches!(self.collection_type(), SubtreeV2));
         BtreeHeader::from_le_bytes(
-            self.data[1..(1 + BtreeHeader::serialized_size())]
+            self.data[1..=BtreeHeader::serialized_size()]
                 .try_into()
                 .unwrap(),
         )
@@ -702,7 +700,7 @@ impl UntypedDynamicCollection {
     }
 
     fn new(data: &[u8]) -> &Self {
-        unsafe { mem::transmute(data) }
+        unsafe { &*(data as *const [u8] as *const UntypedDynamicCollection) }
     }
 
     fn make_subtree_data(header: BtreeHeader) -> Vec<u8> {
@@ -727,7 +725,7 @@ impl UntypedDynamicCollection {
     fn as_subtree(&self) -> BtreeHeader {
         assert!(matches!(self.collection_type(), SubtreeV2));
         BtreeHeader::from_le_bytes(
-            self.data[1..(1 + BtreeHeader::serialized_size())]
+            self.data[1..=BtreeHeader::serialized_size()]
                 .try_into()
                 .unwrap(),
         )
@@ -851,7 +849,7 @@ impl<'a, V: Key + 'static> Drop for MultimapValue<'a, V> {
         drop(mem::take(&mut self.inner));
         if !self.free_on_drop.is_empty() {
             let mut freed_pages = self.freed_pages.as_ref().unwrap().lock().unwrap();
-            for page in self.free_on_drop.iter() {
+            for page in &self.free_on_drop {
                 if !self.mem.as_ref().unwrap().free_if_uncommitted(*page) {
                     freed_pages.push(*page);
                 }

src/table.rs

@@ -223,7 +223,7 @@ impl<'txn, K: Key + 'static, V: MutInPlaceValue + 'static> Table<'txn, K, V> {
     ///
     /// If key is already present it is replaced
     ///
-    /// The returned reference will have length equal to value_length
+    /// The returned reference will have length equal to `value_length`
     pub fn insert_reserve<'a>(
         &mut self,
         key: impl Borrow<K::SelfType<'a>>,
@@ -304,7 +304,7 @@ fn debug_helper<K: Key + 'static, V: Value + 'static>(
     first: Result<Option<(AccessGuard<K>, AccessGuard<V>)>>,
     last: Result<Option<(AccessGuard<K>, AccessGuard<V>)>>,
 ) -> std::fmt::Result {
-    write!(f, "Table [ name: \"{}\", ", name)?;
+    write!(f, "Table [ name: \"{name}\", ")?;
     if let Ok(len) = len {
         if len == 0 {
             write!(f, "No entries")?;

src/transaction_tracker.rs

@@ -16,11 +16,11 @@ impl TransactionId {
         Self(value)
     }
 
-    pub(crate) fn raw_id(&self) -> u64 {
+    pub(crate) fn raw_id(self) -> u64 {
         self.0
     }
 
-    pub(crate) fn next(&self) -> TransactionId {
+    pub(crate) fn next(self) -> TransactionId {
         TransactionId(self.0 + 1)
     }
 
@@ -30,7 +30,7 @@ impl TransactionId {
         next
     }
 
-    pub(crate) fn parent(&self) -> Option<TransactionId> {
+    pub(crate) fn parent(self) -> Option<TransactionId> {
         if self.0 == 0 {
             None
         } else {
@@ -43,7 +43,7 @@ impl TransactionId {
 pub(crate) struct SavepointId(pub u64);
 
 impl SavepointId {
-    pub(crate) fn next(&self) -> SavepointId {
+    pub(crate) fn next(self) -> SavepointId {
         SavepointId(self.0 + 1)
     }
 }
@@ -65,7 +65,6 @@ impl Value for SavepointId {
 
     fn as_bytes<'a, 'b: 'a>(value: &'a Self::SelfType<'b>) -> Self::AsBytes<'a>
     where
-        Self: 'a,
         Self: 'b,
     {
         value.0.to_le_bytes()
@@ -243,6 +242,6 @@ impl TransactionTracker {
             .live_read_transactions
             .keys()
             .next()
-            .cloned()
+            .copied()
     }
 }

src/transactions.rs

@@ -534,7 +534,7 @@ impl WriteTransaction {
         println!("Tables");
         for p in table_pages {
             all_allocated.remove(&p);
-            println!("{p:?}")
+            println!("{p:?}");
         }
 
         let system_table_allocators = self
@@ -551,15 +551,15 @@ impl WriteTransaction {
         println!("System tables");
         for p in system_table_pages {
             all_allocated.remove(&p);
-            println!("{p:?}")
+            println!("{p:?}");
         }
 
         println!("Free table");
         if let Some(freed_iter) = self.freed_tree.lock().unwrap().all_pages_iter().unwrap() {
             for p in freed_iter {
                 let p = p.unwrap();
                 all_allocated.remove(&p);
-                println!("{p:?}")
+                println!("{p:?}");
             }
         }
         println!("Pending free (i.e. in freed table)");
@@ -575,7 +575,7 @@ impl WriteTransaction {
             for i in 0..value.len() {
                 let p = value.get(i);
                 all_allocated.remove(&p);
-                println!("{p:?}")
+                println!("{p:?}");
             }
         }
         if !all_allocated.is_empty() {
@@ -1066,8 +1066,7 @@ impl WriteTransaction {
         let free_until_transaction = self
             .transaction_tracker
             .oldest_live_read_transaction()
-            .map(|x| x.next())
-            .unwrap_or(self.transaction_id);
+            .map_or(self.transaction_id, |x| x.next());
 
         let user_root = self
             .tables

src/tree_store/btree.rs

@@ -192,13 +192,11 @@ impl UntypedBtreeMut {
                     }
                 }
 
-                drop(accessor);
                 let mut mutator = BranchMutator::new(&mut page);
                 for (child_index, child_page, child_checksum) in new_children.into_iter().flatten()
                 {
                     mutator.write_child_page(child_index, child_page, child_checksum);
                 }
-                drop(mutator);
 
                 branch_checksum(&page, self.key_width)
             }
@@ -553,7 +551,7 @@ impl<K: Key + 'static, V: Value + 'static> BtreeMut<'_, K, V> {
 
 impl<'a, K: Key + 'a, V: MutInPlaceValue + 'a> BtreeMut<'a, K, V> {
     /// Reserve space to insert a key-value pair
-    /// The returned reference will have length equal to value_length
+    /// The returned reference will have length equal to `value_length`
     // Return type has the same lifetime as &self, because the tree must not be modified until the mutable guard is dropped
     pub(crate) fn insert_reserve(
         &mut self,
@@ -613,7 +611,7 @@ impl RawBtree {
     }
 
     pub(crate) fn len(&self) -> Result<u64> {
-        Ok(self.root.map(|x| x.length).unwrap_or(0))
+        Ok(self.root.map_or(0, |x| x.length))
     }
 
     pub(crate) fn verify_checksum(&self) -> Result<bool> {
@@ -811,7 +809,7 @@ impl<K: Key, V: Value> Btree<K, V> {
     }
 
     pub(crate) fn len(&self) -> Result<u64> {
-        Ok(self.root.map(|x| x.length).unwrap_or(0))
+        Ok(self.root.map_or(0, |x| x.length))
     }
 
     pub(crate) fn stats(&self) -> Result<BtreeStats> {

src/tree_store/btree_base.rs

@@ -324,9 +324,9 @@ impl<'a> LeafAccessor<'a> {
     pub(super) fn print_node<K: Key, V: Value>(&self, include_value: bool) {
         let mut i = 0;
         while let Some(entry) = self.entry(i) {
-            eprint!(" key_{}={:?}", i, K::from_bytes(entry.key()));
+            eprint!(" key_{i}={:?}", K::from_bytes(entry.key()));
             if include_value {
-                eprint!(" value_{}={:?}", i, V::from_bytes(entry.value()));
+                eprint!(" value_{i}={:?}", V::from_bytes(entry.value()));
             }
             i += 1;
         }
@@ -536,7 +536,7 @@ impl<'a, 'b> LeafBuilder<'a, 'b> {
     pub(super) fn push(&mut self, key: &'a [u8], value: &'a [u8]) {
         self.total_key_bytes += key.len();
         self.total_value_bytes += value.len();
-        self.pairs.push((key, value))
+        self.pairs.push((key, value));
     }
 
     pub(super) fn push_all_except(
@@ -897,7 +897,6 @@ impl<'b> LeafMutator<'b> {
             .value_range(i)
             .map(|(start, end)| end - start)
             .unwrap_or_default();
-        drop(accessor);
 
         let value_delta = if overwrite {
             isize::try_from(value.len()).unwrap() - isize::try_from(existing_value_len).unwrap()
@@ -1015,7 +1014,6 @@ impl<'b> LeafMutator<'b> {
         let value_start = accessor.value_start(i).unwrap();
         let value_end = accessor.value_end(i).unwrap();
         let last_value_end = accessor.value_end(accessor.num_pairs() - 1).unwrap();
-        drop(accessor);
 
         // Update all the pointers
         let key_ptr_size = if self.fixed_key_size.is_none() {
@@ -1112,7 +1110,6 @@ impl<'b> LeafMutator<'b> {
             self.fixed_value_size,
         );
         let num_pairs = accessor.num_pairs();
-        drop(accessor);
         let mut offset = 4 + size_of::<u32>() * i;
         if self.fixed_key_size.is_none() {
             offset += size_of::<u32>() * num_pairs;
@@ -1158,8 +1155,8 @@ impl<'a: 'b, 'b, T: Page + 'a> BranchAccessor<'a, 'b, T> {
         for i in 0..(self.count_children() - 1) {
             if let Some(child) = self.child_page(i + 1) {
                 let key = self.key(i).unwrap();
-                eprint!(" key_{}={:?}", i, K::from_bytes(key));
-                eprint!(" child_{}={:?}", i + 1, child);
+                eprint!(" key_{i}={:?}", K::from_bytes(key));
+                eprint!(" child_{}={child:?}", i + 1);
             }
         }
         eprint!("]");

src/tree_store/btree_iters.rs

@@ -33,8 +33,7 @@ pub enum RangeIterState {
 impl RangeIterState {
     fn page_number(&self) -> PageNumber {
         match self {
-            Leaf { page, .. } => page.get_page_number(),
-            Internal { page, .. } => page.get_page_number(),
+            Leaf { page, .. } | Internal { page, .. } => page.get_page_number(),
         }
     }
 
@@ -138,7 +137,7 @@ impl RangeIterState {
                         .entry_ranges(*entry)?;
                 Some(EntryGuard::new(page.clone(), key, value))
             }
-            _ => None,
+            Internal { .. } => None,
         }
     }
 }