bpo-37406: Remove debug checks in unicodeobject.c

[vstinner]

When Python is built in release mode (when Py_DEBUG is not defined),
remove debug/sanity runtime checks in unicodeobject.c (checks using
PyErr_BadInternalCall() or PyErr_BadArgument()).

https://bugs.python.org/issue37406

[vstinner]

There are still a few PyErr_BadInternalCall() and PyErr_BadArgument() in:

• PyUnicode_RichCompare (invalid op)
• PyUnicode_Append()
• _PyUnicode_FormatLong(): Py_REFCNT(result) == 1 sanity check

[vstinner]

Oh, this change breaks the sqlite module which expect that PyUnicode_AsUTF8AndSize() raises an excpetion if its argument is not a Unicode object:

    sql_cstr = PyUnicode_AsUTF8AndSize(sql, &sql_cstr_len);
    if (sql_cstr == NULL) {
        rc = PYSQLITE_SQL_WRONG_TYPE;
        return rc;
    }

[vstinner]

I wrote PR #14386 to change the sqlite3 module.

[jdemeyer]

Wouldn't it be simpler to replace those checks by assertions then? For example

    assert(PyUnicode_Check(from));
    assert(PyUnicode_Check(to));

instead of

#ifdef Py_DEBUG
    if (!PyUnicode_Check(from) || !PyUnicode_Check(to)) {
        PyErr_BadInternalCall();
        return -1;
    }
#endif

[vstinner]

Wouldn't it be simpler to replace those checks by assertions then?

That's a big behavior difference: currrently, it's possible to catch and handle the exception. If we start using assert(), the process is killed immediately by SIGABRT using abort(). I prefer to not change too many things at once :-)

[jdemeyer]

That's a big behavior difference

Given that the default is to build without debug info, this PR is a big behavior difference: explicit checks are removed.

If we start using assert(), the process is killed immediately by SIGABRT using abort().

I actually consider that a good thing. It's easier to debug a SIGABRT than it is to debug a Python exception raised in C code.

But I'm not going to argue this further, it was just a suggestion.

[vstinner]

Given that the default is to build without debug info, this PR is a big behavior difference: explicit checks are removed.

Almost everybody use release builds of Python. So yes, this PR changes the default behavior. It's a deliberate choice: https://bugs.python.org/issue37406

I was talking about no changing the behavior of the debug build.

[jdemeyer]

There were some problems with the Azure CI. Can you close and re-open such that it gets tested again?

[jdemeyer]

currrently, it's possible to catch and handle the exception

Yes, it's possible but it's completely meaningless to do that. It indicates a serious bug in the code, not something that should be handled in a try/except block.

[vstinner]

There are discussions against this change in https://bugs.python.org/issue37406 My plan is to finish an article to explain the problem I am trying to solve here.

[jdemeyer]

I don't think that the code paths using PyErr_BadArgument instead of PyErr_BadInternalCall are meant for debug builds only. It's raising a TypeError and catching that is a reasonable thing to do. So this could cause applications that work fine in debug to crash in non-debug mode. That would be pretty bad.

So there are two ways to fix this inconsistency:

1. Replace PyErr_BadArgument by PyErr_BadInternalCall inside those #ifdef Py_DEBUG blocks.
2. Only use #ifdef Py_DEBUG for code already using PyErr_BadInternalCall

I would vote for option 1.

[jdemeyer]

And this absolutely deserves a NEWS entry, as it's changing the behaviour of the C API.

[vstinner]

@jdemeyer: Please discuss https://bugs.python.org/issue37406 since more people are involved there, and I prefer to not have the discussion at two different places.

[vstinner]

https://bugs.python.org/issue37406#msg348012