Skip to content

Add type_check to _check()#5095

Draft
dannasman wants to merge 1 commit into
RustPython:mainfrom
dannasman:getdescr_check
Draft

Add type_check to _check()#5095
dannasman wants to merge 1 commit into
RustPython:mainfrom
dannasman:getdescr_check

Conversation

@dannasman

Copy link
Copy Markdown
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

youknowone commented Oct 20, 2023

Copy link
Copy Markdown
Member

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
Copy Markdown
Contributor Author

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

@youknowone

Copy link
Copy Markdown
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

dannasman commented Oct 22, 2023

Copy link
Copy Markdown
Contributor Author

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