Repository navigation
Fix native ABC instance checks on Python 3.15 - #9067
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to No actionable issue remains from the supplied evidence; the PR is mergeable after normal checks. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 30.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 69 functions across 43 files. (10 skipped… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly identifies the native ABC instance-check change, which is the stated objective. The Python 3.15 reference is consistent with the compatibility update context.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 30.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 69 functions across 43 files. (10 skipped: 9 unsupported, 1 too large.)
✨ Finishing Touches
-
-
-
-
-
-
-
-
-
-
-
-
🧪 Generate unit tests (beta)
-
-
-
-
-
-
-
-
-
-
-
-
- Create a new PR
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Comment @coderabbitai help to get the list of available commands.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c12cc7360b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
|
|
||
| instance = NoClassAttribute() | ||
| assert not isinstance(instance, CustomInterface) |
There was a problem hiding this comment.
This assertion is part of a new test scenario added to extra_tests/snippets/stdlib_abc.py, but the repository's critical test-modification policy permits only adding expected-failure decorators or removing those decorators and their TODO comments after a fix. Since this change is neither permitted case, remove the test-file addition before landing the Rust fix.
AGENTS.md reference: AGENTS.md:L277-L279
Useful? React with 👍 / 👎.
Head branch was pushed to by a user without write access
f10a041 to
44eab61
Compare
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [ ] test: cpython/Lib/test/test_mmap.py (TODO: 19) dependencies: dependent tests: (2 tests)
Legend:
|
44eab61 to
098141b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 098141bbbf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for wrapper in (textio, uninitialized_textio, detached_textio): | ||
| with assert_raises(AttributeError) as error: | ||
| del wrapper._CHUNK_SIZE | ||
| assert str(error.exception) == ( | ||
| "attribute '_CHUNK_SIZE' of '_io.TextIOWrapper' objects cannot be deleted" |
There was a problem hiding this comment.
The repository's critical test-modification rule permits only adding expected-failure decorators or removing them with their TODO comments, whereas this block replaces the prior deletion expectation and adds new assertion logic and test data. Revert the extra_tests/snippets/stdlib_io.py edits and leave the behavioral correction in Rust.
AGENTS.md reference: AGENTS.md:L273-L279
Useful? React with 👍 / 👎.
| # Python 3.14.8 retains ParamSpec bounds without converting or validating them. | ||
| assert ParamSpec("Unbounded").__bound__ is None | ||
| for bound in (None, "int", "not valid Python!", (int, str), object()): | ||
| assert ParamSpec("BoundSpec", bound=bound).__bound__ is bound |
There was a problem hiding this comment.
This loop and its assertions add test logic and test data, but the repository's critical test-file policy allows only adding expected-failure decorators or removing those decorators and their TODO comments. Remove this test-file addition rather than landing it alongside the Rust fix.
AGENTS.md reference: AGENTS.md:L273-L279
Useful? React with 👍 / 👎.
| let subclass = vm | ||
| .get_attribute_opt(&instance, "__class__")? | ||
| .unwrap_or_else(|| instance.class().to_owned().into()); |
There was a problem hiding this comment.
When callers explicitly use _py_abc.ABCMeta (a backend the standard suite exercises at Lib/test/test_abc.py:687-690), this native-only fallback is bypassed because Lib/_py_abc.py:95 still evaluates instance.__class__ directly. Such objects therefore still raise AttributeError with the pure backend, including after virtual registration, leaving the advertised 3.14.8 ABC fix incomplete; apply the fallback to that backend too.
Useful? React with 👍 / 👎.
098141b to
695ebec
Compare
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
youknowdot
changed the title
695ebec to
74c9775
Compare
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
74c9775 to
eaec863
Compare
eaec863 to
ab355b8
Compare
Retain the native fallback and Python regression without modifying bundled Lib sources. Assisted-by: OpenAI Codex:model identifier unavailable
ab355b8 to
082afb3
Compare
Merging this PR will improve performance by 15.4%
Performance Changes
Tip Curious why performance improved? Comment Comparing 
Footnotes
|
2 participants
Current revision
Targets
mainafter #8950, with one logical commit:082afb3de1722a1ca279ecb8323ec7a7be003849(parent4e3b2f40395a4235841aaca811843d97ed68af0d). The unique patch is byte-identical to the prior reviewed head and now includes main's merged #9082 shared CI fix and #9081 REPL fix; original author attribution and assistance trailers are retained. No files underLib/are changed.Normal hooks, whitespace checks, and independent tree/history review passed. The temporary
[skip ci]marker is removed and normal CI is enabled for this final head. No new local build or runtime test is claimed. Validation recorded below belongs to earlier revisions unless explicitly stated otherwise.Rebased once after #9082 and #9081 merged to repair the shared Python 3.15 IO reference failure. Normal commit hooks and exact range-diff/patch-equivalence checks pass. Exact-head hosted CI is running; no additional local runtime validation is claimed.
Summary
Extract the native ABC instance-check fallback from #8954, matching the Python 3.15.0 reference used by #8950.
If reading an instance's
__class__raisesAttributeError, use its actual type for ABC checks. Other exceptions still propagate unchanged. This preserves virtual-subclass registration and negative-cache invalidation for such instances.The unique commit changes native
_abcand preserves the original Python regression for missing__class__, registration, and propagation of other exceptions. The pure-Python_py_abcfallback remains outside this PR's unique change.Reference: CPython 3.15.0 source.
Scope
The unique commit changes no files under
Lib/and inherits.python-version3.15.0 from #8950. The former shared Python 3.14.8 version pin and TextIOWrapper deletion change have been removed. The Python 3.15.0 TextIOWrapper setter checks attachment before rejecting_CHUNK_SIZEdeletion, so the old I/O prerequisite does not belong in this stack.Validation
74c9775a5a9ae1f5fbdba476f4fb5a616da5f293.AI-assisted with OpenAI Codex.
Summary by CodeRabbit
__class__attribute is unavailable.