The type of cached objects is mutable

[serhiy-storchaka]

BPO	24912
Nosy	@malemburg, @gvanrossum, @brettcannon, @rhettinger, @ncoghlan, @pitrou, @vstinner, @larryhastings, @benjaminp, @ned-deily, @njsmith, @markshannon, @eltoder, @ericsnowcurrently, @serhiy-storchaka, @erlend-aasland
PRs	bpo-43973: Use Py_TPFLAGS_IMMUTABLETYPE to check for class assignments #25714
Files	issue24912.patch issue24912-v2.patch

^{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/larryhastings'
closed_at = <Date 2015-09-06.07:53:19.607>
created_at = <Date 2015-08-21.22:02:50.644>
labels = ['interpreter-core', 'type-bug', 'release-blocker']
title = 'The type of cached objects is mutable'
updated_at = <Date 2021-04-30.11:27:56.656>
user = 'https://github.com/serhiy-storchaka'

bugs.python.org fields:

activity = <Date 2021-04-30.11:27:56.656>
actor = 'erlendaasland'
assignee = 'larry'
closed = True
closed_date = <Date 2015-09-06.07:53:19.607>
closer = 'larry'
components = ['Interpreter Core']
creation = <Date 2015-08-21.22:02:50.644>
creator = 'serhiy.storchaka'
dependencies = []
files = ['40310', '40312']
hgrepos = []
issue_num = 24912
keywords = ['patch']
message_count = 69.0
messages = ['248981', '248982', '248983', '249019', '249180', '249189', '249198', '249345', '249350', '249351', '249354', '249357', '249360', '249363', '249369', '249388', '249392', '249393', '249396', '249397', '249398', '249438', '249446', '249450', '249455', '249456', '249457', '249467', '249468', '249470', '249471', '249473', '249483', '249484', '249489', '249495', '249499', '249500', '249502', '249504', '249505', '249506', '249509', '249510', '249511', '249604', '249607', '249618', '249623', '249685', '249854', '249869', '249871', '249888', '249889', '249890', '249891', '249893', '249930', '249931', '249934', '249938', '249940', '249943', '249944', '249948', '249954', '249979', '392411']
nosy_count = 18.0
nosy_names = ['lemburg', 'gvanrossum', 'brett.cannon', 'rhettinger', 'ncoghlan', 'pitrou', 'vstinner', 'larry', 'benjamin.peterson', 'ned.deily', 'Arfrever', 'njs', 'Mark.Shannon', 'python-dev', 'eltoder', 'eric.snow', 'serhiy.storchaka', 'erlendaasland']
pr_nums = ['25714']
priority = 'release blocker'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue24912'
versions = ['Python 3.5', 'Python 3.6']

[serhiy-storchaka]

The type of non-heap types can be changed in 3.5. This means that the type of cached immutable objects such as small ints, empty or 1-character strings, etc can be changed.

>>> class I(int):
...     __slots__ = ()
...     def __repr__(self):
...         return 'Answer to The Ultimate Question of Life, the Universe, and Everything'
...     def __add__(self, other):
...         return self * other
... 
>>> (42).__class__ = I
>>> ord('*')
Answer to The Ultimate Question of Life, the Universe, and Everything
>>> x = 42; x + 2
84
>>> class S(str):
...     __slots__ = ()
...     def __len__(self):
...         return 123
... 
>>> 'a'.__class__ = S
>>> i = 1; len('abc'[:i])
123

[pitrou]

Isn't there already an issue open for this?

[serhiy-storchaka]

I don't remember. I noticed this once in comments on Rietveld, but don't remember the issue.

[markshannon]

If there is another issue for this, then it doesn't seem to be a release blocker. I think it should be.

[larryhastings]

As Python 3.5 Release Manager, my official statement is: Eek!

[markshannon]

Eek! indeed :)

I intend to track down the cause of this on Monday, and hopefully come up with a fix, unless anyone beats me to it.

Does anyone know if there is another issue for this?

[larryhastings]

This exciting new feature was added in checkin c0d25de5919e addressing issue bpo-22986. Perhaps the core devs who added it would like to chime in.

[njsmith]

Well, yeah, that indeed sucks.

Not sure what the best solution is. Some options:

1. "Don't do that then"

2. Explicitly add a "__class__" property to every immutable type, that unconditionally errors out on assignment.

3. Add a hack to typeobject.c checking for the important immutable types

4. Something cleverer...? A new type flag?

The immutable types are: int, float, str, tuple, bool, frozenset, complex, bytes, and... anything else?

[serhiy-storchaka]

Later I had got a crash.

>>> class S(str): __slots__ = ()
... 
>>> 'a'.__class__ = S
>>> 
>>> def f(a): pass
... 
Fatal Python error: non-string found in code slot

Current thread 0xb7583700 (most recent call first):
Aborted (core dumped)

The stdlib is full of implicit caches. Virtually any hashable object can be cached and shared. Why __class__ assignment is allowed at all? There are only two uses of __class__ assignment in the stdlib besides tests (in Lib/importlib/util.py and in Lib/xml/sax/saxutils.py), and in both cases it looks as optimization trick.

[benjaminp]

Probably the patch on that bug should be reverted.

[njsmith]

Python goes to great lengths to make __class__ assignment work in general (I was surprised too). Historically the exception has been that instances of statically allocated C extension type objects could not have their __class__ reassigned. Semantically, this is totally arbitrary, and Guido even suggested that fixing this would be a good idea in this thread:
https://mail.python.org/pipermail/python-dev/2014-December/137430.html
The actual rule that's causing problems here is that *immutable* objects should not allow __class__ reassignment. (And specifically, objects which are assumed to be immutable by the interpreter. Most semantically immutable types in Python are defined by users and their immutability falls under the "consenting adults" rule; it's not enforced by the interpreter. Also this is very different from "hashable" -- even immutability isn't a requirement for being hashable, e.g., all user-defined types are mutable and hashable by default!)

By accident this category of "immutable-by-interpreter-invariant" has tended to be a subset of "defined in C using the old class definition API", so fixing the one issue uncovered the other.

This goal that motivated this patch was getting __class__ assignment to work on modules, which are mutable and uncacheable and totally safe. With this patch it becomes possible to e.g. issue deprecation warnings on module attribute access, which lets us finally deprecate some horrible global constants in numpy that have been confusing users for a decade now:
http://mail.scipy.org/pipermail/numpy-discussion/2015-July/073205.html
https://pypi.python.org/pypi/metamodule

I'd *really* prefer not to revert this totally, given the above. (Also, if you read that python-dev message above, you'll notice that the patch also caught and fixed a different really obscure interpreter-crashing bug.)

I think the Correct solution would be to disallow __class__ assignment specifically for the classes where it violates invariants.

If this is considered to be release critical -- and I'm not entirely sure it is, given that similar tricks have long been possible and are even referenced in the official docs?
https://www.reddit.com/r/Python/comments/2441cv/can_you_change_the_value_of_1/ch3dwxt
https://docs.python.org/3/c-api/long.html#c.PyLong_FromLong
-- but if it is considered to be release critical, and it's considered too short notice to accomplish a proper fix, then a simple hack would be to add something like

if (!(oldto->tp_flags & Py_TPFLAGS_HEAPTYPE) && oldto != &PyModule_Type) {
    PyErr_Format(PyExc_TypeError, ...);
    return -1;
}

to typeobject.c:object_set_class. As compared to reverting the whole patch, this would preserve the most important case, which is one that we know is safe, and we could then progressively relax the check further in the future...

[markshannon]

Please revert c0d25de5919e.
Breaking the interpreter in order to facilitate some obscure use case is unacceptable.

If you want to do fancy stuff with modules, fine. Take a look at the source of the six module
https://bitbucket.org/gutworth/six/src/cd1e81d33eaf3d6944f859c2aa7d5d3f515013c8/six.py?at=default for some tips.

I think immutability is a fundamental property of an object. The "consenting" adults ideas is fine for accessibility. However, making previously immutable object mutable forces developers to use lots of defensive copying and causes obscure bugs (like this one).

I do not regard the immutable of numbers as an historical accident, but as vitally important for any sort of numerical reasoning.
Just take a look at a language with mutable strings to see the problems that causes.