gh-112634: remove dead code in PyNumber_AsSsize_t()

[skirpichev]

• Issue: Dead code in PyNumber_AsSsize_t() at L1542 #112634

[skirpichev]

I think this can skip news...

[skirpichev]

Another alternative: drop this line. In that case we, probably, could use PyErr_* functions instead, as before 61b6492 @vstinner?

[mdickinson]

LGTM.

Just as a general comment, I'm not excited by this sort of change, and I'd like to discourage future changes along these lines.

The associated issue has a "bug" label, but this is certainly not a bug - the code is working as designed and intended, and there's no user-visible issue being fixed here.

The docs for PyLong_AsSsize_t only explicitly mention OverflowError, but they don't explicitly say that no other error can possibly be raised.

The current implementation for PyLong_AsSsize_t clearly cannot raise exceptions other than OverflowError, but implementations can and do change.

The correctness of the new version of the code is in effect closely coupled to implementation details elsewhere, and in my experience this sort of long-distance coupling contributes to overall fragility, making code harder to maintain.

In this particular case it seems unlikely that a future version of PyLong_AsSsize_t would raise new exceptions, so this particular change seems reasonable. But more generally I'd consider it a mistake to try to treat a currently unused codepath as a bug, if future changes to other parts of the codebase might cause that unused codepath to become a used codepath.

[skirpichev]

The docs for PyLong_AsSsize_t only explicitly mention OverflowError, but they don't explicitly say that no other error can possibly be raised.

In fact, it can (TypeError and SystemError), but not in this particular code flow, as it was mentioned in the issue.

IIUIC, raised exceptions are part of C API (looking on PEP 387), just as e.g. the function signature. (Despite sometimes they are documented all, sometimes only partially, as in this case.) Or not?

The current implementation for PyLong_AsSsize_t clearly cannot raise exceptions other than OverflowError, but implementations can and do change.

With this in mind, shouldn't we surround every C API call with such watchers?

[mdickinson]

IIUIC, raised exceptions are part of C API

Not really, no; I don't believe there's any expectation that the C-API docs should document every exception that might possibly be raised.

With this in mind, shouldn't we surround every C API call with such watchers?

No; very few things in software development are so black and white. Like most decisions, it's a judgement call that needs to be made on a case-by-case basis, with a full understanding of the context and the trade-offs. But I believe this particular change turns code that's fairly easy to reason about into code that's significantly harder to reason about, since that reasoning now requires a detailed understanding of what PyLong_As_Ssize_t is doing, and to my eyes that's not an improvement.

[skirpichev]

I don't believe there's any expectation that the C-API docs should document every exception

That's slightly a different story. The presence of TypeError case more or less obvious for all C API functions, operating on concrete types (i.e. not PyObject_* stuff).

My point was, the set of possible exception types of a given function is still a part of API in sense of PEP 387.

to my eyes that's not an improvement

Ok, I trust you opinion.