Optimiser breaks attribute lookup from metaclass property

Bug report

This comes from a SymPy issue: sympy/sympy#23774

It looks like there is a nondeterministic error in the optimisation introduced in 96346cb from #27722 first included in CPython version 3.11.0a1. The optimisation speeds up calling methods but sometimes retrieves the wrong attribute from a class whose metaclass defines a property method.

The way that this arises is quite sensitive to small changes so I haven't been able to distil a standalone reproducer. I'll show how to reproduce this using SymPy below but first this is a simplified schematic of the situation:

class MetaA(type):
    def __init__(cls, *args, **kws):
        pass

class A(metaclass=MetaA):
    def method(self, rule):
        return 'A method'

class MetaB(MetaA):
    @property
    def method(self):
        def method_inner(rule):
            return 'MetaB function'
        return method_inner

class B(A, metaclass=MetaB):
    pass

print(B.method(1))   # MetaB function
print(B().method(1)) # A method

Here B is a subclass of A but an instance of MetaB. Both define method but the MetaB method should be used when accessed from the class B rather than an instance B(). The failure seen in SymPy is that sometimes B.method(1) will execute as A.method(1) which fails because of the missing self argument.

The following code reproduces the problem and fails something like 50% of the time with SymPy 1.10.1 and CPython 3.11.0a1-3.11.0b4:

from sympy import *

x, y = symbols('x, y')
f = Function('f')

# These two lines look irrelevant but are needed:
expr = sin(x*exp(y))
Derivative(expr, y).subs(y, x).doit()

expr = Subs(Derivative(f(f(x)), x), f, cos)

# This is where it blows up:
expr.doit()

The failure is not deterministic:

$ python bug.py
$ python bug.py
$ python bug.py
Traceback (most recent call last):
  File "/home/oscar/current/sympy/sympy.git/bug.py", line 13, in <module>
    expr.doit()
    ^^^^^^^^^^^
  File "/home/oscar/current/sympy/sympy.git/sympy/core/function.py", line 2246, in doit
    e = e.subs(vi, p[i])
        ^^^^^^^^^^^^^^^^
  File "/home/oscar/current/sympy/sympy.git/sympy/core/basic.py", line 997, in subs
    rv = rv._subs(old, new, **kwargs)
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/oscar/current/sympy/sympy.git/sympy/core/cache.py", line 70, in wrapper
    retval = cfunc(*args, **kwargs)
             ^^^^^^^^^^^^^^^^^^^^^^
  File "/home/oscar/current/sympy/sympy.git/sympy/core/basic.py", line 1109, in _subs
    rv = self._eval_subs(old, new)
         ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/oscar/current/sympy/sympy.git/sympy/core/function.py", line 1737, in _eval_subs
    nfree = new.xreplace(syms).free_symbols
            ^^^^^^^^^^^^^^^^^^
TypeError: Basic.xreplace() missing 1 required positional argument: 'rule'

The failure can be reproduced deterministically by setting the hash seed:

$ PYTHONHASHSEED=1 python bug.py
...
TypeError: Basic.xreplace() missing 1 required positional argument: 'rule'

In the debugger the same code that already failed succeeds:

$ PYTHONHASHSEED=1 python -m pdb bug.py 
> /home/oscar/current/sympy/sympy.git/bug.py(1)<module>()
-> from sympy import *
(Pdb) c
Traceback (most recent call last):
  File "/media/oscar/EXT4_STUFF/src/cpython/Lib/pdb.py", line 1768, in main
    pdb._run(target)
  File "/media/oscar/EXT4_STUFF/src/cpython/Lib/pdb.py", line 1646, in _run
    self.run(target.code)
  File "/media/oscar/EXT4_STUFF/src/cpython/Lib/bdb.py", line 597, in run
    exec(cmd, globals, locals)
  File "<string>", line 1, in <module>
  File "/home/oscar/current/sympy/sympy.git/bug.py", line 13, in <module>
    expr.doit()
  File "/home/oscar/current/sympy/sympy.git/sympy/core/function.py", line 2255, in doit
    e = e.subs(vi, p[i])
        ^^^^^^^^^^^^^^^^
  File "/home/oscar/current/sympy/sympy.git/sympy/core/basic.py", line 993, in subs
    rv = rv._subs(old, new, **kwargs)
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/oscar/current/sympy/sympy.git/sympy/core/cache.py", line 70, in wrapper
    retval = cfunc(*args, **kwargs)
             ^^^^^^^^^^^^^^^^^^^^^^
  File "/home/oscar/current/sympy/sympy.git/sympy/core/basic.py", line 1105, in _subs
    rv = self._eval_subs(old, new)
         ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/oscar/current/sympy/sympy.git/sympy/core/function.py", line 1746, in _eval_subs
    nfree = new.xreplace(syms).free_symbols
            ^^^^^^^^^^^^^^^^^^
TypeError: Basic.xreplace() missing 1 required positional argument: 'rule'
Uncaught exception. Entering post mortem debugging
Running 'cont' or 'step' will restart the program
> /home/oscar/current/sympy/sympy.git/sympy/core/function.py(1746)_eval_subs()
-> nfree = new.xreplace(syms).free_symbols
(Pdb) p new.xreplace
<function FunctionClass.xreplace.<locals>.<lambda> at 0x7f0dd0f763e0>
(Pdb) p new.xreplace(syms)
cos
(Pdb) p new.xreplace(syms).free_symbols
set()

The actual arrangement of SymPy classes is something like this:

class ManagedProperties(type):
    def __init__(cls, *args, **kws):
        pass

class Basic(metaclass=ManagedProperties):
    def xreplace(self, rule):
        print('Basic')

class Expr(Basic):
    pass

class FunctionClass(ManagedProperties):
    @property
    def xreplace(self):
        return lambda rule: print('functionclass')

class Application(Basic, metaclass=FunctionClass):
    pass

class Function(Application, Expr):
    pass

class cos(Function):
    pass

cos.xreplace(1)

Your environment

CPython versions tested on: 3.11.0a1-3.11.0b4 (3.10.5 or lower does not have the bug)
Operating system and architecture: Ubuntu x86-64.

@pablogsal

@pablogsal this sounds like a release blocker. Also cc @markshannon.

@markshannon

Ouch. Looking at it now (@markshannon is at EuroPython and may not be 100% responsive).

I'm able to get a consistent reproducer without based on the original example without SymPy. I'm currently minimizing it further.

Minimal reproducer that fails consistently on main:

class Metaclass(type):
    @property
    def attribute(self):
        return True

class Class(metaclass=Metaclass):
    attribute = False

for _ in range(8):
    print(Class.attribute)

It prints:

True
True
True
True
True
True
True
False

This failure occurs immediately after specializing the LOAD_ATTR_ADAPTIVE into LOAD_ATTR_CLASS. That specialization is incorrect.

My gut says we should fail to perform this specialization in the presence of a metaclass as a quick fix. Not sure what makes the most sense long term.

The following patch fixes the reproducer:

diff --git a/Python/specialize.c b/Python/specialize.c
index 66cae44fa8..72667c4905 100644
--- a/Python/specialize.c
+++ b/Python/specialize.c
@@ -947,6 +947,10 @@ specialize_class_load_attr(PyObject *owner, _Py_CODEUNIT *instr,
     _PyLoadMethodCache *cache = (_PyLoadMethodCache *)(instr + 1);
     PyObject *descr = NULL;
     DescriptorClassification kind = 0;
+    if (_PyType_Lookup(Py_TYPE(owner), name)) {
+        SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_ATTR_METACLASS_ATTRIBUTE);
+        return -1;
+    }
     kind = analyze_descriptor((PyTypeObject *)owner, name, &descr, 0);
     switch (kind) {
         case METHOD:
@@ -957,12 +961,7 @@ specialize_class_load_attr(PyObject *owner, _Py_CODEUNIT *instr,
             return 0;
 #ifdef Py_STATS
         case ABSENT:
-            if (_PyType_Lookup(Py_TYPE(owner), name) != NULL) {
-                SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_ATTR_METACLASS_ATTRIBUTE);
-            }
-            else {
-                SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_EXPECTED_ERROR);
-            }
+            SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_EXPECTED_ERROR);
             return -1;
 #endif
         default:

I'll just need some time with this to make sure it's both correct and not overly-restrictive. I'd also appreciate a second (or third) pair of eyes on this, if anybody has the time.

That patch is still buggy in other ways. Adding or changing properties on the metaclass doesn't invalidate the caches:

class M(type):
    pass
    
class C(metaclass=M):
    a = False

@property
def a(self):
    return True

for i in range(9):
    if i == 8:
        M.a = a
    print(C.a)

That should print True on the last iteration, but doesn't.

@Fidget-Spinner

I'm thinking that, for 3.11 at least, we should probably just bail on caching class attributes if a metaclass is involved. I wonder if attribute assignment is affected as well.

The bigger issue is that changes to metaclasses don't invalidate tp_version_tags of classes that use them. So it seems that we have two choices for 3.12: either don't cache class attributes when a metaclass is involved, or also cache the metaclass tp_version_tag (maybe in _PyLoadMethodCache.keys_version?) and check that each time.

I'd love to hear others' thoughts. @Fidget-Spinner, I think you've worked on this code in the past?

Argh, it's not enough to ignore metaclasses. type itself has descriptors that can trigger this; any class that has a conflicting attribute name is susceptible.

New minimal reproducer:

class C:
    __name__ = "Spam"

for _ in range(8):
    print(C.__name__)

@brandtbucher

@brandtbucher yeah I implemented this opcode I think. I'm at Europython too so I'll try to take a look tomorrow.

The fixes on your branch LGTM. What's stopping you from opening a PR? (Also I like the branch name :).

What's stopping you from opening a PR?

Just making sure today that there's a reasonable way to backport the metaclass versioning to 3.11. Also seeing if I can incorporate a fix for the __getattribute__ issue I found in the same PR. I'll have something up today either way.

GH-94822: Don't specialize when metaclasses are involved (GH-94892)

[3.11] pythonGH-94822: Don't specialize when metaclasses are involved (…

[3.11] GH-94822: Don't specialize when metaclasses are involved (GH-9…

#94980 landed on the 3.11 branch - it should be possible to remove the release blocker now?

I can confirm that SymPy CI is all green now with CPython 3.11.0b5.

I can confirm that SymPy CI is all green now with CPython 3.11.0b5.

Okay, closing. Feel free to reopen if required.

Navigation Menu

Repository navigation

Optimiser breaks attribute lookup from metaclass property #94822

3 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Optimiser breaks attribute lookup from metaclass property #94822

Description

Activity

JelleZijlstra commented on Jul 13, 2022

brandtbucher commented on Jul 13, 2022

brandtbucher commented on Jul 13, 2022

brandtbucher commented on Jul 13, 2022

brandtbucher commented on Jul 13, 2022

brandtbucher commented on Jul 13, 2022

brandtbucher commented on Jul 13, 2022

brandtbucher commented on Jul 13, 2022

brandtbucher commented on Jul 13, 2022

Fidget-Spinner commented on Jul 13, 2022

3 remaining items

Fidget-Spinner commented on Jul 14, 2022

brandtbucher commented on Jul 14, 2022

h-vetinari commented on Jul 19, 2022

oscarbenjamin commented on Jul 28, 2022

kumaraditya303 commented on Sep 17, 2022

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions