classification
Title: tab-completition on instances repeatedly accesses attribute/descriptors values
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.6, Python 3.4, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: martin.panter Nosy List: ezio.melotti, martin.panter, python-dev, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2015-11-09 22:15 by ezio.melotti, last changed 2015-11-14 02:27 by ezio.melotti. This issue is now closed.

Files
File name Uploaded Description Edit
getattr-once.patch martin.panter, 2015-11-11 01:31 review
uncreated-attr.patch martin.panter, 2015-11-11 02:02 For 3.6 review
Messages (12)
msg254415 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2015-11-09 22:15
Pressing <tab> to invoke autocompletition on instances repeatedly accesses attributes/descriptors values:

>>> # <tab> is used to indicate when/where I press <tab>
>>> class Foo:
...   @property
...   def bar(self): print('Foo.bar called')
... 
>>> f = Foo()
>>> f.<tab>Foo.bar called 
Foo.bar called
Foo.bar called
Foo.bar called
<tab>Foo.bar called
Foo.bar called
Foo.bar called
Foo.bar called
<tab>
f.__class__(         f.__doc__            f.__getattribute__(  f.__le__(            f.__new__(           f.__setattr__(       f.__weakref__
f.__delattr__(       f.__eq__(            f.__gt__(            f.__lt__(            f.__reduce__(        f.__sizeof__(        f.bar
f.__dict__           f.__format__(        f.__hash__(          f.__module__         f.__reduce_ex__(     f.__str__(           
f.__dir__(           f.__ge__(            f.__init__(          f.__ne__(            f.__repr__(          f.__subclasshook__(  
>>> f.b<tab>Foo.bar called
Foo.bar called
Foo.bar called
Foo.bar called
ar<enter>
Foo.bar called

Each time I press <tab>, the property is called 4 times.  I'm not sure why the value is accessed at all, but even if there was a valid reason to do so, doing it once should be enough.
msg254419 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-11-09 23:11
Long story short: it is accessed due to the callable suffix check (Issue 449227), and this check is less than optimal, meaning the attribute gets accessed up to four times per Tab press.

In 3.6, there are only two calls, one for the hasattr() call, and one for the getattr() call:

Foo.bar called
  File "/home/proj/python/cpython/Lib/rlcompleter.py", line 85, in complete
    self.matches = self.attr_matches(text)
  File "/home/proj/python/cpython/Lib/rlcompleter.py", line 164, in attr_matches
    hasattr(thisobject, word)):
  File "<stdin>", line 3, in bar
Foo.bar called
  File "/home/proj/python/cpython/Lib/rlcompleter.py", line 85, in complete
    self.matches = self.attr_matches(text)
  File "/home/proj/python/cpython/Lib/rlcompleter.py", line 165, in attr_matches
    val = getattr(thisobject, word)
  File "<stdin>", line 3, in bar

Before revision 4dbb315fe667 (Issue 25011), “words” was a list rather than a set. It gets two “bar” entries, one from dir(f) and one from dir(f.__class__). Maybe it is worth backporting set(), I dunno.

The hasattr() call was added in r65168 (Issue 3396) as a look-before-you-leap check to avoid the completer from dying from getattr() exceptions. IMO in this case it is better to “ask for forgiveness” by catching Exception from getattr(). This would be more robust to quirky and buggy attributes anyway.

Finally (or really, initially), getattr() was added in r64664 (Issue 449227). I think even if getattr() fails, the attribute name could still be output, just not with the callable suffix (which I partly disagree with anyway). It would have been like that previously. But I guess this is debatable, and a separate issue.
msg254422 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-11-09 23:35
Illustration of why I think removing the attribute on errors is bad:

>>> class Foo:
...   @property
...   def bar(self): return self._bar
...   @bar.setter
...   def bar(self, value): self._bar = value
... 
>>> f = Foo()
>>> f.bar = ...  # Tab completion fails in this case
msg254435 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-11-10 07:32
Agreed with all your suggestions Martin. Do you want to write a patch?
msg254468 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-11-11 01:31
getattr-once.patch handles my first two points. It backports the set() change, and avoids the hasattr() check. I guess we apply this to 3.4+; expect merge conflicts with 3.6.

As for the last point, I will make another patch to include special attributes that don’t actually exist yet, where they are currently omitted. Would we consider this a new feature for 3.6? Going by Serhiy’s logic in <https://bugs.python.org/issue25209#msg251514> I would say yes.
msg254469 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-11-11 02:02
uncreated-attr.patch is against the 3.6 branch, and includes the excessive getattr() test, and new code to include uncreated attribute names. I added a paragraph to What’s New describing the new completion behaviour.
msg254475 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-11-11 06:15
Both patches LGTM. Thank you Martin.

> Would we consider this a new feature for 3.6?

We can consider this a fix of the regression in issue449227. But this behavior here is so long. I agree with applying it to 3.6 only.

For nicer Mercurial history, it would be better to apply both patches separately in 3.6.

Would not be simpler to use uninitialized slot attribute in test_uncreated_attr?
msg254583 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-11-13 03:02
Yeah I plan to merge the first patch (fixing conflicts) into 3.6 without any of the uncreated-property additions, and then apply that extra stuff in a separate commit.

Using __slots__ would be simpler; I’ll switch it over.
msg254637 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-11-13 23:57
New changeset 92f989bfeca2 by Martin Panter in branch '3.4':
Issue #25590: Make rlcompleter only call getattr() once per attribute
https://hg.python.org/cpython/rev/92f989bfeca2

New changeset 808279e14700 by Martin Panter in branch '3.5':
Issue #25590: Merge rlcompleter change from 3.4 into 3.5
https://hg.python.org/cpython/rev/808279e14700

New changeset ff68304356fe by Martin Panter in branch 'default':
Issue #25590: Merge rlcompleter getattr change from 3.5
https://hg.python.org/cpython/rev/ff68304356fe

New changeset bbf63749a129 by Martin Panter in branch 'default':
Issue #25590: Complete attribute names even if they are not yet created
https://hg.python.org/cpython/rev/bbf63749a129
msg254638 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-11-14 00:45
New changeset 16d83e11675f by Martin Panter in branch '2.7':
Issue #25590: Make rlcompleter only call getattr() once per attribute
https://hg.python.org/cpython/rev/16d83e11675f
msg254640 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-11-14 02:11
I also ported the getattr patch to the 2.7 branch.
msg254641 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2015-11-14 02:27
Thanks for taking care of this!
History
Date User Action Args
2016-05-13 13:20:23martin.panterlinkissue5062 superseder
2016-05-13 12:39:24martin.panterlinkissue22143 superseder
2015-11-14 02:27:23ezio.melottisetmessages: + msg254641
2015-11-14 02:11:41martin.pantersetstatus: open -> closed
versions: + Python 2.7
messages: + msg254640

resolution: fixed
stage: commit review -> resolved
2015-11-14 00:45:13python-devsetmessages: + msg254638
2015-11-13 23:57:41python-devsetnosy: + python-dev
messages: + msg254637
2015-11-13 03:02:16martin.pantersetmessages: + msg254583
2015-11-11 06:15:39serhiy.storchakasetassignee: martin.panter
messages: + msg254475
stage: patch review -> commit review
2015-11-11 02:02:49martin.pantersetfiles: + uncreated-attr.patch

messages: + msg254469
2015-11-11 01:31:56martin.pantersetfiles: + getattr-once.patch
keywords: + patch
messages: + msg254468

stage: needs patch -> patch review
2015-11-10 07:32:02serhiy.storchakasetmessages: + msg254435
stage: test needed -> needs patch
2015-11-09 23:35:50martin.pantersetmessages: + msg254422
2015-11-09 23:11:56martin.pantersetnosy: + serhiy.storchaka, martin.panter
messages: + msg254419
components: + Library (Lib)
2015-11-09 22:15:43ezio.melotticreate