bug: Ensure cache_readonly is only applied on instance

Author: fjetter

pandas/_libs/properties.pxd

@@ -0,0 +1,5 @@
+
+cdef class PropertyCacheMixin:
+
+    cdef public:
+        dict _cache_property

pandas/_libs/properties.pyx

@@ -10,25 +10,27 @@ from cpython.dict cimport (
 cdef class CachedProperty:
 
     cdef readonly:
-        object func, name, __doc__
+        object func, name
 
     def __init__(self, func):
         self.func = func
         self.name = func.__name__
-        self.__doc__ = getattr(func, '__doc__', None)
 
     def __get__(self, obj, typ):
         if obj is None:
             # accessed on the class, not the instance
-            return self
+            return self.func
 
         # Get the cache or set a default one if needed
-        cache = getattr(obj, '_cache', None)
+        cache = getattr(obj, '_cache_property', None)
         if cache is None:
             try:
-                cache = obj._cache = {}
+                cache = obj._cache_property = {}
             except (AttributeError):
-                return self
+                raise raise TypeError(
+                    f"Cython extension type {type(obj)} must declare attribute "
+                    "`_cache_property` to use ``@cache_readonly``."
+                )
 
         if PyDict_Contains(cache, self.name):
             # not necessary to Py_INCREF
@@ -41,6 +43,10 @@ cdef class CachedProperty:
     def __set__(self, obj, value):
         raise AttributeError("Can't set attribute")
 
+cdef class PropertyCacheMixin:
+
+    def __cinit__(self):
+        self._cache_property = {}
 
 cache_readonly = CachedProperty
 

pandas/_libs/tslibs/offsets.pxd

@@ -1,11 +1,13 @@
 from numpy cimport int64_t
 
+from pandas._libs.properties cimport PropertyCacheMixin
+
 
 cpdef to_offset(object obj)
 cdef bint is_offset_object(object obj)
 cdef bint is_tick_object(object obj)
 
-cdef class BaseOffset:
+cdef class BaseOffset(PropertyCacheMixin):
     cdef readonly:
         int64_t n
         bint normalize

pandas/_libs/tslibs/offsets.pyx

@@ -33,6 +33,7 @@ cnp.import_array()
 
 # TODO: formalize having _libs.properties "above" tslibs in the dependency structure
 
+from pandas._libs.properties cimport PropertyCacheMixin
 from pandas._libs.properties import cache_readonly
 
 from pandas._libs.tslibs cimport util
@@ -343,7 +344,7 @@ class ApplyTypeError(TypeError):
 # ---------------------------------------------------------------------
 # Base Classes
 
-cdef class BaseOffset:
+cdef class BaseOffset(PropertyCacheMixin):
     """
     Base class for DateOffset methods that are not overridden by subclasses.
     """

pandas/core/series.py

@@ -268,7 +268,7 @@ class Series(base.IndexOpsMixin, generic.NDFrame):
 
     # Override cache_readonly bc Series is mutable
     hasnans = property(
-        base.IndexOpsMixin.hasnans.func, doc=base.IndexOpsMixin.hasnans.__doc__
+        base.IndexOpsMixin.hasnans, doc=base.IndexOpsMixin.hasnans.__doc__
     )
     __hash__ = generic.NDFrame.__hash__
     _mgr: SingleManager

pandas/tests/libs/test_lib.py

@@ -9,6 +9,7 @@
 
 from pandas import Index
 import pandas._testing as tm
+from pandas.util import cache_readonly
 
 
 class TestMisc:
@@ -206,3 +207,38 @@ def test_get_reverse_indexer(self):
 def test_cache_readonly_preserve_docstrings():
     # GH18197
     assert Index.hasnans.__doc__ is not None
+
+
+def test_cache_readonly():
+    """
+    Test that we do not accidentally cache the properties in a class attribute
+    """
+
+    class ClassWithoutCache:
+        _cache = {}
+
+        def __init__(self, prop):
+            self.counter = 0
+            self._prop = prop
+
+        @cache_readonly
+        def expensive_property(self):
+            self.counter += 1
+            if self.counter > 1:
+                raise RuntimeError("Called too many times")
+            return self._prop
+
+    class ClassWithCache(ClassWithoutCache):
+        _cache = {}
+
+    for typ in [ClassWithCache, ClassWithoutCache]:
+
+        instanceA = typ("A")
+
+        instanceB = typ("B")
+
+        assert instanceA.expensive_property == "A"
+        assert instanceA.expensive_property == "A"
+        assert instanceA.counter == 1
+
+        assert instanceB.expensive_property == "B"