Skip to content

Fix test_bool test_subclass#4715

Closed
minhrongcon2000 wants to merge 2 commits intoRustPython:mainfrom
minhrongcon2000:fix/not_allow_init_int
Closed

Fix test_bool test_subclass#4715
minhrongcon2000 wants to merge 2 commits intoRustPython:mainfrom
minhrongcon2000:fix/not_allow_init_int

Conversation

@minhrongcon2000
Copy link
Contributor

@minhrongcon2000 minhrongcon2000 commented Mar 18, 2023 •
edited
Loading

Fix unsafe __new__ for int function

@youknowone youknowone force-pushed the fix/not_allow_init_int branch from 2b03849 to 9ede5fd Compare March 21, 2023 05:41
youknowone
youknowone reviewed Mar 21, 2023
View reviewed changes
Copy link
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

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

Thank you for contributing, but this fix doesn't seem to correct implementation.

Could you check corresponding cpython implementation?
https://github.com/python/cpython/blob/main/Objects/typeobject.c#L7694-L7701

@minhrongcon2000
Copy link
Contributor Author

minhrongcon2000 commented Mar 21, 2023

Sure! I will check it when I have time :D

@minhrongcon2000
Copy link
Contributor Author

minhrongcon2000 commented Mar 23, 2023 •
edited
Loading

@youknowone The block of code that you show me in the previous comment is to check whether cls is a subtype of the current class that called __new__. In CPython, however, bool is a subtype of int and in CPython, int is not allowed to call __new__ with subtype as well. See https://github.com/python/cpython/blob/87be8d95228ee95de9045cf2952311d20dc5de45/Objects/typeobject.c#L7711-L7718

I will fix it in the next commit

@youknowone
Copy link
Member

youknowone commented Mar 23, 2023

Oh, i am sorry. I left wrong link

@Snowapril
Copy link
Contributor

Snowapril commented Mar 24, 2023

Oh .... It seems duplicate PR exist here #4455 😭 (cc. @youknowone )

@DimitrisJim
Copy link
Member

DimitrisJim commented Mar 25, 2023

yeah and that PR goes about things in the approach CPython has by modifying type.__new__ to catch this.

@minhrongcon2000
Copy link
Contributor Author

minhrongcon2000 commented Mar 25, 2023

Oh! So I will close this PR now :D

@DimitrisJim
Copy link
Member

DimitrisJim commented Mar 25, 2023

we should just coordinate with garychia and see if they still want to work on it, I'll leave a comment there to check. If they don't have time, you can take inspiration from their approach 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@youknowone youknowone youknowone left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@minhrongcon2000 @youknowone @Snowapril @DimitrisJim