gh-106307: Add PyMapping_GetOptionalItem()

[serhiy-storchaka]

Replacement of PyObject_GetItem() which doesn't raise KeyError.

int _PyMapping_LookupItem(PyObject *obj, PyObject *key, PyObject **result)

Return 1 and set *result != NULL if a key is found.
Return 0 and set *result == NULL if a key is not found; a KeyError is silenced.
Return -1 and set *result == NULL if an error other than KeyError is raised.

• Issue: Add PyMapping_GetOptionalItem() #106307

[erlend-aasland]

Out of interest, why not explicitly _PyErr_Clear(tstate) if _PyMapping_LookupItem returns -1?

[serhiy-storchaka]

Because the current code does not clear arbitrary errors (and doing this would be wrong). It only clears KeyError, what is done internally by _PyMapping_LookupItem.

[carljm]

The bytecodes.c cleanups alone (i.e. the existing cases where we cared enough about perf to special-case the exact-dict fast path) clearly show the value of this API! Now all call sites can get the same perf benefit, with much less verbosity.

[carljm]

To me, "KeyError is silenced" suggests internal implementation details that the caller doesn't need to care about, and b) may or may not be true (in the exact-dict case, no KeyError is silenced because it isn't set in the first place). IMO "no KeyError is raised" is a clearer statement of the contract that is relevant to the caller.

Suggested change

	Return ``1`` and set ``*result != NULL`` if a key is found.
	Return ``0`` and set ``*result == NULL`` if a key is not found;
	a :exc:`KeyError` is silenced.
	Return ``-1`` and set ``*result == NULL`` if an error other than
	Return ``1`` and set ``*result != NULL`` if the key is found.
	Return ``0`` and set ``*result == NULL`` if the key is not found;
	the :exc:`KeyError` is silenced.
	Return ``-1`` and set ``*result == NULL`` if an error other than

[carljm]

On second thought, it may be important to specify "the KeyError is silenced" to clarify the behavior with a custom __getitem__ method that raises KeyError explicitly. I take back that part of the suggestion; I would just replace all three instances of a with the.

[vstinner]

I dislike specify that *pvalue is set to NULL on error. I would prefer to leave it undefined.

Code like (void)PyMapping_GetOptionalItem(modules, name, &m); would be unsafe: I prefer to handle explicitly the error case and no rely on m value.

In the implementation, I prefer to set it to NULL to avoid... the undefined behavior :-) Maybe in debug mode we could even set the value to a marker value to detect the usage of the undefined value.

[serhiy-storchaka]

I disagree. It will require explicitly setting result = NULL before every call of this function, and if you forgot to do this, it will crash your code in rare case of error, which may be not covered by tests. The risk of writing errorenous code is too high even if there would be a benefit from not setting the result to NULL.

Also, it makes the code simpler. Instead of saving the return code into a variable and checking it in two if\ s

int rc = PyMapping_GetOptionalItem(...);
if (rc < 0) {
    goto error;
}
if (rc == 0) {
    // handle default
}

you can get rid of that variable and insert the call directly in the if.

if (PyMapping_GetOptionalItem(...) < 0) {
    goto error;
}
if (value == NULL) {
    // handle default
}

And there are many other cases in which it is convenient that you have two ways to check for result. I suggest you to make PyMapping_GetOptionalItem() not setting the result to NULL on error and to look how much code it will require to rewrite and how much uglier it will become. It is the same for PyObject_GetOptionalAttr.

[erlend-aasland]

I disagree. It will require explicitly setting result = NULL before every call of this function, and if you forgot to do this, it will crash your code in rare case of error, which may be not covered by tests.

+1 and slightly related, I have a PR for the devguide repo regarding explicitly clearing output parameters: python/devguide#1129 python/devguide#1128

cc. @serhiy-storchaka, @carljm

[vstinner]

I dislike specify that *pvalue is set to NULL on error. I would prefer to leave it undefined.

Code like (void)PyMapping_GetOptionalItem(modules, name, &m); would be unsafe: I prefer to handle explicitly the error case and no rely on m value.

In the implementation, I prefer to set it to NULL to avoid... the undefined behavior :-) Maybe in debug mode we could even set the value to a marker value to detect the usage of the undefined value.

[vstinner]

LGTM