Skip to content

gh-122982: Prohibit bitwise inversion on bool type #122983

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

Closed
wants to merge 12 commits into from

Conversation

Eclips4
Copy link
Member

@Eclips4 Eclips4 commented Aug 13, 2024

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thanks!

@JelleZijlstra JelleZijlstra removed their request for review August 13, 2024 20:31
@Eclips4 Eclips4 requested review from AA-Turner and picnixz August 14, 2024 09:44
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Apart from Nikita's comment, this looks good to me now (TIL I cannot do class A(bool): ...).

Copy link
Member

@encukou encukou left a comment

Choose a reason for hiding this comment

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

If you do this, you should also want to edit Doc/deprecations/pending-removal-in-future.rst.
But note that it's not in Doc/deprecations/pending-removal-in-3.14.rst; removing this feature now would be a surprise for some.

@bedevere-app
Copy link

bedevere-app bot commented Aug 14, 2024

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@Eclips4
Copy link
Member Author

Eclips4 commented Aug 14, 2024

I have made the requested changes; please review again

@bedevere-app
Copy link

bedevere-app bot commented Aug 14, 2024

Thanks for making the requested changes!

@encukou, @sobolevn: please review the changes made to this pull request.

@bedevere-app bedevere-app bot requested a review from encukou August 14, 2024 18:59
@bedevere-app bedevere-app bot requested a review from sobolevn August 14, 2024 18:59
@@ -11,7 +11,6 @@ although there is currently no date scheduled for their removal.

* :mod:`builtins`:

* ``~bool``, bitwise inversion on bool.
Copy link
Member

@AA-Turner AA-Turner Aug 14, 2024

Choose a reason for hiding this comment

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

If this gets merged, you should open a new PR to move this note to the 3.14 pending removals & backport that.

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Just wondering: what if simply set PyBool_Type.tp_as_number->nb_invert = NULL in _PyTypes_InitTypes()?

@Eclips4
Copy link
Member Author

Eclips4 commented Aug 15, 2024

Just wondering: what if simply set PyBool_Type.tp_as_number->nb_invert = NULL in _PyTypes_InitTypes()?

Yeah, this works. Do you think it's worth doing or should we continue with the current approach?

@serhiy-storchaka
Copy link
Member

I do not know. How does True.__inv__() work in this case?

On one hand, using the same code to generate error messages guarantees that there is no difference in error messages. On other hand, we may want to have a different error message, for example add a suggestion about not.

@Eclips4
Copy link
Member Author

Eclips4 commented Aug 16, 2024

I do not know. How does True.__inv__() work in this case?

bool.__invert__ would work in this case:

Python 3.14.0a0 (heads/remove_bool_invert-dirty:4760de8ecbb, Aug 16 2024, 09:24:28) [Clang 15.0.0 (clang-1500.3.9.4)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> True.__invert__()
-2

So, this is a wrong approach.

On other hand, we may want to have a different error message, for example add a suggestion about not.

That's sounds good. I'll add a suggestion and tests for it.

@encukou encukou dismissed their stale review August 16, 2024 07:11

pending-removal-in-future.rst is fixed

Copy link
Member

@sobolevn sobolevn 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! I don't click merge, because @encukou's comment here: #122983 (review) seems like it is better to wait for resolution.

Copy link
Member

@AA-Turner AA-Turner left a comment

Choose a reason for hiding this comment

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

Wordsmithing: We seem to use "Maybe you meant" for SyntaxErrors, with "Did you mean" used more generally. Non-blocking suggestion though.

A

Copy link
Contributor

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

I mentioned this on the issue, but I think in this case we should extend the deprecation period beyond the minimum #122982 (comment)

return PyLong_Type.tp_as_number->nb_invert(v);
PyErr_SetString(
PyExc_TypeError,
"bad operand type for unary ~: 'bool'. Did you mean 'not' instead of '~'?"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the text in the warning was helpful, especially since the previous behaviour has been around forever.

Maybe something like:

bad operand type for unary ~: 'bool'. Did you mean 'not' instead of '~'? Use 'not x' for boolean negation and '~int(x)` in the rare case you want bitwise inversion of the underlying int"

@Eclips4
Copy link
Member Author

Eclips4 commented Aug 25, 2024

Closing per #122982 (comment)

@Eclips4 Eclips4 closed this Aug 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants