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