Skip to content

Defer staticmethod/classmethod callable storage to __init__ - #7697

Merged
youknowone merged 1 commit into
RustPython:mainfrom
changjoon-park:fix-staticmethod-classmethod-new-init
Apr 27, 2026
Merged

Defer staticmethod/classmethod callable storage to __init__#7697
youknowone merged 1 commit into
RustPython:mainfrom
changjoon-park:fix-staticmethod-classmethod-new-init

Conversation

@changjoon-park

@changjoon-park changjoon-park commented Apr 27, 2026

Copy link
Copy Markdown
Contributor

Background

Python's two-phase object construction splits responsibility between __new__ (allocation, no required state) and __init__ (initialization). Subclasses can override either phase independently.

CPython's staticmethod/classmethod follow this contract strictly: __new__ allocates a bare descriptor with __func__ = None, and __init__ assigns the callable plus copies wrapper attributes (__doc__, __name__, __qualname__, __module__, __annotations__) via functools_wraps (Objects/funcobject.c::sm_init / cm_init).

RustPython instead did the same work twice — once in slot_new and again in the Initializer — so a subclass overriding __init__ without calling super().__init__() still had __func__ set from slot_new.

Repro

class MyStatic(staticmethod):
    def __init__(self, func):
        pass  # deliberately not calling super().__init__()

def f(): pass
sm = MyStatic(f)

# CPython 3.14 (and now RustPython):
#   repr(sm)          == '<staticmethod(None)>'
#   sm.__func__       is None
#   sm.__wrapped__    is None
#
# RustPython before this PR:
#   repr(sm)          == '<staticmethod(<function f at 0x...>)>'
#   sm.__func__       == f
#   sm.__wrapped__    == f

This pattern shows up in libraries that subclass the descriptor types to customize their state (e.g. property-like wrappers, mocking libraries, internal CPython tests for descriptor semantics).

Fix

  • slot_new now validates the signature (so staticmethod() / staticmethod(a, b) still error eagerly) but stores None for the callable and skips the wrapper-attribute copy.
  • Initializer::init stores the callable and runs the wrapper copy, mirroring CPython's split.

Net diff: +35 / -44 (the wrapper-attr setup just moves between methods, and the slot_new path collapses).

Tests unmasked

  • test_descr.ClassPropertiesAndMethods.test_staticmethod_new
  • test_descr.ClassPropertiesAndMethods.test_classmethod_new

(Both were marked @unittest.expectedFailure # TODO: RUSTPYTHON; Wrong error message — the failure was actually behavioral, not a wording issue, but the new behavior matches the test's assertions exactly.)

Verification

  • 16 modules pass with no regressions: test_descr test_decorators test_funcattrs test_inspect test_functools test_class test_super test_abc test_property test_pickle test_dataclasses test_dis test_metaclass test_call test_module test_typing test_keywordonlyarg (1,071+ tests)
  • CPython 3.14.4 byte-identical on the repro and unmask scenarios
  • Edge cases probed: 0-arg / 2-arg construction errors, re-init via sm.__init__(other), subclass that does call super().__init__(), pickling round-trip, @staticmethod / @classmethod decorator usage — all match CPython

CPython's staticmethod and classmethod set __func__ and copy wrapper
attributes (__doc__, __name__, etc.) only inside __init__
(Objects/funcobject.c::sm_init / cm_init). RustPython did this work in
slot_new and again in __init__, so subclasses that override __init__
without calling super().__init__() saw __func__ pointing at the
original callable instead of None.

Move the callable assignment and the wrapper-attribute copy into
Initializer::init; slot_new now just validates the signature and stores
None for the callable, matching the CPython contract.
@coderabbitai

coderabbitai Bot commented Apr 27, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

The PR refactors PyClassMethod and PyStaticMethod to defer callable binding and wrapper-attribute population from object construction (slot_new) to a later initialization phase (init). Argument validation remains in slot_new, while actual callable storage and attribute copying occur in init.

Changes

Cohort / File(s) Summary
Builtin Method Initialization Refactoring
crates/vm/src/builtins/classmethod.rs, crates/vm/src/builtins/staticmethod.rs
Both files moved callable binding and attribute copying (__doc__, __name__, __qualname__, __module__, __annotations__ for classmethod; __doc__ for staticmethod) from slot_new to init, deferring initialization work to a later lifecycle phase. Argument validation remains in slot_new.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A callable once eager to bind,
Now waits for init to unwind,
The slots delay with graceful care,
While attributes float through the air!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: deferring callable storage from new to init for staticmethod and classmethod, matching the core objective of aligning with CPython behavior.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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:

[ ] test: cpython/Lib/test/test_descr.py (TODO: 39)
[ ] test: cpython/Lib/test/test_descrtut.py (TODO: 3)

dependencies:

dependent tests: (no tests depend on descr)

Legend:

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

@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.

🧹 Nitpick comments (1)
crates/vm/src/builtins/classmethod.rs (1)

92-92: Consider a non-panicking fallback for the __dict__ lookup.

expect("classmethod has __dict__") is currently safe because slot_new always attaches a dict, but it couples init's correctness to that invariant. If a future refactor of slot_new (or a subclass path that constructs via a different code route) ever lands an instance without a dict, this becomes a hard panic in user code.

A small defensive change keeps the happy path identical and falls back gracefully:

♻️ Suggested refactor
-        let dict = zelf.as_object().dict().expect("classmethod has __dict__");
-        for attr in [
-            identifier!(vm.ctx, __doc__),
-            identifier!(vm.ctx, __name__),
-            identifier!(vm.ctx, __qualname__),
-            identifier!(vm.ctx, __module__),
-            identifier!(vm.ctx, __annotations__),
-        ] {
-            if let Ok(value) = callable.get_attr(attr, vm) {
-                dict.set_item(attr, value, vm)?;
-            }
-        }
+        if let Some(dict) = zelf.as_object().dict() {
+            for attr in [
+                identifier!(vm.ctx, __doc__),
+                identifier!(vm.ctx, __name__),
+                identifier!(vm.ctx, __qualname__),
+                identifier!(vm.ctx, __module__),
+                identifier!(vm.ctx, __annotations__),
+            ] {
+                if let Ok(value) = callable.get_attr(attr, vm) {
+                    dict.set_item(attr, value, vm)?;
+                }
+            }
+        }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/builtins/classmethod.rs` at line 92, Replace the panicking
lookup `zelf.as_object().dict().expect("classmethod has __dict__")` with a
non-panicking branch: call `zelf.as_object().dict()` and if it returns
Some(dict) use it, otherwise create a new empty dict (or obtain a canonical
empty/allocated PyDict), attach it to the instance if appropriate, and proceed;
update the code in classmethod handling in classmethod.rs around the
`zelf.as_object().dict()` use so the happy path is unchanged but
missing-__dict__ cases gracefully allocate/attach and avoid panics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@crates/vm/src/builtins/classmethod.rs`:
- Line 92: Replace the panicking lookup
`zelf.as_object().dict().expect("classmethod has __dict__")` with a
non-panicking branch: call `zelf.as_object().dict()` and if it returns
Some(dict) use it, otherwise create a new empty dict (or obtain a canonical
empty/allocated PyDict), attach it to the instance if appropriate, and proceed;
update the code in classmethod handling in classmethod.rs around the
`zelf.as_object().dict()` use so the happy path is unchanged but
missing-__dict__ cases gracefully allocate/attach and avoid panics.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 5305d229-9f35-4b8a-a29b-98717202f258

📥 Commits

Reviewing files that changed from the base of the PR and between 02c454b and 96a82a7.

⛔ Files ignored due to path filters (1)
  • Lib/test/test_descr.py is excluded by !Lib/**
📒 Files selected for processing (2)
  • crates/vm/src/builtins/classmethod.rs
  • crates/vm/src/builtins/staticmethod.rs

@ShaharNaveh ShaharNaveh 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.

lgtm!
tysm!

@changjoon-park

Copy link
Copy Markdown
Contributor Author

lgtm! tysm!

lgtm! tysm!

thanks for the review :)

@youknowone youknowone left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@youknowone
youknowone merged commit f10f441 into RustPython:main Apr 27, 2026
21 checks passed
@changjoon-park
changjoon-park deleted the fix-staticmethod-classmethod-new-init branch April 27, 2026 13:24
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.

3 participants