Reject non-mappings missing keys during dictionary conversion - #1309

Mmmh. PyMapping_Check only checks for the presence of __getitem__, yet PyDict_Merge requires PyMapping_Keys to be implemented for it. I would expect that everything passing PyMapping_Check to be accepted by PyDict_Merge, but its docstring says

Iterate over mapping object b adding key-value pairs to dictionary a. b may be a dictionary, or any object supporting PyMapping_Keys() and PyObject_GetItem()

so they have slightly different definitions of what constitute a mapping. @sobolevn do you happen to know if there's a rationale for this?

Otherwise, I'm inclined to say we maybe should support "mappings" without .keys()?

I'll leave it up to the msgspec maintainers to decide on how it handles mappings and key-less mappings.

My goal for this PR was very narrow: The current dict conversion requires .keys(). This keeps current behavior for those that have it and gives inputs that lack it a ValidationError instead of raising AttributeError.

@provinzkraut I don't know the exact reason, but I would say that legacy is probably a good guess :)

Maybe we should switch to https://docs.python.org/3/c-api/dict.html#c.PyDict_Update instead? It does not require .keys to be implemented.

Checking for .keys and requiring it does not feel right.

Maybe we should switch to https://docs.python.org/3/c-api/dict.html#c.PyDict_Update instead? It does not require .keys to be implemented.

Checking for .keys and requiring it does not feel right.

We should! Its docs say

This is the same as PyDict_Merge(a, b, 1)

which is how we call it, so it feels like the proper fix for this.

@maldoinc would you be willing to make that change in this PR? Otherwise I suggest we close this one and implement the other fix in a new PR.

Unless I'm misunderstanding the ask here is to use PyDict_Update in convert_mapping_to_dict. I removed the keys() check and tried the following but it doesn't fix the issue.

static PyObject *
convert_mapping_to_dict(
    ConvertState *self, PyObject *obj, TypeNode *type, PathNode *path
) {
    PyObject *out = NULL;
    PyObject *temp = PyDict_New();
    if (temp == NULL) return NULL;
-    if (PyDict_Merge(temp, obj, 1) == 0) {
+    if (PyDict_Update(temp, obj) == 0) {
        out = convert_dict_to_dict(self, temp, type, path);
    }
    Py_DECREF(temp);
    return out;
}

After running the test suite via just rebuild=true test the tests fail.

tests/unit/test_convert.py:1232: AttributeError
========================================================= short test summary info =========================================================
FAILED tests/unit/test_convert.py::TestDict::test_dict_getitem_without_keys[True] - AttributeError: 'Table' object has no attribute 'keys'
FAILED tests/unit/test_convert.py::TestDict::test_dict_getitem_without_keys[False] - AttributeError: 'Table' object has no attribute 'keys

Interesting. That means we require .keys somewhere else. I don't think we should.

Hm, looks like PyObject *keys = PyMapping_Keys(b); is now called in PyDict_Update :(
Which means that this is a bug in CPython :(

Wait, I misread it :)

int PyDict_Update(PyObject *a, PyObject *b)¶
This is the same as PyDict_Merge(a, b, 1) in C, and is similar to a.update(b) in Python except that
PyDict_Update() doesn’t fall back to the iterating over a sequence of key value pairs if the second argument has no “keys” attribute.

So, it means that according to the docs: PyDict_Merge must fallback to iterating over a sequence of key value pairs if the second argument has no “keys” attribute

But, it doesn't, does it?

This is a bug.

I created python/cpython#160669

Alright, let's leave this one open then until we can clarify upstream.

Wait (sorry, I am dumb today, I've spent the whole day working on vscode themes, it drained all of me), but how is that possible to merge a mapping without keys?

We don't know what to merge in this case. It is different from sequences, where indexes start from 0

Wait (sorry, I am dumb today, I've spent the whole day working on vscode themes, it drained all of me), but how is that possible to merge a mapping without keys?

We don't know what to merge in this case. It is different from sequences, where indexes start from 0

Well.. I guess you have a point there 😅
I was a bit too focused on the docs, and assumed I must have missed something.

Right, soo.. We keep the keys check, because it's actually needed (makes sense), and the CPython docs just need correcting.

Docs pipeline is failing with * [503] <https://github.com/msgspec/msgspec/blob/main/LICENSE> | Rejected status code (this depends on your "accept" configuration): Service Unavailable which seems transient. I can't rerun the job myself so if either of you could that be great.