Skip to content

make magic method new to follow cpython way #3057

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

Merged
youknowone merged 3 commits into from
Sep 24, 2021
Merged

make magic method new to follow cpython way #3057

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1c7509a
make magic method new to follow cpython way
eldpswp99 Sep 14, 2021
897a7b0
bring back tp_new_wrapper
youknowone Sep 14, 2021
00b33f8
fix __new__ as bound method for every type
youknowone Sep 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions vm/src/builtins/function.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -541,8 +541,6 @@ impl PyBoundMethod {
// Special case: we work with `__new__`, which is not really a method.
// It is a function, so its `__qualname__` is just `__new__`.
// We need to add object's part manually.
// Note: at the moment, `__new__` in the form of `tp_new_wrapper`
// is the only instance of `builtin_function_or_method_type`.
let obj_name = vm.get_attribute_opt(self.object.clone(), "__qualname__")?;
let obj_name: Option<PyStrRef> = obj_name.and_then(|o| o.downcast().ok());
return Ok(vm.ctx.new_str(format!(
Expand Down
7 changes: 3 additions & 4 deletions vm/src/builtins/object.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@ use super::dict::{PyDict, PyDictRef};
use super::list::PyList;
use super::pybool;
use super::pystr::{PyStr, PyStrRef};
use super::pytype::PyTypeRef;
use crate::builtins::pytype::PyType;
use super::pytype::{PyType, PyTypeRef};
use crate::common::hash::PyHash;
use crate::function::FuncArgs;
use crate::slots::PyComparisonOp;
Expand Down Expand Up @@ -364,8 +363,8 @@ pub(crate) fn setattr(
}
}

pub fn init(context: &PyContext) {
PyBaseObject::extend_class(context, &context.types.object_type);
pub fn init(ctx: &PyContext) {
PyBaseObject::extend_class(ctx, &ctx.types.object_type);
}

fn common_reduce(obj: PyObjectRef, proto: usize, vm: &VirtualMachine) -> PyResult {
Expand Down
40 changes: 15 additions & 25 deletions vm/src/builtins/pytype.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,19 @@ impl PyTypeRef {

#[pyimpl(with(SlotGetattro, SlotSetattro, Callable), flags(BASETYPE))]
impl PyType {
// bound method for every type
pub(crate) fn __new__(zelf: PyRef<Self>, args: FuncArgs, vm: &VirtualMachine) -> PyResult {

@DimitrisJim DimitrisJim Sep 22, 2021

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.

__new__ is sort of confusing to me, I'd prefer if this kept tp_new_wrapper so we can easily see which function this maps to in CPython.

@youknowone youknowone Sep 23, 2021 •
edited
Loading

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.

This is a body of bound method __new__ just like mro. The only difference of them is that __new__ is a bound method for every type including type but mro is an unbound method for type and bound as boundmethod for other types.
I think we'd better to keep this name as same as other methods when it works like other methods.

@youknowone youknowone Oct 16, 2021

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 couldn't clarify this concepts well at this time, but now things seem well aligned.
new_wrapper is now meaning python-side wrapper of __new__. And this is a wrapper for type slot new.

let (subtype, args): (PyRef<Self>, FuncArgs) = args.bind(vm)?;
if !subtype.issubclass(&zelf) {
return Err(vm.new_type_error(format!(
"{zelf}.__new__({subtype}): {subtype} is not a subtype of {zelf}",
zelf = zelf.name(),
subtype = subtype.name(),
)));
}
call_tp_new(zelf, subtype, args, vm)
}

#[pyproperty(name = "__mro__")]
fn get_mro(zelf: PyRef<Self>) -> PyTuple {
let elements: Vec<PyObjectRef> = zelf.iter_mro().map(|x| x.as_object().clone()).collect();
Expand Down Expand Up @@ -765,43 +778,20 @@ impl<T: DerefToPyType> DerefToPyType for &'_ T {
}
}

fn call_tp_new(
pub(crate) fn call_tp_new(
typ: PyTypeRef,
subtype: PyTypeRef,
mut args: FuncArgs,
args: FuncArgs,
vm: &VirtualMachine,
) -> PyResult {
for cls in typ.deref().iter_mro() {
if let Some(new_meth) = cls.get_attr("__new__") {
if !vm.ctx.is_tp_new_wrapper(&new_meth) {
let new_meth = vm.call_if_get_descriptor(new_meth, typ.clone().into_object())?;
args.prepend_arg(typ.clone().into_object());
return vm.invoke(&new_meth, args);
}
}
if let Some(tp_new) = cls.slots.new.load() {
return tp_new(subtype, args, vm);
}
}
unreachable!("Should be able to find a new slot somewhere in the mro")
}

pub fn tp_new_wrapper(
zelf: PyTypeRef,
cls: PyTypeRef,
args: FuncArgs,
vm: &VirtualMachine,
) -> PyResult {
if !cls.issubclass(&zelf) {
return Err(vm.new_type_error(format!(
"{zelf}.__new__({cls}): {cls} is not a subtype of {zelf}",
zelf = zelf.name(),
cls = cls.name(),
)));
}
call_tp_new(zelf, cls, args, vm)
}

fn take_next_base(bases: &mut Vec<Vec<PyTypeRef>>) -> Option<PyTypeRef> {
for base in bases.iter() {
let head = base[0].clone();
Expand Down
2 changes: 0 additions & 2 deletions vm/src/macros.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ macro_rules! py_class {
$($crate::py_class!(@extract_slots($ctx, &mut slots, $name, $value));)*
let py_class = $ctx.new_class($class_name, $class_base, slots);
$($crate::py_class!(@extract_attrs($ctx, &py_class, $name, $value));)*
$ctx.add_slot_wrappers(&py_class);
py_class
}
};
Expand All @@ -50,7 +49,6 @@ macro_rules! extend_class {
$(
$class.set_str_attr($name, $value);
)*
$ctx.add_slot_wrappers(&$class);
};
}

Expand Down
21 changes: 7 additions & 14 deletions vm/src/pyobject.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use crate::builtins::list::PyList;
use crate::builtins::namespace::PyNamespace;
use crate::builtins::object;
use crate::builtins::pystr;
use crate::builtins::pytype::{self, PyType, PyTypeRef};
use crate::builtins::pytype::{PyType, PyTypeRef};
use crate::builtins::set::{self, PyFrozenSet};
use crate::builtins::singletons::{PyNone, PyNoneRef, PyNotImplemented, PyNotImplementedRef};
use crate::builtins::slice::PyEllipsis;
Expand Down Expand Up @@ -126,7 +126,7 @@ impl PyContext {

let new_str = PyRef::new_ref(pystr::PyStr::from("__new__"), types.str_type.clone(), None);
let tp_new_wrapper = create_object(
PyNativeFuncDef::new(pytype::tp_new_wrapper.into_func(), new_str).into_function(),
PyNativeFuncDef::new(PyType::__new__.into_func(), new_str).into_function(),
&types.builtin_function_or_method_type,
)
.into_object();
Expand Down Expand Up @@ -376,17 +376,6 @@ impl PyContext {
pub fn new_base_object(&self, class: PyTypeRef, dict: Option<PyDictRef>) -> PyObjectRef {
PyObject::new(object::PyBaseObject, class, dict)
}

pub fn add_slot_wrappers(&self, ty: &PyTypeRef) {
let new_wrapper =
self.new_bound_method(self.tp_new_wrapper.clone(), ty.clone().into_object());
ty.set_str_attr("__new__", new_wrapper);
}

pub fn is_tp_new_wrapper(&self, obj: &PyObjectRef) -> bool {
obj.payload::<PyBoundMethod>()
.map_or(false, |bound| bound.function.is(&self.tp_new_wrapper))
}
}

impl Default for PyContext {
Expand Down Expand Up @@ -1100,13 +1089,17 @@ pub trait PyClassImpl: PyClassDef {
);
}
Self::impl_extend_class(ctx, class);
ctx.add_slot_wrappers(class);
if let Some(doc) = Self::DOC {
class.set_str_attr("__doc__", ctx.new_str(doc));
}
if let Some(module_name) = Self::MODULE_NAME {
class.set_str_attr("__module__", ctx.new_str(module_name));
}
if class.slots.new.load().is_some() {
let bound =
ctx.new_bound_method(ctx.tp_new_wrapper.clone(), class.clone().into_object());
class.set_str_attr("__new__", bound);
}
}

fn make_class(ctx: &PyContext) -> PyTypeRef
Expand Down