-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
gh-82012: Deprecate bitwise inversion (~) of bool #103487
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
Conversation
b94d85b
to
20dec73
Compare
Please document this in What's New at https://docs.python.org/3.12/whatsnew/3.12.html#deprecated And is this operator documented in the main reference? Let's also mention the deprecation there. Is the plan to turn this into an error in 3.14, or keep it open ended? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Let's just merge this.
We should probably add a comment about this to https://github.com/python/cpython/blob/main/Doc/whatsnew/3.12.rst. |
Yes! |
8290ea7
to
bf02fd4
Compare
I've added the deprecation notice to whatsnew.
I suggest to turn this into an error (and have indicated so in the whatsnew). There is a real danger that some users write |
self.assertEqual(~True, -2) | ||
with self.assertWarns(DeprecationWarning): | ||
# We need to put the bool in a variable, because the constant | ||
# ~False is evaluated at compile time due to constant folding; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should test this behavior separately (doing something like with assertWarns(DeprecationWarning): exec("~False")
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean instead of or in addition to the current test? We originally had eval("~True")
but I figured it's clearer to have the warning context only around the operation and not around a comparably complex eval()
or exec()
. I'm happy to change if there's a benefit of these though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have both. The existing tests check that the deprecation warning is emitted correctly at runtime. The new tests would ensure that if the operation occurs at compile time, we still emit the DeprecationWarning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new behavior should also be mentioned in https://docs.python.org/3.12/library/stdtypes.html#boolean-values
It currently says "In numeric contexts (for example when used as the argument to an arithmetic operator), they behave like the integers 0 and 1, respectively.", which will no longer be true in all contexts.
3ac9b82
to
a6e3366
Compare
I suggest to not document this as it would become more confusing than helpful: In my view, there should be no reason to use bitwise operators on In particular, if we document
But maybe you have better ideas what to document. |
d599825
to
edd07f2
Compare
The bitwise inversion operator on bool returns the bitwise inversion of the underlying int value; i.e. `~True == -2` such that `bool(~True) == True`. It's a common pitfall that users mistake `~` as negation operator and actually want `not`. Supporting `~` is an artifact of bool inheriting from int. Since there is no real use-case for the current behavior, let's deprecate `~` on bool and later raise an error. This removes a potential source errors for users. Full reasoning: python#82012 (comment) Co-authored-by: Jelle Zijlstra <[email protected]>
edd07f2
to
1f0351a
Compare
Whether we like it or not, these operators have worked this way for a long time in Python, and backwards compatibility alone means we can't just make them go away. So it's better to document clearly how they work and what is changing.
I think the whole reason that we're deprecating only the
Your wording sounds good to me! |
This also pulls the bool type to top-level of the type description page. Before it was only documented in the section "Other Built-in Types / Boolean Values".
@JelleZijlstra thanks for the feedback. I've rewritten and restructured the bool docs a bit so that it's hopefully more clear and still precise. Please check if this is ok. - It's in a seprate commit, so could be easily modified/reverted. In particular, I've created a top-level section "Boolean Type - bool", which does the importance of the type more justice. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but I'd like to have another core dev look at the docs before we merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd prefer to remove the Boolean Values
section. We can preserve the .. _bltin-boolean-values:
and index
roles, which should hopefully preserve links.
You mean moving them to the new "Boolean Type" section? |
Yup! |
HTML links to the section cannot be preseved the URL was https://docs.python.org/3.12/library/stdtypes.html#boolean-values and the anchor I've rewritten internal references (because the label |
Co-authored-by: Shantanu <[email protected]>
Co-authored-by: Shantanu <[email protected]>
Is this waiting for anyone? |
Not anymore. Thanks all! |
@@ -5394,27 +5427,6 @@ information. There is exactly one ``NotImplemented`` object. | |||
It is written as ``NotImplemented``. | |||
|
|||
|
|||
.. _bltin-boolean-values: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello. The removal of this ref
tag broke intersphinx downstream. I have not tracked down yet which middleware is calling this tag when API docstring has a "bool" mentioned. I suspect it is numpydoc. Is it not possible to reuse this tag for your new section above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you open a PR, I can backport it to the 3.12 branch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Please see #110371 . Thank you for your consideration!
IMHO deprecating the |
Unfortunately, this is not how
The other bitwise operations happen to behave like the logical variants, e.g.
As above, if you have used |
It is not quite true that "If you have used ~ as a logical negation, your code is broken." Using bitwise operations has some advantages over logical operations, e.g., in the context of MyHDL package, which is for hardware design. Not matter how many bits are in signals, we can write the same expression. We just need to mask out unnecessary bits in the end.
Edit: I did more testing. It seems the above code actually works. I only saw warnings on (single) bit indexing. |
Agreed. But to have and advantage of bitwise, your data should have multiple bits, i.e. the underlying data type should be In that sense, inputs to your examples should be ints. Remark: While I agree that it would be nice if |
I have been unavailable this Sunday, so I will respond chronologically
So it is basically flawed? (rhetorical question ...) A
Oh, we can get away with this ... In MyHDL (which is 100% Python) this: a = Signal(intbv(0)[2:])
y.next = (~a[1] & a[0]) | (a[1] & ~a[0])
Above code simulates (== running the actual code as it is Python) returning correct results - so it is not broken, but we will have to fix it ... For the record: above codes comes from @zhijieshi; I always use @zhijieshi y.next = ~r But MyHDL will throw a ValueError as -1 nor -2 will fit into the destination; be it an
You are defending the choice of deprecating the
With I took a look into the GitHub repo: /* Arithmetic operations redefined to return bool if both args are bool. */
static PyObject *
bool_invert(PyObject *v)
{
if (PyErr_WarnEx(PyExc_DeprecationWarning,
"Bitwise inversion '~' on bool is deprecated. This "
"returns the bitwise inversion of the underlying int "
"object and is usually not what you expect from negating "
"a bool. Use the 'not' operator for boolean negation or "
"~int(x) if you really want the bitwise inversion of the "
"underlying int.",
1) < 0) {
return NULL;
}
return PyLong_Type.tp_as_number->nb_invert(v);
}
static PyObject *
bool_and(PyObject *a, PyObject *b)
{
if (!PyBool_Check(a) || !PyBool_Check(b))
return PyLong_Type.tp_as_number->nb_and(a, b);
return PyBool_FromLong((a == Py_True) & (b == Py_True));
}
static PyObject *
bool_or(PyObject *a, PyObject *b)
{
if (!PyBool_Check(a) || !PyBool_Check(b))
return PyLong_Type.tp_as_number->nb_or(a, b);
return PyBool_FromLong((a == Py_True) | (b == Py_True));
}
static PyObject *
bool_xor(PyObject *a, PyObject *b)
{
if (!PyBool_Check(a) || !PyBool_Check(b))
return PyLong_Type.tp_as_number->nb_xor(a, b);
return PyBool_FromLong((a == Py_True) ^ (b == Py_True));
} You can fix it by: static PyObject *
bool_invert(PyObject *v)
{
return bool_xor(v, Py_True);
} (I am aware it may not be as simple as that, but I guess I am pretty close ...) >>> ~False & True
1 I would have expected to see This fix will break far less code than the deprecation, if any at all; certainly not @zhijieshi 's |
@josyb thanks for your feedback. Allow me to answer on a higher level. I think we're drifting too much into details. Yes, the current implementation is flawed. (*)
Good job @zhijieshi, but I'm not that optimistic. It's easy to write Starting from the current bool implementation, we have three options:
All three options have their downsides which have been discussed in #82012: (1) is prone to misuse and can lead to unnoticted incorrect results; (2) has conceptual arguments related to expected behavior as a So there are trade offs to be made. I'm unclear whether your case makes a difference in the conclusion. That's beyond my level and I leave that to the core devs. (*) Theoretical background: The issues we see are caused by a variant of the circle-ellipse problem. The base class |
output_1bit = (a & ~b) & 1
output_4bits = (x & ~y) & 0xF There should be no need to mask out the unused bits; @timhoffm
I checked several examples of that list. It looks to me that they all test for the actual variable being 0. They get away by using the if s.some_condition:
# a comment why there is nothing to do here (which incidentally also documents what is done in the other case)
pass
else:
do_this() in stead of: if not s.some_condition:
do_this()
Same can be said for 2. -> you could do something fundamental: remove the Let's try to wind down. |
The majority of the above cases is likely valid and not affected: Note that e.g. numpy provides its own
It's not quite the same. Very few people read release notes and quite some people ignore warnings. So an announced change may well slip through unnoticed. While ignoring warnings is a flaw, we try to also care for the less able/experienced users. Eventually breaking code is the ultimate way to force them to take notice. - In that sense, removing functionality is safer than changing behavior.
Not necessarily. There might be other ways around. As the author of the deprecation, I'd be willing to take a look at that, if you point me to two or three representative examples that now give warnings. But that's getting off-topic here. Let's do that in myhdl/myhdl#429. |
I meant issue a Functionality Change Warning like the Deprecation Warning not just adding it to the release notes.
I read the mail before going to bed and between sleeping and waking found a possible way out: as in MyHDL single hardware connections are declared as either As you indicate we can close here. Best regards, P.S. I started checking MyHDL against 3.12, and get quite a few deprecation warnings ... |
You've broken indexing. I've just been bitten by this change. I sometimes want to get the first or second value of a list based on a Boolean condition. Naturally, I use And I sometimes want to index from the end. Python supports negative indexes, so naturally I use Now I needed both, so naturally I used |
Thanks for the feedback. I‘m sorry this affects your code. I regard the inferred |
I have two things I would like to say. Firstly, from reading the comments here, I feel like very little consideration was taken into people that actually do make use of the Secondly, to anyone whose code broke because of this change. The easiest workaround that I've found is to switch out all |
Thanks for the feedback. I'm sorry that this broke your code. Let me assure you that the effect of this change has been carefully considered. There were good reasons to change and also good reasons not to change. In the end, it was a trade-off decision. Breaking some rare justified usage and violating the Liskov substitution principle vs. having an API that is prone to misuse and whose misuse is not easily detected. See #82012 (comment) The broken cases I've seen so far were either real bugs or cases that were technically correct but could be written more clearly without bitwise inversion of bools. I'm happy to discuss your cases if you want to. |
See also the discussion https://discuss.python.org/t/bool-deprecation/62232 |
The bitwise inversion operator on bool returns the bitwise inversion of the underlying int value; i.e.
~True == -2
such thatbool(~True) == True
.It's a common pitfall that users mistake
~
as negation operator and actually wantnot
. Supporting~
is an artifact of bool inheriting from int. Since there is no real use-case for the current behavior, let's deprecate~
on bool and later raise an error. This removes a potential source errors for users.Full reasoning: #82012 (comment)