[issue28797] Modifying class __dict__ inside __set_name__

[Donald Stufft]

Changes by Donald Stufft :


--
pull_requests: +941

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28797] Modifying class __dict__ inside __set_name__

[Ned Deily]

Changes by Ned Deily :


--
priority: release blocker -> 

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28797] Modifying class __dict__ inside __set_name__

[Nick Coghlan]

Nick Coghlan added the comment:

Regarding the docs suggestion above, I need to make some other docs changes for 
issue 23722, so I'll roll that update in with those.

Given that, I think we can mark this issue as resolved.

--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28797] Modifying class __dict__ inside __set_name__

[Roundup Robot]

Roundup Robot added the comment:

New changeset 6b8f7d1e2ba4 by Serhiy Storchaka in branch '3.6':
Issue #28797: Modifying the class __dict__ inside the __set_name__ method of
https://hg.python.org/cpython/rev/6b8f7d1e2ba4

New changeset 18ed518d2eef by Serhiy Storchaka in branch 'default':
Issue #28797: Modifying the class __dict__ inside the __set_name__ method of
https://hg.python.org/cpython/rev/18ed518d2eef

--
nosy: +python-dev

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28797] Modifying class __dict__ inside __set_name__

[Nick Coghlan]

Nick Coghlan added the comment:

(Plus, as Martin noted, the tuple creation overhead could easily make the more 
complex patch slower in many cases)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28797] Modifying class __dict__ inside __set_name__

[Nick Coghlan]

Nick Coghlan added the comment:

I'd be OK with starting with the simpler patch, and only looking at the more 
complex one if the benchmark suite shows a significant slowdown (I'd be 
surprised if it did, as it should mainly impact start-up, and class dicts 
generally aren't that big)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28797] Modifying class __dict__ inside __set_name__

[Martin Teichmann]

Martin Teichmann added the comment:

I personally prefer the first patch, which iterates over a copy of __dict__. 
Making a copy of a dict is actually a pretty fast operation, I would even 
expect that it is faster than the proposed alternative, creating tuples.

Even if the second approach should be faster, the added code complexity is not 
worth the effort, as this is a code path where speed shouldn't matter much.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28797] Modifying class __dict__ inside __set_name__

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Here is much more complex patch that implements Nick's suggestion.

--
Added file: http://bugs.python.org/file45681/set_name-filtered-copy.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28797] Modifying class __dict__ inside __set_name__

[Nick Coghlan]

Nick Coghlan added the comment:

Ah, I'd missed that.

In that case, I think my suggestion to change the name of the local variable is 
still valid, while the specific approach just needs a comment saying it's 
handled as a full namespace snapshot so the descriptor type name can be 
reported in any error messages.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28797] Modifying class __dict__ inside __set_name__

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

> Have I missed something that would prevent that from working?

Error message contains value->ob_type->tp_name.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28797] Modifying class __dict__ inside __set_name__

[Nick Coghlan]

Nick Coghlan added the comment:

I was thinking that the notification dict could consist of mappings from 
attribute names to already bound __set_name__ methods, so the double lookup 
would be avoided that way (but hadn't actually sent the review where I 
suggested that).

That is, on the second loop, the actual method call would be:

tmp = PyObject_CallFunctionObjArgs(value, type, key, NULL);

Have I missed something that would prevent that from working?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28797] Modifying class __dict__ inside __set_name__

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

I considered this option, but if save only descriptors with the __set_name__ 
attribute, this would need either double looking up the __set_name__ attribute 
or packing it in a tuple with a value. All this too cumbersome. I chose copying 
all dictionary as the simplest way.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28797] Modifying class __dict__ inside __set_name__

[Nick Coghlan]

Nick Coghlan added the comment:

Rather than taking a snapshot of the whole class namespace, I think it will 
make more sense to make a new empty dictionary of descriptors to notify, and 
then split the current loop into two loops:

- the first loop adds descriptors with __set_name__ attributes to the 
notification dict
- the second loop calls __set_name__ on all the descriptors in the notification 
dict

Serhiy's patch effectively already does that via the initial PyDict_Copy, but 
that approach also redundantly copies items that don't define __set_name__ into 
the snapshot and then filters them out on the second pass.

Reviewing the patch also made me realise we're currently missing a 
specification of the expected behaviour in 
https://docs.python.org/dev/reference/datamodel.html#creating-the-class-object.

I suggest adding the following paragraph between the one about setting 
__class__ and the one about calling class descriptors:

"""
When using the default metaclass :cls:`type`, or any metaclass that ultimately 
calls ``type.__new__``, the following additional customisation steps are 
invoked:

* first, ``type.__new__`` collects all of the descriptors in the class 
namespace that define a ``__set_name__`` method
* second, all of these ``__set_name__`` methods are called with the class being 
defined and the assigned name of that particular descriptor
* finally, the ``__init_subclass__`` hook is called on the immediate parent of 
the new class in its method resolution order
"""

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28797] Modifying class __dict__ inside __set_name__

[Ned Deily]

Ned Deily added the comment:

Nick, as the original shepherd of PEP 487 (Issue27366), can you review Serhiy's 
proposed patch for 3.6.0rc1?  Thanks!

--
nosy: +ncoghlan

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28797] Modifying class __dict__ inside __set_name__

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Proposed patch iterates a copy of __dict__.

--
keywords: +patch
stage: needs patch -> patch review
versions: +Python 3.7
Added file: http://bugs.python.org/file45635/set_name-dict-copy.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28797] Modifying class __dict__ inside __set_name__

[Emanuel Barry]

Changes by Emanuel Barry :


--
nosy: +Martin.Teichmann

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28797] Modifying class __dict__ inside __set_name__

[Emanuel Barry]

Emanuel Barry added the comment:

Ow!

I can confirm the bug still happens on latest trunk. Nice finding!

--
nosy: +ebarry, ned.deily, rhettinger, serhiy.storchaka
priority: normal -> release blocker
stage:  -> needs patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28797] Modifying class __dict__ inside __set_name__

[Wombatz]

New submission from Wombatz:

This behavior occurs at least in python-3.6.0b3 and python-3.6.0b4.

Modifying the class __dict__ inside the __set_name__ method of a descriptor 
that is used inside that class can lead to a bug that possibly prevents other 
descriptors from being initialized (the call to their __set_name__ is 
prevented).

That happens because internally the cpython interpreter iterates the class 
__dict__ when calling all the __set_name__ methods. Changing the class __dict__ 
while iterating it leads to undefined behavior. This is the line in the source 
code https://github.com/python/cpython/blob/master/Objects/typeobject.c#L7010

See the attached file for an example where the bug can be observed. It defines 
a "desc" class that implements the __set_name__ method where it prints the name 
and modifies the class __dict__ of the containing class. Later a class is 
defined that has multiple instances of the "desc" class as class attributes. 
Depending on the number of attributes not all of them are initialized.

When you see the underlying C-Code the reason for this behavior is obvious but 
in python itself the problem might not be apparent.

To fix this bug the class __dict__ could be cashed and then the __set_name__ 
methods could be called while iterating over that copy.

--
components: Interpreter Core
files: reproduce.py
messages: 281674
nosy: Wombatz
priority: normal
severity: normal
status: open
title: Modifying class __dict__ inside __set_name__
type: behavior
versions: Python 3.6
Added file: http://bugs.python.org/file45632/reproduce.py

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com