Skip to content

Ensure &Node<T> is not removed #198

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
russellbanks wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Ensure &Node<T> is not removed #198

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4d3bc7a
Ensure `&Node<T>` is not removed
russellbanks May 7, 2026
d0a082e
Apply suggestions from code review
russellbanks May 30, 2026
22a15de
Apply suggestions from code review
russellbanks Jun 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
137 changes: 56 additions & 81 deletions indextree/src/arena.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,7 @@
use alloc::vec::Vec;

#[cfg(not(feature = "std"))]
use core::{
mem,
num::NonZeroUsize,
ops::{Index, IndexMut},
slice,
};
use core::{mem, num::NonZeroUsize};

#[cfg(feature = "par_iter")]
use rayon::prelude::*;
Expand All @@ -22,20 +17,15 @@ use rayon::prelude::*;
use serde::{Deserialize, Serialize};

#[cfg(feature = "std")]
use std::{
mem,
num::NonZeroUsize,
ops::{Index, IndexMut},
slice,
};
use std::{mem, num::NonZeroUsize};

use crate::{Node, NodeId, node::NodeData};

#[derive(PartialEq, Eq, Clone, Debug)]
#[cfg_attr(feature = "deser", derive(Deserialize, Serialize))]
/// An `Arena` structure containing certain [`Node`]s.
pub struct Arena<T> {
nodes: Vec<Node<T>>,
pub(crate) nodes: Vec<Node<T>>,
first_free_slot: Option<usize>,

@saschagrunert saschagrunert Jun 5, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Widening visibility to pub(crate) gives every module direct access to the raw vec, bypassing the removed-node filtering this PR introduces. Internal code can accidentally index a removed node without the compiler catching it.

Consider a pub(crate) fn get_node_unchecked(&self, id: NodeId) -> &Node<T> helper instead. That documents the intent ("I know this might be removed") while keeping the field private.

last_free_slot: Option<usize>,
}
Expand Down Expand Up @@ -84,7 +74,7 @@ impl<T> Arena<T> {
/// let node = arena.get(foo).unwrap();
///
/// let node_id = arena.get_node_id(node).unwrap();
/// assert_eq!(*arena[node_id].get(), "foo");
/// assert_eq!(arena.get(node_id).map(|node| *node.get()), Some("foo"));
/// ```
pub fn get_node_id(&self, node: &Node<T>) -> Option<NodeId> {
let nodes_range = self.nodes.as_ptr_range();
Expand Down Expand Up @@ -128,7 +118,7 @@ impl<T> Arena<T> {
let index0 = index.get() - 1; // we use 1 based indexing.
self.nodes
.get(index0)
.filter(|n| !n.is_removed())
.filter(|node| !node.is_removed())
.map(|node| NodeId::from_non_zero_usize(index, node.stamp))
}

Expand All @@ -141,11 +131,11 @@ impl<T> Arena<T> {
/// # Examples
///
/// ```
/// # use indextree::Arena;
/// # use indextree::{Arena, Node};
/// let mut arena = Arena::new();
/// let foo = arena.new_node("foo");
///
/// assert_eq!(*arena[foo].get(), "foo");
/// assert_eq!(arena.get(foo).map(Node::get), Some(&"foo"));
/// ```
pub fn new_node(&mut self, data: T) -> NodeId {
let (index, stamp) = if let Some(index) = self.pop_front_free_node() {
Expand All @@ -167,11 +157,9 @@ impl<T> Arena<T> {
/// Returns the number of nodes in the arena, including removed nodes.
///
/// Removed nodes are still counted because they remain in the
/// internal storage. Use [`iter()`] with [`Node::is_removed()`]
/// to count only live nodes.
/// internal storage. Use [`count()`] to count only live nodes.
///
/// [`iter()`]: Arena::iter
/// [`Node::is_removed()`]: crate::Node::is_removed
/// [`count()`]: Arena::count
///
/// # Examples
///
Expand All @@ -180,17 +168,39 @@ impl<T> Arena<T> {
/// let mut arena = Arena::new();
/// let foo = arena.new_node("foo");
/// let _bar = arena.new_node("bar");
/// assert_eq!(arena.count(), 2);
/// assert_eq!(arena.len(), 2);
///
/// foo.remove(&mut arena);
/// // The removed node is still counted.
/// assert_eq!(arena.len(), 2);
/// ```
#[inline]
pub fn len(&self) -> usize {
self.nodes.len()
}

/// Returns the number of non-removed nodes in the arena.
///
/// This function has a time complexity of O(n) since it needs to iterate
/// through the nodes to determine whether they have been removed.
///
/// # Examples
///
/// ```
/// # use indextree::Arena;
/// let mut arena = Arena::new();
/// let foo = arena.new_node("foo");
/// let _bar = arena.new_node("bar");
/// assert_eq!(arena.count(), 2);
///
/// foo.remove(&mut arena);
/// assert_eq!(arena.count(), 1);
/// ```
pub fn count(&self) -> usize {
self.nodes.len()
self.iter().count()
Comment thread
russellbanks marked this conversation as resolved.
}

/// Returns `true` if arena has no nodes, `false` otherwise.
/// Returns `true` if the arena has no nodes, `false` otherwise.
///
/// # Examples
///
Expand All @@ -205,13 +215,14 @@ impl<T> Arena<T> {
/// foo.remove(&mut arena);
/// assert!(!arena.is_empty());
/// ```
#[inline]
pub fn is_empty(&self) -> bool {
self.count() == 0
self.nodes.is_empty()

@saschagrunert saschagrunert Jun 5, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_empty() now checks the backing storage while count() only counts live nodes. After removing all nodes, count() == 0 is true but is_empty() is false. This inconsistency will likely surprise users.

Either align is_empty() with count() (e.g. self.iter().next().is_none()), or document that is_empty tracks storage occupancy, not live node count.

}

/// Returns a reference to the node with the given id if in the arena.
///
/// Returns `None` if not available.
/// Returns `None` if the node does not exist or has been removed.
///
/// # Examples
///
Expand All @@ -238,13 +249,15 @@ impl<T> Arena<T> {
/// assert!(another_arena.get(bar).is_none());
/// ```
pub fn get(&self, id: NodeId) -> Option<&Node<T>> {
self.nodes.get(id.index0())
self.nodes
.get(id.index0())
.filter(|node| !node.is_removed())
}

/// Returns a mutable reference to the node with the given id if in the
/// arena.
///
/// Returns `None` if not available.
/// Returns `None` if the node does not exist or has been removed.
///
/// # Examples
///
Expand All @@ -258,14 +271,13 @@ impl<T> Arena<T> {
/// assert_eq!(arena.get(foo).map(|node| *node.get()), Some("FOO!"));
/// ```
pub fn get_mut(&mut self, id: NodeId) -> Option<&mut Node<T>> {
self.nodes.get_mut(id.index0())
self.nodes
.get_mut(id.index0())
.filter(|node| !node.is_removed())
}

/// Returns an iterator of all nodes in the arena in storage-order.
///
/// Note that this iterator returns also removed elements, which can be
/// tested with the [`is_removed()`] method on the node.
///
/// # Examples
///
/// ```
Expand All @@ -288,21 +300,17 @@ impl<T> Arena<T> {
/// bar.remove(&mut arena);
///
/// let mut iter = arena.iter();
/// assert_eq!(iter.next().map(|node| (*node.get(), node.is_removed())), Some(("foo", false)));
/// assert_eq!(iter.next().map_or(false, |node| node.is_removed()), true);
/// assert_eq!(iter.next().map(|node| (*node.get(), node.is_removed())), None);
/// assert_eq!(iter.next().map(|node| *node.get()), Some("foo"));
/// assert_eq!(iter.next(), None);
/// ```
///
/// [`is_removed()`]: Node::is_removed
pub fn iter(&self) -> slice::Iter<'_, Node<T>> {
self.nodes.iter()
pub fn iter(&self) -> impl Iterator<Item = &Node<T>> {
self.nodes.iter().filter(|node| !node.is_removed())
}

/// Returns an iterator of [`NodeId`]s of all non-removed nodes in
/// the arena in storage-order.
///
/// Unlike [`iter()`], this skips removed nodes and yields `NodeId`s
/// instead of `&Node<T>`.
/// Unlike [`iter()`], yields `NodeId`s instead of `&Node<T>`.
///
/// # Examples
///
Expand Down Expand Up @@ -331,9 +339,6 @@ impl<T> Arena<T> {

/// Returns a mutable iterator of all nodes in the arena in storage-order.
///
/// Note that this iterator returns also removed elements, which can be
/// tested with the [`is_removed()`] method on the node.
///
/// # Example
///
/// ```
Expand All @@ -351,9 +356,8 @@ impl<T> Arena<T> {
/// let node_refs = arena.iter().map(|i| i.get().clone()).collect::<Vec<_>>();
/// assert_eq!(node_refs, vec![5, 6]);
/// ```
/// [`is_removed()`]: Node::is_removed
pub fn iter_mut(&mut self) -> slice::IterMut<'_, Node<T>> {
self.nodes.iter_mut()
pub fn iter_mut(&mut self) -> impl Iterator<Item = &mut Node<T>> {
self.nodes.iter_mut().filter(|node| !node.is_removed())
}

/// Clears all the nodes in the arena, but retains its allocated capacity.
Expand All @@ -372,22 +376,12 @@ impl<T> Arena<T> {
self.last_free_slot = None;
}

/// Returns a slice of the inner nodes collection.
///
/// The slice contains all nodes in storage order, including removed
/// nodes. Use [`Node::is_removed()`] to filter them out.
///
/// [`Node::is_removed()`]: crate::Node::is_removed
pub fn as_slice(&self) -> &[Node<T>] {
self.nodes.as_slice()
}

pub(crate) fn free_node(&mut self, id: NodeId) {
let node = &mut self[id];
let node = &mut self.nodes[id.index0()];
node.data = NodeData::NextFree(None);
node.stamp.as_removed();
let stamp = node.stamp;
if stamp.reuseable() {
if stamp.reusable() {
if let Some(index) = self.last_free_slot {
let new_last = id.index0();
self.nodes[index].data = NodeData::NextFree(Some(new_last));
Expand All @@ -407,7 +401,7 @@ impl<T> Arena<T> {
if let NodeData::NextFree(next_free) = self.nodes[index].data {
self.first_free_slot = next_free;
} else {
unreachable!("A data node considered as a freed node");
unreachable!("a data node considered as a freed node");
}
if self.first_free_slot.is_none() {
self.last_free_slot = None;
Expand All @@ -425,9 +419,6 @@ impl<T: Sync> Arena<T> {
/// Requires the `par_iter` feature. Uses [rayon](https://docs.rs/rayon)
/// for data parallelism across all nodes in storage order.
///
/// Note that this iterator returns also removed elements, which can be
/// tested with the [`is_removed()`] method on the node.
///
/// # Examples
///
/// ```
Expand All @@ -441,10 +432,8 @@ impl<T: Sync> Arena<T> {
/// let sum: i64 = arena.par_iter().map(|node| *node.get()).sum();
/// assert_eq!(sum, 6);
/// ```
///
/// [`is_removed()`]: Node::is_removed
pub fn par_iter(&self) -> rayon::slice::Iter<'_, Node<T>> {
self.nodes.par_iter()
pub fn par_iter(&self) -> impl ParallelIterator<Item = &Node<T>> {
self.nodes.par_iter().filter(|node| !node.is_removed())
}
}

Expand All @@ -458,20 +447,6 @@ impl<T> Default for Arena<T> {
}
}

impl<T> Index<NodeId> for Arena<T> {
type Output = Node<T>;

fn index(&self, node: NodeId) -> &Node<T> {
&self.nodes[node.index0()]
}
}

impl<T> IndexMut<NodeId> for Arena<T> {
fn index_mut(&mut self, node: NodeId) -> &mut Node<T> {
&mut self.nodes[node.index0()]
}
}

#[test]
fn reuse_node() {
let mut arena = Arena::new();
Expand Down
37 changes: 21 additions & 16 deletions indextree/src/debug_pretty_print.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use alloc::vec::Vec;
use std::vec::Vec;

use crate::{
Node,
arena::Arena,
id::NodeId,
traverse::{NodeEdge, Traverse},
Expand Down Expand Up @@ -238,7 +239,7 @@ pub struct DebugPrettyPrint<'a, T> {
impl<'a, T> DebugPrettyPrint<'a, T> {
/// Creates a new `DebugPrettyPrint` object for the node.
#[inline]
pub(crate) fn new(id: &'a NodeId, arena: &'a Arena<T>) -> Self {
pub(crate) const fn new(id: &'a NodeId, arena: &'a Arena<T>) -> Self {
Self { id, arena }
}
}
Expand All @@ -251,8 +252,7 @@ impl<T: fmt::Display> fmt::Display for DebugPrettyPrint<'_, T> {

// Print the first (root) node.
traverser.next();
{
let data = self.arena[*self.id].get();
if let Some(data) = self.arena.get(*self.id).map(Node::get) {

@saschagrunert saschagrunert Jun 5, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the root node is removed, this if let silently produces empty output with no indication anything was wrong. Could be confusing for someone debugging a tree.

Consider returning Err or printing a placeholder like <removed> so the caller knows the root was invalid.

if is_alternate {
write!(writer, "{data:#}")?
} else {
Expand All @@ -262,11 +262,12 @@ impl<T: fmt::Display> fmt::Display for DebugPrettyPrint<'_, T> {

// Print the descendants.
while let Some(id) = prepare_next_node_printing(&mut writer, &mut traverser)? {
let data = traverser.arena()[id].get();
if is_alternate {
write!(writer, "{data:#}")?
} else {
write!(writer, "{data}")?
if let Some(data) = traverser.arena().get(id).map(Node::get) {
if is_alternate {
write!(writer, "{data:#}")?
} else {
write!(writer, "{data}")?
}
}
}

Expand All @@ -282,8 +283,7 @@ impl<T: fmt::Debug> fmt::Debug for DebugPrettyPrint<'_, T> {

// Print the first (root) node.
traverser.next();
{
let data = self.arena[*self.id].get();
if let Some(data) = self.arena.get(*self.id).map(Node::get) {
if is_alternate {
write!(writer, "{data:#?}")?
} else {
Expand All @@ -293,11 +293,12 @@ impl<T: fmt::Debug> fmt::Debug for DebugPrettyPrint<'_, T> {

// Print the descendants.
while let Some(id) = prepare_next_node_printing(&mut writer, &mut traverser)? {
let data = traverser.arena()[id].get();
if is_alternate {
write!(writer, "{data:#?}")?
} else {
write!(writer, "{data:?}")?
if let Some(data) = traverser.arena().get(id).map(Node::get) {
if is_alternate {
write!(writer, "{data:#?}")?
} else {
write!(writer, "{data:?}")?
}
}
}

Expand Down Expand Up @@ -325,7 +326,11 @@ fn prepare_next_node_printing<T>(
}
}
};
let is_last_sibling = traverser.arena()[id].next_sibling().is_none();
let is_last_sibling = traverser
.arena()
.get(id)
.and_then(Node::next_sibling)
.is_none();
writer.open_item(is_last_sibling)?;

return Ok(Some(id));
Expand Down
Loading