Make optional parameters of sqlite3.connect() keyword-only

[serhiy-storchaka]

sqlite3.connect() has a large number of parameters: 1 required and 7 optional. It is much more convenient to pass optional arguments by keyword, because you can pass only values which differs from default. And if you pass positional arguments, it is easy to make an error and pass value as wrong argument.

It is recommended to make optional rarely used arguments keyword-only. I do not think it will break much code, but we need a deprecation period for this.

The problem is that sqlite3.connect() was converted to Argument Clinic, and it was much easier to add deprecation warning in the old code.

Linked PRs

• gh-93057: Use 'sqlite3' as module name for clinic input in Modules/_sqlite/connection.c #107879
• gh-93057: Deprecate positional use of optional sqlite3.connect() params #107948

[erlend-aasland]

The problem is that sqlite3.connect() was converted to Argument Clinic, and it was much easier to add deprecation warning in the old code.

Can we enhance AC with such functionality first?

[serhiy-storchaka]

Can we enhance AC with such functionality first?

It would be not easy. 😫

[erlend-aasland]

It would be not easy. 😫

That I assumed 🙂 Do you have a coarse outline in mind of how such a change could be implemented?

[erlend-aasland]

Can we enhance AC with such functionality first?

It would be not easy. 😫

Continuing the AC digression :)
I made a proof-of-concept of such AC functionality here: erlend-aasland#37
It is a very crude implementation; there's a lot of corner cases missing checks. See details for more info. Feel free to play around with it, and/or improve it (or even finish it). We make an issue to follow up this now.

I introduced a new special symbol, x, that means "all optional parameters from now on are deprecated as positional parameters". We can use any other symbol or string. For example d/ (deprecated positional), -/, deprecated/. I liked the brevity of x, and how it visually kinda resembles *.

Example AC code:

/*[clinic input]
_sqlite3.connect as pysqlite_connect

    database: object
    timeout: double = 5.0
    detect_types: int = 0
    isolation_level: object = NULL
    check_same_thread: bool(accept={int}) = True
    factory: object(c_default='(PyObject*)clinic_state()->ConnectionType') = ConnectionType
    x
    cached_statements: int = 128
    uri: bool = False

Opens a connection to the SQLite database file database.

You can use ":memory:" to open a database connection to a database that resides
in RAM instead of on disk.
[clinic start generated code]*/

[erlend-aasland]

It is recommended to make optional rarely used arguments keyword-only. I do not think it will break much code, but we need a deprecation period for this.

I suggest we make all but the database argument keyword-only. If my AC hack is a possible way forward¹, we can do this in 3.12.

Also, the database argument should be positional-only.

Footnotes

1. It is a hack; I didn't think through all possible scenarios, or even if it is the best path forward. ↩

[erlend-aasland]

Argument Clinic now has support for deprecating positional use of positional-or-keyword parameters. However, there are still some prerequisites that must be in place before we can continue with this issue:

1. Currently, the clinic code in Modules/_sqlite uses module _sqlite3. This means that deprecation warnings will mention _sqlite3.connect(), not sqlite3.connect(). This can be confusing for users. I suggest we change the clinic code to use module sqlite3 instead. This change should not result in any behaviour change.
2. Next, we have some technical dept when it comes to sqlite3.connect and sqlite3.Connection.__init__. Currently, the latter uses clinic, and the former does not use clinic. However, it is sqlite3.connect that needs the docstring, not sqlite3.Connection.__init__. We should make it so that the docstring from sqlite3.Connection.__init__ is used for sqlite3.connect and output to a separate file, for inclusion by Module/_sqlite/module.c.
3. In order to achieve 2. properly, we must make it possible for clinic.py to clone sqlite3.Connection.__init__ to sqlite3.connect.

This is roughly what the relevant parts of Modules/_sqlite/connection.c will look like:

/*[clinic input]
sqlite3.Connection.__init__ as pysqlite_connection_init
    database: object
    * [from 3.15]
    timeout: double = 5.0
    detect_types: int = 0
    isolation_level: IsolationLevel = ""
    check_same_thread: bool = True
    factory: object(c_default='(PyObject*)clinic_state()->ConnectionType') = ConnectionType
    cached_statements as cache_size: int = 128
    uri: bool = False
    *
    autocommit: Autocommit(c_default='LEGACY_TRANSACTION_CONTROL') = sqlite3.LEGACY_TRANSACTION_CONTROL
[clinic start generated code]*/

static int
pysqlite_connection_init_impl(pysqlite_Connection *self, PyObject *database,
                              double timeout, int detect_types,
                              const char *isolation_level,
                              int check_same_thread, PyObject *factory,
                              int cache_size, int uri,
                              enum autocommit_mode autocommit)
/*[clinic end generated code: output=cba057313ea7712f input=a0949fb85339104d]*/
{
    // __init__ function is here; fast forward ...
}

/*[clinic input]
# Save the clinic output config.
output push

# Create a new destination 'connect' for the docstring and methoddef only.
destination connect new file '{dirname}/clinic/_sqlite3.connect.c.h'
output everything suppress
output docstring_definition connect
output methoddef_define connect

# Now, define the connect function.
sqlite3.connect as pysqlite_connect = sqlite3.Connection.__init__

[clinic start generated code]*/
/*[clinic end generated code: output=da39a3ee5e6b4b0d input=7913cd0b3bfc1b4a]*/

/*[clinic input]
# Restore the clinic output config.
output pop
[clinic start generated code]*/

Then, in Modules/_sqlite/module.c:

#include "clinic/_sqlite3.connect.c.h"

static PyObject *
pysqlite_connect(PyObject *module, PyObject *const *args, Py_ssize_t nargsf,
               PyObject *kwnames)
{
    // ...
}

// ... and then:

static PyMethodDef module_methods[] = {
    PYSQLITE_ADAPT_METHODDEF
    PYSQLITE_COMPLETE_STATEMENT_METHODDEF
    PYSQLITE_CONNECT_METHODDEF    // <= this now comes from clinic/_sqlite3.connect.c.h
    PYSQLITE_ENABLE_CALLBACK_TRACE_METHODDEF
    PYSQLITE_REGISTER_ADAPTER_METHODDEF
    PYSQLITE_REGISTER_CONVERTER_METHODDEF
    {NULL, NULL}
};

[erlend-aasland]

Thanks for suggesting this feature and helping land it (and all its implications in Argument Clinic), @serhiy-storchaka. Very helpful, and a good learning experience.

[erlend-aasland]

For the record, it was the Argument Clinic implications of this feature that spawned the desire to add typing to clinic.py (#104050), which in turn got both @AlexWaygood and me highly involved with Argument Clinic. We've now ended up in a position where more core devs have better knowledge of the inner workings of clinic.py; this is a good thing. Thanks, again!

[erlend-aasland]

Also, the database argument should be positional-only.

If this is desired, we should create a new issue for it. cc. @serhiy-storchaka