Actionable comments posted: 1

🔭 Outside diff range comments (2)

compiler/codegen/src/compile.rs (2)

4040-4062: Use qualified name for lambda closures.

The lambda compilation is passing the simple name "<lambda>" instead of using the qualified name that was already set.

Store and use the qualified name:

 let name = "<lambda>".to_owned();
 let func_flags = self
     .enter_function(&name, parameters.as_deref().unwrap_or(&Default::default()))?;
 
 // Set qualname for lambda
-self.set_qualname();
+let qualname = self.set_qualname();
 
 self.ctx = CompileContext {
     loop_data: Option::None,
     in_class: prev_ctx.in_class,
     func: FunctionContext::Function,
 };
 
 self.current_code_info()
     .metadata
     .consts
     .insert_full(ConstantData::None);
 
 self.compile_expression(body)?;
 self.emit_return_value();
 let code = self.exit_scope();
 
 // Create lambda function with closure
-self.make_closure(code, &name, func_flags)?;
+self.make_closure(code, &qualname, func_flags)?;

---

4512-4607: Use qualified name for comprehension closures.

The comprehension is passing the simple name instead of the qualified name.

 // Set qualname for comprehension
-self.set_qualname();
+let qualname = self.set_qualname();
 
 // ... rest of comprehension compilation ...
 
 // Create comprehension function with closure
-self.make_closure(code, &name, bytecode::MakeFunctionFlags::empty())?;
+self.make_closure(code, &qualname, bytecode::MakeFunctionFlags::empty())?;

🧹 Nitpick comments (2)

compiler/codegen/src/compile.rs (2)

1996-1996: Simplify the iterator by removing unnecessary dereferencing.

The &* dereferencing is unnecessary here.

-        for var in &*code.freevars {
+        for var in &code.freevars {

---

2034-2037: Avoid unnecessary variable shadowing.

The mutable variable shadowing is unnecessary here. You can modify the existing binding directly.

-            let mut idx = idx;
-            if let SymbolScope::Free = symbol.scope {
+            let idx = if let SymbolScope::Free = symbol.scope {
-                idx += parent_code.metadata.cellvars.len();
+                idx + parent_code.metadata.cellvars.len()
+            } else {
+                idx
-            }
+            };

📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ac20b00 and acbea17.

📒 Files selected for processing (1)

• compiler/codegen/src/compile.rs (7 hunks)

🧰 Additional context used 📓 Path-based instructions (1) **/*.rs

Instructions used from:

Sources:
📄 CodeRabbit Inference Engine

• .github/copilot-instructions.md

⏰ 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). (10)

• GitHub Check: Run snippets and cpython tests on wasm-wasi
• GitHub Check: Check the WASM package and demo
• GitHub Check: Run tests under miri
• GitHub Check: Run rust tests (windows-latest)
• GitHub Check: Run rust tests (macos-latest)
• GitHub Check: Check Rust code with rustfmt and clippy
• GitHub Check: Ensure compilation on various targets
• GitHub Check: Run snippets and cpython tests (ubuntu-latest)
• GitHub Check: Run snippets and cpython tests (windows-latest)
• GitHub Check: Run snippets and cpython tests (macos-latest)

🔇 Additional comments (1)

compiler/codegen/src/compile.rs (1)

1985-2067: Well-structured refactoring that consolidates closure handling.

The introduction of make_closure successfully centralizes the complex logic of loading closure variables, handling different symbol scopes, and creating function objects. The error handling is comprehensive and the implementation correctly handles all the edge cases including FREE_CLASS symbols.

Actionable comments posted: 0

♻️ Duplicate comments (1)

compiler/codegen/src/compile.rs (1)

2263-2264: Use qualified name instead of simple name for class closures.

The make_closure calls for class definitions are passing the simple class name instead of the qualified name. This is inconsistent with how functions handle it and could lead to incorrect qualified names in the bytecode.

Consider modifying compile_class_body to return both the code object and the qualified name, then use the qualified name in the make_closure calls.

Also applies to: 2324-2325

🧹 Nitpick comments (2)

compiler/codegen/src/compile.rs (2)

4041-4041: Capture and use the qualified name for lambda closures.

The lambda compilation sets the qualified name but doesn't capture it. Consider storing the result of self.set_qualname() and passing it to make_closure for consistency:

-// Set qualname for lambda
-self.set_qualname();
+// Set qualname for lambda
+let qualname = self.set_qualname();

Then use it in the make_closure call:

-self.make_closure(code, &name, func_flags)?;
+self.make_closure(code, &qualname, func_flags)?;

Also applies to: 4058-4059

---

4510-4510: Capture and use the qualified name for comprehension closures.

Similar to lambdas, comprehensions compute but don't use their qualified names. Consider capturing and using the qualified name:

-// Set qualname for comprehension
-self.set_qualname();
+// Set qualname for comprehension
+let qualname = self.set_qualname();

Then use it in the make_closure call:

-self.make_closure(code, name, bytecode::MakeFunctionFlags::empty())?;
+self.make_closure(code, &qualname, bytecode::MakeFunctionFlags::empty())?;

Also applies to: 4603-4604

📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between acbea17 and 1630c71.

📒 Files selected for processing (1)

• compiler/codegen/src/compile.rs (7 hunks)

🧰 Additional context used 📓 Path-based instructions (1) **/*.rs

Instructions used from:

Sources:
📄 CodeRabbit Inference Engine

• .github/copilot-instructions.md

⏰ 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 (ubuntu-latest)
• GitHub Check: Run snippets and cpython tests (macos-latest)
• GitHub Check: Run rust tests (windows-latest)
• GitHub Check: Run snippets and cpython tests (windows-latest)
• GitHub Check: Run tests under miri
• GitHub Check: Check the WASM package and demo
• GitHub Check: Run snippets and cpython tests on wasm-wasi
• 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: Ensure compilation on various targets