Actionable comments posted: 0

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

609-630: Avoid unsafe + unwrap_unchecked() in push_symbol_table

The invariant (“we just pushed, so it can't be empty”) is correct, but unsafe { last().unwrap_unchecked() } is unnecessary here and makes the code harder to audit. A safe version is just as concise and keeps the same “compiler bug” semantics:

-        self.symbol_table_stack.push(table);
-        // SAFETY: We just pushed, so it can't be empty
-        unsafe { &self.symbol_table_stack.last().unwrap_unchecked() }
+        self.symbol_table_stack.push(table);
+        self.symbol_table_stack
+            .last()
+            .expect("symbol_table_stack is empty after push! This is a compiler bug.")

This also matches the style used in current_symbol_table().

---

1192-1210: Make TypeParams parent lookup avoid len() - 2 underflow

In the TypeParams special case, the parent table is fetched via:

self.symbol_table_stack
    .get(self.symbol_table_stack.len() - 2)
    .expect("Symbol has no parent! This is a compiler bug.")
    .lookup(name.as_ref())

Given current invariants, len() >= 2 whenever is_typeparams is true, but if that invariant is ever broken, the code will panic earlier with an attempt to subtract with overflow instead of the intended “compiler bug” message.

You can keep the same semantics while avoiding this sharp edge and using more idiomatic traversal:

-            let symbol = if symbol.is_none() && is_typeparams {
-                self.symbol_table_stack
-                    .get(self.symbol_table_stack.len() - 2) // Try to get parent index
-                    .expect("Symbol has no parent! This is a compiler bug.")
-                    .lookup(name.as_ref())
-            } else {
-                symbol
-            };
+            let symbol = if symbol.is_none() && is_typeparams {
+                let parent = self
+                    .symbol_table_stack
+                    .iter()
+                    .rev()
+                    .nth(1)
+                    .expect("Symbol has no parent! This is a compiler bug.");
+                parent.lookup(name.as_ref())
+            } else {
+                symbol
+            };

This keeps the “compiler bug” panic but removes the intermediate len() - 2 arithmetic.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between f49c185 and ee7377e.

• crates/codegen/src/compile.rs (4 hunks)

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by 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

Files:

• crates/codegen/src/compile.rs

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.rs : Follow Rust best practices for error handling and memory management

Applied to files:

• crates/codegen/src/compile.rs

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

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

558-563: current_symbol_table now uses idiomatic Vec::last access

Using symbol_table_stack.last().expect(...) is clearer and avoids manual indexing while still treating an empty stack as an internal compiler bug. No issues here.

---

657-665: enter_scope symbol-table lookup now uses Vec::get safely

Switching from manual index-based access to self.symbol_table_stack.get(key) with an explicit SyntaxError("unknown symbol table entry") on None is a good, idiomatic improvement and keeps out-of-bounds conditions from turning into generic panics.

Actionable comments posted: 0

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

612-629: Avoid unsafe / unwrap_unchecked in push_symbol_table

The unwrap_unchecked here is correct under the stated invariant, but it introduces unsafe for a very small gain. Since this isn’t an ultra‑hot path and the stack cannot be empty after push, you can keep the code safe and aligned with the PR’s intent (idiomatic Vec access) by reusing current_symbol_table instead of unsafe:

         let table = current_table.sub_tables.remove(0);

         // Push the next table onto the stack
         self.symbol_table_stack.push(table);
-        // SAFETY: We just pushed, so it can't be empty
-        unsafe { self.symbol_table_stack.last().unwrap_unchecked() }
+        self.current_symbol_table()

This preserves behavior, removes unsafe, and keeps all Vec access in one helper. As per coding guidelines, safer patterns are preferred when possible.

---

1193-1204: Make parent lookup in TypeParams scope avoid len() - 2 arithmetic

The TypeParams parent lookup now uses Vec::get(self.symbol_table_stack.len() - 2).expect(...). Logically this matches the existing invariant (“there must be a parent”), but len() - 2 can underflow in debug builds if that invariant is ever broken, yielding a less helpful panic than your custom expect message.

You can stay within the “idiomatic Vec access” goal and avoid manual index math by using an iterator from the back to get “the element before last”:

-            // If not found and we're in TypeParams scope, try parent scope
-            let symbol = if symbol.is_none() && is_typeparams {
-                self.symbol_table_stack
-                    .get(self.symbol_table_stack.len() - 2) // Try to get parent index
-                    .expect("Symbol has no parent! This is a compiler bug.")
-                    .lookup(name.as_ref())
-            } else {
-                symbol
-            };
+            // If not found and we're in TypeParams scope, try parent scope
+            let symbol = if symbol.is_none() && is_typeparams {
+                let parent = self
+                    .symbol_table_stack
+                    .iter()
+                    .rev()
+                    .nth(1) // second-from-last element is the parent
+                    .expect("Symbol has no parent! This is a compiler bug.");
+                parent.lookup(name.as_ref())
+            } else {
+                symbol
+            };

This keeps the same invariant but avoids explicit len() - 2 arithmetic on the Vec.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between ee7377e and e24221b.

• crates/codegen/src/compile.rs (4 hunks)

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by 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

Files:

• crates/codegen/src/compile.rs

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.rs : Follow Rust best practices for error handling and memory management

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

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

560-562: current_symbol_table now uses Vec::last with a clear invariant

Using .last().expect("symbol_table_stack is empty! ...") is an idiomatic way to express the internal invariant and avoids manual indexing. Looks good.

---

658-664: Guarded enter_scope symbol-table lookup is clear and safe

Switching from direct indexing to self.symbol_table_stack.get(key) with an explicit SyntaxError("unknown symbol table entry") on None makes this more robust and self-documenting, while keeping the invariant that callers must pass a valid key. No issues here.

👍