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/vm/src/obj/objtype.rs b/vm/src/obj/objtype.rs index 1b3962ff0..49a0711c1 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; @@ -16,6 +16,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 @@ -290,10 +291,14 @@ impl PyClassRef { } } - let typ = new(metatype, name.as_str(), base.clone(), bases, attributes)?; - typ.slots.borrow_mut().flags = base.slots.borrow().flags; - vm.ctx.add_tp_new_wrapper(&typ); - Ok(typ.into()) + 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)), + } } #[pyslot] @@ -430,43 +435,65 @@ 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); } } - if let Some(head) = next { - for item in &mut bases { - if item[0].is(&head) { - item.remove(0); - } - } - return Some((head, bases)); - } - None + (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. + // 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 Err( + "Unable to find mro order which keeps local precedence ordering".to_owned(), + ); + } + } + } + let mut result = vec![]; loop { 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( @@ -475,12 +502,20 @@ pub fn new( _base: PyClassRef, bases: Vec, dict: HashMap, -) -> 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()) { + return Err(format!("duplicate base class {}", base.name)); + } + } + let mros = bases .iter() .map(|x| x.iter_mro().cloned().collect()) .collect(); - let mro = linearise_mro(mros).unwrap(); + let mro = linearise_mro(mros)?; let new_type = PyObject { payload: PyClass { name: String::from(name), @@ -584,11 +619,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] @@ -619,14 +651,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 a81755a01..fc8caec05 100644 --- a/vm/src/types.rs +++ b/vm/src/types.rs @@ -245,7 +245,7 @@ pub fn create_type(name: &str, type_type: &PyClassRef, base: &PyClassRef) -> PyC vec![base.clone()], dict, ) - .unwrap() + .expect("Failed to create a new type in internal code.") } /// Paritally initialize a struct, ensuring that all fields are