Refactoring to start using getattr_static #832
Conversation
Avoids having to use AttrCleaner in many contexts
bpython/inspection.py
Outdated
| else: | ||
|
|
||
| def get_attr_safe(obj, name): | ||
| """side effect and AttrCleaner free getattr (calls getattr_static).""" |
There was a problem hiding this comment.
"AttrCleaner free" is confusing to me because nobody knows what AttrCleaner does, I'd move that part to a comment.
bpython/autocomplete.py
Outdated
| with inspection.AttrCleaner(value): | ||
| if inspection.is_callable(value): | ||
| word += "(" | ||
| # TODO: do we need this done within an AttrCleaner? |
There was a problem hiding this comment.
It doesn't look like it: I tried running callable on an instance of a class with a broken __getattr__ and a broken __getattribute__, and with or without a __call__() method, in Python 2 and 3, these didn't crash or run their side effects.
https://github.com/python/cpython/blob/cadda52d974937069eeebea1cca4229e2bd400df/Objects/object.c#L1434 also suggests this wouldn't be intercepted by either of these.
bpython/autocomplete.py
Outdated
|
|
||
| # strips leading dot |
There was a problem hiding this comment.
I think the intention of the spacing in front of this comment was to line it up with the m[1:] below it
| @@ -356,16 +356,15 @@ 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) | |||
There was a problem hiding this comment.
The comment about about monkeypatching to prevent side-effects (__getattr__/__getattribute__) is out of date now that we no longer use AttrCleaner here. Let's remove the comment. This is fine because 1) we don't support 2.6 anymore, and 2) we don't call callable anywhere in here anyway.
Also, if we're going to remove AttrCleaner then attr_lookup() and this method should be combined; the whole point of separating them was so that attr_lookup() could be run in the AttrCleaner context manager.
| if not py3: | ||
|
|
||
| def get_attr_safe(obj, name): | ||
| """side effect free getattr""" |
There was a problem hiding this comment.
While we're in this code, how about we rename get_attr_safe to getattr_safe and has_attr_safe to hasattr_safe; getattr and hasattr are Python builtins, and our semantics are trying to match those but be safer.
bpython/autocomplete.py
Outdated
| 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.""" | ||
| words = self.list_attributes(obj) | ||
| if hasattr(obj, "__class__"): | ||
| if inspection.has_attr_safe(obj, "__class__"): | ||
| words.append("__class__") | ||
| words = words + rlcompleter.get_class_members(obj.__class__) |
There was a problem hiding this comment.
Running
>>> class BrokenGetAttr(object):
... def __getattribute__(self, name):
... print('running custom getattribute on', name)
... import traceback
... traceback.print_stack()
...
>>> b = BrokenGetAttr()
>>> b.
shows me four times when a custom __getattribute__ is invoked: this obj.__class__, the one the line below, and twice from the dir() on 390.
For the first two we can grab __class__ once with static_getattr, but I'm not sure about the dir call; I guess it still needs AttrCleaner? It's just __dict__ and __class__ that dir seems to check for. I didn't find this very helpful but the implementation of dir() we're using is usually this one: https://github.com/python/cpython/blob/cadda52d974937069eeebea1cca4229e2bd400df/Objects/typeobject.c#L4867
There was a problem hiding this comment.
If only dir() used getattr_static this would be ok; maybe we should reimplement dir() using getattr_static?
There was a problem hiding this comment.
it'd be great to add a test like this for attr_matches to confirm a custom __getattribute__ isn't being invoked.
|
Thanks for the review. In addition to the small changes you mentioned, I added a test to test_autocomplete.py which ensures that an overridden As for |
I wanted to add open parenthesis completion in the suggestion box for callable functions, and I noticed there's some code we can consolidate or remove thanks to python3's
getattr_staticfunction. #163 mentions this. Here's what I changed.safe_get_attributeandsafe_get_attribute_new_stylefrom simpleeval.py, intoget_attr_safein inspection.py and move the tests from test_simpleeval to test_inspectionhas_attr_safeto inspection.pyOnce we drop python 2 support, we can delete the python2 definition for
get_attr_safeat which point AttrCleaner will be hardly used anywhere and can be removed / lightened up.