Implement dynamic table attributes to generalize the graphic-specific Table type#4050
Implement dynamic table attributes to generalize the graphic-specific Table type#4050
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the Table and TableRow structures to utilize a dynamic, columnar attribute system instead of fixed fields. However, the review highlights several critical issues that must be addressed. The TableRowMut implementation and its iterator are currently unsound, as they allow for multiple mutable references to the same data and potential data races. There are also bugs regarding invariant maintenance in AttributeColumn::push and significant data loss during serialization because the new attributes are ignored. Additionally, the find_or_create_column method causes silent data loss and disrupts attribute ordering upon type mismatches.
| pub struct TableRowMut<'a, T> { | ||
| pub element: &'a mut T, | ||
| pub transform: &'a mut DAffine2, | ||
| pub alpha_blending: &'a mut AlphaBlending, | ||
| pub source_node_id: &'a mut Option<NodeId>, | ||
| element: &'a mut T, | ||
| index: usize, | ||
| columns: *mut AttributeColumns, | ||
| _marker: std::marker::PhantomData<&'a mut AttributeColumns>, | ||
| } |
There was a problem hiding this comment.
The implementation of TableRowMut and its associated iterator TableRowIterMut is unsound. The iterator yields multiple TableRowMut instances that all share a raw mutable pointer to the same AttributeColumns store. This allows the creation of multiple overlapping mutable references (&mut AttributeColumns) to the same data, which is a violation of Rust's memory safety guarantees and leads to undefined behavior. Furthermore, the manual Send implementations (line 1117) allow these rows to be mutated concurrently from different threads without synchronization, causing data races.
| fn push(&mut self, value: Box<dyn AttributeValue>) { | ||
| if let Ok(value) = value.into_any().downcast::<T>() { | ||
| self.0.push(*value); | ||
| } | ||
| } |
There was a problem hiding this comment.
This implementation breaks the fundamental invariant of AttributeColumns that all columns must have the same length. If the downcast fails due to a type mismatch, no value is pushed to the column, causing it to become shorter than the others. This will lead to out-of-bounds access or panics in subsequent operations. A default value should be pushed if the type does not match. The same issue exists in the extend method on line 166.
| fn push(&mut self, value: Box<dyn AttributeValue>) { | |
| if let Ok(value) = value.into_any().downcast::<T>() { | |
| self.0.push(*value); | |
| } | |
| } | |
| fn push(&mut self, value: Box<dyn AttributeValue>) { | |
| if let Ok(value) = value.into_any().downcast::<T>() { | |
| self.0.push(*value); | |
| } else { | |
| self.0.push(T::default()); | |
| } | |
| } |
| fn find_or_create_column<T: Clone + Send + Sync + Default + Debug + 'static>(&mut self, key: &str) -> usize { | ||
| match self.columns.iter().position(|(k, _)| k == key) { | ||
| Some(position) => { | ||
| if (*self.columns[position].1).as_any().downcast_ref::<Column<T>>().is_some() { | ||
| position | ||
| } else { | ||
| self.columns.remove(position); | ||
| self.columns.push((key.to_string(), Box::new(Column::<T>(vec![T::default(); self.len])))); | ||
| self.columns.len() - 1 | ||
| } |
There was a problem hiding this comment.
This method causes silent data loss and disrupts column ordering. If a column with the requested key exists but has a different type, the entire column is deleted and replaced with defaults. This destroys existing data for all rows without warning. Additionally, by removing and re-pushing the column, its relative order in the table is changed, which may affect UI presentation. Type mismatches should be handled more gracefully, perhaps by returning a Result.
Keavon
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 issues found across 76 files
Confidence score: 1/5
node-graph/libraries/core-types/src/table.rshas a severity 10 issue with a manualSendimpl onTableRowMutthat appears unsound; shared unsynchronized raw pointers can allow concurrent mutation and undefined behavior, which is merge-blocking risk.- Two high-confidence invariant bugs in
node-graph/libraries/core-types/src/table.rs(Column<T>::extendandColumn<T>::push) silently skip type-mismatched writes whileAttributeColumns::lenstill increments, creating concrete out-of-bounds/regression risk later. - There are additional user-impacting correctness/perf issues (
Image to Bytesdropping alpha innode-graph/nodes/gstd/src/platform_application_io.rs, plus row-cloning overhead in raster/texture paths), so this is not just housekeeping. - Pay close attention to
node-graph/libraries/core-types/src/table.rs,node-graph/nodes/gstd/src/platform_application_io.rs,node-graph/libraries/wgpu-executor/src/texture_conversion.rs, andnode-graph/nodes/raster/src/std_nodes.rs- memory-safety, data-integrity, and output-format correctness are at risk.
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed. cubic prioritises the most important files to review.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="node-graph/nodes/gstd/src/platform_application_io.rs">
<violation number="1" location="node-graph/nodes/gstd/src/platform_application_io.rs:123">
P2: `Image to Bytes` currently serializes each pixel as RGB (3 bytes) instead of RGBA (4 bytes), so alpha is lost and output length is incorrect for RGBA consumers.</violation>
</file>
<file name="editor/src/messages/portfolio/document/overlays/utility_types_native.rs">
<violation number="1" location="editor/src/messages/portfolio/document/overlays/utility_types_native.rs:1178">
P2: Avoid repeated per-segment transform attribute cloning inside the inner loop; compute it once per row.</violation>
</file>
<file name="node-graph/nodes/raster/src/std_nodes.rs">
<violation number="1" location="node-graph/nodes/raster/src/std_nodes.rs:122">
P2: Avoid cloning the full raster row when only attributes are needed; this adds unnecessary image-buffer copies in `combine_channels`.</violation>
</file>
<file name="node-graph/libraries/wgpu-executor/src/texture_conversion.rs">
<violation number="1" location="node-graph/libraries/wgpu-executor/src/texture_conversion.rs:157">
P2: Avoid `row.into_cloned()` here; it clones the entire CPU raster just to copy attributes, causing unnecessary per-row image duplication and extra memory/CPU overhead.</violation>
</file>
<file name="node-graph/libraries/core-types/src/table.rs">
<violation number="1" location="node-graph/libraries/core-types/src/table.rs:149">
P1: Silent downcast failure in `Column<T>::push` breaks the column-length invariant. When the type doesn't match, no value is pushed but `AttributeColumns::len` still increments, causing later out-of-bounds accesses. Push a default on mismatch to maintain the invariant.</violation>
<violation number="2" location="node-graph/libraries/core-types/src/table.rs:166">
P1: Silent downcast failure in `Column<T>::extend` breaks the column-length invariant. When two tables have the same attribute key but different types, the column silently skips the extend while `AttributeColumns::len` still grows. Pad with defaults on mismatch.</violation>
<violation number="3" location="node-graph/libraries/core-types/src/table.rs:1117">
P0: The manual `Send` impl on `TableRowMut` is unsound because rows share an unsynchronized raw pointer to `AttributeColumns`, enabling concurrent mutation and undefined behavior.</violation>
</file>
<file name="node-graph/graph-craft/Cargo.toml">
<violation number="1" location="node-graph/graph-craft/Cargo.toml:25">
P3: Custom agent: **PR title enforcement**
PR title is not in the required imperative verb form.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| // for `'a`, and `AttributeColumns` is `Send`. The pointer is only used to split the borrow between | ||
| // the element slice and the column store, which are disjoint fields. | ||
| unsafe impl<T: Send> Send for TableRowIterMut<'_, T> {} | ||
| unsafe impl<T: Send> Send for TableRowMut<'_, T> {} |
There was a problem hiding this comment.
P0: The manual Send impl on TableRowMut is unsound because rows share an unsynchronized raw pointer to AttributeColumns, enabling concurrent mutation and undefined behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/core-types/src/table.rs, line 1117:
<comment>The manual `Send` impl on `TableRowMut` is unsound because rows share an unsynchronized raw pointer to `AttributeColumns`, enabling concurrent mutation and undefined behavior.</comment>
<file context>
@@ -237,88 +721,397 @@ unsafe impl<T: StaticTypeSized> StaticType for Table<T> {
+// for `'a`, and `AttributeColumns` is `Send`. The pointer is only used to split the borrow between
+// the element slice and the column store, which are disjoint fields.
+unsafe impl<T: Send> Send for TableRowIterMut<'_, T> {}
+unsafe impl<T: Send> Send for TableRowMut<'_, T> {}
</file context>
| } | ||
|
|
||
| /// Appends all values from another column, downcasting it to the same `Column<T>` type. | ||
| fn extend(&mut self, other: Box<dyn AttributeColumn>) { |
There was a problem hiding this comment.
P1: Silent downcast failure in Column<T>::extend breaks the column-length invariant. When two tables have the same attribute key but different types, the column silently skips the extend while AttributeColumns::len still grows. Pad with defaults on mismatch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/core-types/src/table.rs, line 166:
<comment>Silent downcast failure in `Column<T>::extend` breaks the column-length invariant. When two tables have the same attribute key but different types, the column silently skips the extend while `AttributeColumns::len` still grows. Pad with defaults on mismatch.</comment>
<file context>
@@ -1,137 +1,651 @@
+ }
+
+ /// Appends all values from another column, downcasting it to the same `Column<T>` type.
+ fn extend(&mut self, other: Box<dyn AttributeColumn>) {
+ if let Ok(other) = (other as Box<dyn std::any::Any>).downcast::<Self>() {
+ self.0.extend(other.0);
</file context>
| fn push(&mut self, value: Box<dyn AttributeValue>) { | ||
| if let Ok(value) = value.into_any().downcast::<T>() { | ||
| self.0.push(*value); | ||
| } |
There was a problem hiding this comment.
P1: Silent downcast failure in Column<T>::push breaks the column-length invariant. When the type doesn't match, no value is pushed but AttributeColumns::len still increments, causing later out-of-bounds accesses. Push a default on mismatch to maintain the invariant.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/core-types/src/table.rs, line 149:
<comment>Silent downcast failure in `Column<T>::push` breaks the column-length invariant. When the type doesn't match, no value is pushed but `AttributeColumns::len` still increments, causing later out-of-bounds accesses. Push a default on mismatch to maintain the invariant.</comment>
<file context>
@@ -1,137 +1,651 @@
+ }
+
+ /// Pushes a scalar attribute value onto the end of this column, downcasting it to `T`.
+ fn push(&mut self, value: Box<dyn AttributeValue>) {
+ if let Ok(value) = value.into_any().downcast::<T>() {
+ self.0.push(*value);
</file context>
| fn push(&mut self, value: Box<dyn AttributeValue>) { | |
| if let Ok(value) = value.into_any().downcast::<T>() { | |
| self.0.push(*value); | |
| } | |
| fn push(&mut self, value: Box<dyn AttributeValue>) { | |
| if let Ok(value) = value.into_any().downcast::<T>() { | |
| self.0.push(*value); | |
| } else { | |
| self.0.push(T::default()); | |
| } |
| fn image_to_bytes(_: impl Ctx, image: Table<Raster<CPU>>) -> Vec<u8> { | ||
| let Some(image) = image.iter().next() else { return vec![] }; | ||
| image.element.data.iter().flat_map(|color| color.to_rgb8_srgb().into_iter()).collect::<Vec<u8>>() | ||
| image.element().data.iter().flat_map(|color| color.to_rgb8_srgb().into_iter()).collect::<Vec<u8>>() |
There was a problem hiding this comment.
P2: Image to Bytes currently serializes each pixel as RGB (3 bytes) instead of RGBA (4 bytes), so alpha is lost and output length is incorrect for RGBA consumers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/nodes/gstd/src/platform_application_io.rs, line 123:
<comment>`Image to Bytes` currently serializes each pixel as RGB (3 bytes) instead of RGBA (4 bytes), so alpha is lost and output length is incorrect for RGBA consumers.</comment>
<file context>
@@ -120,7 +120,7 @@ fn string_to_bytes(_: impl Ctx, string: String) -> Vec<u8> {
fn image_to_bytes(_: impl Ctx, image: Table<Raster<CPU>>) -> Vec<u8> {
let Some(image) = image.iter().next() else { return vec![] };
- image.element.data.iter().flat_map(|color| color.to_rgb8_srgb().into_iter()).collect::<Vec<u8>>()
+ image.element().data.iter().flat_map(|color| color.to_rgb8_srgb().into_iter()).collect::<Vec<u8>>()
}
</file context>
| image.element().data.iter().flat_map(|color| color.to_rgb8_srgb().into_iter()).collect::<Vec<u8>>() | |
| image.element().data.iter().flat_map(|color| color.to_rgba8_srgb().into_iter()).collect::<Vec<u8>>() |
| last_point = Some(end_id); | ||
|
|
||
| self.bezier_to_path(bezier, *row.transform, move_to, &mut path); | ||
| self.bezier_to_path(bezier, row.attribute_cloned_or_default("transform"), move_to, &mut path); |
There was a problem hiding this comment.
P2: Avoid repeated per-segment transform attribute cloning inside the inner loop; compute it once per row.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/overlays/utility_types_native.rs, line 1178:
<comment>Avoid repeated per-segment transform attribute cloning inside the inner loop; compute it once per row.</comment>
<file context>
@@ -1171,11 +1171,11 @@ impl OverlayContextInternal {
last_point = Some(end_id);
- self.bezier_to_path(bezier, *row.transform, move_to, &mut path);
+ self.bezier_to_path(bezier, row.attribute_cloned_or_default("transform"), move_to, &mut path);
}
</file context>
| .iter() | ||
| .find_map(|i| i.as_ref()) | ||
| .map(|i| (i.transform, i.alpha_blending, i.source_node_id))?; | ||
| let (_, attributes) = [&red, &green, &blue, &alpha].iter().find_map(|i| i.as_ref()).map(|i| i.clone().into_parts())?; |
There was a problem hiding this comment.
P2: Avoid cloning the full raster row when only attributes are needed; this adds unnecessary image-buffer copies in combine_channels.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/nodes/raster/src/std_nodes.rs, line 122:
<comment>Avoid cloning the full raster row when only attributes are needed; this adds unnecessary image-buffer copies in `combine_channels`.</comment>
<file context>
@@ -114,23 +113,20 @@ pub fn combine_channels(
- .iter()
- .find_map(|i| i.as_ref())
- .map(|i| (i.transform, i.alpha_blending, i.source_node_id))?;
+ let (_, attributes) = [&red, &green, &blue, &alpha].iter().find_map(|i| i.as_ref()).map(|i| i.clone().into_parts())?;
// Get the common width and height of the channels, which must have equal dimensions
</file context>
| let (_, attributes) = [&red, &green, &blue, &alpha].iter().find_map(|i| i.as_ref()).map(|i| i.clone().into_parts())?; | |
| let attributes = [&red, &green, &blue, &alpha].iter().find_map(|i| i.as_ref()).map(|i| i.attributes().clone())?; |
| let image = row.element; | ||
| let image = row.element(); | ||
| let texture = upload_to_texture(device, queue, image); | ||
| let (_, attributes) = row.into_cloned().into_parts(); |
There was a problem hiding this comment.
P2: Avoid row.into_cloned() here; it clones the entire CPU raster just to copy attributes, causing unnecessary per-row image duplication and extra memory/CPU overhead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/wgpu-executor/src/texture_conversion.rs, line 157:
<comment>Avoid `row.into_cloned()` here; it clones the entire CPU raster just to copy attributes, causing unnecessary per-row image duplication and extra memory/CPU overhead.</comment>
<file context>
@@ -152,15 +152,11 @@ impl<'i> Convert<Table<Raster<GPU>>, &'i WgpuExecutor> for Table<Raster<CPU>> {
- let image = row.element;
+ let image = row.element();
let texture = upload_to_texture(device, queue, image);
+ let (_, attributes) = row.into_cloned().into_parts();
- TableRow {
</file context>
Performance Benchmark Results
|
1 participant
Closes #1173, as well as its parent: closes #1832.