classification
Title: getattr silences an unrelated AttributeError
Type: Stage: resolved
Components: Versions: Python 3.9
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: ammar2, pasenor, rhettinger, tim.peters
Priority: normal Keywords: patch

Created on 2020-03-05 19:24 by pasenor, last changed 2020-04-15 07:33 by rhettinger. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 19303 closed ammar2, 2020-04-02 10:40
Messages (5)
msg363449 - (view) Author: (pasenor) * Date: 2020-03-05 19:24
if a class has a descriptor and a defined __getattr__ method, and an AttributeError (unrelated to the descriptor lookup) is raised inside the descriptor, it will be silenced:

class A:
    @property
    def myprop(self):
        print("property called")
        a = 1
        a.foo  # <-- AttributeError that should not be silenced

    def __getattr__(self, attr_name):
        print("__getattr__ called")

a = A()
a.myprop


In this example myprop() is called, the error silenced, then __getattr__() is called.
This can lead to rather subtle bugs. Probably an explicit AttributeError should be raised instead.
msg365369 - (view) Author: Ammar Askar (ammar2) * (Python committer) Date: 2020-03-31 09:05
As unfortunate as this is, I don't think there's an easy way to solve this while adhering to descriptors and the attribute lookup model. This is a consequence of the following rule:

> object.__getattr__(self, name):
>   Called when the default attribute access fails with an
>   AttributeError (either __getattribute__() raises an AttributeError
>   because name is not an instance attribute or an attribute in the
>   class tree for self; or __get__() of a name property raises
>   AttributeError)

As it notes, if the __get__() raises an AttributeError then a fallback to __getattr__ is initiated. One may think that maybe we can just catch AttributeError in @property to try to fix this problem but a quick search shows that people do intentionally raise AttributeError in @property methods:

* https://github.com/kdschlosser/EventGhost-TPLink/blob/a4a642fde8dd4deba66262a36d673cbbf71b8ceb/TPLink/tp_link/rule.py#L148-L152

* https://github.com/ajayau404/sniffer/blob/cd0c813b8b526a3c791735a41b13c7677eb4aa0e/lib/python3.5/site-packages/vpython/vpython.py#L1942-L1944

While this in combination with a __getattr__ is rare, I was able to find one example:

* https://github.com/xrg/behave_manners/blob/19a5feb0b67fe73cd902a959f0d038b905a69b38/behave_manners/context.py#L37

I don't think that changing this behavior is acceptable as people might be relying on it and it's well documented.


In your case, I think it's really the "catch-all" __getattr__ that's at fault here which really shouldn't be returning None for all attributes blindly. This does bring up a good point though that in the process of this fall-back behavior, the original AttributeError from A's property does get masked. What can be done and might be a good idea is to show the original AttributeError failure as the cause for the second. Something like this:

  Traceback (most recent call last):
    File "<stdin>", line 2, in <module>
    File "<stdin>", line 5, in myprop
  AttributeError: 'int' object has no attribute 'foo'

  The above exception was the direct cause of the following exception:

  Traceback (most recent call last):
    File "<stdin>", line 7, in <module>
    File "<stdin>", line 5, in <module>
    File "<stdin>", line 7, in __getattr__
  AttributeError: not found in __getattr__

This at least somewhat indicates that the original descriptor __get__ failed and then __getattr__ also raised. As opposed to now where the original exception gets masked:

  >>> class A:
  ...   @property
  ...   def myprop(self):
  ...     a = 1
  ...     a.foo
  ...   def __getattr__(self, attr_name):
  ...     raise AttributeError("not found in __getattr__")
  ...
  >>> a = A()
  >>> a.myprop
  Traceback (most recent call last):
    File "<stdin>", line 1, in <module>
    File "<stdin>", line 7, in __getattr__
  AttributeError: not found in __getattr__

I'm gonna take a stab at implementing this real quick to see if it's actually useful and viable.
msg365580 - (view) Author: Ammar Askar (ammar2) * (Python committer) Date: 2020-04-02 10:43
Update: opened up https://github.com/python/cpython/pull/19303 as a quick first pass at implementing this. It works as expected and retains the context from the original exception just in case it's needed. The code isn't too pretty, looks like exception chaining was primarily designed to be a first class citizen using the `raise e2 from e` syntax. A helper in exceptions.c would definitely go a long way to making it more readable.
msg366275 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2020-04-12 23:07
My instincts are to leave this alone and not gum up heavily trafficked core business logic.  I don't like the external calls, the extra increfs and decrefs (and possible rentrancy issues), performance impact, or the pattern of holding the exception across a call that can invoke arbitrary code.   The code for slot_tp_getattr_hook() is currently very clean and it would be nice to keep it that way.

ISTM that the problem is better addressed by unittesting, linting, and static type checking rather than by gumming-up runtime.
msg366491 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2020-04-15 07:33
Am going to mark this as closed for the reasons listed above.  If another coredev wants to champion this, feel free to resurrect the issue.
History
Date User Action Args
2020-04-15 07:33:34rhettingersetstatus: open -> closed
resolution: rejected
messages: + msg366491

stage: patch review -> resolved
2020-04-12 23:07:32rhettingersetnosy: + tim.peters

messages: + msg366275
versions: - Python 2.7, Python 3.5, Python 3.6, Python 3.7, Python 3.8
2020-04-02 10:43:41ammar2setmessages: + msg365580
2020-04-02 10:40:35ammar2setkeywords: + patch
stage: patch review
pull_requests: + pull_request18665
2020-03-31 09:05:14ammar2setnosy: + ammar2
messages: + msg365369
2020-03-06 07:31:35rhettingersetnosy: + rhettinger
2020-03-05 19:24:01pasenorcreate