Add SymbolUsage::TypeParams #5941
Conversation
WalkthroughThe changes add explicit support for the 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 detailsConfiguration used: .coderabbit.yml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used📓 Path-based instructions (1)`**/*.rs`: Follow the default rustfmt code style (`cargo fmt` to format) Always ...
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md) List of files the instruction was applied to:
⏰ 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)
🔇 Additional comments (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. 🪧 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
compiler/codegen/src/symboltable.rs (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?
🧹 Nitpick comments (1)
compiler/codegen/src/compile.rs (1)
1558-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(), });
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
compiler/codegen/src/compile.rs(8 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/compile.rs
⏰ 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 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
🔇 Additional comments (9)
compiler/codegen/src/symboltable.rs (4)
84-84: LGTM! Consistent enum additions for type parameter support.The addition of
TypeParamsto bothSymbolTableTypeandSymbolScopeenums 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
TypeParamsscope toCellscope 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_paramsas a special symbol and the consistent use ofTypeParamusage 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_outputwith 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.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor