Refactor Mapping Protocol and Item Protocol #3462
Conversation
|
Oh yes. |
b53efcc to
e06afd2
Compare
|
@Snowapril could you review this PR? |
vm/src/protocol/mapping.rs
Outdated
| self.methods(vm).subscript.is_some() | ||
| } | ||
|
|
||
| pub fn obj_as<T: PyObjectPayload>(&self) -> &PyObjectView<T> { |
There was a problem hiding this comment.
Is this safe function? It looks like to take random T but in that case it will not be always safe.
There was a problem hiding this comment.
This helper function is used to downcast the object to the type who provide the methods and should not used by any case if the type is not certain. maybe add the comment will be better than mark it unsafe because it is used in each methods to get the instance of Self?
There was a problem hiding this comment.
That sounds like this method is strictly required to be call like obj_as::<Self> always. In that case, the function is unsafe beacuse the guarantee of Self came from user, not from automatic constraints.
If you want to have a safe version of the function, I think a trait can wrap it safely. Maybe AsMapping can be the place.
trait AsMapping {
fn mapping_as(mapping: ...) -> &PyObjectView<Self> {
unsafe { mapping.obj_as::<Self>() }
}
...
}| pub fn from_dict_exact(dict: PyDictRef) -> Self { | ||
| Self { | ||
| obj: dict.into(), | ||
| mapping_methods: PyDict::MAPPING_METHODS, | ||
| } | ||
| } |
There was a problem hiding this comment.
is PyDictRef means exact PyDict not the subclass?
There was a problem hiding this comment.
oh, i am sorry. exact meant that exact.
|
does PyMapping and ArgMapping separated to keep PyObjectRef in ArgMapping? |
PyMapping do not take the ownership, it bind the lifetime to &PyObject. ArgMapping keeps PyObjectRef and the cached methods to avoid mro search. |
vm/src/protocol/mapping.rs
Outdated
| pub fn has_protocol(&self, vm: &VirtualMachine) -> bool { | ||
| self.methods(vm).subscript.is_some() | ||
| } |
There was a problem hiding this comment.
How about just using check? PyIter also adopt check for PyIter::Check
There was a problem hiding this comment.
make sense, but I think I'd like to keep fn try_protocol() as it is more rust style.
vm/src/protocol/object.rs
Outdated
| if let Ok(mapping) = PyMapping::try_protocol(self, vm) { | ||
| mapping.ass_subscript(&needle, Some(value), vm) |
There was a problem hiding this comment.
According to cpython code, they check ass_subscript exist then call it, otherwise pass. For now, It seems we just check only mapping & subscript and call ass_subscript
vm/src/protocol/object.rs
Outdated
| if let Ok(mapping) = PyMapping::try_protocol(self, vm) { | ||
| mapping.ass_subscript(&needle, None, vm) | ||
| } else { |
There was a problem hiding this comment.
Same with set_item. We must check ass_subscript exist before calling it
#3316