Skip to content

Fix best_base to select proper base class #5324

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
merged 7 commits into from
May 19, 2024
Merged

Fix best_base to select proper base class #5324

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ac10798
add __basicsize__ getter
Snowapril May 12, 2024
792ba47
modify best_base function to get proper base
Snowapril May 12, 2024
a6accc1
modify best_base to select proper base
Snowapril May 15, 2024
ffa8950
remove expected failures
Snowapril May 19, 2024
c1fe1f9
fix clippy warning
Snowapril May 18, 2024
abf8b13
inherit basicsize from the base type
Snowapril May 18, 2024
d679165
run cargo fmt
Snowapril May 18, 2024
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
4 changes: 0 additions & 4 deletions Lib/test/test_builtin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2368,8 +2368,6 @@ def __del__(self):


class TestType(unittest.TestCase):
# TODO: RUSTPYTHON
@unittest.expectedFailure
def test_new_type(self):
A = type('A', (), {})
self.assertEqual(A.__name__, 'A')
Expand Down Expand Up @@ -2472,8 +2470,6 @@ def test_type_doc(self):
A.__doc__ = doc
self.assertEqual(A.__doc__, doc)

# TODO: RUSTPYTHON
@unittest.expectedFailure
def test_bad_args(self):
with self.assertRaises(TypeError):
type()
Expand Down
4 changes: 0 additions & 4 deletions Lib/test/test_descr.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -823,8 +823,6 @@ class X(C, int()):
class X(int(), C):
pass

# TODO: RUSTPYTHON
@unittest.expectedFailure
def test_module_subclasses(self):
# Testing Python subclass of module...
log = []
Expand Down Expand Up @@ -1500,8 +1498,6 @@ class someclass(metaclass=dynamicmetaclass):
pass
self.assertNotEqual(someclass, object)

# TODO: RUSTPYTHON
@unittest.expectedFailure
def test_errors(self):
# Testing errors...
try:
Expand Down
2 changes: 0 additions & 2 deletions Lib/test/test_posix.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1680,8 +1680,6 @@ def test_initgroups(self):
posix.initgroups(name, g)
self.assertIn(g, posix.getgroups())

# TODO: RUSTPYTHON: TypeError: Unexpected keyword argument setpgroup
@unittest.expectedFailure
@unittest.skipUnless(hasattr(posix, 'setgroups'),
"test needs posix.setgroups()")
def test_setgroups(self):
Expand Down
81 changes: 48 additions & 33 deletions vm/src/builtins/type.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,9 @@ impl PyType {
if base.slots.flags.has_feature(PyTypeFlags::HAS_DICT) {
slots.flags |= PyTypeFlags::HAS_DICT
}
if slots.basicsize == 0 {
slots.basicsize = base.slots.basicsize;
}

if let Some(qualname) = attrs.get(identifier!(ctx, __qualname__)) {
if !qualname.fast_isinstance(ctx.types.str_type) {
Expand Down Expand Up @@ -253,6 +256,9 @@ impl PyType {
if base.slots.flags.has_feature(PyTypeFlags::HAS_DICT) {
slots.flags |= PyTypeFlags::HAS_DICT
}
if slots.basicsize == 0 {
slots.basicsize = base.slots.basicsize;
}

let bases = vec![base.clone()];
let mro = base.iter_mro().map(|x| x.to_owned()).collect();
Expand Down Expand Up @@ -470,6 +476,11 @@ impl PyType {
self.slots.flags.bits()
}

#[pygetset(magic)]
fn basicsize(&self) -> usize {
self.slots.basicsize
}

#[pygetset]
pub fn __name__(&self, vm: &VirtualMachine) -> PyStrRef {
self.name_inner(
Expand Down Expand Up @@ -1313,22 +1324,29 @@ fn calculate_meta_class(
Ok(winner)
}

fn solid_base(typ: &PyTypeRef, vm: &VirtualMachine) -> PyTypeRef {
let base = if let Some(base) = &typ.base {
solid_base(base, vm)
} else {
vm.ctx.types.object_type.to_owned()
};

// TODO: itemsize comparation also needed
if typ.basicsize() != base.basicsize() {
typ.clone()
} else {
base
}
}

fn best_base(bases: &[PyTypeRef], vm: &VirtualMachine) -> PyResult<PyTypeRef> {
// let mut base = None;
// let mut winner = None;
let mut base: Option<PyTypeRef> = None;
let mut winner: Option<PyTypeRef> = None;

for base_i in bases {
// base_proto = PyTuple_GET_ITEM(bases, i);
// if (!PyType_Check(base_proto)) {
// PyErr_SetString(
// PyExc_TypeError,
// "bases must be types");
// return NULL;
// }
// base_i = (PyTypeObject *)base_proto;
// if (base_i->slot_dict == NULL) {
// if (PyType_Ready(base_i) < 0)
// return NULL;
// if !base_i.fast_issubclass(vm.ctx.types.type_type) {
// println!("base_i type : {}", base_i.name());
// return Err(vm.new_type_error("best must be types".into()));
// }

if !base_i.slots.flags.has_feature(PyTypeFlags::BASETYPE) {
Expand All @@ -1337,28 +1355,25 @@ fn best_base(bases: &[PyTypeRef], vm: &VirtualMachine) -> PyResult<PyTypeRef> {
base_i.name()
)));
}
// candidate = solid_base(base_i);
// if (winner == NULL) {
// winner = candidate;
// base = base_i;
// }
// else if (PyType_IsSubtype(winner, candidate))
// ;
// else if (PyType_IsSubtype(candidate, winner)) {
// winner = candidate;
// base = base_i;
// }
// else {
// PyErr_SetString(
// PyExc_TypeError,
// "multiple bases have "
// "instance lay-out conflict");
// return NULL;
// }

let candidate = solid_base(base_i, vm);
if winner.is_none() {
winner = Some(candidate.clone());
base = Some(base_i.clone());
} else if winner.as_ref().unwrap().fast_issubclass(&candidate) {
// Do nothing
} else if candidate.fast_issubclass(winner.as_ref().unwrap()) {
winner = Some(candidate.clone());
base = Some(base_i.clone());
} else {
return Err(
vm.new_type_error("multiple bases have instance layout conflict".to_string())
);
}
}

// FIXME: Ok(base.unwrap()) is expected
Ok(bases[0].clone())
debug_assert!(base.is_some());
Ok(base.unwrap())
Comment on lines +1375 to +1376
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unwrap implies panic!. So debug_assert! is duplication.

Suggested change
debug_assert!(base.is_some());
Ok(base.unwrap())
Ok(base.expect("best_base must exist"))

}

#[cfg(test)]
Expand Down
Loading