feat(dojo-compiler): add sierra to cairo debug information #2392
Conversation
WalkthroughOhayo, sensei! The recent changes enhance the DojoCompiler by integrating debug information into the compilation process. A new structure, Changes
Sequence Diagram(s)sequenceDiagram
participant Compiler
participant DebugInfo
participant Workspace
Compiler->>DebugInfo: Request debug information
DebugInfo-->>Compiler: Return debug data
Compiler->>Workspace: Save compiled classes with debug info
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (1)
Additional comments not posted (5)
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.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range, codebase verification and nitpick comments (1)
crates/dojo-lang/src/scarb_internal/mod.rs (1)
35-35: Ohayo, sensei! New module declaration looks good.The
pub mod debug;declaration is syntactically correct and properly placed within the file. However, it would be beneficial to add documentation to clarify the responsibilities and usage of thedebugmodule to help other developers understand its role in the system.
| fn compile_contract_with_prepared_and_checked_db_to_debug_info( | ||
| db: &RootDatabase, | ||
| contract: &ContractDeclaration, | ||
| ) -> Result<SierraProgramDebugInfo> { | ||
| let SemanticEntryPoints { external, l1_handler, constructor } = | ||
| extract_semantic_entrypoints(db, contract)?; | ||
| let SierraProgramWithDebug { program: _sierra_program, debug_info } = Arc::unwrap_or_clone( | ||
| db.get_sierra_program_for_functions( | ||
| chain!(&external, &l1_handler, &constructor).map(|f| f.value).collect(), | ||
| ) | ||
| .to_option() | ||
| .with_context(|| "Compilation failed without any diagnostics.")?, | ||
| ); | ||
|
|
||
| Ok(debug_info) |
There was a problem hiding this comment.
Consider enhancing error messages.
The function compile_contract_with_prepared_and_checked_db_to_debug_info is well-implemented. However, consider adding more specific error messages or handling for different cases to improve debugging and maintenance.
| #[derive(Debug, Clone, Serialize)] | ||
| pub struct SierraToCairoDebugInfo { | ||
| pub sierra_statements_to_cairo_info: HashMap<usize, SierraStatementToCairoDebugInfo>, | ||
| } |
There was a problem hiding this comment.
Add documentation for clarity.
The SierraToCairoDebugInfo structure is crucial for mapping debug information. Consider adding documentation to explain its purpose and usage within the system.
| /// Returns a map from Sierra statement indexes to Cairo function names. | ||
| pub fn get_sierra_to_cairo_debug_info( | ||
| sierra_program_debug_info: &SierraProgramDebugInfo, | ||
| compiler_db: &RootDatabase, | ||
| ) -> SierraToCairoDebugInfo { | ||
| let mut sierra_statements_to_cairo_info: HashMap<usize, SierraStatementToCairoDebugInfo> = | ||
| HashMap::new(); | ||
|
|
||
| for (statement_idx, locations) in | ||
| sierra_program_debug_info.statements_locations.locations.iter_sorted() | ||
| { | ||
| let mut cairo_locations: Vec<Location> = Vec::new(); | ||
| for location in locations { | ||
| let syntax_node = location.syntax_node(compiler_db); | ||
| let file_id = syntax_node.stable_ptr().file_id(compiler_db); | ||
| let _file_name = file_id.file_name(compiler_db); | ||
| let syntax_node_location_span = syntax_node.span_without_trivia(compiler_db); | ||
|
|
||
| let (originating_file_id, originating_text_span) = | ||
| get_originating_location(compiler_db, file_id, syntax_node_location_span); | ||
| let cairo_location = get_location_from_text_span( | ||
| originating_text_span, | ||
| originating_file_id, | ||
| compiler_db, | ||
| ); | ||
| if let Some(cl) = cairo_location { | ||
| cairo_locations.push(cl); | ||
| } | ||
| } | ||
| sierra_statements_to_cairo_info | ||
| .insert(statement_idx.0, SierraStatementToCairoDebugInfo { cairo_locations }); | ||
| } | ||
|
|
||
| SierraToCairoDebugInfo { sierra_statements_to_cairo_info } | ||
| } |
There was a problem hiding this comment.
Optimize data handling.
The function get_sierra_to_cairo_debug_info performs multiple iterations and data transformations. Consider optimizing these processes to improve performance and maintainability.
| pub fn get_location_from_text_span( | ||
| text_span: TextSpan, | ||
| file_id: FileId, | ||
| compiler_db: &RootDatabase, | ||
| ) -> Option<Location> { | ||
| let current_dir = env::current_dir().expect("Failed to get current directory"); | ||
| // dbg!(¤t_dir); | ||
| // let file_path = match compiler_db.lookup_intern_file(file_id) { | ||
| // FileLongId::OnDisk(path) => { | ||
| // path.strip_prefix(current_dir).expect("Failed to get relative | ||
| // path").to_path_buf().to_str().unwrap_or("<unknown>").to_string() }, | ||
| // FileLongId::Virtual(_) => file_id.full_path(compiler_db), | ||
| // }; | ||
| let file_path = match compiler_db.lookup_intern_file(file_id) { | ||
| FileLongId::OnDisk(path) => match path.strip_prefix(¤t_dir) { | ||
| Ok(relative_path) => relative_path.to_str().unwrap_or("<unknown>").to_string(), | ||
| Err(_) => { | ||
| return None; | ||
| } | ||
| }, | ||
| FileLongId::Virtual(_) => { | ||
| return None; | ||
| } | ||
| }; | ||
|
|
||
| // let file_path = file_id.full_path(compiler_db); | ||
|
|
||
| let start: Option<TextPosition> = text_span | ||
| .start | ||
| .position_in_file(compiler_db, file_id) | ||
| .map(|s| TextPosition { line: s.line, col: s.col }); | ||
|
|
||
| let end = text_span | ||
| .end | ||
| .position_in_file(compiler_db, file_id) | ||
| .map(|e| TextPosition { line: e.line, col: e.col }); | ||
|
|
||
| start.zip(end).map(|(start, end)| Location { start, end, file_path }) | ||
| } |
There was a problem hiding this comment.
Improve file path handling.
The function get_location_from_text_span handles file paths and positions. Consider making the file path handling more robust to avoid potential issues with path resolutions and errors.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2392 +/- ##
==========================================
+ Coverage 68.37% 68.42% +0.04%
==========================================
Files 357 358 +1
Lines 47188 47328 +140
==========================================
+ Hits 32267 32386 +119
- Misses 14921 14942 +21 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
crates/dojo-lang/src/compiler.rs (1)
Line range hint
104-206: Major changes in thecompilemethod to support debug information.The
compilemethod has been significantly refactored to support the collection of debug information based on thewith_debug_infoflag. This is a robust enhancement that aligns with the PR's objectives to improve debugging capabilities for the Dojo compiler.However, there are a couple of TODO comments (lines 104 and 146) that suggest further simplifications and enhancements are planned. It would be wise to track these in your project management tool to ensure they are not forgotten.
Would you like me to open a GitHub issue to track the completion of these TODO items?
related #2333
Summary by CodeRabbit
New Features
Bug Fixes
Documentation