Add SymbolUsage::TypeParams - #5941
Conversation
Sorry, something went wrong.
WalkthroughThe changes add explicit support for the TypeParams variant in the symbol table and compiler. Type parameters are treated as cell variables during symbol analysis, registration, and closure building. Additionally, the bytecode execution logic for LoadClassDeref is corrected to properly index cell and free variables. Changes
Sequence Diagram(s)sequenceDiagram
participant Compiler
participant SymbolTable
participant CodeObject
participant VMFrame
Compiler->>SymbolTable: scan_type_params(type_params)
SymbolTable-->>Compiler: registers .type_params and individual type params as TypeParam usage
Compiler->>SymbolTable: analyze_symbol(symbol)
alt symbol scope is TypeParams
SymbolTable->>SymbolTable: convert TypeParams scope to Cell scope
end
Compiler->>Compiler: compile_name(name, usage)
alt SymbolScope is TypeParams
Compiler->>Compiler: treat as Cell scope, use cellvar_cache, emit deref instructions
end
Compiler->>Compiler: build_closure(code)
alt SymbolScope is TypeParams
Compiler->>Compiler: treat as Cell scope for closure variables
end
VMFrame->>VMFrame: execute_instruction(LoadClassDeref)
alt i < cellvars.len()
VMFrame->>cellvars: get name at i
else
VMFrame->>freevars: get name at i - cellvars.len()
end
Poem
📜 Recent review details Configuration used: .coderabbit.yml Reviewing files that changed from the base of the PR and between 4894ab4 and 84d1a32. ⛔ Files ignored due to path filters (1)
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md) List of files the instruction was applied to:
vm/src/frame.rs (1)✨ 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. ❤️ Share 🪧 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.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)compiler/codegen/src/symboltable.rs (1)🧹 Nitpick comments (1)545-547: Implement TypeParams handling in comprehensions.
The TODO indicates missing implementation for type parameters in comprehensions. This will cause a panic if encountered. Consider implementing this case or documenting why it's not needed.
Would you like me to help implement the TypeParams case or open an issue to track this?
compiler/codegen/src/compile.rs (1)📜 Review details1558-1566: Consider simplifying lambda qualname handling.
The code retrieves the qualname from the code object, but for lambdas it's already set to "<lambda>" on line 1539. This indirection is unnecessary.
Consider using the constant directly:
- // Get the qualname from the code object - let qualname = code.qualname.clone(); - self.emit_load_const(ConstantData::Code { code: Box::new(code), }); self.emit_load_const(ConstantData::Str { - value: qualname.into(), + value: "<lambda>".into(), });
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Reviewing files that changed from the base of the PR and between 2c30e01 and 4894ab4.
📒 Files selected for processing (2)- compiler/codegen/src/compile.rs (8 hunks)
- compiler/codegen/src/symboltable.rs (7 hunks)
**/*.rs: Follow the default rustfmt code style (cargo fmt to 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.rs
- compiler/codegen/src/compile.rs
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run tests under miri
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Ensure compilation on various targets
compiler/codegen/src/symboltable.rs (4)84-84: LGTM! Consistent enum additions for type parameter support.
The addition of TypeParams to both SymbolTableType and SymbolScope enums is well-integrated, with proper display formatting that matches the pattern of other variants.
Also applies to: 94-94, 116-116
363-366: Correct handling of TypeParams scope conversion.
The conversion of TypeParams scope to Cell scope ensures type parameters are properly treated as cell variables, which aligns with Python's type parameter semantics.
1276-1278: Well-structured type parameter registration.
The registration of .type_params as a special symbol and the consistent use of TypeParam usage for all type parameter variants (TypeVar, ParamSpec, TypeVarTuple) provides clear semantic distinction from regular assignments.
Also applies to: 1288-1288, 1298-1298, 1305-1305
1556-1560: Proper TypeParam registration as cell variables.
The handling correctly sets type parameters as cell variables with the ASSIGNED flag, ensuring they're properly accessible within their scope and consistent with the scope conversion in analyze_symbol.
compiler/codegen/src/compile.rs (5)432-434: LGTM! Correct handling of qualname for global classes.
The logic correctly sets the qualname to just the class name for global classes, which aligns with Python's behavior.
664-672: LGTM! Correct implementation of TypeParams scope handling.
The code correctly treats type parameters as cell variables, using the cellvar_cache and Deref operations consistently with the Cell scope handling.
1540-1541: LGTM! Consistent qualname handling for functions.
The code correctly uses the qualname from the compiled code object, ensuring consistency with how the qualname was set during function compilation.
Also applies to: 1625-1628
1761-1771: LGTM! Proper qualname retrieval for classes.
The code correctly retrieves the qualname from the current code info, which was set during push_output with the global class logic applied.
1664-1664: LGTM! Consistent TypeParams handling in closures.
The code correctly handles TypeParams scope in closure building, treating type parameters as cell variables consistently with the rest of the implementation.
Sorry, something went wrong.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor