[Python-ideas] AtributeError inside __get__

[Zahari Dim]

Hi,

The other day I came across a particularly ugly bug. A simplified case
goes like:

class X:
@property
def y(self):
return self.nonexisting

hasattr(X(),'y')

This returns False because hasattr calls the property which in turn
raises an AttributeError which is used to determine that the property
doesn't exist, even if it does. This is arguably unexpected and
surprising and can be very difficult to understand if it happens
within a large codebase. Given the precedent with generator_stop,
which solves a similar problem for StopIteration, I was wondering if
it would be possible to have the __get__ method convert the
AttributeErrors raised inside it to RuntimeErrors.

The situation with this is a little more complicated because there
could be a (possibly strange) where one might want to raise an
AttributeError inside __get__. But maybe the specification can be
changed so either `raise ForceAttributeError()` or `return
NotImplemented` achieves the same effect.


Merry Christmas!
Zahari.
_______________________________________________
Python-ideas mailing list
Python...@python.org
https://mail.python.org/mailman/listinfo/python-ideas
Code of Conduct: http://python.org/psf/codeofconduct/

[Nick Timkovich]

Are you saying that hasattr returning False was hiding a bug or is a bug? The former could be annoying to track down, though hasattr(X, 'y') == True. For the latter, having hasattr return False if an AttributeError is raised would allow the property decorator to retain identical functionality if it is used to replace a (sometimes) existing attribute.

[Chris Angelico]

On Mon, Dec 26, 2016 at 8:03 AM, Nick Timkovich <promet...@gmail.com> wrote:
> Are you saying that hasattr returning False was hiding a bug or is a bug?
> The former could be annoying to track down, though hasattr(X, 'y') == True.
> For the latter, having hasattr return False if an AttributeError is raised
> would allow the property decorator to retain identical functionality if it
> is used to replace a (sometimes) existing attribute.

This was touched on during the StopIteration discussions, but left
aside (it's not really connected, other than that exceptions are used
as a signal).

It's more that a property function raising AttributeError makes it
look like it doesn't exist.

Worth noting, though: The confusion only really comes up with hasattr.
If you simply try to access the property, you get an exception that
identifies the exact fault:

>>> X().y
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "<stdin>", line 4, in y
AttributeError: 'X' object has no attribute 'nonexisting'

Interestingly, the exception doesn't seem to have very useful arguments:

>>> ee.args
("'X' object has no attribute 'nonexisting'",)

So here's a two-part proposal that would solve Zaheri's problem:

1) Enhance AttributeError to include arguments for the parts in
quotes, for i18n independence.
2) Provide, in the docs, a hasattr replacement that checks the exception's args.

The new hasattr would look like this:

def hasattr(obj, name):
try:
getattr(obj, name)
return True
except AttributeError as e:
if e.args[1] == obj.__class__.__name__ and e.args[2] == name:
return False
raise

Since it's just a recipe in the docs, you could also have a version
that works on current Pythons, but it'd need to do string manipulation
to compare - something like:

def hasattr(obj, name):
try:
getattr(obj, name)
return True
except AttributeError as e:
if e.args[0] == "%r object has no attribute %r" % (
obj.__class__.__name__, name):
return False
raise

I can't guarantee that this doesn't get some edge cases wrong, eg if
you have weird characters in your name. But it'll deal with the normal
cases, and it doesn't need any language changes - just paste that at
the top of your file.

Zaheri, would this solve your problem?

ChrisA

[Nick Coghlan]

On 26 December 2016 at 04:24, Zahari Dim <zah...@gmail.com> wrote:

Hi,

The other day I came across a particularly ugly bug. A simplified case
goes like:

class X:
    @property
    def y(self):
        return self.nonexisting

hasattr(X(),'y')

This returns False because hasattr calls the property which in turn
raises an AttributeError which is used to determine that the property
doesn't exist, even if it does. This is arguably unexpected and
surprising and can be very difficult to understand if it happens
within a large codebase. Given the precedent with generator_stop,
which solves a similar problem for StopIteration, I was wondering if
it would be possible to have the __get__ method convert the
AttributeErrors raised inside it to RuntimeErrors.

The situation with this is a little more complicated because there
could be a (possibly strange) where one might want to raise an
AttributeError inside __get__.

There are a lot of entirely valid properties that look something like this:

    @property

    def attr(self):

        try:

            return data_store[lookup_key]

        except KeyError:

            raise AttributeError("attr")

And unlike StopIteration (where either "return" or "raise StopIteration" could be used), that *is* the way for a property method to indicate "attribute not actually present".

This is one of the many cases where IDEs with some form of static structural checking really do make development easier - the "self.nonexisting" would be flagged as non-existent directly in the editor, even before you attempted to run the code.

Cheers,

Nick.

--

Nick Coghlan   |   ncog...@gmail.com   |   Brisbane, Australia

[Zahari Dim]

> There are a lot of entirely valid properties that look something like this:
>
>
> @property
> def attr(self):
> try:
> return data_store[lookup_key]
> except KeyError:
> raise AttributeError("attr")

But wouldn't something like this be implemented more commonly with
__getattr__ instead (likely there is more than one such property in a
real example)? Even though __getattr__ has a similar problem (a bad
AttributeError inside can cause many bugs), I'd agree it would
probably be too difficult to change that without breaking a lot of
code. For __get__, the errors are arguably more confusing (e.g. when
used with @property) and the legitimate use case, while existing,
seems more infrequent to me: I did a github search and there was a
small number of cases, but most were for code written in python 2
anyway. Here a couple of valid ones:

https://github.com/dimavitvickiy/server/blob/a9a6ea2a155b56b84d20a199b5948418d0dbf169/orm/decorators.py
https://github.com/dropbox/pyston/blob/75562e57a8ec2f6f7bd0cf52012d49c0dc3d2155/test/tests/static_class_methods.py

Cheers,

Zahari

>
> This is one of the many cases where IDEs with some form of static structural
> checking really do make development easier - the "self.nonexisting" would be
> flagged as non-existent directly in the editor, even before you attempted to
> run the code.

In my particular case, the class had a __getattr__ that generated
properties dynamically. Therefore an IDE was unlikely to be helpful.

>
> Cheers,
> Nick.
>
> --
> Nick Coghlan | ncog...@gmail.com | Brisbane, Australia

[Zahari]

This looks like a good idea. Note that there is also getattr(X(), 'y', 'default') that would have to behave like this.

Cheers,

Zahari

 

[Chris Angelico]

On Tue, Dec 27, 2016 at 9:11 PM, Zahari <zah...@gmail.com> wrote:
> This looks like a good idea. Note that there is also getattr(X(), 'y',
> 'default') that would have to behave like this.
>

Forgot about that. Feel free to enhance the hasattr replacement. I
still think the parameterization of AttributeError would be worth
doing, but the two are independent.

[Nick Coghlan]

On 26 December 2016 at 21:23, Zahari Dim <zah...@gmail.com> wrote:

> There are a lot of entirely valid properties that look something like this:
>
>
>     @property
>     def attr(self):
>         try:
>             return data_store[lookup_key]
>         except KeyError:
>             raise AttributeError("attr")

But wouldn't something like this be implemented more commonly with
__getattr__ instead (likely there is more than one such property in a
real example)?  Even though __getattr__ has a similar problem (a bad
AttributeError inside can cause many bugs), I'd agree it would
probably be too difficult to change that without breaking a lot of
code. For __get__, the errors are arguably more  confusing (e.g. when
used with @property) and the legitimate use case, while existing,
seems more infrequent to me: I did a github search and there was a
small number of cases, but most were for code written in python 2
anyway.

Aye, I agree this pattern is far more common in __getattr__ than it is in descriptor __get__ implementations or in property getter implementations.

Rather than changing the descriptor protocol in general, I'd personally be more amenable to the idea of *property* catching AttributeError from the functions it calls and turning it into RuntimeError (after a suitable deprecation period). That way folks that really wanted the old behaviour could define their own descriptor that works the same way property does today, whereas if the descriptor protocol itself were to change, there's very little people could do to work around it if it wasn't what they wanted.

Exploring that possible approach a bit further:

- after a deprecation period, the "property" builtin would change to convert any AttributeError raised by the methods it calls into RuntimeError

- the current "property" could be renamed "optionalproperty": the methods may raise AttributeError to indicate the attribute isn't *really* present, even though the property is defined at the class level

- the deprecation warning would indicate that the affected properties should switch to using optionalproperty instead

[Chris Angelico]

On Sat, Dec 31, 2016 at 1:55 AM, Nick Coghlan <ncog...@gmail.com> wrote:
> Rather than changing the descriptor protocol in general, I'd personally be
> more amenable to the idea of *property* catching AttributeError from the
> functions it calls and turning it into RuntimeError (after a suitable
> deprecation period). That way folks that really wanted the old behaviour
> could define their own descriptor that works the same way property does
> today, whereas if the descriptor protocol itself were to change, there's
> very little people could do to work around it if it wasn't what they wanted.
>

Actually, that makes a lot of sense. And since "property" isn't magic
syntax, you could take it sooner:

from somewhere import property

and toy with it that way.

What module would be appropriate, though?

ChrisA

[Ethan Furman]

On 12/30/2016 06:55 AM, Nick Coghlan wrote:

> Rather than changing the descriptor protocol in general, I'd personally be
> more amenable to the idea of *property* catching AttributeError from the
> functions it calls and turning it into RuntimeError (after a suitable
> deprecation period). That way folks that really wanted the old behaviour
> could define their own descriptor that works the same way property does
> today, whereas if the descriptor protocol itself were to change, there's
> very little people could do to work around it if it wasn't what they wanted.

Sounds good to me. :)

--
~Ethan~

[Ethan Furman]

On 12/30/2016 07:10 AM, Chris Angelico wrote:

> Actually, that makes a lot of sense. And since "property" isn't magic
> syntax, you could take it sooner:
>
> from somewhere import property
>
> and toy with it that way.
>
> What module would be appropriate, though?

Well, DynamicClassAttribute is kept in the types module, so that's probably the place to put optionalproperty as well.

--
~Ethan~

[Nick Coghlan]

On 31 December 2016 at 05:53, Ethan Furman <et...@stoneleaf.us> wrote:

On 12/30/2016 07:10 AM, Chris Angelico wrote:

Actually, that makes a lot of sense. And since "property" isn't magic
syntax, you could take it sooner:

from somewhere import property

and toy with it that way.

What module would be appropriate, though?

Well, DynamicClassAttribute is kept in the types module, so that's probably the place to put optionalproperty as well.

I'd also be OK with just leaving it as a builtin.

[Eric Snow]

On Sat, Dec 31, 2016 at 12:33 AM, Nick Coghlan <ncog...@gmail.com> wrote:
> On 31 December 2016 at 05:53, Ethan Furman <et...@stoneleaf.us> wrote:
>> On 12/30/2016 07:10 AM, Chris Angelico wrote:
>>> What module would be appropriate, though?
>>
>> Well, DynamicClassAttribute is kept in the types module, so that's
>> probably the place to put optionalproperty as well.
>
> I'd also be OK with just leaving it as a builtin.

FWIW, I've felt for a while that the "types" module is becoming a
catchall for stuff that would be more appropriate in a new
"classtools" module (a la functools). I suppose that's what "types"
has become, but I personally prefer the separate modules that make the
distinction and would rather that "types" looked more like it does in
2.7. Perhaps this would be a good time to get that ball rolling or
maybe it's just too late. I'd like to think it's the former,
especially since I consider "classtools" a module that has room to
grow.

-eric