bpo-42064: Move sqlite3 types to global state

[erlend-aasland]

https://bugs.python.org/issue42064

[erlend-aasland]

Internal change only; skipping news.

[erlend-aasland]

@vstinner or @encukou: could one of you take a look at this? It's mostly search-and-replace (ish), except for the one case I've pointed out. Let me know if I should break this up in multiple PR's.

[encukou]

Hmm, I'm not too familiar with the plan for the sqlite module. The calls to pysqlite_get_state(NULL) will be replaced with non-NULL in the future, right?

It seems that so far, the only module that uses dynamic types with Argument Clinic's subclass_of is _ssl, and AFAICS, it does things by walking the MRO twice. It might be worth it to add better support for this need. For example, I could imagine checking if any superclass was defined using a given PyType_Spec.

[erlend-aasland]

Hmm, I'm not too familiar with the plan for the sqlite module.

Well... my plan is multi-phase init (which implies module state). Heap type conversion is done, and Argument Clinic is mostly in place; establishing a global state is a natural next step.

The calls to pysqlite_get_state(NULL) will be replaced with non-NULL in the future, right?

Correct. I could add a comment about that above pysqlite_get_state; those lines may look weird if that piece of information is missing.

EDIT: now I remember: grepping for pysqlite_get_state.NULL makes it easy to identify state lookups that need special care when we move from global to module state.

It seems that so far, the only module that uses dynamic types with Argument Clinic's subclass_of is _ssl, and AFAICS, it does things by walking the MRO twice. It might be worth it to add better support for this need. For example, I could imagine checking if any superclass was defined using a given PyType_Spec.

You're talking about a new API to optimise this, right?

EDIT: Yes, I see the twin clinic_state calls in the clinic files. That's unfortunate. _ssl solves this by adding a pointer to the state in the object state.

[encukou]

OK, then this PR looks like a good step!

You're talking about a new API to optimise this, right?

Yes. I noted it and will try to make some time for it.

[erlend-aasland]

OK, then this PR looks like a good step!

Yes, I believe it is. I think it's best to keep the "establish global state" PR's as simple as possible, for the reviewers convenience. Optimisations, for example passing state as a parameter to a function, or storing state members in the object context (as functools does), are easier add afterwards, IMO.

You're talking about a new API to optimise this, right?

Yes. I noted it and will try to make some time for it.

Fantastic. I'd be interested in following that process, for the sake of learning.

[erlend-aasland]

LGTY, @encukou?

[encukou]

Looks good; thanks for the clarification!

[erlend-aasland]

Thanks for reviewing, @encukou !

[encukou]

Fantastic. I'd be interested in following that process, for the sake of learning.

I don't yet have time to get into this, but I put down some notes: encukou/abi3#19