performance shortcut in functools.partial behaves differently in C and in Python version

@mgorny

Bug

functools.partial is implemented in functools.py and in _functoolsmodule.c. The former is almost never used, so libraries come to depend on the quirks and corner cases of the C implementation. This is a problem for PyPy, where the Python implementation is the only one as of the most recent PyPy version. Here's one such difference, which was uncovered by the lxml library. The following code leads to a RecursionError:

import sys
sys.modules['_functools'] = None # force use of pure python version, if this is commented out it works

from functools import partial

class Builder:
    def __call__(self, tag, *children, **attrib):
        return (tag, children, attrib)

    def __getattr__(self, tag):
        return partial(self, tag)

B = Builder()
m = B.m

this is the traceback:

Traceback (most recent call last):
  File "/home/cfbolz/projects/cpython/bug.py", line 14, in <module>
    m = B.m
        ^^^
  File "/home/cfbolz/projects/cpython/bug.py", line 11, in __getattr__
    return partial(self, tag)
           ^^^^^^^^^^^^^^^^^^
  File "/home/cfbolz/projects/cpython/Lib/functools.py", line 287, in __new__
    if hasattr(func, "func"):
       ^^^^^^^^^^^^^^^^^^^^^
  File "/home/cfbolz/projects/cpython/bug.py", line 11, in __getattr__
    return partial(self, tag)
           ^^^^^^^^^^^^^^^^^^
  File "/home/cfbolz/projects/cpython/Lib/functools.py", line 287, in __new__
    if hasattr(func, "func"):
       ^^^^^^^^^^^^^^^^^^^^^
... and repeated

The problem is the following performance shortcut in partial.__new__:

class partial:
    ...
    def __new__(cls, func, /, *args, **keywords):
        if not callable(func):
            raise TypeError("the first argument must be callable")

        if hasattr(func, "func"): # <------------------- problem
            args = func.args + args
            keywords = {**func.keywords, **keywords}
            func = func.func

Basically in this case func is an object where calling hasattr(func, "func") is not safe. The equivalent C code does this check:

    if (Py_TYPE(func)->tp_call == (ternaryfunc)partial_call) {
        // The type of "func" might not be exactly the same type object
        // as "type", but if it is called using partial_call, it must have the
        // same memory layout (fn, args and kw members).
        // We can use its underlying function directly and merge the arguments.
        partialobject *part = (partialobject *)func;

In particular, it does not simply call hasattr on func.

Real World Version

This is not an artificial problem, we discovered this via the class lxml.builder.ElementMaker. It has a __call__ method implemented. It also has __getattr__ that looks like this:

    def __getattr__(self, tag):
        return partial(self, tag)

Which yields the above RecursionError on PyPy.

Solution ideas

One approach would be to file a bug with lxml, but it is likely that more libraries depend on this behaviour. So I would suggest to change the __new__ Python code to add an isinstance check, to bring its behaviour closer to that of the C code:

    def __new__(cls, func, /, *args, **keywords):
        if not callable(func):
            raise TypeError("the first argument must be callable")

        if isinstance(func, partial) and hasattr(func, "func"):
            args = func.args + args
        ...

I'll open a PR with this approach soon. /cc @mgorny

Linked PRs

GH-100242: bring functools.py partial implementation more in line with C code #100244

Note that I needed to modify the tests which assume the c-import worked:

# HG changeset patch
# Branch py3.9
# Node ID 4016ae64db38512857aea9987146e897f6b25ebd
# Parent  f5d3e9e0fa5cb1ee62ccd2867bcf5fe094d4eae8
fix tests for no c_functools

diff -r f5d3e9e0fa5c -r 4016ae64db38 lib-python/3/test/test_functools.py
--- a/lib-python/3/test/test_functools.py	Fri Dec 02 16:15:06 2022 +0200
+++ b/lib-python/3/test/test_functools.py	Sat Dec 03 10:42:01 2022 +0200
@@ -199,7 +199,9 @@
         kwargs = {'a': object(), 'b': object()}
         kwargs_reprs = ['a={a!r}, b={b!r}'.format_map(kwargs),
                         'b={b!r}, a={a!r}'.format_map(kwargs)]
-        if self.partial in (c_functools.partial, py_functools.partial):
+        if c_functools and self.partial is c_functools.partial:
+            name = 'functools.partial'
+        elif self.partial is py_functools.partial:
             name = 'functools.partial'
         else:
             name = self.partial.__name__
@@ -221,7 +223,9 @@
                        for kwargs_repr in kwargs_reprs])
 
     def test_recursive_repr(self):
-        if self.partial in (c_functools.partial, py_functools.partial):
+        if c_functools and self.partial is c_functools.partial:
+            name = 'functools.partial'
+        elif self.partial is py_functools.partial:
             name = 'functools.partial'
         else:
             name = self.partial.__name__
@@ -1753,9 +1757,10 @@
 def py_cached_func(x, y):
     return 3 * x + y
 
-@c_functools.lru_cache()
-def c_cached_func(x, y):
-    return 3 * x + y
+if c_functools:
+    @c_functools.lru_cache()
+    def c_cached_func(x, y):
+        return 3 * x + y
 
 
 class TestLRUPy(TestLRU, unittest.TestCase):
@@ -1772,18 +1777,20 @@
         return 3 * x + y
 
 
-class TestLRUC(TestLRU, unittest.TestCase):
-    module = c_functools
-    cached_func = c_cached_func,
-
-    @module.lru_cache()
-    def cached_meth(self, x, y):
-        return 3 * x + y
-
-    @staticmethod
-    @module.lru_cache()
-    def cached_staticmeth(x, y):
-        return 3 * x + y
+if c_functools:
+    @unittest.skipUnless(c_functools, 'requires the C _functools module')
+    class TestLRUC(TestLRU, unittest.TestCase):
+        module = c_functools
+        cached_func = c_cached_func,
+
+        @module.lru_cache()
+        def cached_meth(self, x, y):
+            return 3 * x + y
+
+        @staticmethod
+        @module.lru_cache()
+        def cached_staticmeth(x, y):
+            return 3 * x + y
 
 
 class TestSingleDispatch(unittest.TestCase):

GH-100242: bring functools.py partial implementation more in line wit…

Should this issue be closed? It looks like there's no plan to backport this to 3.12

Yeah, I'm happy with the current situation, I'm closing it.

Just curious, so changes are only going to be applied in Python 3.13? Isn't it considered a bug/a subject to be addressed in a bugfix release? The commit 5a0209f only has a v3.13.0b1 tag and I don't know how to figure it out otherwise.

My toy example reproducing the issue:

plant("flower")

Prints Planting a flower, but

plant_a_flower = functools.partial(plant, "flower")
plant_a_flower()

suddenly prints Planting nuclear bomb to destroy the flower

plant.py.txt

def nuke(thing):
    print("Planting nuclear bomb to destroy the", thing)


def plant(thing):
    print("Planting a", thing)
plant.func = nuke  # func = Flower UNdo Command, use very carefully
plant.args = ()
plant.keywords = dict()


plant("flower")
plant_a_flower = functools.partial(plant, "flower")
plant_a_flower()  # expecting the same output as plant("flower")

Navigation Menu

Repository navigation

performance shortcut in functools.partial behaves differently in C and in Python version #100242

Bug

Real World Version

Solution ideas

Linked PRs

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

performance shortcut in functools.partial behaves differently in C and in Python version #100242

Description

Bug

Real World Version

Solution ideas

Linked PRs

Activity

mattip commented on Dec 14, 2022

graingert commented on May 19, 2024

cfbolz commented on May 19, 2024

belkka commented on May 26, 2024

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions