[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

[STINNER Victor]

STINNER Victor added the comment:

Sorry, I didn't have time to review this issue :-(

Thanks Xiang and Serhiy for fixing the issue! I prefer the new API (don't
ignore the error).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

[Serhiy Storchaka]

Changes by Serhiy Storchaka :


--
resolution:  -> fixed
stage: commit review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

[Roundup Robot]

Roundup Robot added the comment:

New changeset d06a6b0fd992 by Serhiy Storchaka in branch '3.6':
Issue #28123: _PyDict_GetItem_KnownHash() now can raise an exception as
https://hg.python.org/cpython/rev/d06a6b0fd992

New changeset 805467de22fc by Serhiy Storchaka in branch 'default':
Issue #28123: _PyDict_GetItem_KnownHash() now can raise an exception as
https://hg.python.org/cpython/rev/805467de22fc

--
nosy: +python-dev

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

[INADA Naoki]

INADA Naoki added the comment:

LGTM.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

LGTM. I'll commit the patch soon if there are no comments from other core 
developers.

--
stage: patch review -> commit review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

[Xiang Zhang]

Xiang Zhang added the comment:

> If _PyDict_GetItem_KnownHash() returns an error, it is very likely that 
> following insertdict() with same key will return an error.

Make sense.

--
assignee: haypo -> serhiy.storchaka
Added file: http://bugs.python.org/file45336/issue28123_v6.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

If _PyDict_GetItem_KnownHash() returns an error, it is very likely that 
following insertdict() with same key will return an error. I would prefer to 
return an error right after _PyDict_GetItem_KnownHash() is failed. This would 
look more straightforward.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

[Xiang Zhang]

Changes by Xiang Zhang :


Added file: http://bugs.python.org/file45299/issue28123_v5.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

[Xiang Zhang]

Xiang Zhang added the comment:

dict_merge was altered after the patch. I make it ignore explicitly the error 
now, to not affect former behaviour.

Serhiy, I apply your suggestion to use _PyLong_AsByteArray for Py_hash_t, but I 
am not familiar with the API. It needs a review.

--
Added file: http://bugs.python.org/file45291/issue28123_v4.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

[Serhiy Storchaka]

Changes by Serhiy Storchaka :


--
priority: normal -> high
stage:  -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Actually ignoring exceptions in _PyDict_GetItem_KnownHash causes a subtle 
difference between Python and C implementations. Making 
_PyDict_GetItem_KnownHash not ignoring exceptions looks right thing.

But dict_merge expects that _PyDict_GetItem_KnownHash don't raise an exception.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

[Stéphane Wirtel]

Changes by Stéphane Wirtel :


--
assignee:  -> haypo

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

[Xiang Zhang]

Xiang Zhang added the comment:

Victor, v3 now applies your suggestion.

--
Added file: http://bugs.python.org/file44713/issue28123_v3.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

[Serhiy Storchaka]

Changes by Serhiy Storchaka :


--
nosy: +rhettinger, serhiy.storchaka

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

[Xiang Zhang]

Xiang Zhang added the comment:

Hah, Okay. I'll make the corresponding change then.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

[STINNER Victor]

STINNER Victor added the comment:

Xiang Zhang: "I am still afraid changing it may break knowledge of
devs that are already familiar with dict APIs"

Come on, the function is only called 4 times in the whole code base,
and the function is private. Private means that we are free to break
its behaviour anytime, it's part of the contract.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

[Xiang Zhang]

Xiang Zhang added the comment:

Yes, ignoring exceptions is due to historical reasons. Although it's used 
rarely but I am still afraid changing it may break knowledge of devs that are 
already familiar with dict APIs. And what's more is there already exists two 
versions of getitem, one version with no exceptions and one version propagates 
exceptions (witherror), maybe we can also introduce a 
_PyDict_GetItem_KnownHashWithError?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

[STINNER Victor]

STINNER Victor added the comment:

If __eq__() raise an exception, _PyDict_GetItem_KnownHash() currently returns 
NULL and pass through the exception. To me, it looks like the correct behaviour.

With your patch, it looks like the _PyDict_GetItem_KnownHash() clears the 
__eq__() exception: return NULL with no exception set, as if the key is simply 
missing.

PyDict_GetItem() ignores *almost* all exceptions, but for bad reasons:

/* Note that, for historical reasons, PyDict_GetItem() suppresses all errors
 * that may occur (originally dicts supported only string keys, and exceptions
 * weren't possible).  (...) */

I would prefer to not ignore __eq__ exceptions, but pass them through.

To be clear: this is a behaviour change compared to Python 3.5 which works as 
PyDict_GetItem(), ignore *all* exceptions:

ep = (mp->ma_keys->dk_lookup)(mp, key, hash, _addr);
if (ep == NULL) {
PyErr_Clear();
return NULL;
}

I consider that it's ok to change _PyDict_GetItem_KnownHash() behaviour because 
this function is private and only used 4 times in 250k lines of C code.

Would you be interested to write a different patch to pass through the 
exception?

Note: It seems like _count_elements() (Modules/_collectionsmodule.c) doesn't 
handle correctly such error.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

[Xiang Zhang]

Xiang Zhang added the comment:

Update the patch with unittest.

--
Added file: http://bugs.python.org/file44655/issue28123_v2.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

[STINNER Victor]

STINNER Victor added the comment:

Xiang Zhang added the comment:
> How about let PyDict_GetItem call it?

PyDict_GetItem() is like the most important function in term of
performance for Python. If you want to touch it, you must benchmark
it.

I would prefer to keep it as it is.

> Just like the relationship of delitem and delitem_knownhash.

delitem is less important in term of performance, so I decided to
accept Naoki's change to merge duplicated code.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

[Xiang Zhang]

Xiang Zhang added the comment:

How about let PyDict_GetItem call it? Just like the relationship of delitem and 
delitem_knownhash. You can see they share most codes. If we do that, it seems 
we can easily write a test(or there has already been a test) for it.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

[STINNER Victor]

STINNER Victor added the comment:

> _PyDict_GetItem_KnownHash is not invoked by any other dict methods.

Oh, I missed that. In this case, I suggest you to expose the function at Python 
level using the _testcapi module. And then use 
_testcapi._PyDict_GetItem_KnownHash() in test_dict.

It would be nice to have unit tests on _PyDict_GetItem_KnownHash(), this 
function starts to become important in Python.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

[Xiang Zhang]

Xiang Zhang added the comment:

_PyDict_GetItem_KnownHash is not invoked by any other dict methods. So to 
achieve it in pure Python level, we have to rely on others modules and objects 
such as OrderedDict, lru_cache. Is it a good idea to rely on those?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

[STINNER Victor]

STINNER Victor added the comment:

I understand the the code doesn't handle correctly lookup failures. Such 
failure is easy to trigger in pure Python using a custom __eq__() method for 
example. Can you please write an unit test for it?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

[Xiang Zhang]

Xiang Zhang added the comment:

Hmm, I thought this is trivial so I didn't. Now upload the file patch ;).

--
keywords: +patch
Added file: http://bugs.python.org/file44625/issue28123.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

[STINNER Victor]

STINNER Victor added the comment:

Please create a patch file and attach it to the issue, so we can review it more 
easily.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

[Xiang Zhang]

New submission from Xiang Zhang:

_PyDict_GetItem_KnownHash should handle dk_lookup return value the same as 
PyDict_GetItem.

BTW, it seems PyDict_GetItem can call _PyDict_GetItem_KnownHash to remove 
duplicate code, if you like, maybe another issue?

diff -r 6acd2b575a3c Objects/dictobject.c
--- a/Objects/dictobject.c  Tue Sep 13 07:56:45 2016 +0300
+++ b/Objects/dictobject.c  Tue Sep 13 17:46:08 2016 +0800
@@ -1370,12 +1370,12 @@
 ix = (mp->ma_keys->dk_lookup)(mp, key, hash, _addr, NULL);
 /* ignore errors */
 PyErr_Restore(err_type, err_value, err_tb);
-if (ix == DKIX_EMPTY)
+if (ix < 0)
 return NULL;
 }
 else {
 ix = (mp->ma_keys->dk_lookup)(mp, key, hash, _addr, NULL);
-if (ix == DKIX_EMPTY) {
+if (ix < 0) {
 PyErr_Clear();
 return NULL;

--
components: Interpreter Core
messages: 276230
nosy: haypo, methane, xiang.zhang
priority: normal
severity: normal
status: open
title: _PyDict_GetItem_KnownHash ignores DKIX_ERROR return
type: behavior
versions: Python 3.6, Python 3.7

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com