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

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

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

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