SymbolTable::varnames, fblock#5948
Conversation
Sorry, something went wrong.
|
Warning Rate limit exceeded@youknowone has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 35 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 (2)
WalkthroughThe changes introduce a structured mechanism for managing control flow blocks (such as loops and exception handlers) during code generation by adding an Changes
Sequence Diagram(s)sequenceDiagram
participant Compiler
participant CodeInfo
participant FBlockStack
participant SymbolTable
Compiler->>CodeInfo: Start compiling code unit
CodeInfo->>SymbolTable: Retrieve varnames in definition order
CodeInfo->>FBlockStack: push_fblock (on entering loop/try)
loop For each statement
Compiler->>FBlockStack: push_fblock/pop_fblock as control flow changes
Compiler->>FBlockStack: Search for enclosing loop on break/continue
end
CodeInfo->>CodeUnitMetadata: Update metadata (names, consts, etc.)
CodeInfo->>Compiler: Finalize code object
Possibly related PRs
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
Sorry, something went wrong.
![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 (2)
compiler/codegen/src/compile.rs (2)
1150-1205: Consider refactoring duplicated loop-finding logic.The Break and Continue statement handlers have nearly identical code for finding the innermost loop in the fblock stack. Consider extracting this into a helper method to reduce duplication.
+fn find_innermost_loop(&self) -> Option<(BlockIdx, BlockIdx)> { + let code = self.current_code_info(); + for i in (0..code.fblock.len()).rev() { + match code.fblock[i].fb_type { + FBlockType::WhileLoop | FBlockType::ForLoop => { + return Some((code.fblock[i].fb_block, code.fblock[i].fb_exit)); + } + _ => continue, + } + } + None +} Stmt::Break(_) => { - // Find the innermost loop in fblock stack - let found_loop = { - let code = self.current_code_info(); - let mut result = None; - for i in (0..code.fblock.len() as usize).rev() { - match code.fblock[i].fb_type { - FBlockType::WhileLoop | FBlockType::ForLoop => { - result = Some(code.fblock[i].fb_exit); - break; - } - _ => continue, - } - } - result - }; - - match found_loop { - Some(exit_block) => { + match self.find_innermost_loop() { + Some((_, exit_block)) => { emit!(self, Instruction::Break { target: exit_block }); } None => { return Err( self.error_ranged(CodegenErrorType::InvalidBreak, statement.range()) ); } } } Stmt::Continue(_) => { - // Find the innermost loop in fblock stack - let found_loop = { - let code = self.current_code_info(); - let mut result = None; - for i in (0..code.fblock.len() as usize).rev() { - match code.fblock[i].fb_type { - FBlockType::WhileLoop | FBlockType::ForLoop => { - result = Some(code.fblock[i].fb_block); - break; - } - _ => continue, - } - } - result - }; - - match found_loop { - Some(loop_block) => { + match self.find_innermost_loop() { + Some((loop_block, _)) => { emit!(self, Instruction::Continue { target: loop_block }); } None => { return Err( self.error_ranged(CodegenErrorType::InvalidContinue, statement.range()) ); } } }
1155-1155: Remove unnecessary cast to usize.The expression
code.fblock.len() as usizehas a redundant cast sinceVec::len()already returnsusize.-for i in (0..code.fblock.len() as usize).rev() { +for i in (0..code.fblock.len()).rev() {Also applies to: 1183-1183
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
compiler/codegen/src/compile.rs(25 hunks)compiler/codegen/src/ir.rs(5 hunks)compiler/codegen/src/symboltable.rs(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.rs`: Follow the default rustfmt code style (`cargo fmt` to format) Always ...
**/*.rs: Follow the default rustfmt code style (cargo fmtto format)
Always run clippy to lint code (cargo clippy) before completing tasks. Fix any warnings or lints that are introduced by your changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md)
List of files the instruction was applied to:
compiler/codegen/src/symboltable.rscompiler/codegen/src/ir.rscompiler/codegen/src/compile.rs
🧠 Learnings (1)
compiler/codegen/src/compile.rs (1)
undefined
<retrieved_learning>
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T10:08:48.858Z
Learning: Applies to **/*.rs : Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust
</retrieved_learning>
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Check Rust code with rustfmt and clippy
🔇 Additional comments (8)
compiler/codegen/src/symboltable.rs (1)
49-51: Good addition for tracking variable definition orderThe
varnamesfield properly captures the requirement to store variable names in definition order. The integration with the symbol table building process is well-designed.compiler/codegen/src/ir.rs (4)
10-26: Well-structured metadata consolidationThe
CodeUnitMetadatastruct effectively consolidates all code unit-related metadata fields that were previously scattered inCodeInfo. The field names are clear and the comment references to the CPython equivalents (u_name,u_consts, etc.) help with understanding the mapping.
94-103: Clean refactoring with useful additionsThe refactoring successfully consolidates metadata into a single field while adding new functionality:
fblockstack for tracking nested control structures (loops, exception handlers)static_attributesfor class scope attribute trackingin_inlined_compflag for comprehension contextAll these additions align well with the PR objectives for enhanced control flow management.
211-239: Correct migration to metadata accessThe
cell2arg()method has been properly updated to access all required fields from themetadatastruct. The logic remains unchanged while the field access is now properly scoped.
330-347: Consistent InstrDisplayContext implementationAll methods in the
InstrDisplayContextimplementation have been correctly updated to fetch data from themetadatafield. The implementation maintains the same interface while adapting to the new structure.compiler/codegen/src/compile.rs (3)
18-43: Well-structured control flow block types.The
FBlockTypeenum andFBlockInfostruct provide a clean abstraction for managing different control flow contexts. The design aligns well with CPython's approach while adapting to RustPython's needs.
2129-2130: Correct integration of fblock tracking in loops.The while and for loop implementations properly push and pop fblocks at the appropriate points, ensuring the control flow stack remains consistent.
Also applies to: 2144-2146, 2258-2259, 2283-2285, 2299-2301
337-354: Clean metadata organization and proper initialization.The consolidation of metadata into
CodeUnitMetadataimproves code organization. The initialization ofvarname_cachefromSymbolTable::varnamesensures variable names are properly tracked in definition order.Also applies to: 438-440, 467-474
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
compiler/codegen/src/compile.rs (1)
486-515: The TODO comment for type validation should be addressed.The
pop_fblockmethod accepts an_expected_typeparameter but doesn't validate it against the actual block type being popped. This could lead to subtle bugs if blocks are popped in the wrong order.The previous review comment about implementing type validation is still applicable and should be addressed.
🧹 Nitpick comments (1)
compiler/codegen/src/compile.rs (1)
2132-2134: Consider removing redundant loop_data context.The code still maintains the old
loop_datacontext alongside the new fblock system. This appears redundant since the fblock stack now handles loop context tracking. Consider removing the oldloop_datamechanism to avoid confusion and potential inconsistencies.- let was_in_loop = self.ctx.loop_data.replace((while_block, after_block)); self.compile_statements(body)?; - self.ctx.loop_data = was_in_loop;Also applies to: 2291-2293
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
compiler/codegen/src/compile.rs(25 hunks)compiler/codegen/src/ir.rs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- compiler/codegen/src/ir.rs
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.rs`: Follow the default rustfmt code style (`cargo fmt` to format) Always ...
**/*.rs: Follow the default rustfmt code style (cargo fmtto format)
Always run clippy to lint code (cargo clippy) before completing tasks. Fix any warnings or lints that are introduced by your changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md)
List of files the instruction was applied to:
compiler/codegen/src/compile.rs
🧠 Learnings (1)
compiler/codegen/src/compile.rs (1)
undefined
<retrieved_learning>
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T10:08:48.858Z
Learning: Applies to **/*.rs : Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust
</retrieved_learning>
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
🔇 Additional comments (6)
compiler/codegen/src/compile.rs (6)
18-43: LGTM: Well-structured control flow block management.The
FBlockTypeenum andFBlockInfostruct provide a clear and comprehensive way to manage different types of control flow blocks during compilation. The enum covers all necessary block types including loops, exception handlers, and async constructs.
1149-1204: Excellent implementation of break/continue with fblock stack.The new break and continue statement handling correctly searches the fblock stack from top to bottom to find the nearest enclosing loop. Using
fb_exitfor break statements andfb_blockfor continue statements is the correct approach.
2127-2144: Good integration of fblock management in while loops.The while loop compilation correctly pushes and pops the fblock around the loop body, enabling proper break/continue handling.
2255-2299: Proper fblock management for for loops.The for loop compilation (both sync and async versions) correctly integrates fblock management, matching the pattern used in while loops.
337-353: Good consolidation of metadata into CodeUnitMetadata.The refactoring to consolidate code unit metadata fields into a single
CodeUnitMetadatastruct improves code organization and makes the codebase more maintainable.Also applies to: 453-473
279-280: Proper initialization of inlined comprehension flag.The
in_inlined_compfield is correctly initialized and will be properly managed for comprehension contexts.
Sorry, something went wrong.
youknowone
force-pushed
the
fblock
branch
2 times, most recently
from
beb9432 to
2a8ee18
Compare
July 11, 2025 13:40
youknowone
merged commit c4234c1
into
RustPython:main
Jul 11, 2025
Summary by CodeRabbit
New Features
breakandcontinue) for more accurate and robust code generation.Refactor