Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.1k
gh-132617: Fixdict.update() mutation check#134815
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
Use `ma_used` instead of `ma_keys->dk_nentries` for modification checkso that we only check if the dictionary is modified, not if new keys areadded to a different dictionary that shared the same keys object.
dict.update() mutation checkdict.update() mutation checkLib/test/test_dict.py Outdated
| # Create an object that shares the same PyDictKeysObject as | ||
| # the dict |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm confused by this comment and the blurb. I think the instances ofMyClass (obj andobj2) share the same keys object, but that keys object is not shared withx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I updated the comment to make it explicit that it's the same keys object asobj.__dict__.
The mutation check is on the argument todict.update(). I don't think there's any check on the receiver.
| @@ -3858,7 +3858,7 @@ dict_dict_merge(PyInterpreterState *interp, PyDictObject *mp, PyDictObject *othe | |||
| } | |||
| } | |||
| Py_ssize_t orig_size = other->ma_keys->dk_nentries; | |||
| Py_ssize_t orig_size = other->ma_used; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ma_used is too weak. It cannot detectdel d["a"]; d["b"] = None.
For better check:
- For all table, check
other->ma_keysis not changed. - For combined table, check
other->ma_keys->dk_nentriesis not changed. - For split table, check
other->ma_values->sizeis not changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's the same condition we use in iterators and elsewhere. I don't think we should strengthen any of the checks. These are heuristic checks, they're not going to be perfect, but if we decide to do so, I think we should do that in a subsequent PR and not backport that change.
Lines 5169 to 5174 in469a564
| if (di->di_used!=d->ma_used) { | |
| PyErr_SetString(PyExc_RuntimeError, | |
| "dictionary changed size during iteration"); | |
| di->di_used=-1;/* Make this state sticky */ | |
| returnNULL; | |
| } |
Uh oh!
There was an error while loading.Please reload this page.
Use
ma_usedinstead ofma_keys->dk_nentriesfor modification check so that we only check if the dictionary is modified, not if new keys are added to a different dictionary that shared the same keys object.dict.update()mutation check too broad #132617