From 3c83439404babca7d8417a804bb28db461eae78c Mon Sep 17 00:00:00 2001 From: Raymond Hettinger Date: Fri, 19 Apr 2019 15:33:13 -1000 Subject: [PATCH 1/4] bpo-36650: Fix handling of empty keyword args in C version of lru_cache. --- Lib/test/test_functools.py | 13 +++++++++++++ .../2019-04-19-15-29-55.bpo-36650._EVdrz.rst | 4 ++++ Modules/_functoolsmodule.c | 2 +- 3 files changed, 18 insertions(+), 1 deletion(-) create mode 100644 Misc/NEWS.d/next/Library/2019-04-19-15-29-55.bpo-36650._EVdrz.rst diff --git a/Lib/test/test_functools.py b/Lib/test/test_functools.py index 4b2b9ab61fa7564..25cd8dc020b8b00 100644 --- a/Lib/test/test_functools.py +++ b/Lib/test/test_functools.py @@ -1271,6 +1271,19 @@ def f(x): self.assertEqual(f(20), '.20.') self.assertEqual(f.cache_info().currsize, 10) + def test_lru_bug_36650(self): + # C version of lru_cache was treating a call with an empty **kwargs + # dictionary as being distinct from a call with no keywords at all. + # This did not result in an incorrect answer, but it did trigger + # an unexpected cache miss which broke a doctest for a method cache. + @self.module.lru_cache() + def f(x): + pass + + f(0) + f(0, **{}) + self.assertEqual(f.cache_info().hits, 1) + def test_lru_hash_only_once(self): # To protect against weird reentrancy bugs and to improve # efficiency when faced with slow __hash__ methods, the diff --git a/Misc/NEWS.d/next/Library/2019-04-19-15-29-55.bpo-36650._EVdrz.rst b/Misc/NEWS.d/next/Library/2019-04-19-15-29-55.bpo-36650._EVdrz.rst new file mode 100644 index 000000000000000..165103fc87f8c8e --- /dev/null +++ b/Misc/NEWS.d/next/Library/2019-04-19-15-29-55.bpo-36650._EVdrz.rst @@ -0,0 +1,4 @@ +The C version of functools.lru_cache() was treating calls with an empty +*kwargs dictionary as being distinct from calls with no keywords at all. +This did not result in an incorrect answer, but it did trigger an unexpected +cache miss. diff --git a/Modules/_functoolsmodule.c b/Modules/_functoolsmodule.c index 3f1c01651ded1bd..f1181194795c264 100644 --- a/Modules/_functoolsmodule.c +++ b/Modules/_functoolsmodule.c @@ -751,7 +751,7 @@ lru_cache_make_key(PyObject *args, PyObject *kwds, int typed) Py_ssize_t key_size, pos, key_pos, kwds_size; /* short path, key will match args anyway, which is a tuple */ - if (!typed && !kwds) { + if (!typed && (!kwds || PyDict_GET_SIZE(kwds) == 0)) { if (PyTuple_GET_SIZE(args) == 1) { key = PyTuple_GET_ITEM(args, 0); if (PyUnicode_CheckExact(key) || PyLong_CheckExact(key)) { From d08a9a6e0913bddbe69e96cdcc05ff0284d17bc7 Mon Sep 17 00:00:00 2001 From: Raymond Hettinger Date: Fri, 19 Apr 2019 15:57:00 -1000 Subject: [PATCH 2/4] Fix reST markup --- .../next/Library/2019-04-19-15-29-55.bpo-36650._EVdrz.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Library/2019-04-19-15-29-55.bpo-36650._EVdrz.rst b/Misc/NEWS.d/next/Library/2019-04-19-15-29-55.bpo-36650._EVdrz.rst index 165103fc87f8c8e..de10575fc2720eb 100644 --- a/Misc/NEWS.d/next/Library/2019-04-19-15-29-55.bpo-36650._EVdrz.rst +++ b/Misc/NEWS.d/next/Library/2019-04-19-15-29-55.bpo-36650._EVdrz.rst @@ -1,4 +1,4 @@ The C version of functools.lru_cache() was treating calls with an empty -*kwargs dictionary as being distinct from calls with no keywords at all. +``**kwargs`` dictionary as being distinct from calls with no keywords at all. This did not result in an incorrect answer, but it did trigger an unexpected cache miss. From a36d10707b37f062b763c4987cf0363e59397bd7 Mon Sep 17 00:00:00 2001 From: Raymond Hettinger Date: Fri, 19 Apr 2019 21:43:22 -1000 Subject: [PATCH 3/4] Move up definition of kwds_size so that it can be reused --- Modules/_functoolsmodule.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Modules/_functoolsmodule.c b/Modules/_functoolsmodule.c index f1181194795c264..23caced7293ec4d 100644 --- a/Modules/_functoolsmodule.c +++ b/Modules/_functoolsmodule.c @@ -751,7 +751,8 @@ lru_cache_make_key(PyObject *args, PyObject *kwds, int typed) Py_ssize_t key_size, pos, key_pos, kwds_size; /* short path, key will match args anyway, which is a tuple */ - if (!typed && (!kwds || PyDict_GET_SIZE(kwds) == 0)) { + kwds_size = kwds ? PyDict_GET_SIZE(kwds) : 0; + if (!typed && !kwds_size) { if (PyTuple_GET_SIZE(args) == 1) { key = PyTuple_GET_ITEM(args, 0); if (PyUnicode_CheckExact(key) || PyLong_CheckExact(key)) { @@ -764,8 +765,6 @@ lru_cache_make_key(PyObject *args, PyObject *kwds, int typed) Py_INCREF(args); return args; } - - kwds_size = kwds ? PyDict_GET_SIZE(kwds) : 0; assert(kwds_size >= 0); key_size = PyTuple_GET_SIZE(args); From 6bfc0537ca228c73ba95a6b1208b28c6dea02917 Mon Sep 17 00:00:00 2001 From: Raymond Hettinger Date: Sat, 20 Apr 2019 06:30:52 -1000 Subject: [PATCH 4/4] Apply suggestions from PR reviewers --- Lib/test/test_functools.py | 3 ++- Modules/_functoolsmodule.c | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_functools.py b/Lib/test/test_functools.py index 25cd8dc020b8b00..98908405e1401ae 100644 --- a/Lib/test/test_functools.py +++ b/Lib/test/test_functools.py @@ -1275,7 +1275,8 @@ def test_lru_bug_36650(self): # C version of lru_cache was treating a call with an empty **kwargs # dictionary as being distinct from a call with no keywords at all. # This did not result in an incorrect answer, but it did trigger - # an unexpected cache miss which broke a doctest for a method cache. + # an unexpected cache miss. + @self.module.lru_cache() def f(x): pass diff --git a/Modules/_functoolsmodule.c b/Modules/_functoolsmodule.c index 23caced7293ec4d..dcc9129fc6b1834 100644 --- a/Modules/_functoolsmodule.c +++ b/Modules/_functoolsmodule.c @@ -750,8 +750,9 @@ lru_cache_make_key(PyObject *args, PyObject *kwds, int typed) PyObject *key, *keyword, *value; Py_ssize_t key_size, pos, key_pos, kwds_size; - /* short path, key will match args anyway, which is a tuple */ kwds_size = kwds ? PyDict_GET_SIZE(kwds) : 0; + + /* short path, key will match args anyway, which is a tuple */ if (!typed && !kwds_size) { if (PyTuple_GET_SIZE(args) == 1) { key = PyTuple_GET_ITEM(args, 0); @@ -765,7 +766,6 @@ lru_cache_make_key(PyObject *args, PyObject *kwds, int typed) Py_INCREF(args); return args; } - assert(kwds_size >= 0); key_size = PyTuple_GET_SIZE(args); if (kwds_size)