Fix classmethod descr_get to match CPython behavior#7420
Fix classmethod descr_get to match CPython behavior#7420youknowone merged 1 commit intoRustPython:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughPyClassMethod::descr_get was simplified: the prior lookup/invocation of the callable's Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [ ] lib: cpython/Lib/locale.py dependencies:
dependent tests: (96 tests)
[x] lib: cpython/Lib/modulefinder.py dependencies:
dependent tests: (2 tests)
[ ] lib: cpython/Lib/pkgutil.py dependencies:
dependent tests: (7 tests)
[x] lib: cpython/Lib/poplib.py dependencies:
dependent tests: (1 tests)
[ ] lib: cpython/Lib/smtplib.py dependencies:
dependent tests: (5 tests)
[ ] lib: cpython/Lib/asyncio dependencies:
dependent tests: (7 tests)
[x] test: cpython/Lib/test/test_builtin.py (TODO: 25) dependencies: dependent tests: (no tests depend on builtin) [ ] lib: cpython/Lib/collections dependencies:
dependent tests: (302 tests)
[ ] lib: cpython/Lib/concurrent dependencies:
dependent tests: (16 tests)
[x] lib: cpython/Lib/dataclasses.py dependencies:
dependent tests: (90 tests)
[ ] test: cpython/Lib/test/test_decorators.py dependencies: dependent tests: (no tests depend on decorators) [ ] test: cpython/Lib/test/test_descr.py (TODO: 47) dependencies: dependent tests: (no tests depend on descr) [x] lib: cpython/Lib/doctest.py dependencies:
dependent tests: (33 tests)
[x] test: cpython/Lib/test/test_fork1.py (TODO: 1) dependencies: dependent tests: (no tests depend on fork1) [x] test: cpython/Lib/test/test_format.py (TODO: 7) dependencies: dependent tests: (no tests depend on format) [x] test: cpython/Lib/test/test_frame.py (TODO: 14) dependencies: dependent tests: (no tests depend on frame) [x] lib: cpython/Lib/ftplib.py dependencies:
dependent tests: (34 tests)
[x] test: cpython/Lib/test/test_gc.py dependencies: dependent tests: (168 tests)
[ ] test: cpython/Lib/test/test_generators.py (TODO: 12) dependencies: dependent tests: (no tests depend on generator) [x] lib: cpython/Lib/importlib dependencies:
dependent tests: (112 tests)
[ ] lib: cpython/Lib/multiprocessing dependencies:
dependent tests: (11 tests)
[x] lib: cpython/Lib/os.py dependencies:
dependent tests: (171 tests)
[x] lib: cpython/Lib/pickle.py dependencies:
dependent tests: (97 tests)
[ ] test: cpython/Lib/test/test_raise.py dependencies: dependent tests: (no tests depend on raise) [x] lib: cpython/Lib/runpy.py dependencies:
dependent tests: (2 tests)
[x] lib: cpython/Lib/symtable.py dependencies:
dependent tests: (2 tests)
[x] lib: cpython/Lib/tabnanny.py dependencies:
dependent tests: (1 tests)
[x] lib: cpython/Lib/threading.py dependencies:
dependent tests: (150 tests)
[ ] lib: cpython/Lib/tokenize.py dependencies:
dependent tests: (135 tests)
[ ] lib: cpython/Lib/trace.py dependencies:
dependent tests: (1 tests)
[ ] test: cpython/Lib/test/test_tuple.py (TODO: 1) dependencies: dependent tests: (no tests depend on tuple) [x] lib: cpython/Lib/types.py dependencies:
dependent tests: (52 tests)
[ ] test: cpython/Lib/test/test_utf8source.py (TODO: 2) dependencies: dependent tests: (no tests depend on utf8source) [x] lib: cpython/Lib/weakref.py dependencies:
dependent tests: (204 tests)
[x] lib: cpython/Lib/xml dependencies:
dependent tests: (34 tests)
Legend:
|
d282d76 to
5280922
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/vm/src/builtins/classmethod.rs`:
- Around line 60-61: The existing comment above the line `let callable =
zelf.callable.lock().clone();` in descr_get is now inaccurate because this path
no longer calls Python code; update or remove the comment so it no longer states
“before calling Python code.” Replace it with a concise factual note such as
“Clone the callable and release the lock to avoid holding the lock across
subsequent operations” or simply delete the comment to avoid misleading future
readers; ensure the reference to `zelf.callable.lock().clone()` remains so
reviewers can find the changed line.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: ce0f0549-61ab-4a27-80a4-7bf51010cc55
⛔ Files ignored due to path filters (1)
Lib/test/test_decorators.pyis excluded by!Lib/**
📒 Files selected for processing (1)
crates/vm/src/builtins/classmethod.rs
Simplify classmethod.__get__ to always create a PyBoundMethod binding the callable to the class, matching CPython's cm_descr_get which simply calls PyMethod_New(cm->cm_callable, type). The previous implementation incorrectly tried to call __get__ on the wrapped callable, which broke when a bound method was passed to classmethod() (e.g. classmethod(A().foo)).
5280922 to
aa16500
Compare
Simplify classmethod.get to always create a PyBoundMethod binding the callable to the class, matching CPython's cm_descr_get which simply calls PyMethod_New(cm->cm_callable, type).
The previous implementation incorrectly tried to call get on the wrapped callable, which broke when a bound method was passed to classmethod() (e.g. classmethod(A().foo)).
CPython reference
https://github.com/python/cpython/blob/d9c26676b26ab09d8db7265dc22a733d3c358d4b/Objects/funcobject.c#L1458-L1471
Summary by CodeRabbit