From 9d2bd1cf50ce5eec8a1a117881d2d41da07604d7 Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Fri, 10 Nov 2023 04:25:00 -0500 Subject: [PATCH] Lots of lints and clippy things * use `[lints]` Cargo.toml sections * added a few fixme/todos to the code - we don't need to solve them now, but they should remind us to look at them at some point - might be a source of bugs there. --- Cargo.toml | 10 ++++++++++ src/async_reader.rs | 20 ++++++++++++-------- src/cache.rs | 2 +- src/directory.rs | 17 +++++++++-------- src/header.rs | 20 ++++++++++++++------ src/tile.rs | 3 ++- 6 files changed, 48 insertions(+), 24 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index bf7b7cf..9f280cf 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -43,3 +43,13 @@ tokio = { version = "1", features = ["test-util", "macros", "rt"] } [package.metadata.docs.rs] all-features = true + +[lints.rust] +unsafe_code = "forbid" +unused_qualifications = "warn" + +[lints.clippy] +pedantic = { level = "warn", priority = -1 } +missing_errors_doc = "allow" +module_name_repetitions = "allow" +similar_names = "allow" diff --git a/src/async_reader.rs b/src/async_reader.rs index 20820f8..48c56bd 100644 --- a/src/async_reader.rs +++ b/src/async_reader.rs @@ -1,3 +1,7 @@ +// FIXME: This seems like a bug - there are lots of u64 to usize conversions in this file, +// so any file larger than 4GB, or an untrusted file with bad data may crash. +#![allow(clippy::cast_possible_truncation)] + #[cfg(feature = "mmap-async-tokio")] use std::path::Path; @@ -30,18 +34,18 @@ pub struct AsyncPmTilesReader { } impl AsyncPmTilesReader { - /// Creates a new reader from a specified source and validates the provided PMTiles archive is valid. + /// Creates a new reader from a specified source and validates the provided `PMTiles` archive is valid. /// - /// Note: Prefer using new_with_* methods. + /// Note: Prefer using `new_with_*` methods. pub async fn try_from_source(backend: B) -> Result { Self::try_from_cached_source(backend, NoCache).await } } impl AsyncPmTilesReader { - /// Creates a new cached reader from a specified source and validates the provided PMTiles archive is valid. + /// Creates a new cached reader from a specified source and validates the provided `PMTiles` archive is valid. /// - /// Note: Prefer using new_with_* methods. + /// Note: Prefer using `new_with_*` methods. pub async fn try_from_cached_source(backend: B, cache: C) -> Result { // Read the first 127 and up to 16,384 bytes to ensure we can initialize the header and root directory. let mut initial_bytes = backend.read(0, MAX_INITIAL_BYTES).await?; @@ -216,7 +220,7 @@ impl AsyncPmTile #[cfg(feature = "http-async")] impl AsyncPmTilesReader { - /// Creates a new PMTiles reader from a URL using the Reqwest backend. + /// Creates a new `PMTiles` reader from a URL using the Reqwest backend. /// /// Fails if [url] does not exist or is an invalid archive. (Note: HTTP requests are made to validate it.) pub async fn new_with_url(client: Client, url: U) -> Result { @@ -226,7 +230,7 @@ impl AsyncPmTilesReader { #[cfg(feature = "http-async")] impl AsyncPmTilesReader { - /// Creates a new PMTiles reader with cache from a URL using the Reqwest backend. + /// Creates a new `PMTiles` reader with cache from a URL using the Reqwest backend. /// /// Fails if [url] does not exist or is an invalid archive. (Note: HTTP requests are made to validate it.) pub async fn new_with_cached_url( @@ -242,7 +246,7 @@ impl AsyncPmTilesReader { #[cfg(feature = "mmap-async-tokio")] impl AsyncPmTilesReader { - /// Creates a new PMTiles reader from a file path using the async mmap backend. + /// Creates a new `PMTiles` reader from a file path using the async mmap backend. /// /// Fails if [p] does not exist or is an invalid archive. pub async fn new_with_path>(path: P) -> Result { @@ -252,7 +256,7 @@ impl AsyncPmTilesReader { #[cfg(feature = "mmap-async-tokio")] impl AsyncPmTilesReader { - /// Creates a new cached PMTiles reader from a file path using the async mmap backend. + /// Creates a new cached `PMTiles` reader from a file path using the async mmap backend. /// /// Fails if [p] does not exist or is an invalid archive. pub async fn new_with_cached_path>(cache: C, path: P) -> Result { diff --git a/src/cache.rs b/src/cache.rs index 4bc5eee..bd9a844 100644 --- a/src/cache.rs +++ b/src/cache.rs @@ -44,7 +44,7 @@ impl DirectoryCache for NoCache { async fn insert_dir(&self, _offset: usize, _directory: Directory) {} } -/// A simple HashMap-based implementation of a PMTiles directory cache. +/// A simple HashMap-based implementation of a `PMTiles` directory cache. #[derive(Default)] pub struct HashMapCache { pub cache: Arc>>, diff --git a/src/directory.rs b/src/directory.rs index 14705d9..b985ec3 100644 --- a/src/directory.rs +++ b/src/directory.rs @@ -18,6 +18,7 @@ impl Debug for Directory { impl Directory { #[cfg(any(feature = "http-async", feature = "mmap-async-tokio"))] + #[must_use] pub fn find_tile_id(&self, tile_id: u64) -> Option<&DirEntry> { match self.entries.binary_search_by(|e| e.tile_id.cmp(&tile_id)) { Ok(idx) => self.entries.get(idx), @@ -27,7 +28,7 @@ impl Directory { if next_id > 0 { let previous_tile = self.entries.get(next_id - 1)?; if previous_tile.is_leaf() - || tile_id - previous_tile.tile_id < previous_tile.run_length as u64 + || tile_id - previous_tile.tile_id < u64::from(previous_tile.run_length) { return Some(previous_tile); } @@ -49,28 +50,28 @@ impl TryFrom for Directory { // Read tile IDs let mut next_tile_id = 0; - for entry in entries.iter_mut() { + for entry in &mut entries { next_tile_id += buffer.read_u64_varint()?; entry.tile_id = next_tile_id; } // Read Run Lengths - for entry in entries.iter_mut() { + for entry in &mut entries { entry.run_length = buffer.read_u32_varint()?; } // Read Lengths - for entry in entries.iter_mut() { + for entry in &mut entries { entry.length = buffer.read_u32_varint()?; } // Read Offsets let mut last_entry: Option<&DirEntry> = None; - for entry in entries.iter_mut() { + for entry in &mut entries { let offset = buffer.read_u64_varint()?; entry.offset = if offset == 0 { let e = last_entry.ok_or(PmtError::InvalidEntry)?; - e.offset + e.length as u64 + e.offset + u64::from(e.length) } else { offset - 1 }; @@ -116,7 +117,7 @@ mod tests { reader.read_exact(header_bytes.as_mut()).unwrap(); let header = Header::try_from_bytes(header_bytes.freeze()).unwrap(); - let mut directory_bytes = BytesMut::zeroed(header.root_length as usize); + let mut directory_bytes = BytesMut::zeroed(usize::try_from(header.root_length).unwrap()); reader.read_exact(directory_bytes.as_mut()).unwrap(); let mut decompressed = BytesMut::zeroed(directory_bytes.len() * 2); @@ -136,7 +137,7 @@ mod tests { // ...it breaks pattern on the 59th tile assert_eq!(directory.entries[58].tile_id, 58); assert_eq!(directory.entries[58].run_length, 2); - assert_eq!(directory.entries[58].offset, 422070); + assert_eq!(directory.entries[58].offset, 422_070); assert_eq!(directory.entries[58].length, 850); } } diff --git a/src/header.rs b/src/header.rs index 2f704f5..9db9df8 100644 --- a/src/header.rs +++ b/src/header.rs @@ -49,6 +49,7 @@ pub enum Compression { } impl Compression { + #[must_use] pub fn content_encoding(&self) -> Option<&'static str> { Some(match self { Compression::Gzip => "gzip", @@ -75,6 +76,7 @@ impl TryInto for u8 { #[cfg(feature = "tilejson")] impl Header { + #[must_use] pub fn get_tilejson(&self, sources: Vec) -> tilejson::TileJSON { tilejson::tilejson! { tiles: sources, @@ -85,19 +87,21 @@ impl Header { } } + #[must_use] pub fn get_bounds(&self) -> tilejson::Bounds { tilejson::Bounds::new( - self.min_longitude as f64, - self.min_latitude as f64, - self.max_longitude as f64, - self.max_latitude as f64, + f64::from(self.min_longitude), + f64::from(self.min_latitude), + f64::from(self.max_longitude), + f64::from(self.max_latitude), ) } + #[must_use] pub fn get_center(&self) -> tilejson::Center { tilejson::Center::new( - self.center_longitude as f64, - self.center_latitude as f64, + f64::from(self.center_longitude), + f64::from(self.center_latitude), self.center_zoom, ) } @@ -113,6 +117,7 @@ pub enum TileType { } impl TileType { + #[must_use] pub fn content_type(&self) -> &'static str { match self { TileType::Mvt => "application/vnd.mapbox-vector-tile", @@ -143,7 +148,9 @@ static V3_MAGIC: &str = "PMTiles"; static V2_MAGIC: &str = "PM"; impl Header { + #[allow(clippy::cast_precision_loss)] fn read_coordinate_part(mut buf: B) -> f32 { + // TODO: would it be more precise to do `((value as f64) / 10_000_000.) as f32` ? buf.get_i32_le() as f32 / 10_000_000. } @@ -195,6 +202,7 @@ impl Header { #[cfg(test)] mod tests { + #![allow(clippy::unreadable_literal, clippy::float_cmp)] use std::fs::File; use std::io::Read; use std::num::NonZeroU64; diff --git a/src/tile.rs b/src/tile.rs index 0d7005c..c55d2a6 100644 --- a/src/tile.rs +++ b/src/tile.rs @@ -4,7 +4,8 @@ pub(crate) fn tile_id(z: u8, x: u64, y: u64) -> u64 { return 0; } - let base_id: u64 = 1 + (1..z).map(|i| 4u64.pow(i as u32)).sum::(); + // TODO: minor optimization with bit shifting + let base_id: u64 = 1 + (1..z).map(|i| 4u64.pow(u32::from(i))).sum::(); let tile_id = hilbert_2d::u64::xy2h_discrete(x, y, z.into(), hilbert_2d::Variant::Hilbert);