Merged
Conversation
📝 WalkthroughWalkthroughAdds two new workspace crates (haya_ecs, haya_text), implements a no_std ECS core and text color/decoration utilities, refactors haya_ident to return an Ident type, adds i128/u128 serialization support in haya_ser, renames a test module, and updates workspace Cargo.toml (resolver and members). Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Around line 2-3: Remove the redundant explicit resolver = "3" declaration from
the Cargo.toml so the 2024 edition's default resolver is used; locate the line
containing resolver = "3" (near the members = [...] list) and delete it to keep
the manifest clean.
In `@haya_ecs/src/lib.rs`:
- Around line 186-194: The insert method currently calls
self.0.get_unchecked_mut(dense) in the else branch without validating dense,
which can UB if dense > self.0.len(); update the insert(&mut self, dense: usize,
value: T) implementation to first check that dense < self.0.len() before using
unsafe get_unchecked_mut (return an Err or panic or push to extend as
appropriate), or else make the method unsafe and add a clear "# Safety" doc
comment explaining the required invariant; reference the insert function, the
Vec stored in self.0, and the get_unchecked_mut/core::mem::replace usage when
applying the fix.
- Around line 267-276: The unsafe unwrap_unchecked in IterMut::next relies on
the invariant that self.comp and self.sparse iterate in lockstep (equal
lengths); document this safety invariant above the impl or the next method and
add a runtime debug check (e.g., debug_assert_eq!) that verifies the remaining
lengths/positions of self.comp and self.sparse before calling unwrap_unchecked
to catch misuse in debug builds; reference the IterMut type, its next method,
the comp and sparse fields, and the unsafe unwrap_unchecked call when adding the
comment and assertion.
- Around line 131-151: The remove() implementation updates sparse[last_sparse]
even when removing the last entity, re-enabling the removed entity; fix by only
updating the sparse entry when a swap actually occurred: compute last_index =
self.entities.len() - 1 (as you already read last_sparse), call
swap_remove(dense_index), and then only write back to self.sparse[last_sparse]
if dense_index != last_index (i.e. if the removed element was not the last
element); reference symbols: remove, sparse, entities, dense_index, last_sparse,
swap_remove.
- Around line 14-23: The alloc method in EntityAllocator can overflow when
self.next reaches u32::MAX (which conflicts with Dense::NONE sentinel); modify
EntityAllocator::alloc to detect this boundary and fail gracefully—change the
signature to return Option<Entity> (or Result) and return None (or Err) when
self.next == u32::MAX so you never produce an Entity with index u32::MAX; update
callers of EntityAllocator::alloc accordingly and document the new behavior.
In `@haya_ident/src/lib.rs`:
- Around line 76-90: Update the doc comment for Ident::new_unchecked to clearly
list the exact invariants callers must guarantee: that namespace (if Some) is
ASCII lowercase and contains only characters allowed by Minecraft resource
namespaces (lowercase a–z, digits 0–9, underscores, periods and hyphens), that
None for namespace means the default "minecraft", that path is non-empty, uses
'/'-separated segments with no empty segments, and contains only allowed ASCII
characters for resource paths (lowercase a–z, digits 0–9, underscores, periods,
hyphens and forward slashes), and that both namespace and path must be valid
UTF‑8 ASCII subsets as described; reference Ident::new_unchecked and note the
safety contract succinctly so callers can uphold these invariants when
constructing Ident values.
In `@haya_text/src/lib.rs`:
- Around line 216-225: The with_* methods currently set a non-zero sentinel for
None which breaks is_empty(); update with_obfuscated (mask 0x0003) and the other
with_* methods (with_bold mask 0x000C, with_strikethrough mask 0x0030,
with_underlined mask 0x00C0, with_italic mask 0x0300) so that their match arms
map None to 0x0000 instead of the non-zero sentinel and continue to compute the
new value as (self.value & !<mask>) | n; this ensures obfuscated(), bold(),
strikethrough(), underlined(), and italic() return None while is_empty() remains
true when all are cleared.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@haya_ecs/src/lib.rs`:
- Around line 182-183: The Component<T> tuple struct exposes its inner Vec<T>
publicly (Component<T>(pub Vec<T>)), which breaks encapsulation; change the
inner field to non-public (remove pub or make pub(crate)) and rely on the
existing components() and components_mut() accessors to control access and
mutations so the SparseSet stays consistent; update any call sites to use
Component::components() / components_mut() instead of accessing the inner Vec
directly.
- Around line 9-12: The EntityAllocator exposes internal fields next and free
publicly allowing external code to break invariants; make next and free private
(remove pub on those fields) and add a public constructor like
EntityAllocator::new() to initialize next and free safely, plus public methods
(e.g., allocate(), release(entity: Entity)) to manage IDs so callers cannot set
next or mutate free directly; update any call sites that accessed
EntityAllocator.next or EntityAllocator.free to use the new constructor and
allocator API.
- Around line 121-123: The current construction of Dense { index:
self.entities.len() as u32 } can silently truncate when entities.len() >
u32::MAX; change this to a checked conversion and handle the overflow instead of
blind casting: perform a bounds check (e.g., if self.entities.len() > u32::MAX)
or use a fallible conversion (try_into()) and propagate or return an error (or
panic with a clear message) so the overflow is detected, and make the same
bounded-check change in the related EntityAllocator logic; reference the Dense
struct construction and the EntityAllocator where indices are produced to locate
and fix the code paths that currently use as u32 casts.
- Around line 201-204: The current remove(&mut self, dense: usize) calls
self.0.swap_remove(dense) which will panic for out-of-bounds indices; change it
to perform a bounds check and return the removed element as an Option instead of
panicking (e.g., pub fn remove(&mut self, dense: usize) -> Option<T> and return
Some(self.0.swap_remove(dense)) when dense < self.0.len() else None), or if you
prefer the original panic behavior, add clear documentation to the remove method
that dense must be < length and that it will panic otherwise; update the
signature/usages and tests accordingly to handle the Option if you choose the
safe variant.
---
Duplicate comments:
In `@haya_ecs/src/lib.rs`:
- Around line 191-199: The insert() method currently uses unsafe
get_unchecked_mut(dense) without ensuring dense < self.0.len(), causing UB when
dense > len; update insert (the function named insert) to explicitly check
bounds: if dense == self.0.len() push the value as before, else if dense <
self.0.len() perform the replacement (prefer using safe get_mut or keep
get_unchecked_mut guarded by the check), and for dense > self.0.len() handle it
deterministically (e.g., panic with a clear message or return an Err/Option
indicating invalid index) instead of indexing unsafely into self.0. Ensure the
unsafe call is only reached after the explicit bounds check.
- Around line 14-23: EntityAllocator::alloc currently can produce an Entity with
index equal to Dense::NONE (u32::MAX) and then overflow next; update alloc to
prevent returning Dense::NONE and to handle exhaustion: if free.pop_front()
yields Some use it; otherwise before using self.next check if self.next ==
Dense::NONE and if so return an error/None (or change signature to
Result/Option) rather than wrapping or incrementing, or else reuse/compact from
the free list; update callers of alloc to handle the new Result/Option return
and ensure no allocation increments past Dense::NONE.
- Around line 241-254: The iterators created by iter and iter_mut assume comp
and sparse have equal lengths which is required for the subsequent unsafe
unwrap_unchecked; add a debug assertion at the start of both functions (iter and
iter_mut) that checks self.0.len() == sparse.entities().len() to catch
mismatches in debug builds. Reference the functions Iter/IterMut and the fields
comp and sparse so the assertion is placed before constructing Iter { comp:
self.0.iter(), sparse: sparse.entities().iter() } and IterMut { comp:
self.0.iter_mut(), sparse: sparse.entities().iter() } respectively.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@haya_ecs/src/lib.rs`:
- Around line 357-360: The test directly constructs EntityAllocator by setting
fields (let mut a = EntityAllocator { next: 0, free: VecDeque::new(), }), rather
than using the public constructor; change the test to call
EntityAllocator::new() to create the allocator (and remove manual field
initialization) so the test relies on the public API (EntityAllocator::new) and
remains stable against internal field changes.
---
Duplicate comments:
In `@haya_ecs/src/lib.rs`:
- Around line 214-217: The current remove(&mut self, dense: usize) -> T calls
self.0.swap_remove(dense) which will panic on out-of-bounds; change the API to
perform a bounds check and return an Option<T> (or Result<T, _>) instead of
panicking. Update the signature (remove) to return Option<T> and implement: if
dense < self.0.len() { Some(self.0.swap_remove(dense)) } else { None },
adjusting callers accordingly; reference the remove method and the use of
swap_remove when making the change.
- Around line 255-260: The iterator construction in iter(&self, sparse:
&SparseSet) can produce UB if comp and sparse lengths differ (Iter uses
unwrap_unchecked elsewhere); add a debug assertion at the start of iter to
validate the invariant (e.g., debug_assert_eq!(self.0.len(),
sparse.entities().len())) so misuse is caught in debug builds, then return the
Iter { comp: self.0.iter(), sparse: sparse.entities().iter() } as before
(referencing the iter function, Iter struct, SparseSet, and the places that use
unwrap_unchecked).
- Around line 133-136: The direct cast of self.entities.len() to u32 in the
Dense { index: self.entities.len() as u32 } can silently truncate when the
entity count exceeds u32::MAX; replace the unchecked cast with either (a) change
Dense.index to usize so it can hold large counts, or (b) use a checked
conversion (e.g. u32::try_from(self.entities.len()) and propagate or handle the
resulting error) to avoid silent truncation in the Dense construction (symbols
to edit: Dense, Dense::index, and the site creating Dense where
dense_ref.is_none()).
- Around line 28-36: alloc() can overflow when self.next reaches Dense::NONE
(u32::MAX) causing a sentinel conflict and panic/wrap; change alloc to detect
when self.next == Dense::NONE and never allocate that value: if free is
non-empty pop_front() as before, otherwise return an error/Result (or grow by
reusing ids from free) instead of incrementing past u32::MAX, and ensure Entity
creation and any callers handle the Result; update function signature (alloc ->
Result<Entity, AllocError>) or add explicit panic with a clear message if you
prefer failing fast, and reference the alloc function, self.next, Entity, free,
and Dense::NONE when making the changes.
- Around line 205-212: The insert method currently panics when dense >
self.0.len() because it indexes the Vec; add an explicit bounds check at the
start of insert (in function insert) and change the API to return a
Result<Option<T>, InsertError> (or similar) so out-of-range callers get an Err
rather than a panic; implement a small InsertError enum or use a clear error
type, return Ok(None) when dense == len after pushing, Ok(Some(old)) when
replacing, and Err(InsertError::OutOfBounds { dense, len: self.0.len() }) when
dense > self.0.len(); update callers/tests accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
API Changes