getattr silences an unrelated AttributeError

[pasenor]

BPO	39865
Nosy	@tim-one, @rhettinger, @ammaraskar, @pasenor
PRs	bpo-39865: Don't lose exception context when __getattr__ is present #19303

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2020-04-15.07:33:34.519>
created_at = <Date 2020-03-05.19:24:01.041>
labels = ['3.9']
title = 'getattr silences an unrelated AttributeError'
updated_at = <Date 2020-04-15.07:33:34.519>
user = 'https://github.com/pasenor'

bugs.python.org fields:

activity = <Date 2020-04-15.07:33:34.519>
actor = 'rhettinger'
assignee = 'none'
closed = True
closed_date = <Date 2020-04-15.07:33:34.519>
closer = 'rhettinger'
components = []
creation = <Date 2020-03-05.19:24:01.041>
creator = 'pasenor'
dependencies = []
files = []
hgrepos = []
issue_num = 39865
keywords = ['patch']
message_count = 5.0
messages = ['363449', '365369', '365580', '366275', '366491']
nosy_count = 4.0
nosy_names = ['tim.peters', 'rhettinger', 'ammar2', 'pasenor']
pr_nums = ['19303']
priority = 'normal'
resolution = 'rejected'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue39865'
versions = ['Python 3.9']

[pasenor]

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.

[ammaraskar]

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.

[ammaraskar]

Update: opened up #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.

[rhettinger]

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.

[rhettinger]

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.