Skip to content
This repository was archived by the owner on Nov 12, 2025. It is now read-only.

feat: only mark child changed if child is modified in TreeView #79

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+26 −10
Open

feat: only mark child changed if child is modified in TreeView #79

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
36 changes: 26 additions & 10 deletions src/ssz/tree_view.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,15 +78,29 @@ pub fn TreeView(comptime ST: type) type {
allocator: std.mem.Allocator,
pool: *Node.Pool,
data: Data,
parent_change: ?ParentChange = null,
pub const SszType: type = ST;

const Self = @This();

const ParentChange = struct {
map: *std.AutoArrayHashMap(Gindex, void), // Parent's data.changed
gindex: Gindex, // gindex of current node . If current is changed, this will be inserted to parent's data.changed
};

inline fn markChildChanged(self: *Self, gindex: Gindex) !void {
try self.data.changed.put(gindex, {});
if (self.parent_change) |parent_change| {
try parent_change.map.put(parent_change.gindex, {});
}
}

pub fn init(allocator: std.mem.Allocator, pool: *Node.Pool, root: Node.Id) !Self {
return Self{
.allocator = allocator,
.pool = pool,
.data = try Data.init(allocator, pool, root),
.parent_change = null,
};
}

Expand Down Expand Up @@ -144,27 +158,28 @@ pub fn TreeView(comptime ST: type) type {
} else {
const child_data = try self.getChildData(child_gindex);

// TODO only update changed if the subview is mutable
self.data.changed.put(child_gindex, void);

return TreeView(ST.Element){
.allocator = self.allocator,
.pool = self.pool,
.data = child_data,
Copy link

@gemini-code-assist gemini-code-assist bot Oct 21, 2025

Choose a reason for hiding this comment

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

critical

This line copies the child_data struct into the new TreeView. This leads to a critical issue: any modifications made to the returned sub-view (e.g., by calling setElement or setField on it) are applied to this temporary copy and are lost when the view goes out of scope.

While the new parent_change mechanism correctly marks the parent as dirty, the actual data change within the sub-view is not persisted in the parent's children_data cache. Consequently, when commit is called on the parent, the sub-view's changes are not included.

To fix this, TreeView should probably hold a pointer to Data (data: *Data) instead of a value (data: Data). This would ensure that sub-views modify the shared Data instance stored in the parent's cache.

.parent_change = .{
.map = &self.data.changed,
.gindex = child_gindex,
},
};
}
}

/// Set an element by index. If the element is a basic type, pass the value directly.
/// If the element is a complex type, pass a TreeView of the corresponding type.
/// If the element is a composite type, pass a TreeView of the corresponding type.
/// The caller transfers ownership of the `value` TreeView to this parent view.
/// The existing TreeView, if any, will be deinited by this function.
pub fn setElement(self: *Self, index: usize, value: Element) !void {
if (ST.kind != .vector and ST.kind != .list) {
@compileError("setElement can only be used with vector or list types");
}
const child_gindex = Gindex.fromDepth(ST.chunk_depth, index);
try self.data.changed.put(child_gindex, void);
try self.markChildChanged(child_gindex);
if (comptime isBasicType(ST.Element)) {
const child_node = try self.getChildNode(child_gindex);
try self.data.children_nodes.put(
Expand Down Expand Up @@ -214,19 +229,20 @@ pub fn TreeView(comptime ST: type) type {
} else {
const child_data = try self.getChildData(child_gindex);

// TODO only update changed if the subview is mutable
try self.data.changed.put(child_gindex, {});

return TreeView(ChildST){
.allocator = self.allocator,
.pool = self.pool,
.data = child_data,
Copy link

@gemini-code-assist gemini-code-assist bot Oct 21, 2025

Choose a reason for hiding this comment

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

critical

Similar to getElement, this line copies the child_data struct, which means any modifications on the returned TreeView are made on a temporary copy and will be lost.

This is a critical issue for correctness, as it prevents modifications to nested data structures from being persisted and committed. For the API to function as expected, TreeView should likely manage its Data via a pointer (data: *Data) to ensure all views operate on a shared data instance.

.parent_change = .{
.map = &self.data.changed,
.gindex = child_gindex,
},
};
}
}

/// Set a field by name. If the field is a basic type, pass the value directly.
/// If the field is a complex type, pass a TreeView of the corresponding type.
/// If the field is a composite type, pass a TreeView of the corresponding type.
/// The caller transfers ownership of the `value` TreeView to this parent view.
/// The existing TreeView, if any, will be deinited by this function.
pub fn setField(self: *Self, comptime field_name: []const u8, value: Field(field_name)) !void {
Expand All @@ -236,7 +252,7 @@ pub fn TreeView(comptime ST: type) type {
const field_index = comptime ST.getFieldIndex(field_name);
const ChildST = ST.getFieldType(field_name);
const child_gindex = Gindex.fromDepth(ST.chunk_depth, field_index);
try self.data.changed.put(child_gindex, {});
try self.markChildChanged(child_gindex);
if (comptime isBasicType(ChildST)) {
const opt_old_node = try self.data.children_nodes.fetchPut(
child_gindex,
Expand Down
Loading