Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

setattr a read-only property; the AttributeError should show the attribute that failed #71981

Closed
ztane mannequin opened this issue Aug 18, 2016 · 16 comments
Closed

setattr a read-only property; the AttributeError should show the attribute that failed #71981

ztane mannequin opened this issue Aug 18, 2016 · 16 comments
Assignees
Labels
3.10 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@ztane
Copy link
Mannequin

ztane mannequin commented Aug 18, 2016

BPO 27794
Nosy @rhettinger, @ncoghlan, @bitdancer, @ztane, @zhangyangyu, @Vgr255, @tirkarthi, @uriyyo
PRs
  • bpo-27794: Add name attribute to property class #23967
  • 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 = 'https://github.com/rhettinger'
    closed_at = <Date 2020-12-30.09:52:51.256>
    created_at = <Date 2016-08-18.13:57:19.594>
    labels = ['interpreter-core', 'type-feature', '3.10']
    title = 'setattr a read-only property; the AttributeError should show the attribute that failed'
    updated_at = <Date 2020-12-30.09:52:51.253>
    user = 'https://github.com/ztane'

    bugs.python.org fields:

    activity = <Date 2020-12-30.09:52:51.253>
    actor = 'rhettinger'
    assignee = 'rhettinger'
    closed = True
    closed_date = <Date 2020-12-30.09:52:51.256>
    closer = 'rhettinger'
    components = ['Interpreter Core']
    creation = <Date 2016-08-18.13:57:19.594>
    creator = 'ztane'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 27794
    keywords = ['patch']
    message_count = 16.0
    messages = ['273027', '273033', '273035', '273036', '273302', '273385', '273387', '273396', '301045', '383852', '383868', '383870', '383873', '383876', '384053', '384054']
    nosy_count = 10.0
    nosy_names = ['rhettinger', 'ncoghlan', 'r.david.murray', 'ztane', 'xiang.zhang', 'abarry', 'apatrushev', 'Muhammad Alkarouri', 'xtreak', 'uriyyo']
    pr_nums = ['23967']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue27794'
    versions = ['Python 3.10']

    @ztane
    Copy link
    Mannequin Author

    ztane mannequin commented Aug 18, 2016

    Today we had an internal server error in production. I went to see the sentry logs for the error, and was dismayed: the error was AttributeError: can't set attribute, and the faulting line was setattr(obj, attr, value) that happens in a for-loop that uses computed properties coming from who knows.

    Well, I quickly ruled out that it was because the code was trying to set a value to a read-only property descriptor, the only problem was to find out *which of all these read-only properties* was it trying to set:

    Python 3.6.0a3+ (default, Aug 11 2016, 11:45:31) 
    [GCC 5.4.0 20160609] on linux
    Type "help", "copyright", "credits" or "license" for more information.
    >>> class Foo:
    ...     @property
    ...     def bar(self): pass
    ... 
    >>> setattr(Foo(), 'bar', 42)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    AttributeError: can't set attribute

    Could we change this for Python 3.6 so that the message for this could include the name of the property just like AttributeError: has no attribute 'baz' does?

    @ztane ztane mannequin added the type-feature A feature request or enhancement label Aug 18, 2016
    @bitdancer
    Copy link
    Member

    If someone submits a patch in time, we'd probably accept it.

    @Vgr255
    Copy link
    Mannequin

    Vgr255 mannequin commented Aug 18, 2016

    The approach I'd take would be to change how {get,set,del}attr handle AttributeError; possibly by automatically filling in the information and giving a nicer error message. This would fix this as a side-effect (somewhat; some bits of code would need to change). How does that sound to core devs?

    @ztane
    Copy link
    Mannequin Author

    ztane mannequin commented Aug 18, 2016

    Unfortunately it seems that it is not that straightforward. The descriptor object doesn't know the name of the property. The error is raised in property_descr_set. However the error itself can be propagated from setting another property.

    @Vgr255
    Copy link
    Mannequin

    Vgr255 mannequin commented Aug 21, 2016

    Opened bpo-27823 as a followup to this, and carried nosy list over.

    @zhangyangyu
    Copy link
    Member

    One solution I can think of is alter the constructor of property() and add an optional name attribute to it. If users really care about the exception message, they can set the attribute to the property's name. If they don't care, everything remains the same as now. This is just like the doc parameter of property() right now and the first parameter of type() when it is used to create a class dynamically. This is just a suggestion, I am not sure it's worth to do it.

    @bitdancer
    Copy link
    Member

    Not worth it. It would feel like boilerplate, and the situation where you want the information is almost invariably going to be one you didn't anticipate and so didn't "bother" with the name. Either that or you always have to do it, and the elegance of your python code takes a hit not commensurate with the benefit.

    @ztane
    Copy link
    Mannequin Author

    ztane mannequin commented Aug 22, 2016

    I've got one idea about how to implement this, but it would require adding a new flag field to PyExc_AttributeError type.

    This flag, if set, would tell that the AttributeError in question was raised in C descriptor code or under similar circumstances, and that the attribute name was not known, and thus it is OK for setattr/delattr and attribute lookups to append ": attributename" to the end of the message, then clear the flag; then all those places that raise AttributeError in __get__, __set__, __del__ would just need to set this flag.

    @MuhammadAlkarouri
    Copy link
    Mannequin

    MuhammadAlkarouri mannequin commented Aug 31, 2017

    Now that the descriptor protocol has __set_name__, does this make any difference to this issue? The property can know its name, subject to a suitable patch.

    @serhiy-storchaka serhiy-storchaka added 3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Nov 9, 2017
    @uriyyo
    Copy link
    Member

    uriyyo commented Dec 27, 2020

    I have implemented this feature using an idea provided by Xiang Zhang.
    Basically, property has new optional attribute name which will be added to msg of AttributeError in a case when name is set.

    @rhettinger
    Copy link
    Contributor

    Yurii, this looks nice. I question whether *name* should be a part of the constructor because it immediately gets overridden by __set_name__ method.

    >>> class A:
    ...    def x(self):
    ...        return 44
    ...    x = property(x, name='y')
    ...
    >>> vars(A)['x'].name
    'x'

    @rhettinger rhettinger self-assigned this Dec 27, 2020
    @uriyyo
    Copy link
    Member

    uriyyo commented Dec 27, 2020

    Raymond, it's a good question.

    I have added name to property constructor to support cases when a property is added to a class after it was declared.

    For instance:

    class Foo:
        pass
    
    
    Foo.foo = property(name='foo')
    
    f = Foo()
    f.foo = 10
    

    So, in my opinion, it's expected behavior that name is overwritten by __set_name__ method.

    What do you think about that? Should we change this behevior?

    @rhettinger
    Copy link
    Contributor

    In my opinion, it's expected behavior that name is
    overwritten by __set_name__ method.

    It is almost certain that there will be others won't share that expectation. The StackOverflow questions and bug reports are inevitable.

    Most examples of __set_name__() use a private attribute. I recommend that you go that route and stick to the spirit of the original bug report. The OP asked for better error messages when possible. They didn't ask for an API expansion.

    I have added name to property constructor to support cases
    when a property is added to a class after it was declared.

    That rarely occurs in practice. I wouldn't worry about it. Also AFAICT the only time the new error message matters is in the context of a setattr() where the attribute isn't already shown in the traceback. So the case in question is really a rarity inside another rarity. Let's declare YAGNI unless an actual end-user problem arises in real-world code.

    @uriyyo
    Copy link
    Member

    uriyyo commented Dec 27, 2020

    You a totally right, looks like I tried to add a feature that no one asked for:)

    I have updated PR and removed name parameter from property constuctor. Thanks for your advice.

    @rhettinger
    Copy link
    Contributor

    New changeset c56387f by Yurii Karabas in branch 'master':
    bpo-27794: Add name attribute to property class (GH-23967)
    c56387f

    @rhettinger
    Copy link
    Contributor

    Thanks for the PR :-)

    @rhettinger rhettinger added 3.10 only security fixes and removed 3.7 (EOL) end of life labels Dec 30, 2020
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.10 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants