bpo-35381 Remove all static state from posixmodule by eduardo-elizondo · Pull Request #15892 · python/cpython · GitHub

After #9665, this moves the remaining types in posixmodule to be heap-allocated to make it compatible with PEP384 as well as modifying all the type accessors to fully make the type opaque.

The original PR that got messed up a rebase: #10854. All the issues in that commit have now been addressed since #11661 got committed.

This change also removes any state from the data segment and onto the module state itself.

https://bugs.python.org/issue35381

Automerge-Triggered-By: @encukou

As Jeroen pointed out in the issue, PEP 384 (Stable ABI) isn't very relevant to the standard library. PEP 489 (Multi-phase init) would bring benefits (better subinterpreter support), but this isn't quite there.
Is there a practical benefit to this change?

I pushed a few changes directly rather than writing a comment about them. Hope that's OK with you :)

I'm proposing a PyState_AddModule doc update to make things clearer: python#16101

I added some commits here. The core dev sprint is ending, and I won't have time to look at this issue for the next few days (or weeks, unfortunately, depending on work load).
Please take these as comments/conversation starters, rather than the one true way of doing things. Feel free to rebase/modify them.

@encukou thanks for all the reviews! I'll update this to address all the comments

Module-level constant strings should use PyUnicode_InternFromString rather than PyUnicode_FromString

Agreed, thanks for the fix!

I'd rather bring _Py_IDENTIFIER(fspath) back (for now) than reimplement _PyObject_LookupSpecial.

I think there's two options: 1) Solve this by creating a new C-API that handles this without a _Py_IDENTIFIER. Unfortunately, this will add yet another C-API function. 2) Not expand the C-API and fix the pattern at the extension level. There's only 2 C Extensions in the stdlib that use this pattern.
Given that this is not a very common pattern, I think it's best to go for approach # 2. I didn't modify anything just yet but I'd like to hear your thoughts!

A more systematic solution would be to implement another C-API to hide the abstraction there. That being said, I rather not expand the C-API given that this pattern is barely present in the stdlib. I've kept the change for now to

In the stdlib, I'd prefer using tp_free and tp_new directly rather than using the workarounds here.

Unfortunately, under PEP384, tp_free and tp_new are not public api so they can't be used 😞
Also, with the introduced changes, os.stat_result now fails. I will revert this change to be compliant with PEP384 and make all tests pass.

ScandirIterator_new and DirEntry_new are the same function.

Thanks for merging them!

I think there's two options: 1) Solve this by creating a new C-API that handles this without a _Py_IDENTIFIER. Unfortunately, this will add yet another C-API function. 2) Not expand the C-API and fix the pattern at the extension level. There's only 2 C Extensions in the stdlib that use this pattern.
Given that this is not a very common pattern, I think it's best to go for approach # 2. I didn't modify anything just yet but I'd like to hear your thoughts!

As we remove statics, there will need to be a generic solution for _Py_IDENTIFIER. Eric & the team are thinking about how to handle them.
Let's not change things in a single module.

Unfortunately, under PEP384, tp_free and tp_new are not public api so they can't be used

PEP 384, the stable ABI, is not relevant to the standard library.
It is good to „dogfood“ the stable ABI in the stdlib, to make sure the ABI is usable – and to catch places where it can be improved, like this. We should definitely expand the stable ABI to allow handling slots more elegantly.
But I don't want to work around the stable ABI limitations.

Hey Petr, good to hear back from you!

As we remove statics, there will need to be a generic solution for _Py_IDENTIFIER. Eric & the team are thinking about how to handle them. Let's not change things in a single module.

Outside of the special case of _PyObject_LookupSpecial , we solved the _Py_IDENTIFIER problem by setting a unicode object in the module state and using it with non _Py_IDENTIFIER C-APIs. I.e. PyObject_GetAttr instead of _PyObject_GetAttrId.

PEP 384, the stable ABI, is not relevant to the standard library.

It is very relevant to any C Extension Module! That's something that I've been trying to get across but I'm probably not doing a good job at it 😛

The really nice benefit of PEP384 is that it hides all the implementation details of CPython. This allows any other implementation to use these modules as well. So adding these changes here allows 1) to set a pattern for other C Extensions to use PEP384, and 2) enables these extensions to be used by any other implementation.

Let me know if it makes sense and/or if you have other questions/concerns!

We should definitely expand the stable ABI to allow handling slots more elegantly.

Agreed! Any suggestions on the proper avenues to do that?

Sorry for the delay – I needed to cancel my CPython day last week :(

The really nice benefit of PEP384 is that it hides all the implementation details of CPython. This allows any other implementation to use these modules as well. So adding these changes here allows 1) to set a pattern for other C Extensions to use PEP384, and 2) enables these extensions to be used by any other implementation.

Agreed. However, the set a pattern for other C Extensions is an issue here.
I find this PR quite elegant, and I'm happy that the stable ABI is nice to use – with one exception, the setting of structseq slots via PyDescr_NewClassMethod and PyObject_SetAttrString. All the other pieces set a good example, but this one is an example of how to get around a limited API.
I want to find a better API before this is in the standard library and sets an example for everyone else.

Can we merge this PR with commit c1a6d03 reverted, and leave that bit for future work?
It's a big PR and I think it would help to have the majority of it merged.

We should definitely expand the stable ABI to allow handling slots more elegantly.

Agreed! Any suggestions on the proper avenues to do that?

Brainstorming ideas:

Add a new version of PyStructSequence_NewType that would take additional slot information (setting a precedent for other *_NewType functions)
Add a "set" counterpart to PyType_GetSlot (and think through the implications & limitations of that)

Can we merge this PR with commit c1a6d03 reverted, and leave that bit for future work?

Sounds like a plan! I'll update this PR in a bit :)

@encukou done!

See https://www.python.org/dev/peps/pep-0007/

I had a few cosmetic nitpicks, but I don't think they need a review round-trip, so I'll just merge with them.

Thank you very much for this change!

tp_name and _PyType_Name() are not the same. There were some discussions between fully qualified names (module.name1.name2.name3) vs short name (name3), but no consensus was found. Well, I should reopen a discussion ;-) I prefer to provide longest names in error messages to ease debug.

@vstinner I actually debated over that for a bit! I think we are missing something like: PyType_QualName. I can do a quick PR for that but I don't know if there will be push back from other devs. Thoughts?

Reopening the discussion sounds like a good idea.

Was it reopened? A discussion could be useful in https://bugs.python.org/issue42035.

After python#9665, this moves the remaining types in posixmodule to be heap-allocated to make it compatible with PEP384 as well as modifying all the type accessors to fully make the type opaque. The original PR that got messed up a rebase: python#10854. All the issues in that commit have now been addressed since python#11661 got committed. This change also removes any state from the data segment and onto the module state itself. https://bugs.python.org/issue35381 Automerge-Triggered-By: @encukou

eduardo-elizondo added 16 commits December 2, 2018 17:40

Make Posixmodule use PyType_FromSpec

e09cf81

Added NEWS

40f0ff4

Updated hash

5be18a1

Addressed PR Issues

9479848

Rebased

d31df5f

Ran argument clinic

c84689c

Added NEWS

796a8d7

Remove INCREF from GenericAlloc

b7cae5b

Make Posixmodule use PyType_FromSpec

5fa6e46

Added NEWS

df178b4

Updated hash

e59208f

Addressed PR Issues

def6467

Ran argument clinic

bb2ead8

Added NEWS

427c772

Merged to master

7b9b85f

Merge branch 'master' into posixmodule-fromspec

9e8fb83

the-knights-who-say-ni added the CLA signed label Sep 11, 2019

bedevere-bot added the awaiting review label Sep 11, 2019

eduardo-elizondo added 3 commits September 10, 2019 22:36

Cleaned up slot acces

99d5c26

Cleaned up more slot accesses

ca19f51

Nits

8af0c4e

eduardo-elizondo changed the title bpo-35381 Make all posixmodule types heap-allocated [WIP] bpo-35381 Make all posixmodule types heap-allocated Sep 11, 2019

eduardo-elizondo added 3 commits September 10, 2019 23:14

Fixes

cee2cd9

Run clinic

a8d2f56

Run clinic

5e4d2ab

eduardo-elizondo mentioned this pull request Sep 11, 2019

bpo-37879: Suppress subtype_dealloc decref when base type is a C heap type #15323

Merged

encukou self-requested a review September 11, 2019 11:07

encukou reviewed Sep 11, 2019

View reviewed changes

Comment thread Modules/posixmodule.c Outdated

Comment thread Modules/posixmodule.c Outdated

Comment thread Modules/posixmodule.c Outdated

Comment thread Modules/posixmodule.c Outdated

Comment thread Modules/posixmodule.c Outdated

Improve error handling

c42ec45

Don't call PyState_AddModule

b9a49aa

I'm proposing a PyState_AddModule doc update to make things clearer: python#16101

eduardo-elizondo added 4 commits September 16, 2019 23:31

Add __new__ changes

c1a6d03

Merge branch 'master' into posixmodule-fromspec

bbe887f

Regenerate clinic

4a6d4ec

Nits

138ffb6

eduardo-elizondo added 2 commits November 3, 2019 10:18

Merge to master

78fbed3

Revert tp_new changes

1b57f3a

eduardo-elizondo and others added 4 commits November 3, 2019 10:47

Rerun argument clinit

e7e9634

More fixes

3bf99bf

Re-add newlines and fix docstring for os.sched_param.__new__

afb249c

Code style nitpicks

187b5d4

See https://www.python.org/dev/peps/pep-0007/

encukou added the 🤖 automerge label Nov 5, 2019

encukou approved these changes Nov 5, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Nov 5, 2019

miss-islington merged commit b396663 into python:master Nov 5, 2019

bedevere-bot removed the awaiting merge label Nov 5, 2019

vstinner reviewed Nov 5, 2019

View reviewed changes

vstinner mentioned this pull request Jan 20, 2020

bpo-39395: Clear env vars set by Python at exit #18078

Closed

                       PyErr_Format(PyExc_TypeError,
                                    "uid should be integer, not %.200s",
-                                   Py_TYPE(obj)->tp_name);
+                                   _PyType_Name(Py_TYPE(obj)));

bpo-35381 Remove all static state from posixmodule by eduardo-elizondo · Pull Request #15892 · python/cpython · GitHub

Repository navigation

bpo-35381 Remove all static state from posixmodule - #15892

bpo-35381 Remove all static state from posixmodule#15892miss-islington merged 45 commits intopython:masterpython/cpython:masterfrom eduardo-elizondo:posixmodule-fromspeceduardo-elizondo/cpython:posixmodule-fromspecCopy head branch name to clipboard

Conversation

eduardo-elizondo commented Sep 11, 2019 • edited by miss-islington Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

encukou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

encukou commented Sep 11, 2019

Uh oh!

encukou commented Sep 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eduardo-elizondo commented Sep 13, 2019

Uh oh!

eduardo-elizondo commented Sep 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

encukou commented Oct 8, 2019

Uh oh!

eduardo-elizondo commented Oct 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

encukou commented Oct 22, 2019

Uh oh!

eduardo-elizondo commented Oct 25, 2019

Uh oh!

eduardo-elizondo commented Nov 3, 2019

Uh oh!

encukou left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner Nov 5, 2019

Choose a reason for hiding this comment

Uh oh!

eduardo-elizondo Nov 5, 2019

Choose a reason for hiding this comment

Uh oh!

encukou Nov 6, 2019

Choose a reason for hiding this comment

Uh oh!

encukou Dec 29, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bpo-35381 Remove all static state from posixmodule#15892
miss-islington merged 45 commits into
python:masterfrom
eduardo-elizondo:posixmodule-fromspec

eduardo-elizondo commented Sep 11, 2019 •
edited by miss-islington
Loading

encukou commented Sep 13, 2019 •
edited
Loading

eduardo-elizondo commented Sep 17, 2019 •
edited
Loading

eduardo-elizondo commented Oct 9, 2019 •
edited
Loading

encukou left a comment •
edited
Loading