Refactoring to start using getattr_static (#832)

rybarczykj · web-flow · commit abc261a505b3 · 2020-08-04T07:45:14.000-07:00
Avoids having to use AttrCleaner in many contexts.
Also adds some testing.
diff --git a/bpython/autocomplete.py b/bpython/autocomplete.py
@@ -340,9 +340,6 @@ def attr_matches(self, text, namespace):
         """Taken from rlcompleter.py and bent to my will.
         """
 
-        # Gna, Py 2.6's rlcompleter searches for __call__ inside the
-        # instance instead of the type, so we monkeypatch to prevent
-        # side-effects (__getattr__/__getattribute__)
         m = self.attr_matches_re.match(text)
         if not m:
             return []
@@ -356,19 +353,17 @@ def attr_matches(self, text, namespace):
             obj = safe_eval(expr, namespace)
         except EvaluationError:
             return []
-        with inspection.AttrCleaner(obj):
-            matches = self.attr_lookup(obj, expr, attr)
+        matches = self.attr_lookup(obj, expr, attr)
         return matches
 
     def attr_lookup(self, obj, expr, attr):
-        """Second half of original attr_matches method factored out so it can
-        be wrapped in a safe try/finally block in case anything bad happens to
-        restore the original __getattribute__ method."""
+        """Second half of attr_matches."""
         words = self.list_attributes(obj)
-        if hasattr(obj, "__class__"):
+        if inspection.hasattr_safe(obj, "__class__"):
             words.append("__class__")
-            words = words + rlcompleter.get_class_members(obj.__class__)
-            if not isinstance(obj.__class__, abc.ABCMeta):
+            klass = inspection.getattr_safe(obj, "__class__")
+            words = words + rlcompleter.get_class_members(klass)
+            if not isinstance(klass, abc.ABCMeta):
                 try:
                     words.remove("__abstractmethods__")
                 except ValueError:
@@ -388,21 +383,25 @@ def attr_lookup(self, obj, expr, attr):
     if py3:
 
         def list_attributes(self, obj):
-            return dir(obj)
+            # TODO: re-implement dir using getattr_static to avoid using
+            # AttrCleaner here?
+            with inspection.AttrCleaner(obj):
+                return dir(obj)
 
     else:
 
         def list_attributes(self, obj):
-            if isinstance(obj, InstanceType):
-                try:
+            with inspection.AttrCleaner(obj):
+                if isinstance(obj, InstanceType):
+                    try:
+                        return dir(obj)
+                    except Exception:
+                        # This is a case where we can not prevent user code from
+                        # running. We return a default list attributes on error
+                        # instead. (#536)
+                        return ["__doc__", "__module__"]
+                else:
                     return dir(obj)
-                except Exception:
-                    # This is a case where we can not prevent user code from
-                    # running. We return a default list attributes on error
-                    # instead. (#536)
-                    return ["__doc__", "__module__"]
-            else:
-                return dir(obj)
 
 
 class DictKeyCompletion(BaseCompletionType):
@@ -537,10 +536,9 @@ def matches(self, cursor_offset, line, **kwargs):
             obj = evaluate_current_expression(cursor_offset, line, locals_)
         except EvaluationError:
             return set()
-        with inspection.AttrCleaner(obj):
-            #          strips leading dot
-            matches = [m[1:] for m in self.attr_lookup(obj, "", attr.word)]
 
+         #        strips leading dot
+        matches = [m[1:] for m in self.attr_lookup(obj, "", attr.word)]
         return set(m for m in matches if few_enough_underscores(attr.word, m))
 
 
@@ -679,7 +677,6 @@ def get_completer_bpython(cursor_offset, line, **kwargs):
 
 def _callable_postfix(value, word):
     """rlcompleter's _callable_postfix done right."""
-    with inspection.AttrCleaner(value):
-        if inspection.is_callable(value):
-            word += "("
+    if inspection.is_callable(value):
+        word += "("
     return word
diff --git a/bpython/inspection.py b/bpython/inspection.py
@@ -33,6 +33,7 @@
 from six.moves import range
 
 from pygments.token import Token
+from types import MemberDescriptorType
 
 from ._py3compat import PythonLexer, py3
 from .lazyre import LazyReCompile
@@ -214,7 +215,7 @@ def getpydocspec(f, func):
     if s is None:
         return None
 
-    if not hasattr(f, "__name__") or s.groups()[0] != f.__name__:
+    if not hasattr_safe(f, "__name__") or s.groups()[0] != f.__name__:
         return None
 
     args = list()
@@ -276,8 +277,7 @@ def getfuncprops(func, f):
         argspec = ArgSpec(*argspec)
         fprops = FuncProps(func, argspec, is_bound_method)
     except (TypeError, KeyError, ValueError):
-        with AttrCleaner(f):
-            argspec = getpydocspec(f, func)
+        argspec = getpydocspec(f, func)
         if argspec is None:
             return None
         if inspect.ismethoddescriptor(f):
@@ -392,6 +392,47 @@ def get_encoding_file(fname):
     return "ascii"
 
 
+if not py3:
+
+    def getattr_safe(obj, name):
+        """side effect free getattr"""
+        if not is_new_style(obj):
+            return getattr(obj, name)
+
+        with AttrCleaner(obj):
+            to_look_through = (
+                obj.__mro__
+                if inspect.isclass(obj)
+                else (obj,) + type(obj).__mro__
+            )
+            for cls in to_look_through:
+                if hasattr(cls, "__dict__") and name in cls.__dict__:
+                    result = cls.__dict__[name]
+                    if isinstance(result, MemberDescriptorType):
+                        result = getattr(obj, name)
+                    return result
+            raise AttributeError(name)
+
+
+else:
+
+    def getattr_safe(obj, name):
+        """side effect free getattr (calls getattr_static)."""
+        result = inspect.getattr_static(obj, name)
+        # Slots are a MemberDescriptorType
+        if isinstance(result, MemberDescriptorType):
+            result = getattr(obj, name)
+        return result
+
+
+def hasattr_safe(obj, name):
+    try:
+        getattr_safe(obj, name)
+        return True
+    except AttributeError:
+        return False
+
+
 if py3:
 
     def get_source_unicode(obj):
diff --git a/bpython/simpleeval.py b/bpython/simpleeval.py
@@ -38,7 +38,7 @@
 
 from . import line as line_properties
 from ._py3compat import py3
-from .inspection import is_new_style, AttrCleaner
+from .inspection import getattr_safe
 
 _string_type_nodes = (ast.Str, ast.Bytes) if py3 else (ast.Str,)
 _numeric_types = (int, float, complex) + (() if py3 else (long,))
@@ -163,14 +163,15 @@ def _convert(node):
         if isinstance(node, ast.Attribute):
             obj = _convert(node.value)
             attr = node.attr
-            return safe_get_attribute(obj, attr)
+            return getattr_safe(obj, attr)
 
         raise ValueError("malformed string")
 
     return _convert(node_or_string)
 
 
 def safe_getitem(obj, index):
+    """ Safely tries to access obj[index] """
     if type(obj) in (list, tuple, dict, bytes) + string_types:
         try:
             return obj[index]
@@ -251,44 +252,3 @@ def evaluate_current_attribute(cursor_offset, line, namespace=None):
             "can't lookup attribute %s on %r" % (attr.word, obj)
         )
 
-
-def safe_get_attribute(obj, attr):
-    """Gets attributes without triggering descriptors on new-style classes"""
-    if is_new_style(obj):
-        with AttrCleaner(obj):
-            result = safe_get_attribute_new_style(obj, attr)
-            if isinstance(result, member_descriptor):
-                # will either be the same slot descriptor or the value
-                return getattr(obj, attr)
-            return result
-    return getattr(obj, attr)
-
-
-class _ClassWithSlots(object):
-    __slots__ = ["a"]
-
-
-member_descriptor = type(_ClassWithSlots.a)
-
-
-def safe_get_attribute_new_style(obj, attr):
-    """Returns approximately the attribute returned by getattr(obj, attr)
-
-    The object returned ought to be callable if getattr(obj, attr) was.
-    Fake callable objects may be returned instead, in order to avoid executing
-    arbitrary code in descriptors.
-
-    If the object is an instance of a class that uses __slots__, will return
-    the member_descriptor object instead of the value.
-    """
-    if not is_new_style(obj):
-        raise ValueError("%r is not a new-style class or object" % obj)
-    to_look_through = (
-        obj.__mro__ if inspect.isclass(obj) else (obj,) + type(obj).__mro__
-    )
-
-    for cls in to_look_through:
-        if hasattr(cls, "__dict__") and attr in cls.__dict__:
-            return cls.__dict__[attr]
-
-    raise AttributeError()
diff --git a/bpython/test/test_autocomplete.py b/bpython/test/test_autocomplete.py
@@ -251,6 +251,9 @@ def asserts_when_called(self):
 class Slots(object):
     __slots__ = ["a", "b"]
 
+class OverriddenGetattribute(Foo):
+    def __getattribute__(self,name):
+        raise AssertionError("custom get attribute invoked")
 
 class TestAttrCompletion(unittest.TestCase):
     @classmethod
@@ -298,12 +301,20 @@ def test_descriptor_attributes_not_run(self):
             com.matches(2, "a.", locals_={"a": Properties()}),
             set(["a.b", "a.a", "a.method", "a.asserts_when_called"]),
         )
+        
+    def test_custom_get_attribute_not_invoked(self):
+        com = autocomplete.AttrCompletion()
+        self.assertSetEqual(
+            com.matches(2, "a.", locals_={"a": OverriddenGetattribute()}),
+            set(["a.b", "a.a", "a.method"]),
+        )
+
 
     def test_slots_not_crash(self):
         com = autocomplete.AttrCompletion()
         self.assertSetEqual(
             com.matches(2, "A.", locals_={"A": Slots}),
-            set(["A.b", "A.a", "A.mro"]),
+            set(["A.b", "A.a"]),
         )
 
 
diff --git a/bpython/test/test_inspection.py b/bpython/test/test_inspection.py
@@ -155,5 +155,124 @@ def test_get_source_file(self):
         self.assertEqual(encoding, "utf-8")
 
 
+class A(object):
+    a = "a"
+
+
+class B(A):
+    b = "b"
+
+
+class Property(object):
+    @property
+    def prop(self):
+        raise AssertionError("Property __get__ executed")
+
+
+class Slots(object):
+    __slots__ = ["s1", "s2", "s3"]
+
+    if not py3:
+
+        @property
+        def s3(self):
+            raise AssertionError("Property __get__ executed")
+
+
+class SlotsSubclass(Slots):
+    @property
+    def s4(self):
+        raise AssertionError("Property __get__ executed")
+
+
+class OverriddenGetattr(object):
+    def __getattr__(self, attr):
+        raise AssertionError("custom __getattr__ executed")
+
+    a = 1
+
+
+class OverriddenGetattribute(object):
+    def __getattribute__(self, attr):
+        raise AssertionError("custom __getattribute__ executed")
+
+    a = 1
+
+
+class OverriddenMRO(object):
+    def __mro__(self):
+        raise AssertionError("custom mro executed")
+
+    a = 1
+    
+member_descriptor = type(Slots.s1)
+
+class TestSafeGetAttribute(unittest.TestCase):
+    def test_lookup_on_object(self):
+        a = A()
+        a.x = 1
+        self.assertEqual(inspection.getattr_safe(a, "x"), 1)
+        self.assertEqual(inspection.getattr_safe(a, "a"), "a")
+        b = B()
+        b.y = 2
+        self.assertEqual(inspection.getattr_safe(b, "y"), 2)
+        self.assertEqual(inspection.getattr_safe(b, "a"), "a")
+        self.assertEqual(inspection.getattr_safe(b, "b"), "b")
+
+        self.assertEqual(inspection.hasattr_safe(b, "y"), True)
+        self.assertEqual(inspection.hasattr_safe(b, "b"), True)
+
+
+    def test_avoid_running_properties(self):
+        p = Property()
+        self.assertEqual(inspection.getattr_safe(p, "prop"), Property.prop)
+        self.assertEqual(inspection.hasattr_safe(p, "prop"), True)
+    
+    def test_lookup_with_slots(self):
+        s = Slots()
+        s.s1 = "s1"
+        self.assertEqual(inspection.getattr_safe(s, "s1"), "s1")
+        with self.assertRaises(AttributeError):
+            inspection.getattr_safe(s, "s2")
+
+        self.assertEqual(inspection.hasattr_safe(s, "s1"), True)
+        self.assertEqual(inspection.hasattr_safe(s, "s2"), False)
+
+    def test_lookup_on_slots_classes(self):
+        sga = inspection.getattr_safe
+        s = SlotsSubclass()
+        self.assertIsInstance(sga(Slots, "s1"), member_descriptor)
+        self.assertIsInstance(sga(SlotsSubclass, "s1"), member_descriptor)
+        self.assertIsInstance(sga(SlotsSubclass, "s4"), property)
+        self.assertIsInstance(sga(s, "s4"), property)
+
+        self.assertEqual(inspection.hasattr_safe(s, "s1"), False)
+        self.assertEqual(inspection.hasattr_safe(s, "s4"), True)
+
+    @unittest.skipIf(py3, "Py 3 doesn't allow slots and prop in same class")
+    def test_lookup_with_property_and_slots(self):
+        sga = inspection.getattr_safe
+        s = SlotsSubclass()
+        self.assertIsInstance(sga(Slots, "s3"), property)
+        self.assertEqual(inspection.getattr_safe(s, "s3"), Slots.__dict__["s3"])
+        self.assertIsInstance(sga(SlotsSubclass, "s3"), property)
+
+    def test_lookup_on_overridden_methods(self):
+        sga = inspection.getattr_safe
+        self.assertEqual(sga(OverriddenGetattr(), "a"), 1)
+        self.assertEqual(sga(OverriddenGetattribute(), "a"), 1)
+        self.assertEqual(sga(OverriddenMRO(), "a"), 1)
+        with self.assertRaises(AttributeError):
+            sga(OverriddenGetattr(), "b")
+        with self.assertRaises(AttributeError):
+            sga(OverriddenGetattribute(), "b")
+        with self.assertRaises(AttributeError):
+            sga(OverriddenMRO(), "b")
+
+        self.assertEqual(inspection.hasattr_safe(OverriddenGetattr(), "b"), False)
+        self.assertEqual(inspection.hasattr_safe(OverriddenGetattribute(), "b"), False)
+        self.assertEqual(inspection.hasattr_safe(OverriddenMRO(), "b"), False)
+        
+
 if __name__ == "__main__":
     unittest.main()
diff --git a/bpython/test/test_simpleeval.py b/bpython/test/test_simpleeval.py
