Merge pull request RustPython#3748 from youknowone/getattr

youknowone · web-flow · commit 7182111fa9d6 · 2022-06-01T09:38:27.000+09:00
Fix getattr to use `__getattribute__` correctly
diff --git a/Lib/test/test_enum.py b/Lib/test/test_enum.py
@@ -429,8 +429,6 @@ def red(self):
                 green = 2
                 blue = 3
 
-    # TODO: RUSTPYTHON
-    @unittest.expectedFailure
     def test_enum_with_value_name(self):
         class Huh(Enum):
             name = 1
@@ -1668,8 +1666,6 @@ class Test(Base):
             test = 1
         self.assertIs(type(Test.test), Test)
 
-    # TODO: RUSTPYTHON
-    @unittest.expectedFailure
     def test_subclass_duplicate_name_dynamic(self):
         from types import DynamicClassAttribute
         class Base(Enum):
diff --git a/vm/src/builtins/weakproxy.rs b/vm/src/builtins/weakproxy.rs
@@ -2,7 +2,7 @@ use super::{PyStrRef, PyType, PyTypeRef, PyWeak};
 use crate::{
     class::PyClassImpl,
     function::OptionalArg,
-    types::{Constructor, SetAttr},
+    types::{Constructor, GetAttr, SetAttr},
     Context, Py, PyObjectRef, PyPayload, PyRef, PyResult, VirtualMachine,
 };
 
@@ -57,14 +57,8 @@ crate::common::static_cell! {
     static WEAK_SUBCLASS: PyTypeRef;
 }
 
-#[pyimpl(with(SetAttr, Constructor))]
+#[pyimpl(with(GetAttr, SetAttr, Constructor))]
 impl PyWeakProxy {
-    // TODO: callbacks
-    #[pymethod(magic)]
-    fn getattr(&self, attr_name: PyStrRef, vm: &VirtualMachine) -> PyResult {
-        let obj = self.weak.upgrade().ok_or_else(|| new_reference_error(vm))?;
-        obj.get_attr(attr_name, vm)
-    }
     #[pymethod(magic)]
     fn str(&self, vm: &VirtualMachine) -> PyResult<PyStrRef> {
         match self.weak.upgrade() {
@@ -81,6 +75,14 @@ fn new_reference_error(vm: &VirtualMachine) -> PyRef<super::PyBaseException> {
     )
 }
 
+impl GetAttr for PyWeakProxy {
+    // TODO: callbacks
+    fn getattro(zelf: &Py<Self>, name: PyStrRef, vm: &VirtualMachine) -> PyResult {
+        let obj = zelf.weak.upgrade().ok_or_else(|| new_reference_error(vm))?;
+        obj.get_attr(name, vm)
+    }
+}
+
 impl SetAttr for PyWeakProxy {
     fn setattro(
         zelf: &crate::Py<Self>,
diff --git a/vm/src/protocol/object.rs b/vm/src/protocol/object.rs
@@ -233,9 +233,6 @@ impl PyObject {
                 }
                 None => Ok(Some(attr)),
             }
-        } else if let Some(getter) = obj_cls.get_attr(identifier!(vm, __getattr__)) {
-            drop(obj_cls);
-            vm.invoke(&getter, (self.to_owned(), name_str)).map(Some)
         } else {
             Ok(None)
         }
diff --git a/vm/src/types/slot.rs b/vm/src/types/slot.rs
@@ -414,7 +414,15 @@ fn call_wrapper(zelf: &PyObject, args: FuncArgs, vm: &VirtualMachine) -> PyResul
 }
 
 fn getattro_wrapper(zelf: &PyObject, name: PyStrRef, vm: &VirtualMachine) -> PyResult {
-    vm.call_special_method(zelf.to_owned(), identifier!(vm, __getattribute__), (name,))
+    let __getattribute__ = identifier!(vm, __getattribute__);
+    let __getattr__ = identifier!(vm, __getattr__);
+    match vm.call_special_method(zelf.to_owned(), __getattribute__, (name.clone(),)) {
+        Ok(r) => Ok(r),
+        Err(_) if zelf.class().has_attr(__getattr__) => {
+            vm.call_special_method(zelf.to_owned(), __getattr__, (name,))
+        }
+        Err(e) => Err(e),
+    }
 }
 
 fn setattro_wrapper(
@@ -522,7 +530,7 @@ impl PyType {
             "__call__" => {
                 update_slot!(call, call_wrapper);
             }
-            "__getattribute__" => {
+            "__getattr__" | "__getattribute__" => {
                 update_slot!(getattro, getattro_wrapper);
             }
             "__setattr__" | "__delattr__" => {

Original file line number	Diff line number	Diff line change
`@@ -233,9 +233,6 @@ impl PyObject {`
`233`	`233`	`}`
`234`	`234`	`None => Ok(Some(attr)),`
`235`	`235`	`}`
`236`		`- } else if let Some(getter) = obj_cls.get_attr(identifier!(vm, __getattr__)) {`
`237`		`- drop(obj_cls);`
`238`		`- vm.invoke(&getter, (self.to_owned(), name_str)).map(Some)`
`239`	`236`	`} else {`
`240`	`237`	`Ok(None)`
`241`	`238`	`}`
Original file line number	Diff line number	Diff line change
`@@ -414,7 +414,15 @@ fn call_wrapper(zelf: &PyObject, args: FuncArgs, vm: &VirtualMachine) -> PyResul`
`414`	`414`	`}`
`415`	`415`
`416`	`416`	`fn getattro_wrapper(zelf: &PyObject, name: PyStrRef, vm: &VirtualMachine) -> PyResult {`
`417`		`- vm.call_special_method(zelf.to_owned(), identifier!(vm, __getattribute__), (name,))`
	`417`	`+ let __getattribute__ = identifier!(vm, __getattribute__);`
	`418`	`+ let __getattr__ = identifier!(vm, __getattr__);`
	`419`	`+ match vm.call_special_method(zelf.to_owned(), __getattribute__, (name.clone(),)) {`
	`420`	`+ Ok(r) => Ok(r),`
	`421`	`+ Err(_) if zelf.class().has_attr(__getattr__) => {`
	`422`	`+ vm.call_special_method(zelf.to_owned(), __getattr__, (name,))`
	`423`	`+ }`
	`424`	`+ Err(e) => Err(e),`
	`425`	`+ }`
`418`	`426`	`}`
`419`	`427`
`420`	`428`	`fn setattro_wrapper(`
`@@ -522,7 +530,7 @@ impl PyType {`
`522`	`530`	`"__call__" => {`
`523`	`531`	`update_slot!(call, call_wrapper);`
`524`	`532`	`}`
`525`		`- "__getattribute__" => {`
	`533`	`+ "__getattr__" \| "__getattribute__" => {`
`526`	`534`	`update_slot!(getattro, getattro_wrapper);`
`527`	`535`	`}`
`528`	`536`	`"__setattr__" \| "__delattr__" => {`