Skip to content

gh-144030: add check that argument is callable to Python version of functools.lru_cache #144031

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cfbolz wants to merge 4 commits into
base: main
Choose a base branch
from
+13 −0
Open

gh-144030: add check that argument is callable to Python version of functools.lru_cache #144031

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c403793
gh-144030: check that argument is callable in py version of lru_cache
cfbolz Jan 19, 2026
3d579ea
add news entry
cfbolz Jan 19, 2026
d84cab3
Update Lib/test/test_functools.py
cfbolz Jan 19, 2026
914ff17
Update Misc/NEWS.d/next/Library/2026-01-19-12-48-59.gh-issue-144030.7…
cfbolz Jan 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Lib/functools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,6 +602,9 @@ def decorating_function(user_function):
return decorating_function

def _lru_cache_wrapper(user_function, maxsize, typed, _CacheInfo):
if not callable(user_function):
Copy link
Member

@picnixz picnixz Jan 19, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this check in lru_wrapper directly? it should only be done in def decorating_function() I think.

We are doing the check twice otherwise (when we do not call the decorator with ())

Copy link
Contributor Author

@cfbolz cfbolz Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that means we are doing the "is it callable" already twice then. in practice, cpython never uses the pure python version of this code, and the "is it callable" check happens in the C version of _lru_cache_wrapper. my change is just mirroring what the C code is already doing.

unless there is a huge push for it, I kind of don't want to do a big refactoring of the C/Python code here.

Copy link
Member

@picnixz picnixz Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no big refactoring actually. It is just about moving your check in another function (a few lines above). Considering the LRU wrapper cache function is already an implementation detail, I expect the caller to check for callability, not the wrapper itself.

Copy link
Contributor Author

@cfbolz cfbolz Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would definitely be a bigger change. if we move it to the caller, we would also have to touch the C code in _funtoolsmodule.c:

    if (!PyCallable_Check(func)) {
        PyErr_SetString(PyExc_TypeError,
                        "the first argument must be callable");
        return NULL;
    }

my goal in this PR is to get the behaviour of the C-version of _lru_cache_wrapper and the Python version of the same function into alignment (which they should have been from the start).

raise TypeError("the first argument must be callable")

# Constants shared by all lru cache instances:
sentinel = object() # unique object used to signal cache misses
make_key = _make_key # build a key from the function arguments
Expand Down
7 changes: 7 additions & 0 deletions Lib/test/test_functools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2157,6 +2157,13 @@ def fib(n):
with self.assertRaises(RecursionError):
fib(support.exceeds_recursion_limit())

def test_lru_checks_arg_is_callable(self):
with self.assertRaisesRegex(
TypeError,
"the first argument must be callable",
):
self.module.lru_cache(1)('hello')


@py_functools.lru_cache()
def py_cached_func(x, y):
Expand Down
3 changes: 3 additions & 0 deletions Misc/NEWS.d/next/Library/2026-01-19-12-48-59.gh-issue-144030.7OK_gB.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
The Python implementation of :func:`functools.lru_cache` differed from the
default C implementation in that it did not check that its argument is
callable. This discrepancy is now fixed and both raise a :exc:`TypeError`.
Loading