From 1e985e26da38669a86755fb4e42b01958eea6508 Mon Sep 17 00:00:00 2001 From: Idan Dor Date: Fri, 14 Feb 2020 15:08:47 +0200 Subject: [PATCH] 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() }