capi-workgroup / decisions

Discussion and voting on specific issues
8 stars 1 forks source link

Remove PyLong_AS_LONG #38

Closed serhiy-storchaka closed 1 month ago

serhiy-storchaka commented 3 months ago

In Python 2, there was PyInt_AS_LONG. It was a simple macro, it was atomic (did not release the GIL) and never failed. So it could be used in the code that do not expect interruption and did not require error handling.

Then PyLong_AS_LONG was added in Python 3 to help transition from Python 2. It is just an alias of PyLong_AsLong, and it do not support guarantees of PyInt_AS_LONG -- it can fail and can release the GIL if the argument is not an int instance. PyInt_AS_LONG was replaced with PyLong_AS_LONG, but not all such replacements were correct.

Currently PyLong_AS_LONG is only left in few places in unicodeobject.c (the code analysis can show that it can be replaced with PyLong_AsLong without adding additional checks) and in the compiler, where many C API functions are used without proper error checking to avoid complication of already complicated code (we only hope that nothig bad happens whily they are used with internal data created and consumed in the compiler). In all other places the use of PyLong_AS_LONG has been cleared over the years.

But we cannot guarantee the same for third-party code. Moreover, using PyLong_AS_LONG most likely means an error, because the code is likely expect the guarantees of PyInt_AS_LONG. It is not documented, but it is available by default and can be used in third-party code, especially if it was migrated from Python 2.

So I propose to remove PyLong_AS_LONG. If it will break a third-party code, this is a good opportunity to review it and add error checks if they are missed. Or ignore potential bugs and simply add #define PyLong_AS_LONG PyLong_AsLong.


Vote to remove PyLong_AS_LONG:

Vote to keep PyLong_AS_LONG:

encukou commented 3 months ago

When do you want to remove it? In 3.16, after the minimum deprecation period?

vstinner commented 3 months ago

So I propose to remove PyLong_AS_LONG.

I agree to remove it if it's first deprecated for 2 releases. For example, convert it to a static inline function and use Py_DEPRECATED() on it.

serhiy-storchaka commented 3 months ago

Taking into account that it was not documented and has no other value except of signalling "this code migrated from Python 2 was not thoughtfully reviewed", and that the workaround is trivial, I would suggest to omit the deprecation period. But I am fine with deprecation. It will just add more work for us.

vstinner commented 3 months ago

People use undocumented, untested, private API. So I'm sure that they use undocumented public APIs.

serhiy-storchaka commented 3 months ago

What are the reasons of using PyLong_AS_LONG?

vstinner commented 3 months ago

Code search on PyPI top 7,500 projects (2024-03-16). PyLong_AS_LONG string is found in 32 projects:

vstinner commented 3 months ago

If such function works well currently and users are happy about it, but we don't want new projects to use it, maybe it would be better to only soft deprecate the function, and rely on something like https://peps.python.org/pep-0743/ to "remove" the function.

serhiy-storchaka commented 3 months ago

I have looked at the first 5 projects. 3 of them do not use it. MySQL-python uses it once and do not check for errors (bug!). PyBluez uses it twice and checks for errors (can be replaced with PyLong_AsLong).

encukou commented 3 months ago

+1 for soft-deprecation; this is not a maintenance burden nor a security risk.

I doubt removing it would prevent significant errors:


I would suggest to omit the deprecation period.

That would require an exception from the Steering Council, and I really think this is not worth bothering them.

vstinner commented 3 months ago

MySQL-python uses it once and do not check for errors (bug!).

I don't get your point. For me, the obvious replacement for PyLong_AS_LONG() is PyLong_AsLong(). I don't see how this replacement solves the "errors" issue. I don't expect users to suddenly pay more attention to the error case.

If you want users to pay more attention to errors, you should maybe propose an alternative PyLong_AsLong() function which returns -1 on error and sets the result in an long *result parameter.

picnixz commented 3 months ago

Or ignore potential bugs and simply add #define PyLong_AS_LONG PyLong_AsLong.

Maybe I am seeing things but, in my code I see #define PyLong_AS_LONG(op) PyLong_AsLong(op), so isn't it already the case that PyLong_AS_LONG and PyLong_AsLong are actually the same? (sorry if my question is naive...)

vstinner commented 2 months ago

@serhiy-storchaka: It seems like there is a disagreement to remove without good replacement, but @encukou would be supportive of a soft deprecation. Can you update your vote to give the choice between removal and soft deprecation? Or only propose soft deprecation?

serhiy-storchaka commented 2 months ago

How the procedure of the soft deprecation could look?

vstinner commented 2 months ago

How the procedure of the soft deprecation could look?

A soft deprecation is just a sentence in the documentation. Nothing more, nothing less.

Since PyLong_AS_LONG() is not documented, it should be documented first, to be able to soft deprecate it.

da-woods commented 2 months ago

Cython (3.0.9)

I doubt it affects your decision either way but Cython doesn't really use it. The only thing it does with it is to map user-code that directly uses the old Python 2 PyInt_AS_LONG to PyLong_AS_LONG.

serhiy-storchaka commented 2 months ago

A soft deprecation is just a sentence in the documentation. Nothing more, nothing less.

How can it help, in theory? People who are using it in their code now aren't doing this because they read about it in the documentation.

Soft deprecation can prevent new uses. But the absence of documentation works just as well.

erlend-aasland commented 2 months ago

Soft deprecation can prevent new uses. But the absence of documentation works just as well.

No, those are definitely not equal. Soft deprecation explicitly deprecates an API in documentation; it is an unambiguous message. OTOH, the absence of documentation can leave the user believing it's a public API lacking docs; other interpretations are also possible.

vstinner commented 1 month ago

Remove PyLong_AS_LONG

IMO this change was rejected so I suggest to close the issue. Are you ok to close the issue @serhiy-storchaka or do you want to repurpose the issue to deprecate/soft deprecate the API?

serhiy-storchaka commented 1 month ago

I don't see the point in keeping it. Deprecation with the following removing is better than keeping it indefinitely, although I do not think that deprecation will help anybody. Let deprecate it, whatever that means.

vstinner commented 1 month ago

I don't see the point in keeping it.

It's used by many Python projects and it just works as expected. Removing it would be annoying for not benefits. At least, I don't see how just replacing PyLong_AS_LONG() with PyLong_AsLong() makes the code safer or better.

We would need a better API which reports errors: https://github.com/capi-workgroup/problems/issues/1

serhiy-storchaka commented 1 month ago

Many Python projects that contain a reference to it, do not use it. And in some of these that actually use it, it does not work as expected (or rather the author had wrong expectation). This is th reason why it should be removed. Besides the fact that it is not documented and is just an alias to other function.

malemburg commented 1 month ago

PyInt_AS_LONG() used to be a macro to access Python integer object's long value directly. This is why many projects use it.

And it was documented as such in Python 2: https://docs.python.org/2.7/c-api/int.html#c.PyInt_AS_LONG

When the PyInts were merged into PyLongs, the macro was kept around in form of PyLong_AS_LONG().

serhiy-storchaka commented 1 month ago

You confused it with PyInt_AS_LONG.

malemburg commented 1 month ago

Yes and no 😄, please see my (edited) comment for a more accurate summary.

vstinner commented 1 month ago

The vote only had one option: remove the function. It's unclear if people voted "no" by not voting, or if they didn't vote...

So I added a second choice: "Keep the function". Please vote again so we can take a decision on this issue.

vstinner commented 1 month ago

@encukou @erlend-aasland @mdboom @serhiy-storchaka @zooba: Would you mind to vote, so we can take a decision on this issue?

zooba commented 1 month ago

I doubt removing it would prevent significant errors:

  • If users replaced PyInt_AS_LONG with PyLong_AS_LONG without error checking, they can just as easily replace PyLong_AS_LONG by PyLong_AsLong without error checking. (If we force people to update their code, they're likely to do the minimum amount of work needed to get it to compile & test again.)
  • The bug you get if you skip error checking is not too severe -- any exception will be raised later, or ignored.

I was convinced by Petr that we should keep it. But I also think we should generally soft-deprecate (as in, discourage new use of) all our macros. People should certainly reach for real functions by default, and think carefully about using a macro.

Even though it doesn't technically matter much in this case, something like PyBytes_AsString and PyBytes_AS_STRING[^1] matter much more, and until you've proven that your code has a bottleneck on this call, people should always use the function. Applying the same logic to PyLong_AS_LONG, even though it already maps directly to the function, provides a more consistent model for our users to understand.

[^1]: If I recall the names correctly

serhiy-storchaka commented 1 month ago

The proposition to remove PyLong_AS_LONG was not passed.

You can open a new issue for documenting and deprecating it if you know how to do this.

encukou commented 1 month ago

Will do, in https://github.com/python/cpython/issues/124385