Repository navigation
Fix metaclass check classcell - #3986
Conversation
0e7bc4d to
cd0c9d0
Compare
|
Unless we locate the code for setting type to classcell in |
79c9756 to
e4c55a5
Compare
7f0301e to
6c85150
Compare
| let __classcell__ = identifier!(vm, __classcell__); | ||
|
|
||
| // It is correct to get it from tp_dict, not attributes. | ||
| let cell = typ | ||
| .attributes | ||
| .write() | ||
| .get(__classcell__) | ||
| .map(|cell| PyCellRef::try_from_object(vm, cell.clone())); | ||
|
|
||
| if let Some(Ok(cell)) = cell { | ||
| cell.set(Some(typ.clone().to_pyobject(vm))); | ||
| } | ||
|
|
There was a problem hiding this comment.
| let __classcell__ = identifier!(vm, __classcell__); | |
| // It is correct to get it from tp_dict, not attributes. | |
| let cell = typ | |
| .attributes | |
| .write() | |
| .get(__classcell__) | |
| .map(|cell| PyCellRef::try_from_object(vm, cell.clone())); | |
| if let Some(Ok(cell)) = cell { | |
| cell.set(Some(typ.clone().to_pyobject(vm))); | |
| } | |
| typ.attributes | |
| .write() | |
| .get(identifier!(vm, __classcell__)) | |
| .map(|cell| { | |
| if let Some(cell) = PyCellRef::try_from_object(vm, cell.clone()).ok() { | |
| cell.set(Some(typ.clone().to_pyobject(vm))); | |
| Ok(()) | |
| } else { | |
| Err(vm.new_type_error(format!( | |
| "__classcell__ must be a nonlocal cell, not {}", | |
| cell.class() | |
| ))) | |
| } | |
| }) | |
| .transpose()?; | |
It seems that we miss to raise type error when the given __classcell__ is actually not a cell type
There was a problem hiding this comment.
I think previous version is better. cell.set has side effect and the result of map is not used. This is more like control flow than data stream. Using if-let will show the flow better and will not hide side-effect of cell.set.
There was a problem hiding this comment.
Oh I agree. Then we only need to add type error handling
Snowapril
left a comment
Snowapril
left a comment
There was a problem hiding this comment.
BTW, without that change request, looks great !! 😊
c631b7c to
d1e8fdb
Compare
| cell.class().name() | ||
| )) | ||
| })?; | ||
| cell.set(Some(typ.clone().to_pyobject(vm))); |
There was a problem hiding this comment.
This line has side-effect and the closure doesn't return anything. Using if-let statement would be better than map
youknowone
left a comment
youknowone
left a comment
There was a problem hiding this comment.
The logic looks good, but I changed error-handling style in edb4a96
PyObject is not expected to be used as Display object because it generates not very helpful error messages.
This was on TODO list for long time but forgotten. Once #4045 is merged, please rebase this PR and fix error formatting arguments.
Thank you for fixing this hair-pulling problem! Now I am try to getting how it is working.
__build_class__check__class__celltype.__new__set classcellWhen a class object is created with metaclass, the following operation is performed.
https://docs.python.org/3/reference/datamodel.html?highlight=classcell#:~:text=CPython implementation detail%3A In,a RuntimeError in Python 3.8.
CPython implementation detail: In CPython 3.6 and later, the
__class__cell is passed to the metaclass as a__classcell__entry in the class namespace. If present, this must be propagated up to thetype.__new__call in order for the class to be initialised correctly. Failing to do so will result in a [RuntimeError] in Python 3.8.__class__(the class it belongs to) and metaclass are different (bltinmodule.c)type.__new__(typeobject.c)But in Rust Python it sets classcell without comparing
__class__.