Improve and maintain Doc/data/refcounts.dat

[picnixz]

The Doc/data/refcounts.dat file is sometimes updated and sometimes not. I suggest we try to make it up-to-date as much as possible, at least by adding the functions part of the stable ABI.

In addition, the entries are not sorted alphabetically. While they are semantically sorted, it does not help when adding new entries (where should we put it?). So I suggest to reformat the file once I'm done with adding the new entries.

Finally, I suggest adding a small script that checks whether the file is up-to-date, at least by checking whether the stable ABI is a subset of that file and whether the refcounts.dat is sorted or not (again it will be part of a CI workflow).

The main motivation behind this is to help newcomers in understanding whether a reference is borrowed or not (sometimes the comment doesn't say anything). It could also help in managing a future linter. Note that we have no syntax to indicate whether a reference is stolen or not so we should also improve that part.

cc @vstinner @encukou @Eclips4 @ZeroIntensity

Linked PRs

• gh-127443: Update refcounts data with stable ABI functions. #127444
• gh-127443: fix some entries in Doc/data/refcounts.dat #127451
• [3.13] gh-127443: Fix some entries in Doc/data/refcounts.dat (GH-127451) #127496
• [3.12] gh-127443: Fix some entries in Doc/data/refcounts.dat (GH-127451) #127497
• gh-127443: Add semantics for stolen references in Doc/data/refcounts.dat #127468
• gh-127443: add tool for linting Doc/data/refcounts.dat #127476

Tasks

Beta Give feedback

1. Add refcounts entries from the stable ABI (only func entries)
2. Allow to specify when a reference is stolen (use "$" instead of "0")
3. Add a script for checking whether the file is correctly formatted (check order, syntax, but not advanced format)
4. Add a script for checking if the file is up-to-date when a new function is added to the Stable ABI
5. Unify format (sort entries, add blank lines, add space before pointer dereference marker (optional, likely not))
6. Add CI jobs
7. Use a script to auto-generate the refcounts.dat and let the developer fill out the entries

[ZeroIntensity]

I think it's worth trying to maintain this for all of the public C API, not just the stable ABI. If the goal is to design a static analyzer, then it would be ideal to have everything.

[picnixz]

I think it's worth trying to maintain this for all of the public C API, not just the stable ABI. If the goal is to design a static analyzer, then it would be ideal to have everything.

Yes, but I'd like to start with the stable ABI for now. It was quite simple to extract the diff but in order to extract all the exported functions and make the diff, I think my one-liner will be a bit too long...

[picnixz]

Ok, so having an autoformatter may be a bit tricky. I really don't want to create a C parser just to parse the type declaration. I think I'll just manually change them with some powerful regexes and that's it. But for now, I'll just wait for the other PRs to land before changing them. (Anyway, at some point I'll need to sort the file alphabetically, and the diff won't really matter :D)

[tomasr8]

I really don't want to create a C parser just to parse the type declaration.

I've never used it so I don't know what the current status is, but there is a C parser under Tools/c-analyzer/c_parser. Might also be a good starting point if we decide to add a proper static analyzer at some point..

[picnixz]

Yes, but I felt it would have been an overkill to use that one actually. I just wanted a simplest parser for declarations without the rest.

[picnixz]

@ZeroIntensity For the TOML conversion, we have some limitations. We need to quote '$' and 'null' if they appear and we need to use either inline tables or inline arrays to reduce the size of the file IMO. Instead, I suggest to always create a top-level section per function:

[$funcname$]

We can add "deprecated" and "comment" to that top-level section, helping the newcomers.

Followed by the the return section:

[$funcname$.return]
ctype = "PyObject *"
refdelta = 1
# or for functions that always return NULL
[$funcname$.return]
ctype = "PyObject *"
idempotent = true

And finally by the section for the parameters, here's an example:

[$funcname$.params.a]
ctype = "PyObject *"
steals = true
[$funcname$.params.b]
ctype = "PyObject *"
refdelta = 1
[$funcname$.params.c]
ctype = "PyObject *"
refdelta = -1
[$funcname$.params.d]
ctype = "PyObject *"
refdelta = 0
[$funcname$.params.e]
ctype = "const char *"

Each section will always have an optional "comment" key. For instance:

PyBytes_AS_STRING:char*:::
PyBytes_AS_STRING:PyObject*:string:0:

becomes

[PyBytes_AS_STRING]
[PyBytes_AS_STRING.return]
ctype = "char *"
[PyBytes_AS_STRING.params.string]
ctype = "PyObject *"
refdelta = 0

We can also compress it into:

[PyBytes_AS_STRING]
return.ctype = "char *"
params.string.ctype = "PyObject *"
params.string.refdelta = 0

wDYT?

[ZeroIntensity]

I'm not all that worried about compression--it's going to be a big file any way we do it. I think we should use an array of tables for the parameters instead of trying to create new keys for each parameter (it's significantly easier to parse it back into Python that way). For reference, the TOML I generated ended up looking like this:

[PyArg_Parse]
name = "PyArg_Parse"
return_type = "int"

[[PyArg_Parse.args]]
name = "args"
type = "PyObject *"
effect = 0

[[PyArg_Parse.args]]
name = "format"
type = "const char *"

[[PyArg_Parse.args]]
name = "..."
type = "ellipsis"

I'm not sure how we should denote some weird things like "always NULL" though. Special boolean flags like always_null = true or steal = true look pretty, but they'll pile up weirdly over time.

[picnixz]

I denote the always NULL by saying that the function is idempotent. It's not entirely accurate. Or we can have always_null = true as you said which could probably be a better naming.

By the way, it's probably better to have an array rather than a mapping in order to handle the variadic argument.

[ZeroIntensity]

An array rather than a mapping for what?

[picnixz]

For the list of parameters. My first suggestion was using a mapping where the key is the parameter name:

[PyBytes_AS_STRING.params.string]
ctype = "PyObject *"
refdelta = 0

but for you, it would be:

[[PyArg_Parse.params]]
name = "string"
ctype = "PyObject *"
refdelta = 0

[encukou]

I suggest putting the info in C headers rather than a new format -- unless you can generate the headers (or something else that's helpful) from the new format.

That said, I've tried it and found some dead ends. I still think it'd be worthwhile, though: and external file is bound to become unmaintained.
My latest idea (that I haven't really started on yet, alas) is that if you look for PyAPI_FUNC, everything up to the next ; is a function definition.

[picnixz]

Actually refcounts.dat is still used by the docs so we need this file (whether it's auto-generated or not). Adding to the C headers directly still require them to be parsed (we could use macros just for fake annotations but I'm not sure we can do it easily for the entire project).

What we can do however, is generate the (unfilled) entries from the headers directly and the dev should only fill out the reference count effect.

[encukou]

What we can do however, is generate the (unfilled) entries from the headers directly and the dev should only fill out the reference count effect.

Do you want to treat it as the first step of a long project? It would be a good first step :)

[picnixz]

Honestly, yes. Because knowing exactly what a function does on reference counts is always hard when you don't want to read the entire code... or open the docs (which sometimes do not tell you more =/)

[encukou]

I commented with parts elsewhere but I'll consolidate here:

Would it make sense to prefix existing comments with #, and leave field 5 to annotations like stolen?
Existing tools (if there are any) would treat the whole thing as a comment.

IMO, if we're filling this info in, it could make sense to look at each function and add more annotations to it than just stolen. Otherwise we'll need to go through the list again, looking at similar things. We don't need to always add everything, but I'd appreciate a mechanism to add several in one go.

• Adding stolen is an improvement; IMO we should also add borrowed where we know an argument is not stolen/weird, so it's possible to know what's been checked.

• Nullability is a useful category: NULLs OK, non-NULL (crashes/asserts), non-NULL (raises exception)

• PyObject* (and subclasses) can be type-checked (raise TypeError) or unchecked (crashes/asserts)

• stolen/borrowed can apply to any kind of managed resource, not just the refcounted PyObjects. For example, any Py_buffer* has an owner who must call PyBuffer_Release, and the owner can change hands. But you can't incref (=“duplicate” the reference) here. (Some of these can be “immortal”, expected to outlive the function call, e.g. a static const char* name. I hope we don't have many of those left.)

• For a PyObject**, we might want to annotate each “star”: the whole thing can be a stolen/borrowed array or an output argument that the function fills in; if it's an array the individual items can also be stolen/borrowed.

• For return values, we might want to indicate if the function follows the convention that NULL/-1 is returned iff an exception was raised.