feat(dojo-core): add fixed array support in models #2888
Conversation
…g, writing and deleting of fixed arrays
…-support_fixed_arrays
… add-fixed-arrays/remy/0
some fixes + storage tests + some improvements
|
Warning Rate limit exceeded@bengineer42 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 9 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughOhayo, sensei! This pull request introduces comprehensive support for fixed-size arrays across the Dojo framework. The changes span multiple components, including core introspection mechanisms, storage layouts, and compilation utilities. The modifications enable fixed-size arrays to be used in models, with new implementations in the type system, layout handling, and derive macros to support this feature seamlessly. Changes
Assessment against linked issues
Possibly related PRs
Sensei, the pull request comprehensively implements fixed-size array support across the Dojo framework, meeting the primary objective of enabling these arrays in models! Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
crates/dojo/lang/src/derive_macros/introspect/layout.rs (2)
127-142: Consider simplifyingbuild_array_layout_from_typefunction for better readability.The nested
matchonWrappercould be refactored to separate functions forIntrospectandArraycases, making the code more concise and easier to understand.
144-156: Ohayo sensei! Potential performance improvement inbuild_member_layout_from_type.Checking for
is_fixed_arraybeforeis_arraycan optimize the function, as fixed arrays might be more specific and checking them first could reduce unnecessary evaluations.Apply this diff to reorder the condition checks:
pub fn build_member_layout_from_type( diagnostics: &mut Vec<PluginDiagnostic>, diagnostic_item: ids::SyntaxStablePtrId, item_type: &str, ) -> Wrapper { + if is_fixed_array(item_type) { + Wrapper::Array(build_fixed_array_layout_from_type(diagnostics, diagnostic_item, item_type)) + } else if is_array(item_type) { Wrapper::Array(build_array_layout_from_type(diagnostics, diagnostic_item, item_type)) - } else if is_fixed_array(item_type) { - Wrapper::Array(build_fixed_array_layout_from_type(diagnostics, diagnostic_item, item_type)) } else if is_tuple(item_type) { Wrapper::Array(build_tuple_layout_from_type(diagnostics, diagnostic_item, item_type)) } else { Wrapper::Introspect } }crates/dojo/core/src/meta/layout.cairo (1)
35-38: Ohayo sensei! Ensureis_same_type_ofcorrectly handles all variants.The updated match includes
FixedArray, but consider adding a default case to handle unexpected variants for future-proofing.Apply this diff to include a wildcard arm:
(Layout::Enum(_), Layout::Enum(_)) => true, + _ => false }crates/dojo/lang/src/derive_macros/introspect/utils.rs (1)
57-62: Consider optimizingtrim_first_and_last_charsfunction.Instead of manual character manipulation, you could use slicing for better performance and readability.
Apply this diff to refactor the function:
pub fn trim_first_and_last_chars(s: &str) -> &str { - let mut chars = s.trim().chars(); - chars.next(); - chars.next_back(); - chars.as_str().trim() + let trimmed = s.trim(); + &trimmed[1..trimmed.len()-1].trim() }crates/dojo/core-cairo-test/src/tests/model/model.cairo (1)
195-209: Enhance test coverage for fixed arrays.Ohayo sensei! While the basic test case looks good, consider adding these scenarios:
- Test with boundary values (e.g., [u16::MAX, 0, u16::MAX])
- Test partial updates to array elements
- Verify error handling for invalid array indices
Would you like me to provide example test implementations for these scenarios?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockcrates/dojo/core-cairo-test/Scarb.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo(6 hunks)crates/dojo/core-cairo-test/src/tests/model/model.cairo(2 hunks)crates/dojo/core/src/meta/introspect.cairo(2 hunks)crates/dojo/core/src/meta/layout.cairo(3 hunks)crates/dojo/core/src/storage/layout.cairo(12 hunks)crates/dojo/lang/Cargo.toml(1 hunks)crates/dojo/lang/src/derive_macros/introspect/layout.rs(9 hunks)crates/dojo/lang/src/derive_macros/introspect/size.rs(4 hunks)crates/dojo/lang/src/derive_macros/introspect/ty.rs(4 hunks)crates/dojo/lang/src/derive_macros/introspect/utils.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (21)
crates/dojo/lang/src/derive_macros/introspect/layout.rs (7)
15-18: Ohayo sensei! Introducing theWrapperenum enhances code clarity.Defining
Wrapperwith variantsIntrospectandArray(String)provides a structured approach to handle different layout types, improving maintainability and readability.
99-106: Ohayo sensei! Support forExpr::FixedSizeArrayadded toget_layout_from_type_clause.Adding this match arm enables the function to handle fixed-size arrays properly, ensuring accurate layout generation for these types.
358-365: Ohayo sensei! Added support for fixed-size arrays inget_packed_field_layout_from_type_clause.Including handling for
Expr::FixedSizeArrayensures that packed layouts can correctly process fixed-size arrays.
289-292:⚠️ Potential issueOhayo sensei! Potential scoping issue in the
forloop.The curly braces around the
forloop may not be necessary and could cause scoping problems. Ensure that variables likelayoutandmerged_layoutare properly scoped.Apply this diff to adjust the code:
for layout in layouts { - match layout { + match layout {{ dojo::meta::Layout::Fixed(mut l) => merged_layout.append_span(l), _ => panic!("A packed model layout must contain Fixed layouts only."), - }; -}; + }} }Likely invalid or redundant comment.
217-218:⚠️ Potential issueOhayo sensei! Missing fixed-size array handling in
build_item_layout_from_type.The function handles arrays and tuples but should also handle fixed-size arrays to ensure consistency.
Apply this diff to include fixed-size array handling:
} else if is_array(item_type) { build_array_layout_from_type(diagnostics, diagnostic_item, item_type) } else if is_tuple(item_type) { build_tuple_layout_from_type(diagnostics, diagnostic_item, item_type) +} else if is_fixed_array(item_type) { + build_fixed_array_layout_from_type(diagnostics, diagnostic_item, item_type) } else { // For Option<T>, T cannot be a tuple if is_unsupported_option_type(item_type) {Likely invalid or redundant comment.
419-430: Ohayo sensei! Verify the logic inget_packed_fixed_array_layout_from_type.Ensure the array size parsing and repeated layout generation are accurate, especially in the
mapfunction starting at line 429.Run the following script to test array size parsing:
✅ Verification successful
Ohayo sensei! The array layout logic is properly implemented!
The array size parsing and layout generation are accurate because:
- Type and size extraction uses proper string splitting with
;- Size parsing is safe as it operates on compiler-validated array types
- Layout repetition correctly clones the inner type layout for each element
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Test parsing of array sizes in fixed-size arrays. # Expected: Should correctly parse array sizes and generate appropriate layouts. ast-grep --pattern $' fn get_packed_fixed_array_layout_from_type($$) { $$$ }'Length of output: 82
Script:
#!/bin/bash # Find the implementation of get_fixed_array_inner_type_and_size ast-grep --pattern 'fn get_fixed_array_inner_type_and_size($_) { $$$ }' # Find any test cases rg "get_fixed_array_inner_type_and_size|get_packed_fixed_array_layout_from_type" -A 5 # Look for similar array size parsing patterns rg "trim\(\)\.parse\(\)\.unwrap\(\)" -A 2Length of output: 7607
182-204: Ohayo sensei! Verify the correctness ofbuild_fixed_array_layout_from_type.Ensure that the function correctly handles both the
IntrospectandArraycases for fixed-size arrays, especially with the formatting in line 191.Run the following script to check if
Introspectis properly implemented for fixed-size arrays:✅ Verification successful
Ohayo sensei! Your
build_fixed_array_layout_from_typeimplementation is correct!The function correctly handles both cases:
- For types implementing
Introspect, it generates the appropriate layout call- For array types, it properly constructs the
FixedArraylayout with the correct size and inner layoutThe implementation is well-supported by robust utility functions for type parsing and validation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify `Introspect` implementations for fixed-size arrays. # Expected: Should find implementations of `Introspect` for fixed-size arrays. ast-grep --pattern $' impl<T> dojo::meta::introspect::Introspect for [$T; $_] { $$$ }'Length of output: 91
Script:
#!/bin/bash # Search for any Introspect-related code with fixed arrays rg -l "Introspect.*\[.*\]" --type rust # Find test files that might verify this functionality fd "test.*\.rs$" -x rg -l "build_fixed_array_layout_from_type|FixedArray.*layout" # Look for actual usages of this function ast-grep --pattern 'build_fixed_array_layout_from_type($$$)'Length of output: 1072
Script:
#!/bin/bash # Check utils.rs for related implementations cat crates/dojo/lang/src/derive_macros/introspect/utils.rs # Look for get_fixed_array_inner_type_and_size implementation ast-grep --pattern 'fn get_fixed_array_inner_type_and_size($$$)' # Find any layout-related tests rg -A 5 "layout.*test" --type rustLength of output: 4970
crates/dojo/core/src/meta/layout.cairo (1)
19-19: Ohayo sensei! Introduction ofFixedArrayvariant inLayoutenum enhances functionality.Adding
FixedArray: Span<(Layout, u32)>allows theLayoutenum to represent fixed-size arrays, aligning with the new features.crates/dojo/lang/src/derive_macros/introspect/utils.rs (2)
4-5: Ohayo sensei! TheFIXED_ARRAY_REGEXconstant is well-defined.The regex pattern effectively matches fixed-size array types, facilitating accurate type detection.
41-44: Ohayo sensei! Functionis_fixed_arraycorrectly identifies fixed-size arrays.Using the defined regex, this function enhances type introspection capabilities.
crates/dojo/lang/src/derive_macros/introspect/ty.rs (1)
155-164: LGTM! Well-structured fixed array type builder.The implementation correctly handles fixed array type construction by extracting inner type and size, then formatting them into the expected structure.
crates/dojo/core/src/meta/introspect.cairo (1)
94-107: LGTM! Clean implementation of fixed array introspection.Ohayo sensei! The implementation is well-structured and handles all necessary cases:
- Size computation correctly propagates None
- Layout uses FixedArray variant consistently
- Type representation matches the layout structure
crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo (4)
204-208: Uncomment and implement the test forWithFixedArray.Ohayo! The test for
WithFixedArrayis commented out. Since we have the struct definition, we should implement this test to ensure complete coverage.Would you like me to help implement this test case?
120-143: LGTM! Well-structured test entities.The test structures (
StructWithFixedArray,StructWithComplexFixedArray, etc.) provide good coverage of fixed array scenarios, including nested arrays and packed variants.
268-301: LGTM! Comprehensive test coverage.The test cases thoroughly validate the size calculations for fixed arrays in various contexts (basic, complex, packed).
342-390: LGTM! Layout tests are well-implemented.The layout tests ensure proper serialization of fixed arrays, including complex nested scenarios.
crates/dojo/core/src/storage/layout.cairo (4)
81-104: LGTM! Well-implemented fixed array support.Ohayo sensei! The
write_fixed_array_layoutfunction is well-documented and follows the established patterns. The optimization to skip writing array length (since it's fixed at compile-time) is a nice touch.
228-239: LGTM! Clean implementation of delete operation.The
delete_fixed_array_layoutfunction properly handles cleanup of fixed array elements.
393-407: LGTM! Consistent read implementation.The
read_fixed_array_layoutfunction maintains consistency with the write and delete operations.
74-78: LGTM! Improved loop readability.The refactoring from
looptoforloops improves code readability while maintaining functionality.Also applies to: 146-153, 167-174, 284-291, 301-308, 386-391, 446-453, 466-473
crates/dojo/lang/Cargo.toml (1)
23-23: LGTM! Clean dependency addition.Ohayo! The regex dependency is properly added using workspace-level version management.
| } else if is_fixed_array(item_type) { | ||
| build_fixed_array_ty_from_type(item_type) |
There was a problem hiding this comment.
Remove duplicate fixed array condition.
Ohayo sensei! The condition is_fixed_array(item_type) is already handled in the previous if block at line 117. This duplicate condition will never be reached.
- } else if is_fixed_array(item_type) {
- build_fixed_array_ty_from_type(item_type)| (0..array_size.trim().parse().unwrap()) | ||
| .map(|_| inner_type_size.clone()) | ||
| .flatten() | ||
| .collect::<Vec<_>>() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize array size computation and add error handling.
Ohayo sensei! Two suggestions for improvement:
- Move the
clone()operation outside the loop to avoid repeated cloning:
- (0..array_size.trim().parse().unwrap())
- .map(|_| inner_type_size.clone())
+ let inner_size = inner_type_size.clone();
+ (0..array_size.trim().parse().unwrap())
+ .map(|_| inner_size)- Add proper error handling for the size parsing:
- array_size.trim().parse().unwrap()
+ array_size.trim().parse().expect("Failed to parse fixed array size")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (0..array_size.trim().parse().unwrap()) | |
| .map(|_| inner_type_size.clone()) | |
| .flatten() | |
| .collect::<Vec<_>>() | |
| let inner_size = inner_type_size.clone(); | |
| (0..array_size.trim().parse().expect("Failed to parse fixed array size")) | |
| .map(|_| inner_size) | |
| .flatten() | |
| .collect::<Vec<_>>() |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/dojo/lang/src/derive_macros/introspect/utils.rs (1)
59-64:⚠️ Potential issueOhayo sensei! Let's make the trimming function more robust.
The current implementation could panic with empty or single-character strings.
Consider this safer implementation:
pub fn trim_first_and_last_chars(s: &str) -> Option<&str> { let s = s.trim(); if s.len() < 2 { return None; } Some(&s[1..s.len() - 1]) }
🧹 Nitpick comments (1)
crates/dojo/lang/src/derive_macros/introspect/utils.rs (1)
3-6: Ohayo sensei! Consider enhancing the regex pattern for better validation.The regex pattern could be more robust:
- The current pattern allows any character between brackets (
[.+;)- No validation for nested arrays or complex types
Consider this enhanced pattern:
-const FIXED_ARRAY_REGEX: &str = r"\[.+;\s*(\d{1,10}|([a-zA-Z_{1}][a-zA-Z0-9_]*))\s*\]"; +const FIXED_ARRAY_REGEX: &str = r"\[([a-zA-Z_][a-zA-Z0-9_]*(?:<[^>]+>)?|\([^)]+\));\s*(\d{1,10}|[a-zA-Z_][a-zA-Z0-9_]*)\s*\]";This would better validate:
- Complex inner types (generics, tuples)
- Proper type naming conventions
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/dojo/lang/src/derive_macros/introspect/utils.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
| pub fn is_fixed_array(ty: &str) -> bool { | ||
| let re = Regex::new(FIXED_ARRAY_REGEX).unwrap(); | ||
| re.is_match(ty) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei! Let's optimize the regex compilation.
The current implementation creates a new Regex instance on each call, which is inefficient. Also, the unwrap() could panic.
Consider using lazy_static or once_cell for the compiled regex:
use once_cell::sync::Lazy;
static FIXED_ARRAY_REGEX: Lazy<Regex> = Lazy::new(|| {
Regex::new(r"\[.+;\s*(\d{1,10}|([a-zA-Z_{1}][a-zA-Z0-9_]*))\s*\]")
.expect("Failed to compile fixed array regex")
});
pub fn is_fixed_array(ty: &str) -> bool {
FIXED_ARRAY_REGEX.is_match(ty)
}| pub fn get_fixed_array_inner_type_and_size(ty: &str) -> (String, String) { | ||
| let ty = trim_first_and_last_chars(ty); | ||
| let res: Vec<&str> = ty.rsplitn(2, ';').collect(); | ||
| (res[1].trim().to_string(), res[0].trim().to_string()) | ||
| } |
There was a problem hiding this comment.
Ohayo sensei! Let's add proper error handling for array parsing.
The current implementation has multiple panic points and lacks proper error handling.
Consider this safer implementation:
#[derive(Debug)]
pub enum FixedArrayError {
InvalidFormat,
EmptyType,
EmptySize,
}
pub fn get_fixed_array_inner_type_and_size(ty: &str) -> Result<(String, String), FixedArrayError> {
let inner = trim_first_and_last_chars(ty)
.ok_or(FixedArrayError::InvalidFormat)?;
let parts: Vec<&str> = inner.rsplitn(2, ';').collect();
if parts.len() != 2 {
return Err(FixedArrayError::InvalidFormat);
}
let type_str = parts[1].trim();
let size_str = parts[0].trim();
if type_str.is_empty() {
return Err(FixedArrayError::EmptyType);
}
if size_str.is_empty() {
return Err(FixedArrayError::EmptySize);
}
Ok((type_str.to_string(), size_str.to_string()))
}Also, consider adding tests:
#[test]
fn test_fixed_array_parsing() {
assert!(get_fixed_array_inner_type_and_size("[u8; 32]").is_ok());
assert!(get_fixed_array_inner_type_and_size("[]").is_err());
assert!(get_fixed_array_inner_type_and_size("[;]").is_err());
assert!(get_fixed_array_inner_type_and_size("[u8;]").is_err());
assert!(get_fixed_array_inner_type_and_size("[; 32]").is_err());
}
glihm
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/dojo/lang/src/derive_macros/introspect/size.rs (1)
209-216: Consider adding documentation for the new fixed array size computation.Ohayo sensei! Since this is a critical part of the memory layout system, consider adding documentation that explains:
- The relationship between fixed array sizes and memory layout
- Examples of size computation for different array types
- Edge cases and limitations
This will help future maintainers understand the implementation details and constraints.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/dojo/lang/src/derive_macros/introspect/size.rs(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (4)
crates/dojo/lang/src/derive_macros/introspect/size.rs (4)
7-8: LGTM! Clean import organization.Ohayo sensei! The new imports for fixed array utilities are well-organized and properly scoped.
153-156: LGTM! Consistent pattern matching implementation.The new match arm for fixed-size arrays follows the established pattern of other type handlers.
189-190: LGTM! Clean type handling extension.The fixed array type handling is well-integrated into the existing type computation logic.
209-216: Optimize array size computation and add error handling.Ohayo sensei! The implementation would benefit from the previously suggested optimizations:
- Moving the
clone()operation outside the loop- Adding proper error handling for size parsing
Description
Related issue
Fixes #2785
Tests
Added to documentation?
Checklist
scripts/prettier.sh,scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit
Release Notes
New Features
Technical Improvements
Testing