bpo-43753: Add Py_Is() and Py_IsNone() functions

[vstinner]

Add Py_Is(x, y) function testing if the 'x' object is the 'y' object,
same than "x is y" in Python. Add also Py_IsNone(), Py_IsTrue(),
Py_IsFalse() functions to test if an object is, respectively, the
None` singleton, the True singleton or the False singleton.

https://bugs.python.org/issue43753

[vstinner]

I rebased my PR and I reimplemented Py_Is() as a macro.

[vstinner]

And I added unit tests.

[vstinner]

@markshannon: so are you against the principle of adding this macro and using it? Or can I merge it?

[vstinner]

@corona10 @serhiy-storchaka @methane @pitrou: Do you think that these 4 functions are useful? See https://bugs.python.org/issue43753 for the rationale. It's mostly about helping to write C extensions compatible with PyPy, and prepare the bright future of HPy.

[pitrou]

Can this be added to the stable ABI even though it's a macro?

[vstinner]

Can this be added to the stable ABI even though it's a macro?

I don't see the point of adding it to the ABI in CPython right now. Can you please elaborate? I mostly care about the API.

Raymond is concerned by the risk of a negative impact on performance: https://bugs.python.org/issue43753#msg390379 As I only care about the API, I'm perfectly fine with a macro.

PyPy is free to implement it as a function if they want.

[pitrou]

But then people targetting the stable ABI can't use this API? If the goal is to maximize usage, then it should be part of the ABI...

[vstinner]

But then people targetting the stable ABI can't use this API? If the goal is to maximize usage, then it should be part of the ABI...

Ok, I get your point. That's why I declared Py_NewRef() and Py_XNewRef() as static inline functions and regular functions. I modified tests on these functions to test the macros (static inline functions) and the function implementations.

[vstinner]

I prepared a pull request on my pythoncapi_compat project to add Py_Is(), Py_IsNone(), Py_IsTrue() and Py_IsFalse() to Python 3.9 and older and to PyPy: python/pythoncapi-compat#8 ;-) I will merge it if this PR is merged.

[corona10]

I am +1 on adding Py_Is(). It can be quite useful for some use-cases.
But other variations are not convinced to me.
If I understand your intention correctly, we may create all the variations of PyIsXXX for singleton objects. When considering adding is easy but deprecating and removing is hard, I would like to add Py_Is() API only.

if we add _Py_IsXXX variations as the private API, I am okay to add those :)

[vstinner]

I am +1 on adding Py_Is(). It can be quite useful for some use-cases.

Ok.

But other variations are not convinced to me.
If I understand your intention correctly, we may create all the variations of PyIsXXX for singleton objects. When considering adding is easy but deprecating and removing is hard, I would like to add Py_Is() API only.

I don't propose to add a variation for all singletons. Only for these 3 which are the common (None, True, False).

I proposed Py_IsNone(), Py_IsTrue(), Py_IsFalse() to make the transition to Py_Is() more pleasant. IMO if we only add Py_Is(), it's harder to read code which repeat Py_ prefix in Py_Is and in Py_None.

I removed Py_IsNone(), Py_IsTrue(), Py_IsFalse().

I also modified Python/ceval.c in this PR, so you can see the change in practice.

So what do you prefer? Ony Py_Is() or with 4 functions?

Example with the current C API:

if (x == Py_None) {
    return 1;
}
if (x == Py_True) {
    return 2;
}
if (x == Py_False) {
    return 3;
}

Only with Py_Is():

if (Py_Is(x, Py_None)) {
    return 1;
}
if (Py_Is(x, Py_True)) {
    return 2;
}
if (Py_Is(x, Py_False)) {
    return 3;
}

With Py_IsNone(), Py_IsTrue() and Py_IsFalse():

if (Py_IsNone(x)) {
    return 1;
}
if (Py_IsTrue(x)) {
    return 2;
}
if (Py_IsFalse(x)) {
    return 3;
}

[pitrou]

I think having the 4 functions is ok. x is None has been a recommended idiom for at least 20 years.

[vstinner]

After I wrote my comment with examples showing the 3 APIs, I changed my mind. I really want Py_IsNone(), Py_IsTrue(), Py_IsFalse().

Having to replace x == Py_None with Py_Is(x, Py_None) is annoying. The new code is longer and harder is read.

whereas replacing x == Py_None with Py_IsNone(x) makes the code 1 character shorter and IMO it's as simple, or even simpler.

Even if the 3 functions are just helper, I would like to add them.

#define Py_IsNone(x) ((x) == Py_None)
#define Py_IsTrue(x) ((x) == Py_True)
#define Py_IsFalse(x) ((x) == Py_False)

[vstinner]

"Py_IsTrue(x): Test if an object is True."

Hum, I was confused by this description. I rewrote it mentioning the singleton word:

=> "Py_IsTrue(x): Test if an object is the True singleton."

IMO the description now avoids any possible confusion with PyObject_IsTrue(x).

[pablogsal]

I am not super conviced. Why would people would like to use Py_Is if is going to be slower than just pointer comparison?

[vstinner]

The intent is correctness if you care about other Python implelentations than CPython. If you don't care, well, don't use it :-)

[corona10]

I think having the 4 functions is ok. x is None has been a recommended idiom for at least 20 years.

I don't propose to add a variation for all singletons. Only for these 3 which are the common (None, True, False).

I read the comments and I also change my mind to export those APIs.

So +1

[vstinner]

@pablogsal: Oh, thank you for fixing my English :-) I addressed your review.

About exposing Py_Is() and Py_IsNone() as a regular function: please see @pitrou's previous comment.

[vstinner]

The docs CI failed with:

[2] ../Misc/NEWS.d/next/Library/2021-04-10-03-30-36.[bpo-43478](https://bugs.python.org/issue43478).iZcBTq.rst:1: default role used

Does anyone understand what's wrong with my NEWS entry?

Add the :c:func:`Py_Is(x, y) <Py_Is>` function to test if the *x* object is the
*y* object, the same as ``x is y`` in Python. Add also the :c:func:`Py_IsNone`,
:c:func:`Py_IsTrue`, :c:func:`Py_IsFalse` functions to test if an object is,
respectively, the ``None`` singleton, the ``True`` singleton or the ``False``
singleton.
Patch by Victor Stinner.

[pablogsal]

The docs CI failed with:

[2] ../Misc/NEWS.d/next/Library/2021-04-10-03-30-36.[bpo-43478](https://bugs.python.org/issue43478).iZcBTq.rst:1: default role used

Does anyone understand what's wrong with my NEWS entry?

Add the :c:func:`Py_Is(x, y) <Py_Is>` function to test if the *x* object is the
*y* object, the same as ``x is y`` in Python. Add also the :c:func:`Py_IsNone`,
:c:func:`Py_IsTrue`, :c:func:`Py_IsFalse` functions to test if an object is,
respectively, the ``None`` singleton, the ``True`` singleton or the ``False``
singleton.
Patch by Victor Stinner.

I think this warning was due to #25335

[vstinner]

I think this warning was due to #25335

Oh ok. I merged my PR in this case.

[vstinner]

Thanks @erlend-aasland @pitrou @corona10 and @pablogsal for your useful reviews!