crates/vm/src/suggestion.rs (1)

50-55: Pre-existing: offer_suggestions uses .is() for attribute_error but .fast_issubclass() for import_error.

Line 51 uses exact type match (.is()) for attribute_error, while line 73 uses subclass check (.fast_issubclass()) for import_error. CPython's suggestion mechanism uses PyErr_GivenExceptionMatches (subclass-aware) for all branches. If a custom AttributeError subclass is raised, suggestions won't be offered with the current code. Not introduced by this PR, but worth noting for consistency.

crates/vm/src/builtins/frame.rs (1)

149-176: Missing generator/coroutine handling in frame.clear().

CPython's frame.clear() also handles the case where the frame belongs to a suspended generator or coroutine — it closes it by throwing GeneratorExit into it, and if the frame is currently executing, it sets a flag to clear when the frame is next entered rather than raising immediately.

This implementation correctly rejects executing frames and clears locals, but doesn't address generator-owned frames. If clear() is called on a frame backing a suspended generator, the generator may continue to reference stale (cleared) state.

Consider adding a TODO or documenting this limitation if it's intentionally deferred.

Suggested improvement

     #[pymethod]
     fn clear(&self, vm: &VirtualMachine) -> PyResult<()> {
         // Check if the frame is currently executing
         let is_executing = vm.frames.borrow().iter().any(|f| f.is(self.as_object()));
         if is_executing {
             return Err(vm.new_runtime_error("cannot clear an executing frame".to_owned()));
         }
 
+        // TODO: Handle generator/coroutine frames - CPython closes suspended
+        // generators by throwing GeneratorExit before clearing the frame.
+        // See PyFrame_Clear in CPython's frameobject.c.
+
         // Clear fastlocals

crates/vm/src/stdlib/sys.rs (1)

700-727: normalize_exception is called twice on fallback.

When the Python traceback.print_exception path fails (e.g., print_exc.call() raises), the code falls through and normalizes the exception again on line 715. Since normalization should be idempotent this isn't a correctness issue, but you could normalize once and reuse the result for both paths to avoid redundant work.

Suggested refactor to normalize once

     fn excepthook(
         exc_type: PyObjectRef,
         exc_val: PyObjectRef,
         exc_tb: PyObjectRef,
         vm: &VirtualMachine,
     ) -> PyResult<()> {
-        // Use Python traceback module for exception display.
-        // This enables features like keyword suggestions in SyntaxError.
-        if let Ok(tb_mod) = vm.import("traceback", 0)
-            && let Ok(print_exc) = tb_mod.get_attr("print_exception", vm)
-        {
-            let exc = vm.normalize_exception(exc_type.clone(), exc_val.clone(), exc_tb.clone());
-            if let Ok(exc) = exc
-                && print_exc.call((exc.as_object().to_owned(),), vm).is_ok()
-            {
-                return Ok(());
-            }
-        }
-
-        // Fallback to Rust-level exception printing
-        let stderr = super::get_stderr(vm)?;
-        match vm.normalize_exception(exc_type.clone(), exc_val.clone(), exc_tb) {
+        let stderr = super::get_stderr(vm)?;
+        match vm.normalize_exception(exc_type.clone(), exc_val.clone(), exc_tb) {
+            Ok(exc) => {
+                // Try Python traceback module first for richer output
+                if let Ok(tb_mod) = vm.import("traceback", 0)
+                    && let Ok(print_exc) = tb_mod.get_attr("print_exception", vm)
+                    && print_exc.call((exc.as_object().to_owned(),), vm).is_ok()
+                {
+                    return Ok(());
+                }
+                // Fallback to Rust-level exception printing
+                vm.write_exception(&mut crate::py_io::PyWriter(stderr, vm), &exc)
+            }

crates/vm/src/vm/vm_new.rs (1)

343-364: Tab/space detection scans across the entire source, not per-block.

The heuristic flags a TabError if any line uses a leading tab and any other line uses a leading space, even if they belong to completely unrelated blocks. CPython's detection is tighter (per tokenizer context). This is a reasonable approximation for error classification but may misfire on files that legitimately mix indentation styles in separate top-level blocks (unusual but not invalid in Python).

Not a blocker, just worth documenting in a comment.

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

496-554: Deduplicate range restoration before emitting subscript ops.

set_source_range(subscript_range) is repeated in both branches; consider hoisting it after slice compilation and branching only for the emit step to keep this DRY.

Suggested refactor

-        if slice.should_use_slice_optimization() && !matches!(ctx, ast::ExprContext::Del) {
+        let use_slice_opt =
+            slice.should_use_slice_optimization() && !matches!(ctx, ast::ExprContext::Del);
+        if use_slice_opt {
             match slice {
                 ast::Expr::Slice(s) => self.compile_slice_two_parts(s)?,
                 _ => unreachable!(
                     "should_use_slice_optimization should only return true for ast::Expr::Slice"
                 ),
             };
-            // Restore full subscript expression range before emitting
-            self.set_source_range(subscript_range);
-            match ctx {
-                ast::ExprContext::Load => {
-                    emit!(self, Instruction::BinarySlice);
-                }
-                ast::ExprContext::Store => {
-                    emit!(self, Instruction::StoreSlice);
-                }
-                _ => unreachable!(),
-            }
         } else {
             // VISIT(c, expr, e->v.Subscript.slice)
             self.compile_expression(slice)?;
-            // Restore full subscript expression range before emitting
-            self.set_source_range(subscript_range);
-
-            // Emit appropriate instruction based on context
-            match ctx {
-                ast::ExprContext::Load => emit!(
-                    self,
-                    Instruction::BinaryOp {
-                        op: BinaryOperator::Subscr
-                    }
-                ),
-                ast::ExprContext::Store => emit!(self, Instruction::StoreSubscr),
-                ast::ExprContext::Del => emit!(self, Instruction::DeleteSubscr),
-                ast::ExprContext::Invalid => {
-                    return Err(self.error(CodegenErrorType::SyntaxError(
-                        "Invalid expression context".to_owned(),
-                    )));
-                }
-            }
         }
+
+        // Restore full subscript expression range before emitting
+        self.set_source_range(subscript_range);
+
+        // Emit appropriate instruction based on context
+        match (use_slice_opt, ctx) {
+            (true, ast::ExprContext::Load) => emit!(self, Instruction::BinarySlice),
+            (true, ast::ExprContext::Store) => emit!(self, Instruction::StoreSlice),
+            (true, _) => unreachable!(),
+            (false, ast::ExprContext::Load) => emit!(
+                self,
+                Instruction::BinaryOp {
+                    op: BinaryOperator::Subscr
+                }
+            ),
+            (false, ast::ExprContext::Store) => emit!(self, Instruction::StoreSubscr),
+            (false, ast::ExprContext::Del) => emit!(self, Instruction::DeleteSubscr),
+            (false, ast::ExprContext::Invalid) => {
+                return Err(self.error(CodegenErrorType::SyntaxError(
+                    "Invalid expression context".to_owned(),
+                )));
+            }
+        }

As per coding guidelines, "Avoid duplicate code in branches: extract differing values first, then call common logic once instead of duplicating logic in if/else branches."

crates/compiler/src/lib.rs (2)

147-233: find_unclosed_bracket does not account for raw strings.

In raw strings (r"...", r'...'), backslash is not an escape character. The function treats \ as an escape inside all strings (lines 172–173), so r"path\to\" would be misinterpreted — the function would think the string continues past the closing quote. In practice, for EOF errors caused by unclosed brackets, this edge case is unlikely to trigger (the unclosed raw string itself would be the error), and the fallback path handles the None return gracefully. Still worth noting as a known limitation.

Also, closing brackets (line 223) pop without verifying the bracket type matches. This is acceptable here since mismatched brackets would produce a different parser error before EOF, but a comment documenting this assumption would help future maintainers.

---

156-156: Minor: avoid collecting all chars into a Vec.

source.chars().collect::<Vec<_>>() allocates the entire character sequence upfront. Since this is only called during error handling (cold path), it's not performance-critical, but you could use source.char_indices() to iterate lazily and get byte offsets for free, which would also eliminate the manual byte_offset tracking.

Sketch using char_indices

-    let chars: Vec<char> = source.chars().collect();
-    let mut i = 0;
-    let mut byte_offset = 0;
-
-    while i < chars.len() {
-        let ch = chars[i];
-        let ch_len = ch.len_utf8();
+    let chars: Vec<(usize, char)> = source.char_indices().collect();
+    let mut i = 0;
+
+    while i < chars.len() {
+        let (byte_offset, ch) = chars[i];

This still needs the Vec for lookahead (chars[i+1], chars[i+2]), but byte offsets come from the iterator rather than manual tracking, reducing a class of bugs.

crates/compiler/src/lib.rs (1)

220-224: Closing brackets pop without verifying they match the opening bracket.

For code like ([), the ) pops the [ from the stack, potentially hiding the real unclosed bracket. Since this is a heuristic for error messages on already-broken code, this may be acceptable, but matching brackets would improve accuracy.

Optional improvement

             '(' | '[' | '{' => stack.push((ch, byte_offset)),
             ')' | ']' | '}' => {
-                stack.pop();
+                let expected = match ch {
+                    ')' => '(',
+                    ']' => '[',
+                    '}' => '{',
+                    _ => unreachable!(),
+                };
+                if stack.last().is_some_and(|(open, _)| *open == expected) {
+                    stack.pop();
+                }
             }

crates/vm/src/builtins/traceback.rs (1)

65-74: __dir__ hardcodes attributes and omits inherited ones.

CPython's traceback.__dir__() delegates to object.__dir__() which also returns inherited attributes (e.g., __class__, __doc__, __repr__, etc.). This hardcoded list returns only the four traceback-specific attributes. If that's intentional to match a specific CPython behavior, it's fine, but it diverges from the default object.__dir__ pattern shown in builtins/object.rs (which calls obj.dir(vm) to collect all attributes).

Also a minor nit: vec![...].into_iter().map(...) can be simplified.

♻️ Optional simplification

-        let attrs = vec!["tb_frame", "tb_next", "tb_lasti", "tb_lineno"];
-        PyList::from(
-            attrs
-                .into_iter()
-                .map(|s| vm.ctx.new_str(s).into())
-                .collect::<Vec<_>>(),
-        )
+        PyList::from(
+            ["tb_frame", "tb_next", "tb_lasti", "tb_lineno"]
+                .iter()
+                .map(|&s| vm.ctx.new_str(s).into())
+                .collect::<Vec<_>>(),
+        )

crates/vm/src/vm/vm_new.rs (1)

377-443: Consider extracting the repeated allow_incomplete → error type pattern.

The pattern if allow_incomplete { incomplete_input_error } else { syntax_error } appears at least 4 times in this match block (lines 378–382, 390–394, 406–410, 435–442). Extracting a small helper or a local closure would reduce duplication. As per coding guidelines, "Avoid duplicate code in branches: extract differing values first, then call common logic once instead of duplicating logic in if/else branches."

♻️ Example helper

let incomplete_or_syntax = |allow: bool| -> &'static Py<PyType> {
    if allow {
        self.ctx.exceptions.incomplete_input_error
    } else {
        self.ctx.exceptions.syntax_error
    }
};

Then each arm becomes simply incomplete_or_syntax(allow_incomplete).

crates/vm/src/frame.rs (1)

2243-2243: Silently discarding set_attr error for name_from.

let _ = err.as_object().set_attr(...) suppresses any error. If this is intentional (best-effort metadata), a brief comment would clarify intent. If set_attr on a freshly-created exception object can realistically fail, the error should be logged or propagated.

crates/compiler/src/lib.rs (2)

220-224: Closing brackets pop without matching — can misattribute the unclosed bracket.

stack.pop() on a closing bracket doesn't verify it matches the top of the stack. For mismatched input like ([), the ) pops [ instead of (, and the result points to ( — which is an acceptable "best effort" in that case. However, for ([) followed by more code, the stack is left in an inconsistent state that may return None or the wrong bracket.

Since this is only called for EOF errors (where true mismatches are rare), this is a minor accuracy gap. If you want to harden it later, skip the pop when brackets don't match.

Optional: match bracket types before popping

             ')' | ']' | '}' => {
-                stack.pop();
+                let expected = match ch {
+                    ')' => '(',
+                    ']' => '[',
+                    '}' => '{',
+                    _ => unreachable!(),
+                };
+                if stack.last().is_some_and(|&(top, _)| top == expected) {
+                    stack.pop();
+                }
             }

---

149-158: Consider using char_indices() to avoid the upfront Vec<char> allocation.

source.chars().collect::<Vec<char>>() duplicates the entire source as char values. Using source.char_indices() provides (byte_offset, char) pairs without the allocation, and you can peek ahead with source[byte_offset..] slicing when checking triple quotes.

This is error-path-only code so not performance-critical, but it would be a cleaner idiomatic Rust pattern.

crates/vm/src/suggestion.rs (2)

51-58: Consider using .ok()? instead of .unwrap() for resilience.

Lines 55–56 use .unwrap() to retrieve name and obj from the exception. Now that the guard uses fast_issubclass (matching subclasses too), a misbehaving subclass could theoretically lack these attributes, causing a panic. The new ImportError branch on lines 77–78 already uses the safer .ok()? pattern. Aligning these branches would make the function uniformly panic-free.

Suggested diff

-        let name = exc.as_object().get_attr("name", vm).unwrap();
-        let obj = exc.as_object().get_attr("obj", vm).unwrap();
+        let name = exc.as_object().get_attr("name", vm).ok()?;
+        let obj = exc.as_object().get_attr("obj", vm).ok()?;

---

59-60: Same .unwrap() concern applies here.

Line 60 has the same .unwrap() risk as the AttributeError branch above — a NameError subclass could theoretically lack the name attribute.

Suggested diff

-        let name = exc.as_object().get_attr("name", vm).unwrap();
+        let name = exc.as_object().get_attr("name", vm).ok()?;

crates/vm/src/vm/vm_new.rs (1)

351-368: TabError detection now correctly uses per-line checks — addresses the over-broad detection concern.

The logic properly resets per line via the .any() closure and only examines leading whitespace bytes. One minor allocation nit: the Vec<u8> collection could be avoided with a direct scan:

Optional: avoid allocation

                 let is_tab_error = source.is_some_and(|source| {
                     source.lines().any(|line| {
-                        let indent: Vec<u8> = line
-                            .bytes()
-                            .take_while(|&b| b == b' ' || b == b'\t')
-                            .collect();
-                        indent.contains(&b' ') && indent.contains(&b'\t')
+                        let mut seen_tab = false;
+                        let mut seen_space = false;
+                        for b in line.bytes().take_while(|&b| b == b' ' || b == b'\t') {
+                            seen_tab |= b == b'\t';
+                            seen_space |= b == b' ';
+                        }
+                        seen_tab && seen_space
                     })
                 });

crates/vm/src/frame.rs (1)

423-497: Run a clean build after instruction-semantics changes.

These edits adjust instruction handling in the eval loop. Please run the clean-build cleanup command to avoid stale bytecode artifacts.

Clean build commands

rm -r target/debug/build/rustpython-*
find . | grep -E "\.pyc$" | xargs rm -r

As per coding guidelines, "When modifying bytecode instructions, perform a clean build by running: rm -r target/debug/build/rustpython-* && find . | grep -E ".pyc$" | xargs rm -r".

crates/compiler/src/lib.rs (2)

204-228: String-prefix lookback is a nice improvement — one readability nit.

The prefix detection correctly handles r, rb, br, fr, etc. by looking back up to 2 characters.

1..=2.min(i) relies on method-call precedence binding tighter than ..=. It works correctly, but 1..=i.min(2) reads more naturally as "up to min(i, 2)" without requiring the reader to parse precedence rules.

Readability tweak

-            for lookback in 1..=2.min(i) {
+            for lookback in 1..=i.min(2) {

---

337-476: Consider adding unit tests for the new error-handling paths.

The existing tests only cover successful compilation. The new logic — EOF→unclosed-bracket rewriting, IndentationError end-location adjustment, and the find_unclosed_bracket heuristic — would benefit from targeted tests, e.g.:

• find_unclosed_bracket("foo(bar[baz") → Some(('[', …))
• find_unclosed_bracket("r\"[\\\"") → None (bracket inside raw string)
• find_unclosed_bracket("complete()") → None
• A compile(…) call with an unclosed bracket verifying the error message contains was never closed.

crates/vm/src/vm/vm_new.rs (1)

354-376: Avoid per-line Vec<u8> allocation in the tab-detection closure.

The collect::<Vec<u8>>() on each line's indent prefix is unnecessary — you can track the two booleans inline without materialising the bytes.

♻️ Suggested diff

                 let is_tab_error = source.is_some_and(|source| {
                     let mut has_space_indent = false;
                     let mut has_tab_indent = false;
                     for line in source.lines() {
-                        let indent: Vec<u8> = line
-                            .bytes()
-                            .take_while(|&b| b == b' ' || b == b'\t')
-                            .collect();
-                        if indent.is_empty() {
-                            continue;
-                        }
-                        if indent.contains(&b' ') && indent.contains(&b'\t') {
+                        let mut line_has_space = false;
+                        let mut line_has_tab = false;
+                        for b in line.bytes() {
+                            match b {
+                                b' ' => line_has_space = true,
+                                b'\t' => line_has_tab = true,
+                                _ => break,
+                            }
+                        }
+                        if line_has_space && line_has_tab {
                             return true;
                         }
-                        if indent.contains(&b' ') {
+                        if line_has_space {
                             has_space_indent = true;
                         }
-                        if indent.contains(&b'\t') {
+                        if line_has_tab {
                             has_tab_indent = true;
                         }
                     }
                     has_space_indent && has_tab_indent
                 });

crates/vm/src/vm/vm_new.rs (1)

354-376: Avoid per-line Vec allocation in the is_tab_error check.

The Vec<u8> collection on each line is only used to call .contains(). You can achieve the same with direct iteration, avoiding the heap allocation.

♻️ Suggested refactor

                 let is_tab_error = source.is_some_and(|source| {
                     let mut has_space_indent = false;
                     let mut has_tab_indent = false;
                     for line in source.lines() {
-                        let indent: Vec<u8> = line
-                            .bytes()
-                            .take_while(|&b| b == b' ' || b == b'\t')
-                            .collect();
-                        if indent.is_empty() {
-                            continue;
-                        }
-                        if indent.contains(&b' ') && indent.contains(&b'\t') {
+                        let mut line_has_space = false;
+                        let mut line_has_tab = false;
+                        for b in line.bytes().take_while(|&b| b == b' ' || b == b'\t') {
+                            line_has_space |= b == b' ';
+                            line_has_tab |= b == b'\t';
+                        }
+                        if !line_has_space && !line_has_tab {
+                            continue;
+                        }
+                        if line_has_space && line_has_tab {
                             return true;
                         }
-                        if indent.contains(&b' ') {
+                        if line_has_space {
                             has_space_indent = true;
                         }
-                        if indent.contains(&b'\t') {
+                        if line_has_tab {
                             has_tab_indent = true;
                         }
                     }
                     has_space_indent && has_tab_indent
                 });

crates/compiler/src/lib.rs (3)

203-228: String prefix lookback can match trailing letters of identifiers.

The lookback loop (lines 207-216) scans up to 2 characters before the quote for r/b/f/u prefixes, but it doesn't verify that those characters aren't part of a larger identifier. For example, in myvar"text", the lookback sees r from myvar and incorrectly sets is_raw_string = true, causing backslash escapes inside the "string" to be ignored.

This is a best-effort heuristic on an error path, so impact is limited to occasionally mis-reporting bracket positions when identifiers ending in r immediately precede a syntax-error string. Worth noting for future improvement.

Sketch: check that the prefix chars are not preceded by an alphanumeric/underscore

         // Check up to 2 characters before the quote for string prefix
         is_raw_string = false;
-        for look_back in 1..=2.min(i) {
+        let prefix_start = i.saturating_sub(2.min(i));
+        // Only treat as prefix if not part of a larger identifier
+        let is_prefix_boundary = prefix_start == 0
+            || !chars[prefix_start - 1].1.is_alphanumeric()
+               && chars[prefix_start - 1].1 != '_';
+        if is_prefix_boundary {
+          for look_back in 1..=2.min(i) {
             let prev = chars[i - look_back].1;
             if matches!(prev, 'r' | 'R') {
                 is_raw_string = true;
                 break;
             }
             if !matches!(prev, 'b' | 'B' | 'f' | 'F' | 'u' | 'U') {
                 break;
             }
+          }
         }

---

230-247: Mismatched closing brackets are silently ignored — consider popping anyway.

When a closing bracket doesn't match the top of the stack (line 239), it's silently skipped. In source like [(]), the ) doesn't match [ and is ignored, leaving both [ and ( on the stack. The function then returns ( (the last entry), but [ is the actual unclosed bracket from the user's perspective.

This is a known trade-off in simple bracket-matching heuristics and the current behavior is reasonable for error reporting. Just noting it for awareness.

---

154-162: Collecting all char_indices into a Vec allocates on every call.

source.char_indices().collect() materializes the entire source into a vector. This runs only on the error path so it's not performance-critical, but for very large files it's unnecessary memory. The lookback for prefix detection (up to 2 chars) and lookahead for triple quotes (up to 2 chars) could be handled with a small sliding window or by storing just the last few indices, avoiding the full allocation.

Low priority given it's error-path only.