-
Notifications
You must be signed in to change notification settings - Fork 189
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.
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
fix(dojo-lang): raise an error on value before key in model #2891
fix(dojo-lang): raise an error on value before key in model #2891
Conversation
to complement the reason, since the keys are serialized before the values by dojo before returning the model when read from the storage, the order does matter. Thanks @bengineer42 for the good catch. |
WalkthroughOhayo, sensei! The changes in the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🔇 Additional comments (1)
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/dojo/lang/src/attribute_macros/element.rs (2)
63-74
: Consider adding documentation for the key validation logic, sensei!While the implementation is solid, adding a doc comment explaining why keys must precede non-key members would help future maintainers understand the design decision.
+ // Keys must be defined before non-key members to ensure consistent + // serialization order and to maintain a predictable storage layout. if key { if !keys {
Line range hint
76-87
: Enhance the error message with alternative type suggestions, sensei!The error message explains why u256 isn't supported, but it would be more helpful to also suggest what types can be used instead.
- message: "Key is only supported for core types that are 1 felt long once \ - serialized. `u256` is a struct of 2 u128, hence not supported." + message: "Key is only supported for core types that are 1 felt long once \ + serialized. `u256` is a struct of 2 u128, hence not supported. \ + Consider using `felt252` or `u128` instead." .into(),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/dojo/lang/src/attribute_macros/element.rs
(2 hunks)
🔇 Additional comments (2)
crates/dojo/lang/src/attribute_macros/element.rs (2)
44-44
: Ohayo! Clean implementation of the key tracking mechanism, sensei!The initialization and usage of the
keys
flag is well-structured and follows Rust idioms.Also applies to: 48-49, 59-59
Line range hint
44-87
: Ohayo! The implementation successfully achieves the PR objectives!The code effectively enforces the requirement that key members must be defined before non-key members, with proper error handling and clear messages. The additional validation for key types adds robust type safety.
Let's verify that there are no existing models that would be affected by this change:
✅ Verification successful
Ohayo! The implementation correctly enforces key member ordering, sensei!
The codebase scan confirms that all models consistently define key members before non-key members, with no violations found. The implementation successfully enforces this constraint with proper error handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for models with key attributes after non-key members ast-grep --pattern 'struct $name { $$$ $field: $_, #[key] $key_field: $_, $$$ }'Length of output: 102
Script:
#!/bin/bash # Search for struct definitions with key attributes using ripgrep rg "#\[key\]" -B 2 -A 2 # Search for all struct definitions with attributes to analyze patterns ast-grep --pattern 'struct $name { #[$attr] $field: $_, $$$ }'Length of output: 63041
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2891 +/- ##
==========================================
+ Coverage 55.21% 56.03% +0.81%
==========================================
Files 449 449
Lines 58338 57944 -394
==========================================
+ Hits 32213 32469 +256
+ Misses 26125 25475 -650 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/dojo/lang/src/attribute_macros/element.rs (1)
44-44
: Consider adding a doc comment for parsing_keysAdding a brief doc comment would help future maintainers understand the significance of this flag.
+ // Tracks whether we're still in the keys section. Once we encounter a non-key member, + // this becomes false and no more key members are allowed. let mut parsing_keys = true;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/dojo/lang/src/attribute_macros/element.rs
(2 hunks)
🔇 Additional comments (3)
crates/dojo/lang/src/attribute_macros/element.rs (3)
39-43
: Ohayo! The function signature enhancement looks solid, sensei!The addition of the
diagnostics
parameter enables proper error reporting for model structure violations.
44-44
: Elegant key tracking implementation, sensei!The
parsing_keys
flag with the&=
update is a clean way to enforce the key-before-value requirement.Also applies to: 75-75
63-73
: Excellent error handling approach, sensei!The validation logic provides clear error messages while continuing to process remaining members, which helps developers fix all issues at once rather than one at a time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch on this one, thanks @bengineer42 for the fix.
Description
Raise an error when a value appears before a key in a model eg:
Related issue
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
Bug Fixes
Refactor
Message
struct for better organization.Chores
scarb
andscarb-ui
to the latest commit for potential improvements and fixes.