fix: contracts function name clashing #2603
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2603 +/- ##
==========================================
+ Coverage 55.13% 55.22% +0.08%
==========================================
Files 385 385
Lines 47748 47897 +149
==========================================
+ Hits 26328 26453 +125
- Misses 21420 21444 +24 ☔ View full report in Codecov by Sentry. |
WalkthroughOhayo, sensei! This pull request introduces several enhancements to the Changes
Possibly related PRs
Suggested reviewers
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: 4
🧹 Outside diff range and nitpick comments (4)
crates/dojo/bindgen/src/plugins/typescript/writer.rs (1)
Line range hint
99-113: Consider extracting system function names to a constantOhayo! The growing list of excluded system functions could benefit from being moved to a constant or configuration. This would make it easier to maintain and update in the future.
Here's a suggested improvement:
+ /// System function names that should be excluded from TypeScript code generation + const EXCLUDED_SYSTEM_FUNCTIONS: &[&str] = &[ + "contract_name", + "namespace", + "tag", + "name_hash", + "selector", + "dojo_init", + "namespace_hash", + "world", + "dojo_name", + "upgrade", + "world_dispatcher", + ]; .filter(|s| { let name = s.to_function().unwrap().name.as_str(); - ![ - "contract_name", - "namespace", - "tag", - "name_hash", - "selector", - "dojo_init", - "namespace_hash", - "world", - "dojo_name", - "upgrade", - "world_dispatcher", - ] - .contains(&name) + !EXCLUDED_SYSTEM_FUNCTIONS.contains(&name) })crates/dojo/bindgen/src/plugins/mod.rs (2)
111-113: Fix documentation formatting.The documentation comment is split across lines unnecessarily.
- /// At given index, finds the first occurrence of the needle string before the position in - /// string + /// At given index, finds the first occurrence of the needle string before the position in string
183-206: Add more test coverage for edge cases, sensei.While the current tests are good, they only cover the happy path. Consider adding tests for:
- Empty buffer
- Invalid indices
- Patterns not found
- Boundary conditions
Here's an example of additional test cases:
#[test] fn test_buffer_edge_cases() { let mut buff = Buffer::new(); // Empty buffer assert_eq!(buff.get_first_after("foo", "bar", 0), None); // Invalid index buff.push("some content".to_owned()); assert_eq!(buff.get_first_after("foo", "bar", 999), None); // Pattern not found assert_eq!(buff.get_first_after("nonexistent", "pattern", 0), None); }crates/dojo/bindgen/src/plugins/typescript/generator/function.rs (1)
92-92: Sensei, the comment could use a slight rephrasing.The comment is a bit unclear. Consider rephrasing it to something like:
// Check if functions were already appended to the body. If so, append after the other functions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
crates/dojo/bindgen/src/plugins/mod.rs(2 hunks)crates/dojo/bindgen/src/plugins/typescript/generator/function.rs(8 hunks)crates/dojo/bindgen/src/plugins/typescript/writer.rs(1 hunks)
🔇 Additional comments (23)
crates/dojo/bindgen/src/plugins/typescript/writer.rs (1)
110-112: Ohayo sensei! The additional system function exclusions look good!
The added system function names ("dojo_name", "upgrade", "world_dispatcher") are correctly excluded from TypeScript code generation, which helps prevent function name clashing in the generated bindings.
Let's verify that these are indeed system functions that should be excluded:
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Ohayo sensei! The system function exclusions are indeed verified!
The excluded functions are confirmed to be system-level functions:
dojo_name: A core contract interface function used for identificationupgrade: Part of the upgradeable component system for contract upgradesworld_dispatcher: Core world provider component function for world interactions
These functions are part of Dojo's core infrastructure and should not be exposed in TypeScript bindings to prevent name clashing with user-defined functions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the excluded functions are system-level functions
# Search for declarations or implementations of these functions
ast-grep --pattern 'fn $_($$) {
$$$
}'
# Also check for any existing usages to ensure we're not breaking anything
rg -A 3 'dojo_name|upgrade|world_dispatcher'
Length of output: 353775
crates/dojo/bindgen/src/plugins/mod.rs (1)
79-85: LGTM! Clean and idiomatic implementation.
The pos helper method is well-documented and follows Rust best practices.
crates/dojo/bindgen/src/plugins/typescript/generator/function.rs (21)
32-34: Ohayo sensei! The function name generation looks good.
The change to include the contract name as a prefix in the generated function name is a nice improvement. It ensures uniqueness and clarity in the generated code.
98-103: Ohayo! The updated method signature looks good, sensei.
Adding the contract_name parameter to the setup_function_wrapper_end method is a sensible change. It allows for better organization of the generated functions under their respective contract namespaces.
106-112: Sensei, the return object structure update is spot on!
The change to include the contract name as a key in the return object is a great improvement. It properly namespaces the functions according to their contracts, enhancing the clarity and organization of the generated code.
116-144: Ohayo sensei! The logic for appending to the return object looks solid.
The updated logic handles the cases where the contract name is already present or needs to be added to the return object. The use of get_first_after and get_first_before_pos helper methods from the Buffer struct is a nice touch for finding the correct insertion positions.
146-154: Sensei, the fallback logic for appending to the return object is well-handled.
If the contract name is not found in the return object, the code correctly appends it as a new entry. The use of insert_after method ensures the proper positioning of the new contract entry.
170-170: Ohayo sensei! Retrieving the contract name using the naming module is a good approach.
Using the naming::get_name_from_tag function to retrieve the contract name from the contract's tag is a clean and consistent way to obtain the name for generating the function names and return object structure.
175-175: Sensei, passing the contract name to generate_system_function is a necessary change.
With the updated generate_system_function method signature expecting the contract_name parameter, passing it correctly here ensures that the generated function names include the contract prefix.
177-177: Ohayo! Passing the contract name to setup_function_wrapper_end is crucial.
Providing the contract_name argument to the setup_function_wrapper_end method is essential for correctly structuring the return object with the contract namespace.
218-220: Sensei, the updated test case for generate_system_function looks good.
The expected output in the test case correctly reflects the changes made to include the contract name as a prefix in the generated function name.
231-232: Ohayo! The closing brace and semicolon are correctly placed in the expected output.
The test case's expected output properly includes the closing brace and semicolon for the generated function.
285-285: Sensei, the test case for setup_function_wrapper_end is well-structured.
The test case correctly sets up the necessary input data, including the contract name and function, to test the setup_function_wrapper_end method.
288-290: Ohayo! The expected output for the test case is accurate.
The expected output correctly reflects the changes made to the return object structure, including the contract name as a key and the generated function name as the value.
297-301: Sensei, the additional test case for setup_function_wrapper_end is thorough.
The test case covers the scenario where multiple functions from the same contract are processed, ensuring that they are correctly appended to the return object under the same contract namespace.
303-306: Ohayo! The expected output for the additional test case is spot on.
The expected output accurately represents the updated return object structure, with both functions correctly placed under the same contract namespace.
311-312: Sensei, the test case for a different contract is a valuable addition.
Including a test case that processes a function from a different contract ensures that the setup_function_wrapper_end method correctly handles multiple contracts and namespaces them appropriately in the return object.
313-322: Ohayo! The expected output for the different contract test case is perfect.
The expected output correctly demonstrates how functions from different contracts are namespaced separately in the return object, maintaining a clean and organized structure.
323-324: Sensei, the assertion for the different contract test case is correct.
The assertion verifies that the buffer length remains unchanged and the expected output matches the actual buffer content after processing the function from a different contract.
333-334: Ohayo! Creating a test contract using the create_dojo_contract function is a good practice.
Using a dedicated function to create a test contract ensures consistency and maintainability in the test cases.
335-335: Sensei, the assertion for the buffer length after generating a function is appropriate.
The assertion checks that the buffer length has increased by the expected amount after generating a function, indicating that the function was correctly appended to the buffer.
338-338: Ohayo! Generating an additional function in the test case is a good approach.
Testing the generation of multiple functions ensures that the generate method correctly handles appending multiple functions to the buffer without any issues.
355-357: Sensei, the addition of the create_move_function test utility function is useful.
Having a dedicated function to create a test function with the name "move" and no inputs improves the readability and maintainability of the test cases.
| /// Inserts string at the specified position. | ||
| /// | ||
| /// * `s` - The string to insert. | ||
| /// * `pos` - The position to insert the string at. | ||
| /// * `idx` - The index of the string to insert at. | ||
| /// | ||
| pub fn insert_at(&mut self, s: String, pos: usize, idx: usize) { | ||
| if let Some(st) = self.0.get_mut(idx) { | ||
| st.insert_str(pos + 1, &s); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add bounds checking to insert_at, sensei.
The function should validate that pos is within the string's bounds.
Here's a safer implementation:
-pub fn insert_at(&mut self, s: String, pos: usize, idx: usize) {
+pub fn insert_at(&mut self, s: String, pos: usize, idx: usize) -> Option<()> {
if let Some(st) = self.0.get_mut(idx) {
+ if pos >= st.len() {
+ return None;
+ }
st.insert_str(pos + 1, &s);
+ Some(())
+ } else {
+ None
}
}📝 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.
| /// Inserts string at the specified position. | |
| /// | |
| /// * `s` - The string to insert. | |
| /// * `pos` - The position to insert the string at. | |
| /// * `idx` - The index of the string to insert at. | |
| /// | |
| pub fn insert_at(&mut self, s: String, pos: usize, idx: usize) { | |
| if let Some(st) = self.0.get_mut(idx) { | |
| st.insert_str(pos + 1, &s); | |
| } | |
| } | |
| /// Inserts string at the specified position. | |
| /// | |
| /// * `s` - The string to insert. | |
| /// * `pos` - The position to insert the string at. | |
| /// * `idx` - The index of the string to insert at. | |
| /// | |
| pub fn insert_at(&mut self, s: String, pos: usize, idx: usize) -> Option<()> { | |
| if let Some(st) = self.0.get_mut(idx) { | |
| if pos >= st.len() { | |
| return None; | |
| } | |
| st.insert_str(pos + 1, &s); | |
| Some(()) | |
| } else { | |
| None | |
| } | |
| } |
| pub fn get_first_before_pos(&self, search: &str, pos: usize, idx: usize) -> Option<usize> { | ||
| if let Some(st) = self.0.get(idx) { | ||
| let indices = st.match_indices(search).map(|(i, _)| i).collect::<Vec<usize>>(); | ||
| if indices.is_empty() { | ||
| return None; | ||
| } | ||
|
|
||
| return indices.iter().filter(|&&i| i < pos).max().copied(); | ||
| } | ||
| None | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize with iterator chaining here too, sensei.
Similar to get_first_after, we can make this more efficient.
Consider this more efficient implementation:
pub fn get_first_before_pos(&self, search: &str, pos: usize, idx: usize) -> Option<usize> {
if let Some(st) = self.0.get(idx) {
- let indices = st.match_indices(search).map(|(i, _)| i).collect::<Vec<usize>>();
- if indices.is_empty() {
- return None;
- }
-
- return indices.iter().filter(|&&i| i < pos).max().copied();
+ st.match_indices(search)
+ .map(|(i, _)| i)
+ .filter(|&i| i < pos)
+ .max()
}
- None
}📝 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.
| pub fn get_first_before_pos(&self, search: &str, pos: usize, idx: usize) -> Option<usize> { | |
| if let Some(st) = self.0.get(idx) { | |
| let indices = st.match_indices(search).map(|(i, _)| i).collect::<Vec<usize>>(); | |
| if indices.is_empty() { | |
| return None; | |
| } | |
| return indices.iter().filter(|&&i| i < pos).max().copied(); | |
| } | |
| None | |
| } | |
| pub fn get_first_before_pos(&self, search: &str, pos: usize, idx: usize) -> Option<usize> { | |
| if let Some(st) = self.0.get(idx) { | |
| st.match_indices(search) | |
| .map(|(i, _)| i) | |
| .filter(|&i| i < pos) | |
| .max() | |
| } | |
| } |
| /// At given index, finds the first occurrence of the needle string after the search string. | ||
| /// | ||
| /// * `needle` - The string to search for. | ||
| /// * `search` - The string to search after. | ||
| /// * `idx` - The index to search at. | ||
| /// | ||
| pub fn get_first_after(&self, needle: &str, search: &str, idx: usize) -> Option<usize> { | ||
| if let Some(st) = self.0.get(idx) { | ||
| let indices = st.match_indices(needle).map(|(i, _)| i).collect::<Vec<usize>>(); | ||
| if indices.is_empty() { | ||
| return None; | ||
| } | ||
|
|
||
| let start = indices[indices.len() - 1] + 1; | ||
| let search_indices = st.match_indices(search).map(|(i, _)| i).collect::<Vec<usize>>(); | ||
| return search_indices.iter().filter(|&&i| i > start).min().copied(); | ||
| } | ||
| None | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize performance with iterator chaining, sensei.
The current implementation collects indices into vectors twice. We can make this more efficient.
Consider this more efficient implementation:
pub fn get_first_after(&self, needle: &str, search: &str, idx: usize) -> Option<usize> {
if let Some(st) = self.0.get(idx) {
- let indices = st.match_indices(needle).map(|(i, _)| i).collect::<Vec<usize>>();
- if indices.is_empty() {
- return None;
- }
-
- let start = indices[indices.len() - 1] + 1;
- let search_indices = st.match_indices(search).map(|(i, _)| i).collect::<Vec<usize>>();
- return search_indices.iter().filter(|&&i| i > start).min().copied();
+ let last_needle = st.match_indices(needle).map(|(i, _)| i).last()?;
+ st.match_indices(search)
+ .map(|(i, _)| i)
+ .filter(|&i| i > last_needle + 1)
+ .min()
}
- None
}📝 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.
| /// At given index, finds the first occurrence of the needle string after the search string. | |
| /// | |
| /// * `needle` - The string to search for. | |
| /// * `search` - The string to search after. | |
| /// * `idx` - The index to search at. | |
| /// | |
| pub fn get_first_after(&self, needle: &str, search: &str, idx: usize) -> Option<usize> { | |
| if let Some(st) = self.0.get(idx) { | |
| let indices = st.match_indices(needle).map(|(i, _)| i).collect::<Vec<usize>>(); | |
| if indices.is_empty() { | |
| return None; | |
| } | |
| let start = indices[indices.len() - 1] + 1; | |
| let search_indices = st.match_indices(search).map(|(i, _)| i).collect::<Vec<usize>>(); | |
| return search_indices.iter().filter(|&&i| i > start).min().copied(); | |
| } | |
| None | |
| } | |
| /// At given index, finds the first occurrence of the needle string after the search string. | |
| /// | |
| /// * `needle` - The string to search for. | |
| /// * `search` - The string to search after. | |
| /// * `idx` - The index to search at. | |
| /// | |
| pub fn get_first_after(&self, needle: &str, search: &str, idx: usize) -> Option<usize> { | |
| if let Some(st) = self.0.get(idx) { | |
| let last_needle = st.match_indices(needle).map(|(i, _)| i).last()?; | |
| st.match_indices(search) | |
| .map(|(i, _)| i) | |
| .filter(|&i| i > last_needle + 1) | |
| .min() | |
| } | |
| } |
| /// Inserts string after the first occurrence of the separator. | ||
| /// | ||
| /// * `s` - The string to insert. | ||
| /// * `pos` - The string inside inner vec to search position for. | ||
| /// * `sep` - The separator to search for. | ||
| /// * `idx` - The index of the separator to insert after. | ||
| /// | ||
| pub fn insert_after(&mut self, s: String, pos: &str, sep: &str, idx: usize) { | ||
| let pos = self.0.iter().position(|b| b.contains(pos)).unwrap(); | ||
| let pos = self.pos(pos).unwrap(); | ||
| if let Some(st) = self.0.get_mut(pos) { | ||
| let indices = st.match_indices(sep).map(|(i, _)| i).collect::<Vec<usize>>(); | ||
| let append_after = indices[indices.len() - idx] + 1; | ||
| st.insert_str(append_after, &s); | ||
| } | ||
| } |
There was a problem hiding this comment.
Ohayo sensei! Consider adding error handling to insert_after.
The function could panic if:
- pos() returns None (unwrap)
- idx is >= indices.length
Here's a safer implementation:
pub fn insert_after(&mut self, s: String, pos: &str, sep: &str, idx: usize) -> Option<()> {
- let pos = self.pos(pos).unwrap();
+ let pos = self.pos(pos)?;
if let Some(st) = self.0.get_mut(pos) {
let indices = st.match_indices(sep).map(|(i, _)| i).collect::<Vec<usize>>();
+ if idx >= indices.len() {
+ return None;
+ }
let append_after = indices[indices.len() - idx] + 1;
st.insert_str(append_after, &s);
+ Some(())
+ } else {
+ None
}
}📝 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.
| /// Inserts string after the first occurrence of the separator. | |
| /// | |
| /// * `s` - The string to insert. | |
| /// * `pos` - The string inside inner vec to search position for. | |
| /// * `sep` - The separator to search for. | |
| /// * `idx` - The index of the separator to insert after. | |
| /// | |
| pub fn insert_after(&mut self, s: String, pos: &str, sep: &str, idx: usize) { | |
| let pos = self.0.iter().position(|b| b.contains(pos)).unwrap(); | |
| let pos = self.pos(pos).unwrap(); | |
| if let Some(st) = self.0.get_mut(pos) { | |
| let indices = st.match_indices(sep).map(|(i, _)| i).collect::<Vec<usize>>(); | |
| let append_after = indices[indices.len() - idx] + 1; | |
| st.insert_str(append_after, &s); | |
| } | |
| } | |
| /// Inserts string after the first occurrence of the separator. | |
| /// | |
| /// * `s` - The string to insert. | |
| /// * `pos` - The string inside inner vec to search position for. | |
| /// * `sep` - The separator to search for. | |
| /// * `idx` - The index of the separator to insert after. | |
| /// | |
| pub fn insert_after(&mut self, s: String, pos: &str, sep: &str, idx: usize) -> Option<()> { | |
| let pos = self.pos(pos)?; | |
| if let Some(st) = self.0.get_mut(pos) { | |
| let indices = st.match_indices(sep).map(|(i, _)| i).collect::<Vec<usize>>(); | |
| if idx >= indices.len() { | |
| return None; | |
| } | |
| let append_after = indices[indices.len() - idx] + 1; | |
| st.insert_str(append_after, &s); | |
| Some(()) | |
| } else { | |
| None | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
crates/dojo/bindgen/src/plugins/mod.rs (1)
178-201: Ohayo sensei! Consider adding more test cases.While the current tests cover the basic functionality, consider adding tests for edge cases:
- Empty buffer
- String not found scenarios
- Invalid indices
Example test cases:
#[test] fn test_buffer_get_first_after_not_found() { let mut buff = Buffer::new(); buff.push("test string".to_owned()); assert_eq!(buff.get_first_after("missing", "}", 0), None); } #[test] fn test_buffer_get_first_before_empty() { let buff = Buffer::new(); assert_eq!(buff.get_first_before_pos(",", 5, 0), None); }crates/dojo/bindgen/src/plugins/typescript/generator/function.rs (1)
Line range hint
218-325: Test coverage is comprehensive but could be more robust.The test cases cover the basic scenarios well, including multiple functions from different contracts. However, we could enhance the test coverage.
Consider adding these test cases:
- Empty contract name handling
- Special characters in contract names
- Duplicate function names across different contracts
Would you like me to help generate these additional test cases?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
crates/dojo/bindgen/src/plugins/mod.rs(2 hunks)crates/dojo/bindgen/src/plugins/typescript/generator/function.rs(8 hunks)
🔇 Additional comments (8)
crates/dojo/bindgen/src/plugins/mod.rs (6)
51-64: Previous error handling concerns still apply, sensei.
The existing error handling issues identified in the previous review remain valid.
66-75: Previous bounds checking concerns still apply, sensei.
The existing bounds checking issues identified in the previous review remain valid.
77-82: Ohayo sensei! Clean implementation of the pos method.
The method is well-documented and correctly handles the case when the string is not found.
88-105: Previous optimization suggestions still apply, sensei.
The existing performance optimization suggestions from the previous review remain valid.
107-123: Previous optimization suggestions still apply, sensei.
The existing performance optimization suggestions from the previous review remain valid.
51-123: Verify contract function name handling, sensei.
Let's verify how these Buffer modifications handle contract function names.
✅ Verification successful
Let me analyze the function name handling in more detail by checking the contract implementations.
Ohayo sensei! After analyzing the Buffer modifications and function handling, I can now provide a clear assessment.
Buffer modifications handle function names safely, no clashes detected.
The Buffer implementation provides precise control over function name handling through:
insert_after()method that safely inserts after specific separators with index controlinsert_at()for exact position insertions with bounds checkingpos()for safe position lookups that return Options
These modifications ensure:
- Contract function names are inserted at correct positions
- Proper separation between function names is maintained
- No accidental overwrites or name clashes can occur
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential function name clashes in contracts
# Look for function definitions in contract files
echo "Searching for function definitions..."
rg -g "*.cairo" "fn\s+\w+\s*\(" -A 2
# Look for generated TypeScript function names
echo "Searching for generated TypeScript functions..."
rg -g "*.ts" "function\s+\w+\s*\(" -A 2
Length of output: 196673
Script:
#!/bin/bash
# Look for contract function implementations and their prefixes
echo "Checking contract function implementations..."
ast-grep --pattern 'impl $contract$ {
$$$
fn $name$($args$) $ret$ {
$$$
}
$$$
}'
# Look for function name handling in Buffer implementation
echo "Checking Buffer function name handling..."
rg -g "*.rs" "fn insert_after|fn insert_at|fn pos" -A 5 -B 2
Length of output: 3692
crates/dojo/bindgen/src/plugins/typescript/generator/function.rs (2)
170-177: LGTM! Clean integration of contract name in generate method.
The changes to the generate method cleanly integrate the contract name into both function generation and return object setup.
32-34: Ohayo sensei! Function name prefixing looks good but needs validation.
The addition of contract name prefixing to function names helps prevent naming collisions. The implementation correctly formats the function name using camelCase.
Let's verify the contract name extraction is consistent:
✅ Verification successful
Ohayo sensei! Contract name extraction is consistent across the codebase.
The get_name_from_tag function is consistently implemented and used across the codebase:
- Core implementation in
dojo_worldanddojo_typesfollows the same pattern of splitting by separator and taking the last part - Used consistently in bindgen plugins (Unity, TypeScript, RECS) for model and contract naming
- The TypeScript function generator's usage aligns with this pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify contract name extraction across the codebase
# Look for other uses of get_name_from_tag to ensure consistent naming
rg "get_name_from_tag" -A 2
Length of output: 3236
| fn create_move_function() -> Function { | ||
| create_test_function("move", vec![]) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo! Consider enhancing the move function test helper.
The move function test helper could be more realistic by including typical movement parameters.
Consider this enhancement:
fn create_move_function() -> Function {
- create_test_function("move", vec![])
+ create_test_function("move", vec![
+ ("x".to_owned(), Token::CoreBasic(CoreBasic { type_path: "core::integer::u32".to_owned() })),
+ ("y".to_owned(), Token::CoreBasic(CoreBasic { type_path: "core::integer::u32".to_owned() })),
+ ])
}📝 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.
| fn create_move_function() -> Function { | |
| create_test_function("move", vec![]) | |
| } | |
| fn create_move_function() -> Function { | |
| create_test_function("move", vec![ | |
| ("x".to_owned(), Token::CoreBasic(CoreBasic { type_path: "core::integer::u32".to_owned() })), | |
| ("y".to_owned(), Token::CoreBasic(CoreBasic { type_path: "core::integer::u32".to_owned() })), | |
| ]) | |
| } |
| fn setup_function_wrapper_end( | ||
| &self, | ||
| contract_name: &str, | ||
| token: &Function, | ||
| buffer: &mut Buffer, | ||
| ) { | ||
| let return_token = "\treturn {"; | ||
| if !buffer.has(return_token) { | ||
| buffer | ||
| .push(format!("\treturn {{\n\t\t{},\n\t}};\n}}", token.name.to_case(Case::Camel))); | ||
| buffer.push(format!( | ||
| "\treturn {{\n\t\t{}: {{\n\t\t\t{}: {}_{},\n\t\t}},\n\t}};\n}}", | ||
| contract_name, | ||
| token.name.to_case(Case::Camel), | ||
| contract_name, | ||
| token.name.to_case(Case::Camel) | ||
| )); | ||
| return; | ||
| } | ||
|
|
||
| // if buffer has return and has contract_name, we append in this object if contract_name is | ||
| // the same | ||
|
|
||
| let contract_name_token = format!("\n\t\t{}: {{\n\t\t\t", contract_name); | ||
| if buffer.has(contract_name_token.as_str()) { | ||
| // we can expect safely as condition has is true there | ||
| let return_idx = buffer.pos(return_token).expect("return token not found"); | ||
| // find closing curly bracket to get closing of object `contract_name`, get the last | ||
| // comma and insert token just after it. | ||
| if let Some(pos) = buffer.get_first_after( | ||
| format!("\n\t\t{}: {{\n\t\t\t", contract_name).as_str(), | ||
| "}", | ||
| return_idx, | ||
| ) { | ||
| if let Some(insert_pos) = buffer.get_first_before_pos(",", pos, return_idx) { | ||
| buffer.insert_at( | ||
| format!( | ||
| "\n\t\t\t{}: {}_{},", | ||
| token.name.to_case(Case::Camel), | ||
| contract_name, | ||
| token.name.to_case(Case::Camel) | ||
| ), | ||
| insert_pos, | ||
| return_idx, | ||
| ); | ||
| return; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // if buffer has return but not contract_name, we append in this object | ||
| buffer.insert_after( | ||
| format!("\n\t\t{},", token.name.to_case(Case::Camel)), | ||
| format!( | ||
| "\n\t\t{}: {{\n\t\t\t{}: {}_{},\n\t\t}},", | ||
| contract_name, | ||
| token.name.to_case(Case::Camel), | ||
| contract_name, | ||
| token.name.to_case(Case::Camel), | ||
| ), |
There was a problem hiding this comment.
Complex return object manipulation needs defensive programming.
The setup_function_wrapper_end method handles complex string manipulation for the return object structure. While the logic is sound, it could benefit from additional error handling.
Consider adding these safety checks:
fn setup_function_wrapper_end(
&self,
contract_name: &str,
token: &Function,
buffer: &mut Buffer,
) {
+ if contract_name.is_empty() {
+ log::warn!("Empty contract name provided");
+ return;
+ }
let return_token = "\treturn {";
// ... rest of the implementationCommittable suggestion skipped: line range outside the PR's diff.
Description
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
New Features
Bug Fixes