bpo-24275: Don't downgrade unicode-only dicts to mixed on lookups #25186
Conversation
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA). Recognized GitHub usernameWe couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames: This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. You can check yourself to see if the CLA has been received. Thanks again for the contribution, we look forward to reviewing it! |
There was a problem hiding this comment.
Thanks for taking the time to make this PR! Dictionaries aren't my area of expertise, but I have some comments in other areas:
- This probably needs a news entry in
Misc/News, you can see how to do that in step 6 of the devguide's quick reference - Some small nits in the tests.
| @@ -1471,6 +1471,83 @@ def test_dict_items_result_gc(self): | |||
| gc.collect() | |||
| self.assertTrue(gc.is_tracked(next(it))) | |||
|
|
|||
| def test_str_nonstr(self): | |||
There was a problem hiding this comment.
Since this test depends on a CPython implementation detail,
| def test_str_nonstr(self): | |
| @support.cpython_only | |
| def test_str_nonstr(self): |
There was a problem hiding this comment.
@support.cpython_only seems to be used on tests rely on cpython implementation details and may fail if another implementation is used. This test makes sure that a particular cpython implementation detail cannot be observed from python code so it should pass on all implementations.
There was a problem hiding this comment.
Thanks for the explanation! Btw, you don't have to force-push (the core dev will squash before merging). Committing normally without force-pushing makes the changes easier to track.
| d = {key: 42 + i for i,key in enumerate(['key1', 'key2', key3])} | ||
| dicts.append(d) | ||
|
|
||
| for d in dicts: |
There was a problem hiding this comment.
This might be a good fit for unittest.subTest:
https://docs.python.org/3/library/unittest.html#subtests
There was a problem hiding this comment.
I didn't know about unittest.subTest. Thanks!
| @@ -1084,7 +1088,6 @@ insertdict(PyDictObject *mp, PyObject *key, Py_hash_t hash, PyObject *value) | |||
| if (ix == DKIX_ERROR) | |||
| goto Fail; | |||
|
|
|||
| assert(PyUnicode_CheckExact(key) || mp->ma_keys->dk_lookup == lookdict); | |||
There was a problem hiding this comment.
Maybe put this assertion after MAYBE_NONUNICODE when it will be true.
There was a problem hiding this comment.
I couldn't find a good place for this assertion. After MAYBE_NONUNICODE, the condition is obviously true.
|
I think it is worth a news label, but I'm biased. If the submitter ( @hvenev ?) wants to skip news because it is an implementation detail, that seems OK too. With the feature freeze coming this weekend, I would like this to get in. |
https://bugs.python.org/issue24275