Skip to content

Remove Copy from PyPayload #6865

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
youknowone merged 2 commits into from
Jan 24, 2026
Merged

Remove Copy from PyPayload #6865

youknowone merged 2 commits into from
Jan 24, 2026

Conversation

@youknowone
Copy link
Member

@youknowone youknowone commented Jan 24, 2026 •
edited by coderabbitai bot
Loading

PyPayloads are expected to be exposed via PyRef. Copy/Clone for them doesn't make sense for most of cases. If they are used, that must be careful to take risk to confuse it to PyRef clone

Summary by CodeRabbit

  • New Features

    • Added new public methods for module creation and execution on PyModuleDef.

  • Chores

    • Removed workspace linting setting for copy implementations.
    • Refactored internal trait implementations across various type definitions to improve memory efficiency and ownership semantics.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 24, 2026 •
edited
Loading

📝 Walkthrough

Walkthrough

This PR removes Clone and Copy derives from numerous structs and enums across the VM codebase, alongside removing a workspace lint setting, to reduce automatic copy semantics on public types while adding new module creation methods to PyModuleDef.

Changes

Cohort / File(s) Summary
Configuration and workspace
Cargo.toml		 Removed missing_copy_implementations = "warn" workspace lint
Builtin singleton and primitive types
crates/vm/src/builtins/singletons.rs, crates/vm/src/builtins/slice.rs, crates/vm/src/builtins/object.rs		 Removed Clone, Copy derives from PyNone, PyNotImplemented, PyEllipsis, PyBaseObject
Module definition and related types
crates/vm/src/builtins/module.rs		 Removed Clone, Copy from PyModuleDef, PyModuleSlots, PyModule; added public methods create_module() and exec_module() to PyModuleDef for phase-based initialization
Namespace and descriptor types
crates/vm/src/builtins/namespace.rs, crates/vm/src/builtins/descriptor.rs		 Changed PyNamespace from unit-like to empty struct and removed Clone, Copy; removed Clone, Copy from MemberGetter, MemberSetter; removed Clone, Copy from MemberKind
Protocol and method types
crates/vm/src/protocol/mapping.rs, crates/vm/src/protocol/sequence.rs, crates/vm/src/protocol/buffer.rs, crates/vm/src/function/method.rs		 Removed Clone, Copy from PyMappingMethods, PySequenceMethods, BufferMethods; removed Copy from PyMethodDef and HeapMethodDef
Numeric argument conversion types
crates/vm/src/function/number.rs		 Removed Clone, Copy from ArgIntoComplex, ArgIntoFloat, ArgIntoBool
String and collection argument types
crates/vm/src/anystr.rs, crates/vm/src/builtins/zip.rs, crates/vm/src/dict_inner.rs		 Removed Clone, Copy from SplitLinesArgs, ExpandTabsArgs, PyZipNewArgs; removed Clone, Copy from DictSize
Utility and sequence types
crates/vm/src/sliceable.rs, crates/vm/src/stdlib/marshal.rs, crates/vm/src/stdlib/os.rs, crates/vm/src/stdlib/typevar.rs, crates/vm/src/stdlib/typing.rs		 Removed Clone, Copy from SequenceIndex, SaturatedSliceIter, DumpError, TargetIsDirectory, Generic, NoDefault

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • Implement copyslot #6505: Implements copyslot for protocol methods (PyMappingMethods, PySequenceMethods) whose Copy/Clone traits are modified in this PR
  • Apply some clippy lints #6045: Modifies PyNamespace initialization logic in the same file where PyNamespace struct declaration is updated in this PR
  • derive Copy where possible #6844: Inverse operation that adds Copy derives to many of the same types being modified here, creating potential conflicts

Suggested reviewers

  • ShaharNaveh
  • arihant2math

Poem

🐰 Copy no more, we declare with glee,
Traits refined, now lean and free!
Clone removed from every place,
Ownership reclaims its grace.
Modules craft themselves with care—
A tidy codebase, fresh and fair! ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Remove Copy from PyPayload' accurately summarizes the main change in the PR, which is reverting Copy/Clone implementations from PyPayload and all dependent public structs and enums throughout the codebase.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing touches
  • 📝 Generate docstrings

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.

@youknowone youknowone marked this pull request as ready for review January 24, 2026 12:47
coderabbitai[bot]
coderabbitai bot reviewed Jan 24, 2026
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/vm/src/builtins/module.rs (1)

44-71: Guard against custom create slots returning incomplete modules.

create_module always calls __init_dict_from_def, which unwraps module.def and module.name. If a custom create slot returns a module without those fields initialized, this will panic. Add an explicit guard (or normalize the module) before continuing.

🛡️ Suggested guard 
@@
-        // Initialize module dict and methods
-        PyModule::__init_dict_from_def(vm, &module);
+        // Initialize module dict and methods
+        if module.def.is_none() || module.name.is_none() {
+            return Err(vm.new_system_error(format!(
+                "module '{}' was created without a complete PyModuleDef",
+                self.name.as_str()
+            )));
+        }
+        PyModule::__init_dict_from_def(vm, &module);
         module.__init_methods(vm)?;

@youknowone youknowone merged commit e0e2926 into RustPython:main Jan 24, 2026
12 of 13 checks passed
@youknowone youknowone deleted the pypayload-no-copy branch January 24, 2026 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

@ShaharNaveh ShaharNaveh Awaiting requested review from ShaharNaveh

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@youknowone