diff --git a/src/game.rs b/src/game.rs index 61f9e71..05cf20a 100644 --- a/src/game.rs +++ b/src/game.rs @@ -18,7 +18,6 @@ pub mod bytes; pub mod card; pub mod deck; -mod util; pub mod validation; // Exports diff --git a/src/game/card/digimon.rs b/src/game/card/digimon.rs index bf30835..1f59b30 100644 --- a/src/game/card/digimon.rs +++ b/src/game/card/digimon.rs @@ -1,13 +1,15 @@ #![doc(include = "digimon.md")] // Imports -use crate::game::{ - card::property::{self, ArrowColor, CrossMoveEffect, Effect, EffectCondition, Level, Move, Speciality}, +use crate::{ + game::{ + card::property::{self, ArrowColor, CrossMoveEffect, Effect, EffectCondition, Level, Move, Speciality}, + Bytes, + }, util::{ array_split, array_split_mut, null_ascii_string::{self, NullAsciiString}, }, - Bytes, }; use byteorder::{ByteOrder, LittleEndian}; diff --git a/src/game/card/digivolve.rs b/src/game/card/digivolve.rs index 3c9ba90..d554ff7 100644 --- a/src/game/card/digivolve.rs +++ b/src/game/card/digivolve.rs @@ -1,12 +1,12 @@ #![doc(include = "digivolve.md")] // Imports -use crate::game::{ +use crate::{ + game::Bytes, util::{ array_split, array_split_mut, null_ascii_string::{self, NullAsciiString}, }, - Bytes, }; /// A digivolve card diff --git a/src/game/card/item.rs b/src/game/card/item.rs index 190259c..74902e6 100644 --- a/src/game/card/item.rs +++ b/src/game/card/item.rs @@ -1,13 +1,15 @@ #![doc(include = "item.md")] // Imports -use crate::game::{ - card::property::{self, ArrowColor, Effect, EffectCondition}, +use crate::{ + game::{ + card::property::{self, ArrowColor, Effect, EffectCondition}, + Bytes, + }, util::{ array_split, array_split_mut, null_ascii_string::{self, NullAsciiString}, }, - Bytes, }; use byteorder::{ByteOrder, LittleEndian}; diff --git a/src/game/card/property/effect.rs b/src/game/card/property/effect.rs index 2df6e9c..bb85812 100644 --- a/src/game/card/property/effect.rs +++ b/src/game/card/property/effect.rs @@ -1,10 +1,12 @@ #![doc(include = "effect.md")] // Imports -use crate::game::{ - card::property::{self, AttackType, DigimonProperty, EffectOperation, PlayerType, Slot}, +use crate::{ + game::{ + card::property::{self, AttackType, DigimonProperty, EffectOperation, PlayerType, Slot}, + Bytes, + }, util::{array_split, array_split_mut}, - Bytes, }; use byteorder::{ByteOrder, LittleEndian}; diff --git a/src/game/card/property/effect_condition.rs b/src/game/card/property/effect_condition.rs index a03feb8..4255aae 100644 --- a/src/game/card/property/effect_condition.rs +++ b/src/game/card/property/effect_condition.rs @@ -1,10 +1,12 @@ #![doc(include = "effect_condition.md")] // Imports -use crate::game::{ - card::property::{self, DigimonProperty, EffectConditionOperation}, +use crate::{ + game::{ + card::property::{self, DigimonProperty, EffectConditionOperation}, + Bytes, + }, util::{array_split, array_split_mut}, - Bytes, }; use byteorder::{ByteOrder, LittleEndian}; diff --git a/src/game/card/property/moves.rs b/src/game/card/property/moves.rs index 30316be..c5969eb 100644 --- a/src/game/card/property/moves.rs +++ b/src/game/card/property/moves.rs @@ -5,12 +5,12 @@ mod test; // Imports -use crate::game::{ +use crate::{ + game::{Bytes, Validatable, Validation}, util::{ array_split, array_split_mut, null_ascii_string::{self, NullAsciiString}, }, - Bytes, Validatable, Validation, }; use byteorder::{ByteOrder, LittleEndian}; diff --git a/src/game/card/table.rs b/src/game/card/table.rs index 3b73540..9f84126 100644 --- a/src/game/card/table.rs +++ b/src/game/card/table.rs @@ -8,13 +8,16 @@ use crate::{ property::{self, CardType}, Digimon, Digivolve, Item, }, - util::array_split_mut, Bytes, }, io::{address::Data, GameFile}, + util::array_split_mut, }; use byteorder::{ByteOrder, LittleEndian}; -use std::io::{Read, Seek, Write}; +use std::{ + convert::TryInto, + io::{Read, Seek, Write}, +}; /// The table storing all cards #[derive(Debug)] @@ -291,9 +294,9 @@ impl Table { } // Then check the number of each card - let digimon_cards = LittleEndian::read_u16(&header_bytes[0x4..0x6]) as usize; - let item_cards = header_bytes[0x6] as usize; - let digivolve_cards = header_bytes[0x7] as usize; + let digimon_cards: usize = LittleEndian::read_u16(&header_bytes[0x4..0x6]).into(); + let item_cards: usize = header_bytes[0x6].into(); + let digivolve_cards: usize = header_bytes[0x7].into(); log::debug!("[Table Header] Found {} digimon cards", digimon_cards); log::debug!("[Table Header] Found {} item cards", item_cards); log::debug!("[Table Header] Found {} digivolve cards", digivolve_cards); @@ -409,13 +412,15 @@ impl Table { LittleEndian::write_u32(bytes.magic, Self::HEADER_MAGIC); // Write card lens - use std::convert::TryInto; log::debug!("[Table Header] Writing {} digimon cards", self.digimons.len()); log::debug!("[Table Header] Writing {} item cards", self.items.len()); log::debug!("[Table Header] Writing {} digivolve cards", self.digivolves.len()); - LittleEndian::write_u16(bytes.digimons_len, self.digimons.len().try_into().expect("Too many digimons")); - *bytes.items_len = self.items.len().try_into().expect("Too many items"); - *bytes.digivolves_len = self.digivolves.len().try_into().expect("Too many digivolves"); + LittleEndian::write_u16( + bytes.digimons_len, + self.digimons.len().try_into().expect("Number of digimon cards exceeded `u16`"), + ); + *bytes.items_len = self.items.len().try_into().expect("Number of item cards exceeded `u8`"); + *bytes.digivolves_len = self.digivolves.len().try_into().expect("Number of digivolve cards exceeded `u8`"); } file.write_all(&header_bytes).map_err(SerializeError::WriteHeader)?; @@ -435,7 +440,7 @@ impl Table { ); // Write the header - LittleEndian::write_u16(bytes.header_id, cur_id as u16); + LittleEndian::write_u16(bytes.header_id, cur_id.try_into().expect("Card ID exceeded `u16`")); CardType::Digimon.to_bytes(bytes.header_type)?; // Write the digimon @@ -464,7 +469,7 @@ impl Table { ); // Write the header - LittleEndian::write_u16(bytes.header_id, cur_id as u16); + LittleEndian::write_u16(bytes.header_id, cur_id.try_into().expect("Card ID exceeded `u16`")); CardType::Item.to_bytes(bytes.header_type)?; // Write the item @@ -492,7 +497,7 @@ impl Table { ); // Write the header - LittleEndian::write_u16(bytes.header_id, cur_id as u16); + LittleEndian::write_u16(bytes.header_id, cur_id.try_into().expect("Card ID exceeded `u16`")); CardType::Digivolve.to_bytes(bytes.header_type)?; // Write the digivolve diff --git a/src/game/deck/deck.rs b/src/game/deck/deck.rs index 2438e68..579382a 100644 --- a/src/game/deck/deck.rs +++ b/src/game/deck/deck.rs @@ -1,12 +1,12 @@ //! Decks // Imports -use crate::game::{ +use crate::{ + game::Bytes, util::{ array_split, array_split_mut, null_ascii_string::{self, NullAsciiString}, }, - Bytes, }; use byteorder::{ByteOrder, LittleEndian}; diff --git a/src/game/util.rs b/src/game/util.rs deleted file mode 100644 index cc5b8e3..0000000 --- a/src/game/util.rs +++ /dev/null @@ -1,14 +0,0 @@ -//! Utility macros and functions -//! -//! This modules is used for miscellaneous macros, functions that have -//! not been moved to a more permanent location. -//! -//! All items in this module will eventually be deprecated and moved -//! somewhere else, but this change might take some time. - -// Modules -pub mod array_split; -pub mod null_ascii_string; - -// Exports -pub use array_split::{array_split, array_split_mut}; diff --git a/src/io/address/data.rs b/src/io/address/data.rs index dd2b8b1..15aae45 100644 --- a/src/io/address/data.rs +++ b/src/io/address/data.rs @@ -1,7 +1,10 @@ //! File data-only addresses // Imports -use crate::io::address::Real; +use crate::{ + io::address::Real, + util::{abs_diff, signed_offset}, +}; /// A type for defining addresses on the data parts of `.bin` file. /// @@ -49,11 +52,7 @@ impl std::ops::Add for Data { type Output = Self; fn add(self, offset: i64) -> Self { - if offset > 0 { - self + (offset as u64) - } else { - self - (-offset as u64) - } + Self::from(signed_offset(self.0, offset)) } } @@ -101,7 +100,7 @@ impl std::ops::Sub for Data { type Output = i64; fn sub(self, address: Self) -> i64 { - self.0 as i64 - address.0 as i64 + abs_diff(self.0, address.0) } } diff --git a/src/io/address/real.rs b/src/io/address/real.rs index 875d5c2..6435138 100644 --- a/src/io/address/real.rs +++ b/src/io/address/real.rs @@ -1,5 +1,8 @@ //! File real addresses +// Imports +use crate::util::{abs_diff, signed_offset}; + /// A type for defining addresses on the `.bin` file. /// /// All real addresses will depict the actual position @@ -68,7 +71,7 @@ impl std::ops::Add for Real { type Output = Self; fn add(self, offset: i64) -> Self { - Self::from((u64::from(self) as i64 + offset) as u64) + Self::from(signed_offset(self.into(), offset)) } } @@ -84,7 +87,7 @@ impl std::ops::Sub for Real { type Output = i64; fn sub(self, address: Self) -> i64 { - u64::from(self) as i64 - u64::from(address) as i64 + abs_diff(u64::from(self), u64::from(address)) } } diff --git a/src/io/game_file.rs b/src/io/game_file.rs index 8278211..4ccf6a5 100644 --- a/src/io/game_file.rs +++ b/src/io/game_file.rs @@ -4,7 +4,10 @@ // Imports use crate::io::address::{Data as DataAddress, Real as RealAddress, RealToDataError}; -use std::io::{Read, Seek, Write}; +use std::{ + convert::TryInto, + io::{Read, Seek, Write}, +}; /// A type that abstracts over a the game reader. /// @@ -76,7 +79,9 @@ impl Read for GameFile { if cur_real_address == data_section_end { // Seek ahead by skipping the footer and next header self.reader.seek(std::io::SeekFrom::Current( - (RealAddress::FOOTER_BYTE_SIZE + RealAddress::HEADER_BYTE_SIZE) as i64, + (RealAddress::FOOTER_BYTE_SIZE + RealAddress::HEADER_BYTE_SIZE) + .try_into() + .expect("Sector offset didn't fit into `u64`"), ))?; // And restart this loop @@ -95,7 +100,10 @@ impl Read for GameFile { ); // Check how many bytes we can read - let mut bytes_to_read = (data_section_end - cur_real_address) as usize; + // Note: Cannot overflow, max is `2048`, so an error means `cur_real_address` was past the data section end + let mut bytes_to_read = (data_section_end - cur_real_address) + .try_into() + .expect("Current address is past end of data"); // If we were to read more bytes than the buffer has, read less if bytes_to_read > buf.len() { @@ -144,7 +152,9 @@ impl Write for GameFile { if cur_real_address == data_section_end { // Seek ahead by skipping the footer and next header self.reader.seek(std::io::SeekFrom::Current( - (RealAddress::FOOTER_BYTE_SIZE + RealAddress::HEADER_BYTE_SIZE) as i64, + (RealAddress::FOOTER_BYTE_SIZE + RealAddress::HEADER_BYTE_SIZE) + .try_into() + .expect("Sector offset didn't fit into `u64`"), ))?; // And restart this loop @@ -162,8 +172,10 @@ impl Write for GameFile { cur_real_address.offset() ); - // Check how many bytes we can write - let mut bytes_to_write = (data_section_end - cur_real_address) as usize; + // Check how many bytes we can write, up to the buffer's len + let mut bytes_to_write = (data_section_end - cur_real_address) + .try_into() + .expect("Current address is past end of data"); // If we were to write more bytes than the buffer has, write less if bytes_to_write > buf.len() { diff --git a/src/lib.rs b/src/lib.rs index 0c86797..4922ede 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -46,17 +46,16 @@ #![allow(clippy::missing_inline_in_public_items)] // We prefer tail returns where possible, as they help with code readability in most cases. #![allow(clippy::implicit_return)] -// Very useful for arguments such as `arg: impl Into`, then used -// with `let arg = arg.into()`. As well as just going from `Option` -// to `T` without needing to change their names. +// We're fine with shadowing, as long as the variable is used for the same purpose. +// Hence why `clippy::shadow_unrelated` isn't allowed. #![allow(clippy::shadow_reuse, clippy::shadow_same)] -// We use `.expect("...")` when we either know we cannot panic or it -// is the safest alternative, as proceeding would corrupt the program state. +// We use `.expect("...")` when we either know it cannot panic or if +// it is the safest alternative. #![allow(clippy::expect_used)] -// Like-wise with `.expect()`, we use `unreachable!` when we know a branch +// Like-wise with `.expect("...")`, we use `unreachable!` / `todo!` when we know a branch // if unreachable, and if it ever does get reached, panicking would be the // safest option -#![allow(clippy::unreachable)] +#![allow(clippy::unreachable, clippy::todo)] // We find it more important to be able to copy paste literals such as `0xabcd1234` than // being able to read them, which does not provide many benefits #![allow(clippy::unreadable_literal, clippy::unseparated_literal_suffix)] @@ -64,7 +63,8 @@ #![allow(clippy::multiple_inherent_impl)] // Many operations we need to repeat, and to keep symmetry #![allow(clippy::identity_op)] -// We only introduce items before their first usage, which sometimes is half-way through the code +// We only introduce items before their first usage, which sometimes is half-way through the code. +// We make sure that we only use the item after introduced, however. #![allow(clippy::items_after_statements)] // Useful for when they either change a lot with new variants / data, // or for symmetry purposes @@ -73,13 +73,12 @@ // will have it's own error type ideally, so any errors are explicit // by the type, without needing a section for them #![allow(clippy::missing_errors_doc)] -// Incomplete code should be tagged as `todo`. In future versions of the library, -// this lint may be removed, as incomplete code should not lie on a master branch. -#![allow(clippy::todo)] // Although we generally try to avoid this, this can happen due to our module organization. // In the future, this lint should be removed globally and only enabled for modules which // actually require the use of it. #![allow(clippy::module_inception, clippy::module_name_repetitions)] +// Banning arithmetic is too strict for this project +#![allow(clippy::integer_arithmetic)] // False positives: // TODO: Remove them in the future once they are no longer triggered. // We only slice arrays, which are verified at compile time. This @@ -87,19 +86,11 @@ #![allow(clippy::indexing_slicing)] // We don't have any `unsafe` impl for types that derive `Deserialize`. #![allow(clippy::unsafe_derive_deserialize)] -// Banning arithmetic is too strict for this project -#![allow(clippy::integer_arithmetic)] -// TODO: Remove once fixed -#![allow( - clippy::as_conversions, - clippy::cast_possible_wrap, - clippy::cast_sign_loss, - clippy::cast_possible_truncation -)] // Modules pub mod game; pub mod io; +mod util; // Exports pub use game::{Bytes, CardTable, Deck, DeckTable, Digimon, Digivolve, Item, Validatable, Validation}; diff --git a/src/util.rs b/src/util.rs new file mode 100644 index 0000000..02268d1 --- /dev/null +++ b/src/util.rs @@ -0,0 +1,56 @@ +//! Utility macros and functions +//! +//! This modules is used for miscellaneous macros, functions that have +//! not been moved to a more permanent location. +//! +//! All items in this module will eventually be deprecated and moved +//! somewhere else, but this change might take some time. + +// Modules +pub mod array_split; +pub mod null_ascii_string; + +// Exports +pub use array_split::{array_split, array_split_mut}; + + +/// Returns the absolute different between `a` and `b`, `a - b` as a `i64`. +/// +/// If the result would not fit into a `i64`, a panic occurs. +#[allow(clippy::as_conversions)] // We check every operation +#[allow(clippy::panic)] // Rust panics on failed arithmetic operations by default +pub fn abs_diff(a: u64, b: u64) -> i64 { + let diff = if a > b { a - b } else { b - a }; + + if diff > i64::MAX as u64 { + panic!("Overflow when computing signed distance between `u64`"); + } + + #[allow(clippy::cast_possible_wrap)] // We've verified, `diff` is less than `i64::MAX` + if a > b { + diff as i64 + } else { + -(diff as i64) + } +} + +/// Adds a `i64` to a `u64`, performing `a + b`. +/// +/// This function panics if the result would overflow or underflow +#[allow(clippy::as_conversions)] // We check every operation +#[allow(clippy::panic)] // Rust panics on failed arithmetic operations by default +#[allow(clippy::cast_sign_loss)] // We've verify it's positive +pub fn signed_offset(a: u64, b: i64) -> u64 { + // If `b` is positive, check for overflows. Else check for underflows + if b > 0 { + // Note: Cast is safe, as a positive `i64` fits into a `u64`. + a.checked_add(b as u64).expect("Overflow evaluating `u64 + i64`") + } else { + // Note: On `i64::MIN`, `-b` would overflow + if b == i64::MIN || a < (-b) as u64 { + panic!("Underflow evaluating `u64 + i64`"); + } else { + a - ((-b) as u64) + } + } +} diff --git a/src/game/util/array_split.rs b/src/util/array_split.rs similarity index 100% rename from src/game/util/array_split.rs rename to src/util/array_split.rs diff --git a/src/game/util/null_ascii_string.rs b/src/util/null_ascii_string.rs similarity index 100% rename from src/game/util/null_ascii_string.rs rename to src/util/null_ascii_string.rs diff --git a/src/game/util/null_ascii_string/error.rs b/src/util/null_ascii_string/error.rs similarity index 100% rename from src/game/util/null_ascii_string/error.rs rename to src/util/null_ascii_string/error.rs