From 1e985e26da38669a86755fb4e42b01958eea6508 Mon Sep 17 00:00:00 2001 From: Idan Dor Date: Fri, 14 Feb 2020 15:08:47 +0200 Subject: [PATCH 1/5] Fixed issue `MRO computation in RustPython doesn't seem to implement C3 correctly` #1659. To my surprise the C3 algorithm was implemented, however I have found a few problems which cause it to appear like it isn't: - Bad mro usage in super: starting from the current class mro instead of the original class at the index of the current class. This leads to skipping classes gotten from multiple inheritance. - A minor error in mro c3 implementation - not keeping local precedence ordering. - The mro c3 implementation also allowed duplicate base classes. (Not from the issue but I noticed this anyway). To allow the fix in new class creation I added an optional vm argument. This allows the class creation to correctly throw type errors at mro errors. Due to classes being necessary to vm creation in the first place, this argument is optional (and should only be skipped during initial type creation) and if there is an error here, it will panic instead of returning it. --- vm/src/obj/objsuper.rs | 42 +++++++++++++++------------ vm/src/obj/objtype.rs | 66 +++++++++++++++++++++++++++++++++++------- vm/src/types.rs | 2 ++ 3 files changed, 81 insertions(+), 29 deletions(-) diff --git a/vm/src/obj/objsuper.rs b/vm/src/obj/objsuper.rs index 0fcc7fd83..da9deeac0 100644 --- a/vm/src/obj/objsuper.rs +++ b/vm/src/obj/objsuper.rs @@ -11,7 +11,8 @@ use super::objstr::PyStringRef; use super::objtype::{self, PyClass, PyClassRef}; use crate::function::OptionalArg; use crate::pyobject::{ - PyClassImpl, PyContext, PyObjectRef, PyRef, PyResult, PyValue, TryFromObject, TypeProtocol, + IdProtocol, PyClassImpl, PyContext, PyObjectRef, PyRef, PyResult, PyValue, TryFromObject, + TypeProtocol, }; use crate::scope::NameProtocol; use crate::vm::VirtualMachine; @@ -53,27 +54,30 @@ impl PySuper { #[pymethod(name = "__getattribute__")] fn getattribute(&self, name: PyStringRef, vm: &VirtualMachine) -> PyResult { let inst = self.obj.clone(); - let typ = self.typ.clone(); - match typ.payload::() { - Some(PyClass { ref mro, .. }) => { - for class in mro { - if let Ok(item) = vm.get_attribute(class.as_object().clone(), name.clone()) { - if item.payload_is::() { - // This is a classmethod - return Ok(item); - } - return vm.call_if_get_descriptor(item, inst.clone()); - } - } - Err(vm.new_attribute_error(format!( - "{} has no attribute '{}'", - inst, - name.as_str() - ))) - } + // Super should be the next matching class in the object original class' mro after the current one. + let mut mro_iter = match self.obj_type.payload::() { + Some(PyClass { ref mro, .. }) => mro.iter(), _ => panic!("not Class"), + }; + // The type itself is not in its mro, so skip finding the current class if its the type. + if !self.typ.is(&self.obj_type) { + let index = mro_iter.find(|&x| x.as_object().is(&self.typ)); + if index.is_none() { + panic!("Current super type is not in instance's type mro"); + } } + + for class in mro_iter { + if let Ok(item) = vm.get_attribute(class.as_object().clone(), name.clone()) { + if item.payload_is::() { + // This is a classmethod + return Ok(item); + } + return vm.call_if_get_descriptor(item, inst.clone()); + } + } + Err(vm.new_attribute_error(format!("{} has no attribute '{}'", inst, name.as_str()))) } #[pyslot] diff --git a/vm/src/obj/objtype.rs b/vm/src/obj/objtype.rs index 3d5f75ca5..e3bfc73cc 100644 --- a/vm/src/obj/objtype.rs +++ b/vm/src/obj/objtype.rs @@ -1,5 +1,5 @@ use std::cell::RefCell; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::fmt; use super::objdict::PyDictRef; @@ -15,6 +15,7 @@ use crate::pyobject::{ }; use crate::slots::{PyClassSlots, PyTpFlags}; use crate::vm::VirtualMachine; +use itertools::Itertools; /// type(object_or_name, bases, dict) /// type(object) -> the object's type @@ -312,7 +313,14 @@ impl PyClassRef { }; let attributes = dict.to_attributes(); - let typ = new(metatype, name.as_str(), base.clone(), bases, attributes)?; + let typ = new( + metatype, + name.as_str(), + base.clone(), + bases, + attributes, + Some(vm), + )?; typ.slots.borrow_mut().flags = base.slots.borrow().flags; Ok(typ.into()) } @@ -462,19 +470,33 @@ fn take_next_base(mut bases: Vec>) -> Option<(PyClassRef, Vec>) -> Option> { vm_trace!("Linearising MRO: {:?}", bases); + // Python requires that the class direct bases are kept in the same order. + // This is called local precedence ordering. + // This means we must verify that for classes A(), B(A) we must reject C(A, B) even though this + // algorithm will allow the mro ordering of [C, B, A, object]. + // To verify this, we make sure non of the direct bases are in the mro of bases after them. + for (i, base_mro) in bases.iter().enumerate() { + let base = &base_mro[0]; // Mros cannot be empty. + for later_mro in bases[i + 1..].iter() { + // We start at index 1 to skip direct bases. + // This will not catch duplicate bases, but such a thing is already tested for. + if later_mro[1..].iter().any(|cls| cls.is(base)) { + return None; + } + } + } + let mut result = vec![]; loop { if (&bases).iter().all(Vec::is_empty) { @@ -494,9 +516,31 @@ pub fn new( _base: PyClassRef, bases: Vec, dict: HashMap, + optional_vm: Option<&VirtualMachine>, ) -> PyResult { + // Check for duplicates in bases. + let mut unique_bases = HashSet::new(); + for base in bases.iter() { + if !unique_bases.insert(base.get_id()) { + let vm = optional_vm + .expect("Could not create a class made by internal code due to duplicate bases."); + return Err(vm.new_type_error(format!("duplicate base class {}", base.name))); + } + } + let mros = bases.iter().map(|x| _mro(&x)).collect(); - let mro = linearise_mro(mros).unwrap(); + let mro = linearise_mro(mros); + if mro.is_none() { + let vm = + optional_vm.expect("Could not create consistent MRO for class made by internal code."); + let mut error_string = bases.iter().join(", "); + error_string.insert_str( + 0, + "Cannot create a consistent method resolution order (MRO) for bases ", + ); + return Err(vm.new_type_error(error_string)); + } + let mro = mro.unwrap(); let new_type = PyObject { payload: PyClass { name: String::from(name), @@ -619,6 +663,7 @@ mod tests { object.clone(), vec![object.clone()], HashMap::new(), + None, ) .unwrap(); let b = new( @@ -627,6 +672,7 @@ mod tests { object.clone(), vec![object.clone()], HashMap::new(), + None, ) .unwrap(); diff --git a/vm/src/types.rs b/vm/src/types.rs index a81755a01..b88d3ff0d 100644 --- a/vm/src/types.rs +++ b/vm/src/types.rs @@ -244,6 +244,8 @@ pub fn create_type(name: &str, type_type: &PyClassRef, base: &PyClassRef) -> PyC base.clone(), vec![base.clone()], dict, + // We are assuming that anyone calling this internal function will not have errors in type creation so we don't need a VM instance. + None, ) .unwrap() } From 13fa1f464104070d2e8e3aba629b7c0ce415cb05 Mon Sep 17 00:00:00 2001 From: Idan Dor Date: Fri, 14 Feb 2020 15:32:18 +0200 Subject: [PATCH 2/5] Surprisingly the mro fixes only a single test, which had a problem in its mocking attributes not being found when setting `__iter__` at line 1482. --- Lib/test/test_types.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/Lib/test/test_types.py b/Lib/test/test_types.py index 9a1bdf750..47ac8bb15 100644 --- a/Lib/test/test_types.py +++ b/Lib/test/test_types.py @@ -1469,8 +1469,6 @@ class CoroutineTests(unittest.TestCase): self.assertIs(foo(), coro) self.assertIs(foo().__await__(), coro) - # TODO: RUSTPYTHON - @unittest.expectedFailure def test_duck_gen(self): class GenLike: def send(self): pass From 8b80707cf88a6ad5fe35db4694b74e5d7f15420b Mon Sep 17 00:00:00 2001 From: Idan Dor Date: Sat, 22 Feb 2020 16:10:49 +0200 Subject: [PATCH 3/5] Fixes according to PR review and added tests: Moved mro creation from super getattribute to super constructor. Removed optional VM argument in new class construction and changed the function to return String which is later "unwrapped" if a vm is available. Added the relevant tests to `types_snippet.py`. --- tests/snippets/types_snippet.py | 44 ++++++++++++++++ vm/src/obj/objsuper.rs | 28 +++++----- vm/src/obj/objtype.rs | 92 ++++++++++++++------------------- vm/src/types.rs | 4 +- 4 files changed, 98 insertions(+), 70 deletions(-) diff --git a/tests/snippets/types_snippet.py b/tests/snippets/types_snippet.py index 81ee9ef73..db8e3283f 100644 --- a/tests/snippets/types_snippet.py +++ b/tests/snippets/types_snippet.py @@ -90,3 +90,47 @@ assert C.mro() == [C, B, A, BB, AA, object] assert type(Exception.args).__name__ == 'getset_descriptor' assert type(None).__bool__(None) is False + + +class A(): + def f(self): + return 'a' + +class B(A): + def f(self): + return 'b' + super().f() + +class C(A): + def f(self): + return 'c' + super().f() + +class D(B, C): + def f(self): + return 'd' + super().f() + +assert D().f() == 'dbca', "Mro resolution using super failed." + + + +class A(): + pass +try: + class B(A, A): + pass +except TypeError: + pass +else: + assert False, "Managed to create a class with duplicate base classes." + + +class A(): + pass +class B(A): + pass +try: + class C(A, B): + pass +except TypeError: + pass +else: + assert False, "Managed to create a class without local type precedence." diff --git a/vm/src/obj/objsuper.rs b/vm/src/obj/objsuper.rs index da9deeac0..08bb756d8 100644 --- a/vm/src/obj/objsuper.rs +++ b/vm/src/obj/objsuper.rs @@ -25,6 +25,7 @@ pub struct PySuper { obj: PyObjectRef, typ: PyObjectRef, obj_type: PyObjectRef, + mro: Vec, } impl PyValue for PySuper { @@ -55,20 +56,7 @@ impl PySuper { fn getattribute(&self, name: PyStringRef, vm: &VirtualMachine) -> PyResult { let inst = self.obj.clone(); - // Super should be the next matching class in the object original class' mro after the current one. - let mut mro_iter = match self.obj_type.payload::() { - Some(PyClass { ref mro, .. }) => mro.iter(), - _ => panic!("not Class"), - }; - // The type itself is not in its mro, so skip finding the current class if its the type. - if !self.typ.is(&self.obj_type) { - let index = mro_iter.find(|&x| x.as_object().is(&self.typ)); - if index.is_none() { - panic!("Current super type is not in instance's type mro"); - } - } - - for class in mro_iter { + for class in self.mro.iter() { if let Ok(item) = vm.get_attribute(class.as_object().clone(), name.clone()) { if item.payload_is::() { // This is a classmethod @@ -149,10 +137,22 @@ impl PySuper { py_obj.class() }; + // Super should be the next matching class in the object original class' mro after the current one. + let mut mro_iter = obj_type.mro.iter(); + // The type itself is not in its mro, so skip finding the current class if its the type. + if !py_type.is(&obj_type) { + let index = mro_iter.find(|&x| x.as_object().is(&py_type)); + if index.is_none() { + panic!("Current super type is not in instance's type mro"); + } + } + let mro: Vec = mro_iter.map(|c| c.clone()).collect(); + PySuper { obj: py_obj, typ: py_type.into_object(), obj_type: obj_type.into_object(), + mro, } .into_ref_with_type(vm, cls) } diff --git a/vm/src/obj/objtype.rs b/vm/src/obj/objtype.rs index e3bfc73cc..ad244fd44 100644 --- a/vm/src/obj/objtype.rs +++ b/vm/src/obj/objtype.rs @@ -313,16 +313,13 @@ impl PyClassRef { }; let attributes = dict.to_attributes(); - let typ = new( - metatype, - name.as_str(), - base.clone(), - bases, - attributes, - Some(vm), - )?; - typ.slots.borrow_mut().flags = base.slots.borrow().flags; - Ok(typ.into()) + match new(metatype, name.as_str(), base.clone(), bases, attributes) { + Ok(typ) => { + typ.slots.borrow_mut().flags = base.slots.borrow().flags; + Ok(typ.into()) + } + Err(string) => Err(vm.new_type_error(string)), + } } #[pyslot] @@ -457,29 +454,27 @@ impl PyClassRef { } } -fn take_next_base(mut bases: Vec>) -> Option<(PyClassRef, Vec>)> { - let mut next = None; - +fn take_next_base(mut bases: Vec>) -> (Option, Vec>) { bases = bases.into_iter().filter(|x| !x.is_empty()).collect(); for base in &bases { let head = base[0].clone(); if !(&bases).iter().any(|x| x[1..].iter().any(|x| x.is(&head))) { - next = Some(head); - break; + // Remove from other heads. + for item in &mut bases { + if item[0].is(&head) { + item.remove(0); + } + } + + return (Some(head), bases); } } - let head = next?; - for item in &mut bases { - if item[0].is(&head) { - item.remove(0); - } - } - Some((head, bases)) + (None, bases) } -fn linearise_mro(mut bases: Vec>) -> Option> { +fn linearise_mro(mut bases: Vec>) -> Result, String> { vm_trace!("Linearising MRO: {:?}", bases); // Python requires that the class direct bases are kept in the same order. // This is called local precedence ordering. @@ -492,7 +487,9 @@ fn linearise_mro(mut bases: Vec>) -> Option> { // We start at index 1 to skip direct bases. // This will not catch duplicate bases, but such a thing is already tested for. if later_mro[1..].iter().any(|cls| cls.is(base)) { - return None; + return Err( + "Unable to find mro order which keeps local precedence ordering".to_owned(), + ); } } } @@ -502,12 +499,20 @@ fn linearise_mro(mut bases: Vec>) -> Option> { if (&bases).iter().all(Vec::is_empty) { break; } - let (head, new_bases) = take_next_base(bases)?; + let (head, new_bases) = take_next_base(bases); + if head.is_none() { + // Take the head class of each class here. Now that we have reached the problematic bases. + // Because this failed, we assume the lists cannot be empty. + return Err(format!( + "Cannot create a consistent method resolution order (MRO) for bases {}", + new_bases.iter().map(|x| x.first().unwrap()).join(", ") + )); + } - result.push(head); + result.push(head.unwrap()); bases = new_bases; } - Some(result) + Ok(result) } pub fn new( @@ -516,31 +521,17 @@ pub fn new( _base: PyClassRef, bases: Vec, dict: HashMap, - optional_vm: Option<&VirtualMachine>, -) -> PyResult { +) -> Result { // Check for duplicates in bases. let mut unique_bases = HashSet::new(); for base in bases.iter() { if !unique_bases.insert(base.get_id()) { - let vm = optional_vm - .expect("Could not create a class made by internal code due to duplicate bases."); - return Err(vm.new_type_error(format!("duplicate base class {}", base.name))); + return Err(format!("duplicate base class {}", base.name)); } } let mros = bases.iter().map(|x| _mro(&x)).collect(); - let mro = linearise_mro(mros); - if mro.is_none() { - let vm = - optional_vm.expect("Could not create consistent MRO for class made by internal code."); - let mut error_string = bases.iter().join(", "); - error_string.insert_str( - 0, - "Cannot create a consistent method resolution order (MRO) for bases ", - ); - return Err(vm.new_type_error(error_string)); - } - let mro = mro.unwrap(); + let mro = linearise_mro(mros)?; let new_type = PyObject { payload: PyClass { name: String::from(name), @@ -644,11 +635,8 @@ mod tests { use super::{linearise_mro, new}; use super::{HashMap, IdProtocol, PyClassRef, PyContext}; - fn map_ids(obj: Option>) -> Option> { - match obj { - Some(vec) => Some(vec.into_iter().map(|x| x.get_id()).collect()), - None => None, - } + fn map_ids(obj: Result, String>) -> Result, String> { + Ok(obj?.into_iter().map(|x| x.get_id()).collect()) } #[test] @@ -663,7 +651,6 @@ mod tests { object.clone(), vec![object.clone()], HashMap::new(), - None, ) .unwrap(); let b = new( @@ -672,7 +659,6 @@ mod tests { object.clone(), vec![object.clone()], HashMap::new(), - None, ) .unwrap(); @@ -681,14 +667,14 @@ mod tests { vec![object.clone()], vec![object.clone()] ])), - map_ids(Some(vec![object.clone()])) + map_ids(Ok(vec![object.clone()])) ); assert_eq!( map_ids(linearise_mro(vec![ vec![a.clone(), object.clone()], vec![b.clone(), object.clone()], ])), - map_ids(Some(vec![a.clone(), b.clone(), object.clone()])) + map_ids(Ok(vec![a.clone(), b.clone(), object.clone()])) ); } } diff --git a/vm/src/types.rs b/vm/src/types.rs index b88d3ff0d..fc8caec05 100644 --- a/vm/src/types.rs +++ b/vm/src/types.rs @@ -244,10 +244,8 @@ pub fn create_type(name: &str, type_type: &PyClassRef, base: &PyClassRef) -> PyC base.clone(), vec![base.clone()], dict, - // We are assuming that anyone calling this internal function will not have errors in type creation so we don't need a VM instance. - None, ) - .unwrap() + .expect("Failed to create a new type in internal code.") } /// Paritally initialize a struct, ensuring that all fields are From cb2411444400de691bf1688a29658dcee9360515 Mon Sep 17 00:00:00 2001 From: Idan Dor Date: Sat, 22 Feb 2020 16:43:00 +0200 Subject: [PATCH 4/5] Fixed clippy suggestion. --- vm/src/obj/objsuper.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vm/src/obj/objsuper.rs b/vm/src/obj/objsuper.rs index 08bb756d8..81015cfbe 100644 --- a/vm/src/obj/objsuper.rs +++ b/vm/src/obj/objsuper.rs @@ -146,7 +146,7 @@ impl PySuper { panic!("Current super type is not in instance's type mro"); } } - let mro: Vec = mro_iter.map(|c| c.clone()).collect(); + let mro: Vec = mro_iter.cloned().collect(); PySuper { obj: py_obj, From 6ea5d58f2428b1049d811b83f25d032eee4f2bda Mon Sep 17 00:00:00 2001 From: Idan Dor Date: Sat, 29 Feb 2020 16:34:14 +0200 Subject: [PATCH 5/5] Fix previous merge of `objtype.rs`. Moved the tests to their more correct place of `tests/snippets/class.py`. --- tests/snippets/class.py | 46 +++++++++++++++++++++++++++++++++ tests/snippets/types_snippet.py | 44 ------------------------------- vm/src/obj/objtype.rs | 12 +++------ 3 files changed, 50 insertions(+), 52 deletions(-) diff --git a/tests/snippets/class.py b/tests/snippets/class.py index bea71f965..4623d5831 100644 --- a/tests/snippets/class.py +++ b/tests/snippets/class.py @@ -207,3 +207,49 @@ assert B.__doc__ == "Docstring" # assert a == 3 # A.b() # nested_scope() + + + +# Multiple inheritance and mro tests. +class A(): + def f(self): + return 'a' + +class B(A): + def f(self): + return 'b' + super().f() + +class C(A): + def f(self): + return 'c' + super().f() + +class D(B, C): + def f(self): + return 'd' + super().f() + +assert D().f() == 'dbca', "Mro resolution using super failed." + + + +class A(): + pass +try: + class B(A, A): + pass +except TypeError: + pass +else: + assert False, "Managed to create a class with duplicate base classes." + + +class A(): + pass +class B(A): + pass +try: + class C(A, B): + pass +except TypeError: + pass +else: + assert False, "Managed to create a class without local type precedence." diff --git a/tests/snippets/types_snippet.py b/tests/snippets/types_snippet.py index e06862c19..417916767 100644 --- a/tests/snippets/types_snippet.py +++ b/tests/snippets/types_snippet.py @@ -99,47 +99,3 @@ assert C.mro() == [C, B, A, BB, AA, object] assert type(Exception.args).__name__ == 'getset_descriptor' assert type(None).__bool__(None) is False - - -class A(): - def f(self): - return 'a' - -class B(A): - def f(self): - return 'b' + super().f() - -class C(A): - def f(self): - return 'c' + super().f() - -class D(B, C): - def f(self): - return 'd' + super().f() - -assert D().f() == 'dbca', "Mro resolution using super failed." - - - -class A(): - pass -try: - class B(A, A): - pass -except TypeError: - pass -else: - assert False, "Managed to create a class with duplicate base classes." - - -class A(): - pass -class B(A): - pass -try: - class C(A, B): - pass -except TypeError: - pass -else: - assert False, "Managed to create a class without local type precedence." diff --git a/vm/src/obj/objtype.rs b/vm/src/obj/objtype.rs index 82287ef52..49a0711c1 100644 --- a/vm/src/obj/objtype.rs +++ b/vm/src/obj/objtype.rs @@ -291,16 +291,14 @@ impl PyClassRef { } } - let typ = match new(metatype, name.as_str(), base.clone(), bases, attributes) { + match new(metatype, name.as_str(), base.clone(), bases, attributes) { Ok(typ) => { typ.slots.borrow_mut().flags = base.slots.borrow().flags; + vm.ctx.add_tp_new_wrapper(&typ); Ok(typ.into()) } Err(string) => Err(vm.new_type_error(string)), - }?; - typ.slots.borrow_mut().flags = base.slots.borrow().flags; - vm.ctx.add_tp_new_wrapper(&typ); - Ok(typ.into()) + } } #[pyslot] @@ -437,9 +435,7 @@ impl PyClassRef { } } -fn take_next_base(mut bases: Vec>) -> Option<(PyClassRef, Vec>)> { - let mut next = None; - +fn take_next_base(mut bases: Vec>) -> (Option, Vec>) { bases = bases.into_iter().filter(|x| !x.is_empty()).collect(); for base in &bases {