bpo-46730: Add more info to @property AttributeError messages

[Alex-Blade]

The property decorator raises AttributeErrors with messages
that neither help to identify the problem nor lead to a possible solution.
This PR proposes changes in error messages to address the issue.

https://bugs.python.org/issue46730

[the-knights-who-say-ni]

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@Alex-Blade

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

[Alex-Blade]

Now, there are a few things to discuss:
@eryksun made the point that proposed messages could be more consistent. For instance, in the following form: "property %R has no deleter"

I absolutely agree with it, since it was the initial idea, however when trying it I've found out that the testing system expects the name of the attribute to be at the end:

class PropertyUnreachableAttributeWithName(_PropertyUnreachableAttribute, unittest.TestCase): msg_format = "^{} 'foo'$" 

So in order to make the message in the proposed format, this should be subject to change as well.

P.S. I have signed the CLA, just haven't got the response yet.

[sweeneyde]

I absolutely agree with it, since it was the initial idea, however when trying it I've found out that the testing system expects the name of the attribute to be at the end:

I think those test cases can be adjusted appropriately to match the behavior.

P.S. I have signed the CLA, just haven't got the response yet.

Yep, it usually takes a business day or two to go through.

I also think it would be useful to include the type name in the message:

class ThingDoer: @property def thingy(self): return 42 >>> td = ThingDoer() >>> td.thingy = 1000 AttributeError: 'ThingDoer' property 'thingy' has no setter 

Compare to the current behavior for other attributes in other situations:

>>> "a".upper = 2 AttributeError: 'str' object attribute 'upper' is read-only 

I believe this is the relevant code for that case:

cpython/Objects/object.c

Lines 1405 to 1407 in 6331c08

	PyErr_Format(PyExc_AttributeError,
	"'%.50s' object attribute '%U' is read-only",
	tp->tp_name, name);

[TeamSpen210]

Using tp->tp_name would be misleading in cases where the property is accessed from a subclass, since that would give you the subclass name. Instead it should be fetched in __set_name__ like the property's own name.

[sweeneyde]

Isn't Py_TYPE(obj)->tp_name okay? That's the behavior in this situation:

>>>fromcollectionsimportdefaultdict>>>dict.popisdefaultdict.popTrue>>>dd=defaultdict() >>>dd.pop=42Traceback (mostrecentcalllast): File"<stdin>", line1, in<module>AttributeError: 'collections.defaultdict'objectattribute'pop'isread-only

And there are parallel situations like this:

>>>classBase: ... def__float__(self): ... returnNone ... >>>classSub(Base): ... pass ... >>>float(Sub()) Traceback (mostrecentcalllast): File"<pyshell#28>", line1, in<module>float(Sub()) TypeError: Sub.__float__returnednon-float (type NoneType)

My thinking is that specifying the type is helpful for identifying which objects in the client code are being used and which line to look at, not where the implementation happens to live:

>>>classBaseThingDoer: ... @property ... defattr(self): ... pass>>># ( ... Suppose lots of other concrete implementations ... )>>>classSpecialThingDoer(BaseThingDoer): ... pass>>>thing=SpecialThingDoer() # These two lines are the lines I'm looking at.>>>thing.attr=42# I think I would expect the error message to say SpecialThingDoer.AttributeError: can't set attribute 'attr'

It's safer and easier to implement if I understand right.

[Alex-Blade]

@sweeneyde, I think your idea doesn't solve the problem of "which line to look at" because of subclasses.
I've committed some more changes that cover the subclass problem:

classA: @propertydefa(self) ->int: returnNoneclassB(A): @propertydefb(self) ->int: returnNoneclassC(B): pass>>>A().a=2Traceback (mostrecentcalllast): File"<stdin>", line1, in<module>AttributeError: property'a'originatingfrom'A'hasnosetter>>>B().a=2Traceback (mostrecentcalllast): File"<stdin>", line1, in<module>AttributeError: property'a'originatingfrom'A'hasnosetter>>>B().b=2Traceback (mostrecentcalllast): File"<stdin>", line1, in<module>AttributeError: property'b'originatingfrom'B'hasnosetter>>>C().b=2Traceback (mostrecentcalllast): File"<stdin>", line1, in<module>AttributeError: property'b'originatingfrom'B'hasnosetter

Few points to discuss though:

1. This change required an increase in the size of propertyobject (from 5Pi to 6Pi), so I'm not sure if this is justified enough. On the other hand the increase seems to be not drastic.
2. The way I did it seems a bit sketchy to me (PyObject_GetAttrString(PyTuple_GET_ITEM(args, 0), "__name__");), but I cannot find a better way.
3. I wonder if adding full __module__ and __qualname__ would actually be better (for everything but builtins maybe)? Class names can be identical while the module that they come from can be different - may lead to a confusion. On the other hand, since user anyway knows where the error appeared from the traceback, determining which exact class was meant in the message can be considered as a trivial task.

[Alex-Blade]

Oh, actually I think I've misunderstood your comment. To the example of SpecialThingDoer - I think it depends on where the setter should be defined for each particular case, meaning that the type that user would expect there could be different from case to case.

Perhaps this could be resolved by choosing the sentence for the error message:

'type' object property 'prop' has no setter

This one implies that the problem is within the current object and thus SpecialThingDoer is better.

property 'prop' originating from 'type' has no setter

And this one implies that the property is the problem and it should be fixed wherever it's defined - in BaseThingDoer.

I don't see any convenience difference, however implementing the first one requires less changes, resolves the issue with sketchiness of the first attempt and removes unnecessary size increase. Seems like I should retry it :)

[sweeneyde]

I think I agree with lots of your assessments -- either type might be more helpful in different circumstances. But for simplicity of implementation and consistency with other similar situations, I'd prefer to just display the type of obj. You could even use PyType_GetQualName on Py_TYPE(obj).

I don't know what the best verbiage is to convey that that's what is being displayed, but I agree that some word choices are more suggestive of that than others. Maybe something like what Bruce Leban suggested on Python-Ideas, or

AttributeError: Can't set 'attr' property on object of type 'SpecialThingDoer' 

Another reason to avoid adding extra state to the property is that the same property object can theoretically live in multiple classes:

>>> class A: ... @property ... def attr(self): ... return 42 ... >>> class B: ... pass ... >>> B.attr = A.attr >>> B.attr is A.attr True >>> B.attr <property object at 0x000001C29B59F2E0> >>> B().attr 42 >>> B().attr = 17 AttributeError: ... # might be confusing if this mentioned A 

[TeamSpen210]

The misleading behaviour when a property is in two classes isn't really solvable, and also applies if you put it under a different attribute name - functions have the same issue. Since the decorator methods create copies, it's probably not too big of an issue.

[sweeneyde]

The misleading behaviour when a property is in two classes isn't really solvable, and also applies if you put it under a different attribute name - functions have the same issue. Since the decorator methods create copies, it's probably not too big of an issue.

I'm suggesting that it is "solvable" in one sense: just don't store inside a property object any information that couples it to a particular type in the first place, and instead use type(x).__qualname__ in error messages about x.attr.

[Alex-Blade]

I've assessed all variants and came to a conclusion that a combination of all our suggestions would work great:

classA: @propertydefa(self) ->int: returnNoneclassB(A): @propertydefb(self) ->int: returnNoneB().b=2Traceback (mostrecentcalllast): File"<stdin>", line1, in<module>AttributeError: property'b'inobjectof type 'B'hasnosetterB().a=2Traceback (mostrecentcalllast): File"<stdin>", line1, in<module>AttributeError: property'a'inobjectof type 'B'hasnosetter

1. This removes confusion "# might be confusing if this mentioned A", which I agree is confusing.
2. It proposes the solution directly (i.e. suggests to define a setter and etc).
3. It does not imply that solution should be implemented in class B, since it only shows what was the type of current object, meaning that while type of object is B, the origin of the property could be anything and thus the setter could be defined anywhere.
4. The implementation is simple and doesn't require to store information on the actual owner of the property.

UPD. And yes, it's based on qualname

[sweeneyde]

This is mostly looking good, just some minor nits remaining.

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[Alex-Blade]

I have made the requested changes; please review again

P.S. Seems like I also should make a NEWS entry, right?

[bedevere-bot]

Thanks for making the requested changes!

@sweeneyde: please review the changes made to this pull request.

[sweeneyde]

P.S. Seems like I also should make a NEWS entry, right?

Yes, please do. You can either click on "Details" of the bevedere/news check or you can use blurb

[sweeneyde]

LGTM. I'll leave it open for a day or so to see if there are any more comments, but after that, I can merge.

If you want, you can add your name to MISC/ACKS as well.

[sweeneyde]

Shouldn't "object %R" be "%R object"? 42 is an int object, not object int.

[Alex-Blade]

Oh, that's what caused my confusion and I couldn't understand it! :)

[Alex-Blade]

@eryksun, what do you think now?

[eryksun]

what do you think now?

I'm okay with either "property %R of %R objects" or "property %R of %R object". I prefer the former. It's common in English when generalizing to a category or class of things, unless that class is already a mass noun. You can see this style used in "Objects/descrobject.c":

• "<method '%V' of '%s' objects>"
• "<member '%V' of '%s' objects>"
• "<attribute '%V' of '%s' objects>"
• "<slot wrapper '%V' of '%s' objects>"
• "descriptor '%V' for '%.100s' objects "
• "attribute '%V' of '%.100s' objects is not readable"
• "descriptor '%V' for '%.100s' objects "
• "attribute '%V' of '%.100s' objects is not writable"

[sweeneyde]

Reasonable people will disagree, but the pattern I see is that usages of the plural as in "{p} for{T} objects" tend to be situations where the descriptor knows what type it's supposed to apply to, but there is no well-defined notion of "the associated type" for property objects, since you can do A.attr = B.attr = property().

In contrast, other error messages tend to use the singular "object" when they display the type of the particular object in question (as this PR is set to do):

>>> "a".upper = 2 AttributeError: 'str' object attribute 'upper' is read-only >>> for i in 42: pass TypeError: 'int' object is not iterable >>> len(42) TypeError: object of type 'int' has no len() 

[tiran]

The commit introduced a reference leak and code was missing checks for NULL. #31389 addresses both problems.