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.
| 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>>() |
Performance Benchmark Results
|
Co-authored-by: Copilot <copilot@github.com>
Performance Benchmark Results
|
Performance Benchmark Results
|
1 participant
Closes #1173, as well as its parent: closes #1832.