◐ Shell
reader mode source ↗
Skip to content

Fix annotation scope, deadlock, MRO, and HEAPTYPE issues#7087

Merged
youknowone merged 2 commits into
RustPython:mainfrom
youknowone:typing
Feb 11, 2026
Merged

Fix annotation scope, deadlock, MRO, and HEAPTYPE issues#7087
youknowone merged 2 commits into
RustPython:mainfrom
youknowone:typing

Conversation

@youknowone

@youknowone youknowone commented Feb 11, 2026

Copy link
Copy Markdown
Member

Summary by CodeRabbit

  • Bug Fixes
    • Clearer, safer errors when setting name and qualname on types; avoids crashes on immutable/non-heap types.
    • More descriptive MRO failure messages.
    • Enforces runtime errors for async comprehensions used outside async contexts.
    • Correct representation of single-element tuple types in typing output: now displayed as "(x,)".
  • Refactor
    • Reworked annotation-scope handling to reliably preserve/restore context and enforce non-async behavior.
    • Streamlined TypeVar default evaluation and made Generic a heap type to align runtime behavior.

Annotation scope:
- Remove module-level annotation re-scan that created phantom
  sub_tables, breaking annotation closure for comprehensions
- Add async comprehension check in symbol table with
  is_in_async_context(); annotation/type-params scopes are
  always non-async
- Save/restore CompileContext in enter/exit_annotation_scope
  to reset in_async_scope

Deadlock prevention:
- Fix TypeVar/ParamSpec/TypeVarTuple __default__ and
  evaluate_default by cloning lock contents before acquiring
  a second lock or calling Python

Other fixes:
- Add HEAPTYPE flag to Generic for correct pickle behavior
- Guard heaptype_ext access in name_inner/set___name__/
  set___qualname__ with safe checks instead of unwrap
- Fix MRO error message to include base class names
- Add "format" to varnames in TypeAlias annotation scopes
- Fix single-element tuple repr to include trailing comma
@coderabbitai

coderabbitai Bot commented Feb 11, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

This PR refactors annotation scope management in the compiler to explicitly carry and restore CompileContext, strengthens type immutability error handling, improves TypeVar locking semantics, and adjusts tuple type representation formatting. Changes span compilation, type resolution, and stdlib implementations.

Changes

Cohort / File(s) Summary
Annotation Scope Management
crates/codegen/src/compile.rs, crates/codegen/src/symboltable.rs
Refactors enter/exit annotation scope signatures to explicitly pass CompileContext for restoration; enter_annotation_scope now returns Option<CompileContext> instead of bool. Adds is_in_async_context helper and enforces async comprehension validation with location-aware error reporting. Extends annotation handling with format parameter insertion.
Type Resolution Hardening
crates/vm/src/builtins/type.rs
Refactors name resolution to prefer heaptype_ext path when available; centralizes immutability checks for special attributes via dedicated check_set_special_type_attr path; replaces unwraps with typed error returns for static types; updates MRO error messaging to include involved bases.
TypeVar Locking and Flags
crates/vm/src/stdlib/typevar.rs
Improves locking discipline in default/evaluate_default logic by using scoped blocks and cloning to avoid holding locks unnecessarily; adds HEAPTYPE flag to generic TypeVar pyclass declaration, altering runtime type behavior and inheritance semantics.
Type Representation
crates/vm/src/stdlib/typing.rs
Adjusts single-element tuple formatting in type representations to include trailing comma (e.g., (element,) instead of (element)).

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • Implement more typing #7078: Modifies annotation/type-parameter handling with format parameter insertion and scope adjustments in symboltable.rs and compile.rs.
  • typevar as module #6834: Modifies TypeVar/typing implementation including default/evaluate_default behavior and pyclass handling in typevar.rs.
  • heaptype __qualname__ #5848: Modifies heap type qualname handling and heaptype_ext behavior in vm/src/builtins/type.rs.

Suggested reviewers

  • ShaharNaveh
  • arihant2math

Poem

🐰 Context dances in and out,
Annotations scope without a doubt,
Types now truthful, types now kind,
Locking logic finely aligned,
hop hop — the tuple comma springs! ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly references the four main bug areas addressed across the changeset: annotation scope management, deadlock prevention (locking refactoring), MRO error messaging, and HEAPTYPE flag issues.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

Copy link
Copy Markdown
Contributor

📦 Library Dependencies

The following Lib/ modules were modified. Here are their dependencies:

[ ] lib: cpython/Lib/ctypes
[ ] test: cpython/Lib/test/test_ctypes (TODO: 14)
[ ] test: cpython/Lib/test/test_stable_abi_ctypes.py

dependencies:

  • ctypes (native: _ctypes, ctypes._endian, ctypes.macholib.dylib, ctypes.macholib.framework, itertools, sys)
    • shutil (native: errno, sys)
    • subprocess (native: builtins, errno, sys, time)
    • sysconfig (native: os.path, sys)
    • types
    • os, re, struct, warnings

dependent tests: (1 tests)

  • ctypes: test_ctypes

[x] lib: cpython/Lib/dataclasses.py
[x] test: cpython/Lib/test/test_dataclasses (TODO: 10)

dependencies:

  • dataclasses

dependent tests: (6 tests)

  • dataclasses: test__colorize test_genericalias test_patma test_pprint test_regrtest test_zoneinfo

[x] lib: cpython/Lib/typing.py
[ ] test: cpython/Lib/test/test_typing.py (TODO: 7)
[x] test: cpython/Lib/test/test_type_aliases.py
[x] test: cpython/Lib/test/test_type_annotations.py (TODO: 1)
[x] test: cpython/Lib/test/test_type_params.py (TODO: 10)
[x] test: cpython/Lib/test/test_genericalias.py

dependencies:

  • typing

dependent tests: (12 tests)

  • typing: test_annotationlib test_builtin test_enum test_fractions test_functools test_genericalias test_grammar test_isinstance test_type_aliases test_type_params test_types test_typing

Legend:

  • [+] path exists in CPython
  • [x] up-to-date, [ ] outdated

@youknowone youknowone marked this pull request as ready for review February 11, 2026 15:10

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/vm/src/stdlib/typevar.rs (1)

95-122: ⚠️ Potential issue | 🟠 Major

__bound__ and __constraints__ still hold their locks while calling into Python — same deadlock risk you fixed in __default__.

Lines 101 and 116 call evaluate_constraints.call() / evaluate_bound.call() while the constraints / bound mutex guard is alive. If the evaluator callback triggers re-entrant access to __constraints__ or __bound__ on the same TypeVar, it will deadlock. Apply the same scoped-lock + clone-first pattern used in __default__.

Proposed fix
     #[pygetset]
     fn __constraints__(&self, vm: &VirtualMachine) -> PyResult {
-        let mut constraints = self.constraints.lock();
-        if !vm.is_none(&constraints) {
-            return Ok(constraints.clone());
+        {
+            let constraints = self.constraints.lock();
+            if !vm.is_none(&constraints) {
+                return Ok(constraints.clone());
+            }
         }
         let r = if !vm.is_none(&self.evaluate_constraints) {
-            *constraints = self.evaluate_constraints.call((1i32,), vm)?;
-            constraints.clone()
+            let result = self.evaluate_constraints.call((1i32,), vm)?;
+            *self.constraints.lock() = result.clone();
+            result
         } else {
             vm.ctx.empty_tuple.clone().into()
         };
         Ok(r)
     }

     #[pygetset]
     fn __bound__(&self, vm: &VirtualMachine) -> PyResult {
-        let mut bound = self.bound.lock();
-        if !vm.is_none(&bound) {
-            return Ok(bound.clone());
+        {
+            let bound = self.bound.lock();
+            if !vm.is_none(&bound) {
+                return Ok(bound.clone());
+            }
         }
         let r = if !vm.is_none(&self.evaluate_bound) {
-            *bound = self.evaluate_bound.call((1i32,), vm)?;
-            bound.clone()
+            let result = self.evaluate_bound.call((1i32,), vm)?;
+            *self.bound.lock() = result.clone();
+            result
         } else {
             vm.ctx.none()
         };
         Ok(r)
     }
🤖 Fix all issues with AI agents
In `@crates/codegen/src/compile.rs`:
- Around line 2494-2497: The code currently inserts "format" into
current_code_info().metadata.varnames and later assumes its local slot is 0;
update emit_format_validation() (and the similar spots at the other occurrences)
to resolve the local slot via get_local_var_index("format") instead of
hardcoding 0: use get_local_var_index("format") to obtain the parameter index
and emit loads/validation against that index, leaving the varnames insertion
as-is but making the load robust to symbol ordering changes.
🧹 Nitpick comments (2)
crates/vm/src/builtins/type.rs (1)

850-877: Inconsistent name formatting in heaptype_ext error vs. sibling setter.

Line 865 uses self.name() (which strips the module prefix via rsplit_once('.')), while the analogous guard in set___name__ at line 1162 uses self.slot_name() (which preserves the full dotted name). The check_set_special_type_attr helper at line 1138 also uses slot_name(). For static types where heaptype_ext is None, these yield different output (e.g. "typing.TypeAliasType" vs "TypeAliasType").

Consider using self.slot_name() here for consistency:

Suggested fix
         let heap_type = self.heaptype_ext.as_ref().ok_or_else(|| {
             vm.new_type_error(format!(
                 "cannot set '__qualname__' attribute of immutable type '{}'",
-                self.name()
+                self.slot_name()
             ))
         })?;
crates/vm/src/stdlib/typevar.rs (1)

182-192: Consider extracting the duplicated __default__ / evaluate_default logic into shared helpers.

The __default__ body (lines 141–155, 506–520, 704–718) and evaluate_default body (lines 183–192, 524–533, 722–731) are identical across TypeVar, ParamSpec, and TypeVarTuple. A free function taking (&PyMutex<PyObjectRef>, &PyMutex<PyObjectRef>, &VirtualMachine) would eliminate the triplication and ensure future lock-pattern fixes only need to be applied once.

As per coding guidelines, "When branches differ only in a value but share common logic, extract the differing value first, then call the common logic once to avoid duplicate code."

Also applies to: 523-533, 721-731

Hide details View details @youknowone youknowone merged commit 426019e into RustPython:main Feb 11, 2026
14 checks passed
@youknowone youknowone deleted the typing branch February 11, 2026 15:30
youknowone added a commit to youknowone/RustPython that referenced this pull request Mar 22, 2026
…7087)

* Fix annotation scope, deadlock, MRO, and HEAPTYPE issues

Annotation scope:
- Remove module-level annotation re-scan that created phantom
  sub_tables, breaking annotation closure for comprehensions
- Add async comprehension check in symbol table with
  is_in_async_context(); annotation/type-params scopes are
  always non-async
- Save/restore CompileContext in enter/exit_annotation_scope
  to reset in_async_scope

Deadlock prevention:
- Fix TypeVar/ParamSpec/TypeVarTuple __default__ and
  evaluate_default by cloning lock contents before acquiring
  a second lock or calling Python

Other fixes:
- Add HEAPTYPE flag to Generic for correct pickle behavior
- Guard heaptype_ext access in name_inner/set___name__/
  set___qualname__ with safe checks instead of unwrap
- Fix MRO error message to include base class names
- Add "format" to varnames in TypeAlias annotation scopes
- Fix single-element tuple repr to include trailing comma

* Unmark failing markers
@coderabbitai coderabbitai Bot mentioned this pull request Mar 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant