AtributeError inside __get__
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.
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. On Sun, Dec 25, 2016 at 1:24 PM, Zahari Dim <zaharid@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__. 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-ideas@python.org https://mail.python.org/mailman/listinfo/python-ideas Code of Conduct: http://python.org/psf/codeofconduct/
On Mon, Dec 26, 2016 at 8:03 AM, Nick Timkovich <prometheus235@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
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?
This looks like a good idea. Note that there is also getattr(X(), 'y', 'default') that would have to behave like this. Cheers, Zahari
ChrisA _______________________________________________ Python-ideas mailing list Python...@python.org <javascript:> https://mail.python.org/mailman/listinfo/python-ideas Code of Conduct: http://python.org/psf/codeofconduct/
On Tue, Dec 27, 2016 at 9:11 PM, Zahari <zaharid@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. ChrisA
On 26 December 2016 at 04:24, Zahari Dim <zaharid@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 | ncoghlan@gmail.com | Brisbane, Australia
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/a9a6ea2a155b56b84d20a199b594841... https://github.com/dropbox/pyston/blob/75562e57a8ec2f6f7bd0cf52012d49c0dc3d2... 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 | ncoghlan@gmail.com | Brisbane, Australia
On 26 December 2016 at 21:23, Zahari Dim <zaharid@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 Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
On Sat, Dec 31, 2016 at 1:55 AM, Nick Coghlan <ncoghlan@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
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~
On 31 December 2016 at 05:53, Ethan Furman <ethan@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. Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
On Sat, Dec 31, 2016 at 12:33 AM, Nick Coghlan <ncoghlan@gmail.com> wrote:
On 31 December 2016 at 05:53, Ethan Furman <ethan@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
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~
participants (7)
-
Chris Angelico -
Eric Snow -
Ethan Furman -
Nick Coghlan -
Nick Timkovich -
Zahari -
Zahari Dim