Skip to content

weakref: treat explicit None callback as no callback - #8298

Merged
youknowone merged 1 commit into
RustPython:mainfrom
rlaisqls:fix/weakref-none-callback
Jul 17, 2026
Merged

weakref: treat explicit None callback as no callback#8298
youknowone merged 1 commit into
RustPython:mainfrom
rlaisqls:fix/weakref-none-callback

Conversation

@rlaisqls

@rlaisqls rlaisqls commented Jul 17, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

Summary

  • Fix weakref.ref(obj, None) being treated as having a callback instead of no callback.
  • Re-enable test_shared_ref_without_callback, which was previously marked @unittest.expectedFailure due to this bug.

Summary by CodeRabbit

  • Bug Fixes
    • Improved weak reference creation by consistently treating an explicitly provided None callback as no callback.

@coderabbitai

coderabbitai Bot commented Jul 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: b26810b2-9f0d-4110-bfad-6e3edf562ab9

📥 Commits

Reviewing files that changed from the base of the PR and between 0b4e5da and 46df178.

⛔ Files ignored due to path filters (1)
  • Lib/test/test_weakref.py is excluded by !Lib/**
📒 Files selected for processing (1)
  • crates/vm/src/builtins/weakref.rs
📝 Walkthrough

Walkthrough

PyWeak::slot_new now treats an explicit None positional callback as absent when constructing a weak reference.

Changes

Weak reference callback handling

Layer / File(s) Summary
Filter explicit None callbacks
crates/vm/src/builtins/weakref.rs		 The optional callback is captured only when the next positional argument is non-None.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Possibly related PRs

Suggested reviewers: shaharnaveh

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main behavioral change to weakref callback handling.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
✨ 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 17, 2026

Copy link
Copy Markdown
Contributor

📦 Library Dependencies

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

[x] lib: cpython/Lib/weakref.py
[x] lib: cpython/Lib/_weakrefset.py
[x] test: cpython/Lib/test/test_weakref.py (TODO: 12)
[x] test: cpython/Lib/test/test_weakset.py

dependencies:

  • weakref

dependent tests: (222 tests)

  • weakref: test_array test_ast test_asyncio test_code test_concurrent_futures test_context test_contextlib test_copy test_ctypes test_deque test_descr test_dict test_enum test_exceptions test_file test_fileio test_finalization test_frame test_functools test_gc test_generators test_genericalias test_importlib test_inspect test_io test_ipaddress test_itertools test_logging test_memoryio test_memoryview test_mmap test_ordered_dict test_pickle test_picklebuffer test_queue test_re test_scope test_set test_slice test_socket test_sqlite3 test_ssl test_struct test_sys test_tempfile test_thread test_threading test_threading_local test_type_params test_types test_typing test_unittest test_uuid test_weakref test_weakset test_xml_etree
    • asyncio: test_asyncio test_external_inspection test_os test_pdb test_unittest
    • bdb: test_bdb
    • concurrent.futures.process: test_compileall test_concurrent_futures
    • copy: test_bytes test_codecs test_collections test_copyreg test_coroutines test_csv test_decimal test_defaultdict test_dictviews test_email test_fractions test_http_cookies test_minidom test_opcache test_optparse test_platform test_plistlib test_posix test_site test_statistics test_structseq test_super test_sysconfig test_tomllib test_urllib2 test_xml_dom_minicompat test_zlib
      • argparse: test_argparse
      • collections: test_annotationlib test_bisect test_builtin test_c_locale_coercion test_call test_configparser test_contains test_ctypes test_embed test_exception_group test_fileinput test_funcattrs test_hash test_httpservers test_iter test_iterlen test_json test_math test_monitoring test_pathlib test_patma test_pprint test_pydoc test_random test_reprlib test_richcmp test_shelve test_sqlite3 test_string test_traceback test_tuple test_urllib test_userdict test_userlist test_userstring test_with
      • dataclasses: test__colorize test_ctypes test_regrtest test_zoneinfo
      • email.generator: test_email
      • gettext: test_gettext test_tools
      • http.cookiejar: test_http_cookiejar
      • http.server: test_robotparser test_urllib2_localnet test_xmlrpc
      • logging.handlers: test_pkgutil
      • mailbox: test_mailbox
      • smtplib: test_smtplib test_smtpnet
      • tarfile: test_shutil test_tarfile
      • webbrowser: test_webbrowser
    • inspect: test_abc test_asyncgen test_buffer test_clinic test_grammar test_ntpath test_operator test_posixpath test_signal test_turtle test_type_annotations test_yield_from test_zipimport test_zipimport_support
      • ast: test_compile test_compiler_codegen test_dis test_fstring test_future_stmt test_peepholer test_peg_generator test_type_comments test_ucn test_unparse
      • cmd: test_cmd
      • importlib.metadata: test_importlib
      • pkgutil: test_pyrepl test_runpy
      • rlcompleter: test_pyrepl test_rlcompleter
      • trace: test_trace
    • logging: test_hashlib test_support test_urllib2net
      • hashlib: test_hmac test_unicodedata
      • multiprocessing.util: test_concurrent_futures
      • venv: test_venv
    • multiprocessing: test_fcntl test_multiprocessing_main_handling
    • symtable: test_symtable
    • tempfile: test_bz2 test_cmd_line test_cprofile test_ctypes test_doctest test_ensurepip test_faulthandler test_filecmp test_generated_cases test_importlib test_launcher test_linecache test_modulefinder test_peg_generator test_pkg test_pstats test_py_compile test_pyrepl test_selectors test_string_literals test_subprocess test_sys_settrace test_tabnanny test_termios test_threadedtempfile test_tokenize test_urllib_response test_winconsoleio test_zipapp test_zipfile test_zipfile64 test_zstd
      • ctypes.util: test_ctypes
      • urllib.request: test_sax test_urllibnet

Legend:

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

@fanninpm
fanninpm requested a review from youknowone July 17, 2026 02:10
@youknowone youknowone added the z-ca-2026 Tag to track Contribution Academy 2026 label Jul 17, 2026
youknowone
youknowone approved these changes Jul 17, 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.

I expected to use and_then, but looks reasonable. Thanks!

@youknowone
youknowone enabled auto-merge (squash) July 17, 2026 02:38
@youknowone
youknowone merged commit 1205fd2 into RustPython:main Jul 17, 2026
27 checks passed
youknowone pushed a commit that referenced this pull request Sep 16, 2026
@rlaisqls
weakref: treat explicit None callback as no callback (#8298)
abc35e5
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.

2 participants

@rlaisqls @youknowone