Skip to content

Bind method descriptor when __get__ owner is omitted - #8404

Merged
youknowone merged 1 commit into
RustPython:mainfrom
devyubin:fix-method-descriptor-get
Aug 1, 2026
Merged

Bind method descriptor when __get__ owner is omitted#8404
youknowone merged 1 commit into
RustPython:mainfrom
devyubin:fix-method-descriptor-get

Conversation

@devyubin

@devyubin devyubin commented Jul 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

method_descriptor.__get__(obj) raised TypeError: descriptor '...' needs a type, not '...', as arg 2 when the owner (the second argument) was omitted:

import _io, io
_io._TextIOBase.read.__get__(io.StringIO())   # TypeError on RustPython, works on CPython

The owner is optional in the descriptor protocol; CPython binds to obj when it is not supplied, and only rejects a non-type owner.

Cause

The METHOD-flag branch of PyMethodDescriptor::descr_get gated binding on cls.is_some_and(|c| c.fast_isinstance(type)), so an omitted owner (cls == None) fell through to the "needs a type" error.

Fix

Use cls.as_ref().is_none_or(...): a missing owner binds to obj, while a non-type owner still raises "needs a type, not ...".

Test plan

Verified against CPython 3.14.6 — owner omitted binds, non-type owner raises, type owner binds:

  • Unmarks test_types's test_method_descriptor_crash (removes its @expectedFailure); it now passes.
  • test_types / test_descr: pass, no regressions (the non-type-owner "needs a type" path in test_descr still holds).
  • cargo build / cargo clippy -p rustpython-vm / cargo fmt --check: clean.

@devyubin
Bind method descriptor when __get__ owner is omitted
cc216a0 
`method_descriptor.__get__(obj)` raised a TypeError when the owner (the
optional second argument) was omitted, because the METHOD-flag branch
required the owner to be a type. Match CPython: a missing owner binds to
`obj`, while a non-type owner still raises "needs a type, not ...".

Assisted-by: Claude Code:claude-opus-4-8
@coderabbitai

coderabbitai Bot commented Jul 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The method descriptor binding check now accepts an omitted owner when an instance is provided, while still requiring supplied owners to be type objects.

Changes

Descriptor binding

Layer / File(s) Summary
Allow omitted descriptor owners
crates/vm/src/builtins/descriptor.rs		 PyMethodDescriptor::descr_get binds METHOD descriptors when cls is absent or a type, preserving the type error for supplied non-type owners.

Estimated code review effort: 2 (Simple) | ~5 minutes

Possibly related PRs

Suggested reviewers: shaharnaveh

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Linked Issues check ❓ Inconclusive The binding fix matches #8375, but the expected-failure removal in Lib/test/test_types.py is unverifiable because that file was excluded by !Lib/**. Provide the excluded Lib/test/test_types.py diff, or include that path in review, so the test enablement requirement can be confirmed.
✅ Passed checks (4 passed)
Check name Status Explanation
Out of Scope Changes check ✅ Passed The diff is narrowly scoped to the descriptor binding fix and shows no unrelated changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: binding method descriptors when the owner argument is omitted.
✨ 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.

@github-actions

github-actions Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

📦 Library Dependencies

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

[x] lib: cpython/Lib/types.py
[ ] test: cpython/Lib/test/test_types.py (TODO: 4)

dependencies:

  • types

dependent tests: (57 tests)

  • types: test_annotationlib test_ast test_asyncgen test_asyncio test_builtin test_call test_code test_collections test_compile test_compiler_assemble test_coroutines test_descr test_dis test_doctest test_dtrace test_dynamicclassattribute test_email test_enum test_exception_group test_fstring test_funcattrs test_generators test_genericalias test_global test_hmac test_importlib test_inspect test_listcomps test_marshal test_monitoring test_opcache test_optimizer test_os test_pdb test_positional_only_arg test_pprint test_pyclbr test_pydoc test_raise test_rlcompleter test_string test_subclassinit test_subprocess test_tempfile test_threading test_trace test_traceback test_type_aliases test_type_annotations test_type_params test_types test_typing test_unittest test_userdict test_xml_etree test_xml_etree_c test_xxlimited

Legend:

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

@devyubin
devyubin marked this pull request as draft July 27, 2026 23:08
@youknowone youknowone added the z-ca-2026 Tag to track Contribution Academy 2026 label Jul 28, 2026
@devyubin
devyubin marked this pull request as ready for review August 1, 2026 03:54
youknowone
youknowone approved these changes Aug 1, 2026
View reviewed changes

@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 dc1cae4 into RustPython:main Aug 1, 2026
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@youknowone youknowone youknowone approved these changes

Assignees

No one assigned

Labels

z-ca-2026 Tag to track Contribution Academy 2026

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

method_descriptor.__get__(obj) with the owner omitted raises TypeError instead of binding

2 participants

@devyubin @youknowone