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. |
glihm
changed the title
WalkthroughOhayo, sensei! The changes in the Changes
Sequence DiagramsequenceDiagram
participant Parser
participant Member
participant Diagnostics
Parser->>Member: Parse member attributes
alt Key Member Validation
Parser->>Parser: Check key member ordering
Parser->>Diagnostics: Push error if keys out of order
Parser->>Parser: Validate key member type
Parser->>Diagnostics: Push error for invalid key type
end
The sequence diagram illustrates the enhanced validation process for member parsing, showing how the parser checks key member ordering and type constraints before finalizing the member definition. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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
keysflag 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
| @@ -41,9 +41,12 @@ pub fn parse_members( | |||
| members: &[MemberAst], | |||
| diagnostics: &mut Vec<PluginDiagnostic>, | |||
| ) -> Vec<Member> { | |||
| let mut keys = true; | |||
There was a problem hiding this comment.
| let mut keys = true; | |
| let mut parsing_keys = true; |
| members | ||
| .iter() | ||
| .filter_map(|member_ast| { | ||
| let key = member_ast.has_attr(db, "key"); |
There was a problem hiding this comment.
| let key = member_ast.has_attr(db, "key"); | |
| let is_key = member_ast.has_attr(db, "key"); |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2891 +/- ##
==========================================
- Coverage 55.81% 55.79% -0.03%
==========================================
Files 449 449
Lines 57697 57719 +22
==========================================
- Hits 32206 32202 -4
- Misses 25491 25517 +26 ☔ View full report in Codecov by Sentry. |
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
Messagestruct for better organization.