fix: use original dict

[henryiii]

Description

Pulled from #5646, I think this might make sense in general. By replacing __dict__, we throw away the original specialized dict that can do key sharing. We do need to update instead, but I think this makes sense. ChatGPT also liked it. https://chatgpt.com/share/6822d35a-c8b4-8012-8b97-6b387013ea09

Suggested changelog entry:

* Update the dict when restoring pickles, instead of assigning a replacement dict.

[rwgk]

For general background (I looked it up because I was curious), this code goes back to #2972, which in turn goes all the way back to commit c10ac6c.

I'm wondering: Could it be that v_h.inst does not already have a __dict__? — I don't know, TBH.

If it does not, the AttributeError will not be very helpful to most users.

What do you think about a more conservative change?

For example (untested):

    auto dict = getattr((PyObject *) v_h.inst, "__dict__", py::none());
    if (dict.is_none()) {
        setattr((PyObject *) v_h.inst, "__dict__", d);
    } else {
        if (PyDict_Update(dict.ptr(), d.ptr()) < 0) {
            throw error_already_set();
        }
    }

(I think so close to a major release, it's best to err on the conservative side.)

---

After writing the above, I ran it by ChatGPT, this came back:

---

Technical Insight: Is __dict__ guaranteed?

In general, no, an instance's __dict__ is not guaranteed to exist until it has been accessed or an attribute has been set on the instance. Specifically:

• Many built-in types and C extension types do not have __dict__ unless they were defined with tp_dictoffset, and even then it may be nullptr until first use.

• Even for py::class_-wrapped user-defined classes, the backing C++ instance might not have initialized its __dict__ until Python accesses it.

• getattr(obj, "__dict__") raises AttributeError if no __dict__ exists or is inaccessible.

---

I'm not sure if all of this is actually true, for Python 3.8+, but it makes me feel even stronger that the conservative approach is much less risky.

[henryiii]

I think it is, at least I know it is if py::dynamic_attr is set. If it's not set, I don't think you can run this directly anyway, but I'll go the conservative route just in case for now.

[rwgk]

Might be useful to leave a comment, e.g.

// Possibly this is never needed and could/should be removed (see #5658).

[henryiii]

I did but that might be a better way to put it

[rwgk]

Pulled from #5646

What do you think about also pulling in this (ci.yml)?

-        runs-on: [ubuntu-22.04, windows-2022, macos-13]
+        runs-on: [ubuntu-24.04, windows-2022, macos-13]

[henryiii]

It's in #5657.