Skip to content
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.

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

Add type_check to _check() #5095

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

dannasman
Copy link
Contributor

Hi!

After implementing type_check (#5091) I tried to add the type_check call in _check function by following the CPython implementation. I also modified descr_get function of PyGetSet based on its CPython counterpart. These modifications cause the code to panic and I am not quite sure why. Any thoughts?

@dannasman dannasman mentioned this pull request Oct 19, 2023
@youknowone
Copy link
Member

youknowone commented Oct 20, 2023

Welcome to the common RustPython development rabbit hole. Well, I only have general debug advices. It can be either a bug here or combination of other incompatabilty issues there - because getset is shown in error message, I suspect our incomplete getset_descriptor implementation may be releated.
General idea is breaking it down enough and see how it is going. When an exception is raised during initialization(VirtualMachine::initialize) step, you will want to set breakpoint and add some print_exception or other debugging helpers in VirtualMachine::initialize function.

I think the trial was really good.

I am currently on vacation. I can try a quick look, but not able to look deep down stuff for a while. (To be honest, usually not very helpful for these kind of problems even if I do that)

@dannasman
Copy link
Contributor Author

Alright, I'll keep digging deeper. Have a nice vacation!

@youknowone
Copy link
Member

@dannasman Do you have a specific goal to achieve? Otherwise looking around similar issues may be helpful. I also have many abandoned patches when I got stucked https://github.com/RustPython/RustPython/pulls/youknowone

@dannasman
Copy link
Contributor Author

dannasman commented Oct 22, 2023

I was searching for places where PyObject_TypeCheck should be used in the code and found the _check() function. The original goal was to add type_check into it just like in CPython. Further investigation showed that the implementations between RustPython and CPython have some differences when it comes to descriptors. To my current knowledge In RustPython generic_getattr_opt() function descriptor objects seem to always get a type getset_descriptor whereas in corresponding CPython function _PyObject_GenericGetAttrWithDict the descriptor objects are assigned the same type (or subtype) as the self object (and this is later checked with PyObject_TypeCheck to make sure this is the case). So it might be the case that _check() does not actually need type_check() since the type of descr does not depend on the type of zelf.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants