gh-106004: Add PyDict_GetItemRef() function by vstinner · Pull Request #106005 · python/cpython

What's the rationale for not distinguishing between found and not found in the return value?
See python/devguide#1121

Can we come up with a better name than PyDict_GetItemRef?
I see why you are adding Ref to the end, but all API functions should return new references, so it is a bit like calling the function PyDict_GetItemNotWrong.

Obviously, the ideal name is already taken.
Anyone have any suggestions for a better name?

What's the rationale for not distinguishing between found and not found in the return value?

Sure, if you think that it's useful, I can return 1 if the key is present.

It doesn't go against the syntax if (PyDict_GetItemRef(...) < 0) ... to check for error. I dislike having to use 2 variables to check for a function variable, so I prefer to use the implicit test in an if (...) statement. Pseudo-code with a variable to store the function result and dispatch the action depending on it:

PyObject *value;
int res = PyDict_GetItemRef(dict, key, &value);
if (res < 0) ... error ...
else if (res == 0) ... missing key ...
else ... present key

Well, I'm fine with people having different taste (if it doesn't go against my taste ;-)).

Can we come up with a better name than PyDict_GetItemRef?

Nope, that's my local optimum name :-)

Anyone have any suggestions for a better name?

Maybe PyObject_GetItem()? :-D Just kidding, PyObject_GetItem() already exists and works for any type (doesn't crash if it's not a dict) :-)

Well, that would just be 2 more functions on the top of the existing 9 functions to "get an item in dict":

• PyDict_GetItem()
• PyDict_GetItemString()
• PyDict_GetItemWithError()
• _PyDict_GetItemIdWithError()
• _PyDict_GetItemStringWithError()
• _PyDict_GetItemWithError()
• _PyDict_GetItem_KnownHash()

Ok, let me propose a name: PyDict_GetItemOkThisTimeItShouldBeCorrect() :-)

I already added the following functions with the "Ref" suffix:

• PyModule_AddObjectRef()
• PyImport_AddModuleRef()
• PyWeakref_GetRef()

The "Ref" is here to remain that this one it returns a strong reference.

And also: Py_NewRef() and Py_XNewRef() (not really a "strong reference" variant of an existing function).

Testing the result is always at least as efficient as doing a test on *pvalue.
You need to handle all three cases, so there needs to be two tests regardless.

We made python/devguide#1121 to avoid having to repeat this discussion for every new API function.
If you think returning 0 for both found and not-found is better, can you explain why on that issue. Thanks.

I already added the following functions with the "Ref" suffix:

• PyModule_AddObjectRef()
• PyImport_AddModuleRef()
• PyWeakref_GetRef()

Can we please stop adding new functions to the C-API until we have established conventions.
Otherwise, we will need to add yet another variant that conforms to the naming convention.

I modified the API to return 1 when the key is present (and 0 when the key is missing).

Can we please stop adding new functions to the C-API until we have established conventions. Otherwise, we will need to add yet another variant that conforms to the naming convention.

I created capi-workgroup/problems#52 to discuss C API naming convention (when adding new functions).

No, I'm not OK with the proposed API. I've already said that.

Using PyObject * is needlessly throwing away type information, and we haven't established naming conventions yet.
Let's agree on the rules first, before adding more functions that will need to be changed to the API.

What's the rush?

TBH, I don't think you should have opened this PR until there was some feedback on the issue.

Using PyObject * is needlessly throwing away type information,

I replied there: #106005 (comment)

we haven't established naming conventions yet.

I was confused by @gpshead's comment: "I don't think it matters which naming scheme we pick or even if it winds up wholly consistent."

What's the rush?

There is no rush. I just wanted to understand the status of this PR.

commenting here to tell github to remove my approval as this PR seems to be going in exploratory directions and is awaiting various feedback and decisions.

This PR is ready for review, so I remove the draft status again.

@serhiy-storchaka @gpshead @erlend-aasland: Are you ok with the API using PyDictObject* type? Or should I revert this 3rd commit?

Oh. @serhiy-storchaka and @gpshead seem to be strongly against it, so I reverted the change to go back to the initial plan: PyObject* type.

I tried to use a revert, but sadly the Git history became too complicated, so instead of squashed commits and I rebased my PR. Sorry about that :-(

Gregory:

commenting here to tell github to remove my approval as this PR seems to be going in exploratory directions and is awaiting various feedback and decisions.

Well, I was requested to set multiple guidelines for different aspects of the API on this single function addition.

• Return 1 if the key is present?

  • My first version returned 0 if the key is present and if the key is missing, both considered as "success".
  • A guideline was added to the devguide: Document the preferred style for API functions with three, four or five-way returns devguide#1121
  • I modified my PR to return 1 if the key is present (and return 0 if the key is missing).

• Function name

  • My PR adds Ref suffix to the existing function
  • @markshannon dislikes the name and asked to define a naming convention for new functions.
  • I created: "Naming convention" Naming convention for new C API functions capi-workgroup/problems#52
  • @serhiy-storchaka proposed PyDict_GetOptionalItem() but is ok with PyDict_GetItemRef().
  • PEP 703 proposes PyDict_FetchItem() name.
  • The majority seems to be fine with the proposed name: PyDict_GetItemRef().

• Type of the first parameter

  • My first version used PyObject*
  • @markshannon wants PyDictObject*, he created discussion: The C API is weakly typed capi-workgroup/problems#31
  • I changed the parameter type to PyDictObject* to see the consequences of the change: replace runtime check with assertion, remove tests on invalid dict type (since it would crash rather than raising a clean SystemError exception).
  • @serhiy-storchaka and @gpshead want PyObject*: they are against the need for cast between PyObject* and PyDictObject* and do prefer type checking at runtime, since it's too rare that developers have access to a Python binary with assertions enabled
  • After @serhiy-storchaka and @gpshead pushback: I reverted this change, to come back to my initial plan: PyObject*.

• @markshannon would prefer to define some guidelines first, before proposing adding a concrete function:

  • "I'm opposed because making ad-hoc changes like this is going to make the C-API worse, not better."

• Since @markshannon has some disagreements, @gpshead opened a discussion at the Steering Council decision: Should we add non-borrowed-ref public C APIs, if so, is there a naming convention? steering-council#201

PR changelog:

• First version
• Return 1 if the key is present
• Change parameter name from pvalue to result (and simplify dictitems_contains() implementation)
• Change first parameter type to PyDictObject**
• Move the comments from C (dictobject.c) to header file (dictobject.h)
• Revert the parameter type change: come back to PyObject*

I did not propose PyDict_GetOptionalItem() seriously, it would look confusing taking into account that other functions also return an "optional" item. I accept PyDict_GetItemRef() as token name and will accept any name whichever you choose. No need to bakeshed it to death.

I would be fine with the absent of runtime type check, not every C API function has a runtime type check, but the existing functions PyDict_GetItem() and PyDict_GetItemWithError() have a runtime type check, so why this function should be special? And compile time type check would just not work.

I confirm my approve.

Confirming my approval.

I believe introducing more type structs in the Limited API is a very bad decision, so I'm glad that particular change was reverted; I prefer to stick with the existing API convention of PyObject * only. For the "Brand New API", whatever that will be, a generic "PyObject-like struct" might not be the best option.

Regarding naming, I have no preference. Personally, I think I would have considered the PEP 703 naming to try and create slightly less merge conflict havoc for Sam when if nogil is going to be merged.

FWIW, here's a possible new variant: you could set result to NULL in which case the result isn't stored/incref'd. And that would start a convention of how to turn a get operation into a membership test. (And the Lookup name would fit that better.)

If this function is to take PyObject *, as @erlend-aasland seems to insist, then it shouldn't raise a SystemError when passed something other than a dict. It should raise a TypeError.

PyDict_GetItem() and PyDict_GetItemWithError() raise a SystemError. All other functions in the Concrete Objects Layer either raise SystemError or have an undefined behavior. It is not specified what the behavior of the concrete function is, but passing an object of correct type is a requirement. https://docs.python.org/3/c-api/concrete.html

I like SystemError. I used it in the past to detect bugs in C extensions, since TypeError is too common, so it was easy to distinguish "normal" TypeError from "invalid" SystemError. For me, SystemError means "bad API usage".

If this function is to take PyObject *, as @erlend-aasland seems to insist, then it shouldn't raise a SystemError when passed something other than a dict. It should raise a TypeError.

IMO if TypeError is preferred, we should try to keep the whole C API consistent and raise TypeError in all similar situations, not only for newly added functions. Otherwise, it can be misleading.

FWIW, here's a possible new variant: you could set result to NULL in which case the result isn't stored/incref'd. And that would start a convention of how to turn a get operation into a membership test. (And the Lookup name would fit that better.)

For me, it's very surprising that the purpose of a function change "that much" depending on a parameter. For example, in Python, getattr() and hasattr() are two different functions: you cannot pass None to getattr() to check if an attribute exists.

I prefer to have separated functions to "get" an item/attribute and to check if an object "has" an item/attribute.

I dislike having to use locale.setlocale(loc, None) to get a locale :-( I don't want to use locale.getlocale(loc) since it returns different different.

This issue goes in all directions, it's a little bit hard to follow :-(

FWIW, here's a possible new variant: you could set result to NULL in which case the result isn't stored/incref'd.

How does this differ from PyDict_Contains()?

my 2 cents on exception types: If it is a bug in code at the C API level, SystemError makes sense. If it is a bug in code at the Python level, TypeError makes sense. Somewhat of a coin toss on some C APIs when the input in question can have come directly from the Python level (turning the question into one of who is responsible for pre-checking the type). This is also the kind of thing we can change in the future when doing a cleanup sweep of API behaviors to change some SystemErrors coming from PyErr_BadInternalCall into TypeError as no code is ever expected to catch and specifically handle SystemError.

For now, lets let this PR sit while we wait for steering council answers.

my 2 cents on exception types: If it is a bug in code at the C API level, SystemError makes sense. If it is a bug in code at the Python level, TypeError makes sense. Somewhat of a coin toss on some C APIs when the input in question can have come directly from the Python level (turning the question into one of who is responsible for pre-checking the type). This is also the kind of thing we can change in the future when doing a cleanup sweep of API behaviors to change some SystemErrors coming from PyErr_BadInternalCall into TypeError as no code is ever expected to catch and specifically handle SystemError.

PyObject_GetItem() is the safe generic API: it raises TypeError if the API is misused, if the first argument is not a mapping (or a sequence).

PyDict_GetItemWithError() is the fast specialized API: it raises SystemError if it's misused, if the first argument is not a dict or a dict subtype.

I rebased my PR on the main branch to fix conflicts.

Thanks everyone for the great reviews. I think that the final API is better than my first version.

The road for that function was more bumpy than for other functions since PyDict_GetDict() is one of the commonly used function, and as I wrote before, we took this function as an opportunity to revisit some API design choices. There are some disagreements which have been discussed in length, especially in the https://github.com/capi-workgroup/problems/issues/ project. Overall, the majority seems to be in favor of this change (and I didn't see any concrete counter-proposal to solve the issue).

Sadly, this PR lost two approvals in the bumpy discussion. IMO it's now time to move on and see how this function can be used to avoid bugs and how to migrate users from the cursed PyDict_GetItem() to that better PyDict_GetItemRef() function.

Supporters of a new API instead of fixing the current API one function by one, I suggest continuing the discussion in https://github.com/capi-workgroup/problems/issues/ : there are a few issues related to that. So far, I didn't see any clear nor complete proposal, so for me, we are still at the same status quo: we do our best, fixing functions, one by one, when we agree that there is an issue. Same for the question of using PyDictObject* type rather than PyObject* for the first parameter: it's still being discussed and so far, no consensus was reached.

Completely getting rid of PyDict_GetItem() may take time. Maybe we need to consider capi-workgroup/problems#54 approach for users who want to start a new C API with a clean C API without known issues like borrowed references. But well, that's out of the scope of this issue. This issue does not deprecate PyDict_GetItem() on purpose.

If we keep these new functions in Python, IMO we should consider replacing usage of the private _PyDict_GetItemStringWithError() with the public PyDict_GetItemStringRef(). And maybe even remove _PyDict_GetItemStringWithError() later.