extra_tests/snippets/code_co_consts.py (1)

93-96: Consider adding self parameter or @staticmethod decorator to class methods.

The methods are defined without self, which works for this test since you're accessing cls_with_doc.method directly from the class to inspect __code__. However, this is unconventional and could confuse readers. Consider either:

• Adding self to make them proper instance methods, or
• Adding @staticmethod decorator to make the intent explicit

♻️ Suggested clarification

 class cls_with_doc:
+    `@staticmethod`
     def method():
         """Method docstring"""
         return 1

 class cls_no_doc:
+    `@staticmethod`
     def method():
         return 1

Also applies to: 104-106

crates/vm/src/stdlib/thread.rs (2)

901-938: Consider retaining entries when try_lock fails on inner state.

When inner.try_lock() fails at line 912-914, the entry is removed from the registry (return false). However, this thread handle still exists and may be used later. If someone calls join() on it, the thread won't be in the registry but also won't be properly marked as Done, potentially causing unexpected behavior.

Consider retaining entries when the lock cannot be acquired:

♻️ Suggested change

                 // Try to lock the inner state - skip if we can't
                 let Some(mut inner_guard) = inner.try_lock() else {
-                    return false;
+                    return true; // Keep entry - we couldn't safely process it
                 };

Alternatively, if removal is intentional, add a comment explaining that orphaned handles after fork are acceptable because the child process will create new handles for any threads it spawns.

---

309-321: Acknowledged: UTF-8 truncation concern.

The existing TODO comment at lines 311-312 correctly identifies that truncating at byte 15 may split a multi-byte UTF-8 character. This is a pre-existing concern and not introduced by this PR, but worth tracking.

Would you like me to open an issue to track this UTF-8 boundary handling improvement, or generate a fix that properly handles multi-byte character boundaries?

extra_tests/snippets/stdlib_typing.py (1)

21-21: Consider moving import to top of file.

Per PEP 8, imports should be at the top of the file. While the placement here groups the import with its related tests for clarity, moving it alongside the other typing import on line 2 would be more conventional.

Suggested refactor

At the top of the file, consolidate imports:

 from collections.abc import Awaitable, Callable
-from typing import TypeVar
+from typing import Optional, TypeVar, Union

Then remove line 21.

scripts/fix_test.py (1)

192-201: Redundant condition check.

The condition on line 195 (if test.result == "fail" or test.result == "error") is always true since parse_results() already filters to only include tests with these results (line 91-92). The check is harmless but could be simplified.

♻️ Optional cleanup

     # Collect failing tests (with deduplication for subtests)
     seen_tests = set()  # Track (class_name, method_name) to avoid duplicates
     for test in tests.tests:
-        if test.result == "fail" or test.result == "error":
-            test_parts = path_to_test(test.path)
-            if len(test_parts) == 2:
-                test_key = tuple(test_parts)
-                if test_key not in seen_tests:
-                    seen_tests.add(test_key)
-                    print(f"Marking test: {test_parts[0]}.{test_parts[1]}")
+        test_parts = path_to_test(test.path)
+        if len(test_parts) == 2:
+            test_key = tuple(test_parts)
+            if test_key not in seen_tests:
+                seen_tests.add(test_key)
+                print(f"Marking test: {test_parts[0]}.{test_parts[1]}")

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

290-294: Consider enabling or tracking this Python 3.14 feature.

This commented-out code for negative-to-unsigned conversion errors is part of the 3.14 upgrade. If this behavior is required for Python 3.14 compliance, consider enabling it. Otherwise, convert to a proper TODO with an issue reference to track implementation.

♻️ If intended for 3.14, enable the feature

-        // Python 3.14+: ValueError for negative int to unsigned type
-        // if I::min_value() == I::zero() && self.as_bigint().sign() == Sign::Minus {
-        //     return Err(vm.new_value_error("Cannot convert negative int".to_owned()));
-        // }
+        // Python 3.14+: ValueError for negative int to unsigned type
+        if I::min_value() == I::zero() && self.as_bigint().sign() == Sign::Minus {
+            return Err(vm.new_value_error("cannot convert negative int to unsigned".to_owned()));
+        }

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

1556-1583: Minor inconsistency with other initializers.

The PyImportError initializer passes the original args (with kwargs intact) to PyBaseException::slot_init, while PyAttributeError and PyNameError explicitly create a FuncArgs with empty kwargs. Both work correctly since PyBaseException::init only uses args.args, but this is inconsistent.

Optional: Align with other initializers for consistency

             dict.set_item("name_from", vm.unwrap_or_none(name_from), vm)?;
-            PyBaseException::slot_init(zelf, args, vm)
+            let base_args = FuncArgs::new(args.args.clone(), KwArgs::default());
+            PyBaseException::slot_init(zelf, base_args, vm)

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

297-303: Iterator __reduce__ does not preserve iteration state.

The reducer only stores the template reference, not the current index and from_strings state. Unpickling will restart iteration from the beginning. This may be intentional (many Python iterators behave this way), but it differs from iterators that preserve position.

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: 2026-01-14T14:52:10.778Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-14T14:52:10.779Z
Learning: When comparing behavior with CPython, use `python` command to explicitly run CPython and `cargo run -- script.py` to run RustPython

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-14T14:52:10.779Z
Learning: In most cases, Python code should not be edited; bug fixes should be made through Rust code modifications only

Learnt from: youknowone
Repo: RustPython/RustPython PR: 6358
File: crates/vm/src/exception_group.rs:173-185
Timestamp: 2025-12-09T08:46:58.660Z
Learning: In crates/vm/src/exception_group.rs, the derive() method intentionally always creates a BaseExceptionGroup instance rather than preserving the original exception class type. This is a deliberate design decision that differs from CPython's behavior.

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-14T14:52:10.778Z
Learning: Applies to Lib/**/*.py : Minimize modifications to CPython standard library files in the Lib/ directory; these files should be edited very conservatively and modifications should be minimal and only to work around RustPython limitations

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-14T14:52:10.778Z
Learning: Applies to Lib/**/*.py : Add a `# TODO: RUSTPYTHON` comment when modifications are made to Lib/ directory files

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-14T14:52:10.779Z
Learning: Do not edit the Lib/ directory directly except for copying files from CPython to work around RustPython limitations

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-14T14:52:10.779Z
Learning: Use `cargo run -- script.py` instead of `python script.py` when testing Python code with RustPython

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-14T14:52:10.779Z
Learning: Run a full clean build when modifying bytecode instructions using: `rm -r target/debug/build/rustpython-* && find . | grep -E "\.pyc$" | xargs rm -r`

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-14T14:52:10.779Z
Learning: Run `./whats_left.py` to get a list of unimplemented methods when looking for contribution opportunities

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-14T14:52:10.778Z
Learning: Applies to Lib/test/**/*.py : Use `unittest.skip("TODO: RustPython <reason>")` or `unittest.expectedFailure` with `# TODO: RUSTPYTHON <reason>` comment when marking tests in Lib/ that cannot run

Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using `#ifdef` checks rather than providing fallback values for other platforms.

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-14T14:52:10.778Z
Learning: Applies to **/test*.py : Only remove `unittest.expectedFailure` decorators and upper TODO comments from tests when tests actually pass, or add these decorators when tests cannot be fixed

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

crates/compiler-core/src/marshal.rs (1)

205-205: Bump marshal FORMAT_VERSION for 32‑bit CodeFlags.
CodeFlags is now serialized as u32, but the format version is still 4, so older cached data will deserialize incorrectly. Please increment the format version to invalidate stale marshaled objects.

✅ Suggested change

-pub const FORMAT_VERSION: u32 = 4;
+pub const FORMAT_VERSION: u32 = 5;

crates/jit/tests/common.rs (1)

91-108: Forward-ref string annotations are silently dropped.

The _val_is_const flag is ignored at line 94, so when annotations use forward-reference strings like "int" (which compile to LOAD_CONST), the value lookup at line 102 will fail since it only checks code.names, not code.constants. This causes forward-ref annotations to be silently omitted.

Suggested fix

-                        let (_val_is_const, val_idx) = chunk[1];
+                        let (val_is_const, val_idx) = chunk[1];

                         // Key should be a const string (parameter name)
                         if key_is_const
                             && let ConstantData::Str { value } = &code.constants[key_idx]
                         {
                             let param_name = value.to_string_lossy().into_owned();
-                            // Value should be a name (type name)
-                            if let Some(type_name) = code.names.get(val_idx) {
-                                annotations
-                                    .insert(param_name, StackValue::String(type_name.clone()));
-                            }
+                            // Value can be a name (type ref) or a const string (forward ref)
+                            let type_name = if val_is_const {
+                                match &code.constants.get(val_idx) {
+                                    Some(ConstantData::Str { value }) => {
+                                        Some(value.to_string_lossy().into_owned())
+                                    }
+                                    _ => None,
+                                }
+                            } else {
+                                code.names.get(val_idx).cloned()
+                            };
+                            if let Some(type_name) = type_name {
+                                annotations.insert(param_name, StackValue::String(type_name));
+                            }
                         }

extra_tests/snippets/syntax_assignment.py (1)

62-73: Silence F821 lint warnings on __annotate__ usage.

Static analysis flags __annotate__ as undefined on lines 70-71, but this is a runtime-injected name per PEP 649. Consider adding # noqa: F821 comments to keep lint clean if these snippets are linted in CI.

Suggested lint-safe tweak

-assert callable(__annotate__)
-annotations = __annotate__(1)  # 1 = FORMAT_VALUE
+assert callable(__annotate__)  # noqa: F821
+annotations = __annotate__(1)  # noqa: F821  # 1 = FORMAT_VALUE

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

958-988: Verify __annotations__ assignment behavior against CPython 3.14.

Per PEP 649/749, assigning to cls.__annotations__ in CPython 3.14 stores directly to __annotations__ in the class dict and sets __annotate__ to None. The current implementation conditionally updates __annotations__ vs __annotations_cache__ based on whether __annotations__ already exists, which may diverge from CPython's behavior where assignment always writes to __annotations__.

CPython 3.14 PEP 649 class __annotations__ assignment behavior

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

5928-5966: Always record executed simple annotations when conditional tracking is enabled.
Right now indices are only recorded for module scope or when in_conditional_block is true, which drops unconditional class annotations when has_conditional_annotations is set. Record every executed simple annotation to keep indices aligned with compile_module_annotate.

🐛 Proposed fix

-                    let is_module = self.current_symbol_table().typ == CompilerScope::Module;
-                    let is_conditional = is_module || self.in_conditional_block;
-
-                    if is_conditional {
-                        // Get the current annotation index and increment
-                        let annotation_index = self.next_conditional_annotation_index;
-                        self.next_conditional_annotation_index += 1;
-
-                        // Add index to __conditional_annotations__ set
-                        let cond_annotations_name = self.name("__conditional_annotations__");
-                        emit!(self, Instruction::LoadName(cond_annotations_name));
-                        self.emit_load_const(ConstantData::Integer {
-                            value: annotation_index.into(),
-                        });
-                        emit!(self, Instruction::SetAdd { i: 0_u32 });
-                        emit!(self, Instruction::PopTop);
-                    }
+                    // Record every executed simple annotation when conditional handling is enabled
+                    let annotation_index = self.next_conditional_annotation_index;
+                    self.next_conditional_annotation_index += 1;
+
+                    let cond_annotations_name = self.name("__conditional_annotations__");
+                    emit!(self, Instruction::LoadName(cond_annotations_name));
+                    self.emit_load_const(ConstantData::Integer {
+                        value: annotation_index.into(),
+                    });
+                    emit!(self, Instruction::SetAdd { i: 0_u32 });
+                    emit!(self, Instruction::PopTop);

scripts/auto_mark_test.py (1)

67-74: Consider using __init__ instead of mutable class attributes.

tests and unexpected_successes are mutable class attributes shared by all instances. While parse_results() mitigates this by reassigning new lists, this pattern can cause subtle bugs if usage changes.

♻️ Suggested refactor

 class TestResult:
-    tests_result: str = ""
-    tests = []
-    unexpected_successes = []  # Tests that passed but were marked as expectedFailure
-    stdout = ""
+    def __init__(self):
+        self.tests_result: str = ""
+        self.tests: list[Test] = []
+        self.unexpected_successes: list[Test] = []  # Tests that passed but were marked as expectedFailure
+        self.stdout: str = ""

extra_tests/snippets/code_co_consts.py (1)

24-25: Consider defining CO_HAS_DOCSTRING as a named constant.

The magic number 0x4000000 is used repeatedly throughout the file. Defining it as a constant at the top would improve readability and maintainability.

Suggested improvement

+# CO_HAS_DOCSTRING flag added in Python 3.14
+CO_HAS_DOCSTRING = 0x4000000
+
 # Test function with docstring - docstring should be co_consts[0]
 def with_doc():
     """This is a docstring"""
     return 1

 assert with_doc.__code__.co_consts[0] == "This is a docstring", (
     with_doc.__code__.co_consts
 )
 assert with_doc.__doc__ == "This is a docstring"
-# Check CO_HAS_DOCSTRING flag (0x4000000)
-assert with_doc.__code__.co_flags & 0x4000000, hex(with_doc.__code__.co_flags)
+# Check CO_HAS_DOCSTRING flag
+assert with_doc.__code__.co_flags & CO_HAS_DOCSTRING, hex(with_doc.__code__.co_flags)

crates/codegen/src/symboltable.rs (3)

383-412: Consider avoiding clone of class symbols for annotation scope.

The symbols.clone() at line 389 may be expensive for large class scopes. This clone is needed to avoid borrow conflicts, but consider whether passing a reference with appropriate lifetime management could work instead.

However, given the complexity of the borrow checker constraints with the with_append pattern and the fact that annotation blocks are relatively rare, this is acceptable for correctness.

---

932-956: Consider consolidating the helper methods.

Both add_classdict_freevar and add_conditional_annotations_freevar follow identical patterns with only the name differing. Consider a single parameterized helper.

♻️ Optional refactor to reduce duplication

-    fn add_classdict_freevar(&mut self) {
-        let table = self.tables.last_mut().unwrap();
-        let name = "__classdict__";
-        let symbol = table
-            .symbols
-            .entry(name.to_owned())
-            .or_insert_with(|| Symbol::new(name));
-        symbol.scope = SymbolScope::Free;
-        symbol
-            .flags
-            .insert(SymbolFlags::REFERENCED | SymbolFlags::FREE_CLASS);
-    }
-
-    fn add_conditional_annotations_freevar(&mut self) {
-        let table = self.tables.last_mut().unwrap();
-        let name = "__conditional_annotations__";
-        let symbol = table
-            .symbols
-            .entry(name.to_owned())
-            .or_insert_with(|| Symbol::new(name));
-        symbol.scope = SymbolScope::Free;
-        symbol
-            .flags
-            .insert(SymbolFlags::REFERENCED | SymbolFlags::FREE_CLASS);
-    }
+    fn add_class_freevar(&mut self, name: &str) {
+        let table = self.tables.last_mut().unwrap();
+        let symbol = table
+            .symbols
+            .entry(name.to_owned())
+            .or_insert_with(|| Symbol::new(name));
+        symbol.scope = SymbolScope::Free;
+        symbol
+            .flags
+            .insert(SymbolFlags::REFERENCED | SymbolFlags::FREE_CLASS);
+    }

Then call as self.add_class_freevar("__classdict__") and self.add_class_freevar("__conditional_annotations__").

---

1048-1055: Clarify the rationale for module-only re-scan.

The comment explains that class scope should not re-scan to preserve class-local symbol resolution, but the rationale for why module scope needs re-scanning could be clearer. Consider adding a comment explaining that module-level annotations may reference global builtins (like str, int) that need to be registered in the module scope's symbol table for proper code generation.