Hi Victor,

Sorry for making this a deferred blocker. I recall that we had a brief discussion somewhere about an accidental change to the array.array type -- this is now a heap type (Py_TPFLAGS_HEAPTYPE is set), and as a consequence it is no longer immutable.

In 3.9 this is an error:

>>> import array        
>>> array.array.foo = 1
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: can't set attributes of built-in/extension type 'array.array'
>>>

But in 3.10a7 it passes:

>>> array.array.foo = 1
>>> array.array.foo    
1
>>>

I would like this type (and other types that have been or will be converted to heap types) to remain immutable. How can we do that? I think we may need a new flag bit meaning "built-in type, immutable". This bit should not be inherited of course.

What do you think? (Feel free to close if this is a duplicate -- I couldn't find where we discussed this previously, sorry.)

> I would like this type (and other types that have been or will be converted to heap types) to remain immutable.

  Hi, Guido. I am interested in what have been broken in fact? or it's a defined behavior?

> I think we may need a new flag bit meaning "built-in type, immutable". This bit should not be inherited of course.
  
  Aggreed. Adding a new `tp_flag` is a lightweight solution.

FWIW, I was the one who converted arraymodule to use heap types in 75bf107c62fbdc00af51ee4f6ab69df4bd201104 (GH-23124) in January. Sorry 'bout the accidental change of behaviour.

> I think we may need a new flag bit meaning "built-in type, immutable". This bit should not be inherited of course.

+1.

See attached PoC patch. As far as I understand, type flags must be explicitly inherited in Objects/typeobject.c, no?

Should all previous static types be immutable, or are there exceptions?

Adding an extra flag seems like the sensible thing to do for 3.10

Longer term, we should decouple immutability from whether something is a heap type.
I don't know why we would care that it is a heap type at all. Which bit of memory it happens to sit in seems irrelevant to anything but the GC.

Erland, could you turn your patch into PR?

> Erland, could you turn your patch into PR?

Certainly, but despite the issue title, this issue mentions all types that have been converted to heap types. Should we apply the new immutable flag to _all_ heap types in the stdlib?

Guido:
> I would like this type (and other types that have been or will be converted to heap types) to remain immutable.

Guido:
> Sorry for making this a deferred blocker. I recall that we had a brief discussion somewhere about an accidental change to the array.array type -- this is now a heap type (Py_TPFLAGS_HEAPTYPE is set), and as a consequence it is no longer immutable.

*Many* static types have been converted to heap types in Python 3.9 and Python 3.10. Is there a rule to decide which types should be mutable or not?

All types implemented in Python are mutable, unless the very few which use slots.

By the way, some cases can be inherited or not. Do we care about that?

Example:

$ python3
Python 3.9.2 (default, Feb 20 2021, 00:00:00) 
>>> def f(): pass
... 
>>> class MyFuncType(type(f)): pass
... 
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: type 'function' is not an acceptable base type

Let me propose a flag name: Py_TPFLAGS_IMMUTABLE.

Py_TPFLAGS_IMMUTABLE implies that setattr(type, name, value) fails. A type has a dictionary, but it cannot be modified with type.__dict__[name] = value, since type.__dict__ returns a read-only mapping proxy.


Currently, the limitation of not being able to modify a built-in type is arbitrary, it's not a technical limitation. It is implemented in type_setattro() which is the default implementation of tp_setattro: PyType_Type.tp_setattro = type_setattro.


Not having Py_TPFLAGS_HEAPTYPE flag implies "immutable". I suggest to add a new Py_TPFLAGS_IMMUTABLE flag and modify PyType_Ready() to use the flag if Py_TPFLAGS_HEAPTYPE is not set:

if (!(flags & Py_TPFLAGS_HEAPTYPE)) {
   flags |= Py_TPFLAGS_IMMUTABLE;
}


You can try with this change:

diff --git a/Objects/typeobject.c b/Objects/typeobject.c
index 254d12cc97..4bd02d40c1 100644
--- a/Objects/typeobject.c
+++ b/Objects/typeobject.c
@@ -3875,13 +3875,6 @@ static int
 type_setattro(PyTypeObject *type, PyObject *name, PyObject *value)
 {
     int res;
-    if (!(type->tp_flags & Py_TPFLAGS_HEAPTYPE)) {
-        PyErr_Format(
-            PyExc_TypeError,
-            "can't set attributes of built-in/extension type '%s'",
-            type->tp_name);
-        return -1;
-    }
     if (PyUnicode_Check(name)) {
         if (PyUnicode_CheckExact(name)) {
             if (PyUnicode_READY(name) == -1)


It becomes possible to modify built-in types:

$ ./python
Python 3.10.0a7+ (heads/master-dirty:cdad2724e6, Apr 22 2021, 14:44:44) [GCC 10.2.1 20201125 (Red Hat 10.2.1-9)] on linux
>>> str.x=1
>>> str.x
1
>>> del str.x

>>> del str.__repr__

>>> del str.__repr__
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: __repr__

Maybe name it Py_TPFLAGS_IMMUTABLETYPE? Just IMMUTABLE can mean that instances of the type are immutable, but we want to make a type itself immutable.

Is there a full list of types converted to heap types? There may be other issues related to differences between heap and static types.

1. Static type with tp_new = NULL does not have public constructor, but heap type inherits constructor from base class. As result, it allows to create instances without proper initialization, that can lead to crash. It was fixed for few standard heap types in issue23815, then reintroduced, then fixed again in issue42694. But it should be checked for every type without constructor.

2. Static types was not pickleable with protocol 0 and 1 by default. Heap types were pickleable by default, as result, you did not get error when pickle instances of newly converted types, but get an error when try to unpickle. It was fixed in general enough case in issue41052, but it is worth to recheck it for every converted type.

> Maybe name it Py_TPFLAGS_IMMUTABLETYPE?

"TPFLAGS" stands for "type flags". So IMO adding "TYPE" suffix is redundant. If tomorrow, we want to add a flag for immutable instance, we can add a new Py_TPFLAGS_IMMUTABLE_INSTANCE flag.

On the other side, TYPE suffix is consistent with other flags :-)

/* Set if the type object is dynamically allocated */
#define Py_TPFLAGS_HEAPTYPE (1UL << 9)

/* Set if the type allows subclassing */
#define Py_TPFLAGS_BASETYPE (1UL << 10)

Victor, I've updated PR 25520 so PyType_Ready always sets Py_TPFLAGS_IMMUTABLE for static types.

Is there going to be a separate PR to actually *set* the IMMUTABLE flag on all built-in type objects? There are many of those.

Guido, I’ll prepare a separate PR for all converted built-in types, and you can decide what to do with it when consensus is reached :) There are approx. ~90 of these types. Not all of them are added to their respective module dicts though, but I would add the flag nonetheless, as it simplifies creating and reviewing the patch, and it does not have any performance impact.

BTW, slightly related, AFAICT:
- https://github.com/python/cpython/blob/e047239eafefe8b19725efffe7756443495cf78b/Objects/typeobject.c#L4620-L4668
- bpo-24912

I've prepared a diff using grep & sed (see attached patch). Let me know if you want it converted to a PR.

I've excluded the test and the _xx extension modules.

Does the test suite pass for apply-to-all.diff?

Also, quite a hornet's nest you've uncovered (about __class__ assignment). Would that be fixed better with the IMMUTABLE flag?

> Does the test suite pass for apply-to-all.diff?

No, multiple tests fail. First test_distutils fails, then during the re-run, test_multiprocessing_forkserver, test_multiprocessing_spawn, and test_pdb fail.

> Also, quite a hornet's nest you've uncovered (about __class__ assignment).

Yes, indeed. Apropos, I see that bpo-24991 is still open.

> Would that be fixed better with the IMMUTABLE flag?

That would be a change of functionality, given that we apply the immutable flag to all built-in types. Currently __class__ assignment is allowed for heap types, and it has been so for many years.

I feel reluctant to add apply-to-all.diff to the PR right now.

The more I read about these issues, the more I feel reluctant to apply PR 25520. Why change the behaviour/traits of heap types without a proper discussion (or PEP)? Applying this PR feels like an arbitrary change. All the related issues and mailing list discussions seems to end without consensus, and fundamental questions (what is an immutable type? which types should be immutable? why is it a problem that a type is mutable?) seems to be avoided all the time.

And, as Victor points out in msg391596 (and which has also been pointed out by others in other discussions):
> Currently, the limitation of not being able to modify a built-in type is arbitrary, it's not a technical limitation

FWIW #1, in pypy3, datetime.timedelta.foo = 1 is ok; in python3 (3.7-3.10) it's not.

FWIW #2, in Python 3.9, functools.partialmethod is implemented in Python, and functools.partial is implemented in C as a static type:
$ python3.9
>>> import functools
>>> functools.partial.foo = 1  # is this a problem?
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: can't set attributes of built-in/extension type 'functools.partial'
>>> functools.partialmethod.foo = 1  # is this a problem?


If I'm only bringing noise into the discussion, feel free to remove me from the nosy list.

My main concern is that types that were immutable in previous Python versions shouldn't become mutable as a surprise side effect of making them heap types. I don't know which types have become mutable this way though.

My other concern is that the language design *intentionally* disallowed mutating built-in types (unlike some other languages that allow it, e.g. Ruby). Mutating built-in types is one of those "attractive nuisance" anti-patterns where at a small scale this often appears to be the quickest way to solve a problem, but it tends to break unrelated things in larger-scale  applications (when different libraries using such tricks collide).

(There's also a concern about mutating types that are shared between multiple interpreters, but IIUC heap types are not shared, so this shouldn't be a problem.)

Presumably we should review the list of heap types that you are proposing to make immutable (in a form more easily digestible by humans than a diff) and reach agreement on that. And perhaps the list should include information on when a type became a heap type. (Types that were always heap types probably needn't be changed.)

Thanks for the background info, Guido! I'll prepare a list. Should we continue the discussion on Discourse or python-dev?

Let's post the list here and post a brief mention of this issue on
python-dev.

> Let's post the list here and post a brief mention of this issue on
python-dev.

Attached. The list contains commits, commit dates, affected files, and the name of the PyType_Spec variables (I _can_ resolve these to the actual type names, but I figured this would be ok for now).

I've also posted the list on Discsourse bco. readability. I'll post a notification on python-dev as well.

https://discuss.python.org/t/list-of-built-in-types-converted-to-heap-types/8403

I also opened https://bugs.python.org/issue43916 to track some points raised by Serhiy

> No, multiple tests fail. First test_distutils fails, then during the re-run, test_multiprocessing_forkserver, test_multiprocessing_spawn, and test_pdb fail.

Can you provide the error output log you're getting? I'm getting a lot of errors for a lot of tests by applying your patch. But the test_pdb error looks related. If test_pdb fails then all the tests after that also fail. I'm not sure if they are related at all.

> Can you provide the error output log you're getting?

I don't think we should use more time on apply-to-all.diff, since it's probably not going to be applied as it is anyway.

I'm elevating this as a release blocker since this is probably going to involve a moderately big change and we don't want people relying on the new behaviour when we don't intend to.

I think it is safer to merge apply-to-all.diff, and revert the changes for individual types if we have reasons to make them mutable.

Correspondingly, for all converted heap types we should set tp_new = NULL unless they have special tp_new implementation.

Could you please land the new flag and typeobject.c code now? I'm happy to take care of "my" code myself.

> Could you please land the new flag and typeobject.c code now?

I've only got two more questions before I mark it ready for review, and that's regarding Guido's comment in msg391756:
Should the new flag also take care of __class__ assignment, or should we leave that out?
Are there other places where !Py_TPFLAGS_HEAPTYPE is used where we would like to use the immutable flag?

> I'm happy to take care of "my" code myself.

No problem, I'll leave it out of the patch :)

> I think it is safer to merge apply-to-all.diff, and revert the changes for individual types if we have reasons to make them mutable.

I don't think it's a good idea though. About 10 or 8 tests are failing if apply-to-all.diff is applied. The tests that are not related to it are also failing because of it. After test_pdb fails a segfault error occurs and the remaining tests are never executed. If we want to merge apply-to-all.diff then we need to make some modifications first.

These types were immutable before. If merely making them immutable again makes tests failing then perhaps there is something wrong with these tests or there are other bugs which are needed to be fixed.

> If merely making them immutable again makes tests failing then perhaps there is something wrong with these tests or there are other bugs which are needed to be fixed.

I've noticed that the test_pdb fails because it expected the a class method to have the __repr__ "some func" but it got "bound method some func at 0x0000000". This may be the affect of this apply-to-all.diff. Other tests failing were actually fixed in other issues but somehow apply-to-all.diff is suddenly triggering them I think. The most fatal test failures is the test_pdb and test_logging because after those tests failed there were some unexpected behaviors like other tests were executed randomly, segfault and most prominent is that they started compiling by themselves! These are the two tests we should really look into right now.

> > Could you please land the new flag and typeobject.c code now?
>
> I've only got two more questions before I mark it ready for review,
> and that's regarding Guido's comment in msg391756:
> Should the new flag also take care of __class__ assignment,
> or should we leave that out?

Let's leave that out.
It's a separate can of worms and someone else can  look into it later.

> Are there other places where !Py_TPFLAGS_HEAPTYPE is used
> where we would like to use the immutable flag?

Again, I think we can do that later.

So I am in favor of landing your (small) patch that introduces the flag now.

I also think you should try to separately land small patches that add the IMMUTABLETYPE flag to a few very public types, e.g. array.array and the three in _sre (which are exported by re.py).

We can then look into the test failures with a little less time pressure.

No problem, will do!

PS. Don't wait for me. I'm going on vacation this Thursday and won't be
back until well after the 3.10b1 release is done, and before I go I'm
pretty busy already.

@serhiy.storchaka I have no idea what's causing the errors I described in https://bugs.python.org/msg391947. I looked at all the changes in apply-to-all.diff and I don't think any of the errors are related. Do you have any thoughts about this problem? Is it even reproducible?

(I'm attaching a diff combining the patch.diff and apply-to-all.diff together in one file.)

The solution would be bisection -- apply half of the changes from
apply-to-all, and see if you still get the errors. Etc.

> The solution would be bisection -- apply half of the changes from
apply-to-all, and see if you still get the errors. Etc.

Getting the same error. These are the errors :-

0:12:14 load avg: 0.92 [115/426] test_distutils
test test_distutils crashed -- Traceback (most recent call last):
  File "C:\github\cpython\lib\test\libregrtest\runtest.py", line 282, in _runtest_inner
    refleak = _runtest_inner2(ns, test_name)
  File "C:\github\cpython\lib\test\libregrtest\runtest.py", line 229, in _runtest_inner2
    the_module = importlib.import_module(abstest)
  File "C:\github\cpython\lib\importlib\__init__.py", line 126, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1050, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1027, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1006, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 688, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 881, in exec_module
  File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed
  File "C:\github\cpython\lib\test\test_distutils.py", line 15, in <module>
    import distutils.tests
  File "C:\github\cpython\lib\contextlib.py", line 140, in __exit__
    next(self.gen)
  File "C:\github\cpython\lib\test\support\warnings_helper.py", line 179, in _filterwarnings
    raise AssertionError("filter (%r, %s) did not catch any warning" %
AssertionError: filter ('The distutils package is deprecated', DeprecationWarning) did not catch any warning



0:19:05 load avg: 0.07 [212/426/1] test_logging
--- Logging error ---
Traceback (most recent call last):
  File "C:\github\cpython\lib\logging\handlers.py", line 74, in emit
    self.doRollover()
  File "C:\github\cpython\lib\logging\handlers.py", line 179, in doRollover
    self.rotate(self.baseFilename, dfn)
  File "C:\github\cpython\lib\logging\handlers.py", line 117, in rotate
    self.rotator(source, dest)
  File "C:\github\cpython\lib\test\test_logging.py", line 5222, in rotator
    os.rename(source, dest + ".rotated")
FileExistsError: [WinError 183] Cannot create a file when that file already exists: 'C:\\Users\\shrey\\AppData\\Local\\Temp\\test_logging-2-lh2g0lpt.log' -> 'C:\\Users\\shrey\\AppData\\Local\\Temp\\test_logging-2-lh2g0lpt.log.1.test.rotated'
Call stack:
  File "C:\github\cpython\lib\runpy.py", line 196, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "C:\github\cpython\lib\runpy.py", line 86, in _run_code
    exec(code, run_globals)
  File "C:\github\cpython\lib\test\__main__.py", line 2, in <module>
    main()
  File "C:\github\cpython\lib\test\libregrtest\main.py", line 719, in main
    Regrtest().main(tests=tests, **kwargs)
  File "C:\github\cpython\lib\test\libregrtest\main.py", line 641, in main
    self._main(tests, kwargs)
  File "C:\github\cpython\lib\test\libregrtest\main.py", line 694, in _main
    self.run_tests()
  File "C:\github\cpython\lib\test\libregrtest\main.py", line 521, in run_tests
    self.run_tests_sequential()
  File "C:\github\cpython\lib\test\libregrtest\main.py", line 423, in run_tests_sequential
    result = runtest(self.ns, test_name)
  File "C:\github\cpython\lib\test\libregrtest\runtest.py", line 194, in runtest
    return _runtest(ns, test_name)
  File "C:\github\cpython\lib\test\libregrtest\runtest.py", line 154, in _runtest
    result = _runtest_inner(ns, test_name,
  File "C:\github\cpython\lib\test\libregrtest\runtest.py", line 282, in _runtest_inner
    refleak = _runtest_inner2(ns, test_name)
  File "C:\github\cpython\lib\test\libregrtest\runtest.py", line 246, in _runtest_inner2
    test_runner()
  File "C:\github\cpython\lib\test\support\__init__.py", line 682, in inner
    return func(*args, **kwds)
  File "C:\github\cpython\lib\test\test_logging.py", line 5503, in test_main
    support.run_unittest(*tests)
  File "C:\github\cpython\lib\test\support\__init__.py", line 1082, in run_unittest
    _run_suite(suite)
  File "C:\github\cpython\lib\test\support\__init__.py", line 959, in _run_suite
    result = runner.run(suite)
  File "C:\github\cpython\lib\test\support\testresult.py", line 169, in run
    test(self.result)
  File "C:\github\cpython\lib\unittest\suite.py", line 84, in __call__
    return self.run(*args, **kwds)
  File "C:\github\cpython\lib\unittest\suite.py", line 122, in run
    test(result)
  File "C:\github\cpython\lib\unittest\suite.py", line 84, in __call__
    return self.run(*args, **kwds)
  File "C:\github\cpython\lib\unittest\suite.py", line 122, in run
    test(result)
  File "C:\github\cpython\lib\unittest\case.py", line 652, in __call__
    return self.run(*args, **kwds)
  File "C:\github\cpython\lib\unittest\case.py", line 592, in run
    self._callTestMethod(testMethod)
  File "C:\github\cpython\lib\unittest\case.py", line 549, in _callTestMethod
    method()
  File "C:\github\cpython\lib\test\test_logging.py", line 5229, in test_namer_rotator_inheritance
    rh.emit(self.next_rec())


0:26:02 load avg: 0.46 [250/426/1] test_pdb
test test_pdb failed -- Traceback (most recent call last):
  File "C:\github\cpython\lib\doctest.py", line 2205, in runTest
    raise self.failureException(self.format_failure(new.getvalue()))
AssertionError: Failed doctest test for test.test_pdb.test_pdb_issue_20766
  File "C:\github\cpython\lib\test\test_pdb.py", line 1270, in test_pdb_issue_20766

----------------------------------------------------------------------
File "C:\github\cpython\lib\test\test_pdb.py", line 1282, in test.test_pdb.test_pdb_issue_20766
Failed example:
    with PdbTestInput(['continue',
                       'continue']):
        test_function()
Expected:
    > <doctest test.test_pdb.test_pdb_issue_20766[0]>(6)test_function()
    -> print('pdb %d: %s' % (i, sess._previous_sigint_handler))
    (Pdb) continue
    pdb 1: <built-in function default_int_handler>
    > <doctest test.test_pdb.test_pdb_issue_20766[0]>(5)test_function()
    -> sess.set_trace(sys._getframe())
    (Pdb) continue
    pdb 2: <built-in function default_int_handler>
Got:
    > <doctest test.test_pdb.test_pdb_issue_20766[0]>(6)test_function()
    -> print('pdb %d: %s' % (i, sess._previous_sigint_handler))
    (Pdb) continue
    pdb 1: <bound method Pdb.sigint_handler of <pdb.Pdb object at 0x0000025D07BD5950>>
    > <doctest test.test_pdb.test_pdb_issue_20766[0]>(5)test_function()
    -> sess.set_trace(sys._getframe())
    (Pdb) continue
    pdb 2: <bound method Pdb.sigint_handler of <pdb.Pdb object at 0x0000025D07BD5950>>

So keep cutting in half until the error disappears?

I ran the test suite without the changes and they still fail. Therefore apply-to-all.diff does not have any bug at all. But the test failures are also a major issue not only for this change but also for others. I'm opening a new issue for those test failures.

(apply-to-all.diff is outdated because Erlend changed the flag name from Py_TPFLAGS_IMMUTABLE to Py_TPFLAGS_IMMUTABLETYPE in the PR. I'm attaching the modified apply-to-all.diff)

Re-running the failed tests doesn't trigger the errors but running the whole test suite does. And some errors I described are fixed in any of the last 22 commits and PRs. Therefore should we actually look into this matter or should we leave it as it is?

Pablo, there seems to be some instability in the tests.

Shreyan, Erlend, maybe merging in master would help?

I've not seen any failing tests for GH-25653 on the CI or on my computer.

The PR is based off of 1b1f9852bda85c01ef124858f293e9c13e04ffce, which is pretty recent. I can rebase onto master, but I don't see any need for it as long as the tests suite passes and there's no conflicts.

Sure. What do you think is causing the failures for Shreyan then? (Are you
two collaborating?)
-- 
--Guido (mobile)

> Pablo, there seems to be some instability in the tests.

Apart from the typical test_asyncio random timeouts I don't see anything on the buildbots or the CI. What are those failing test? Maybe I can try to reproduce on my side

Let me re-run the test suite again and see if the tests have been fixed in the most recent commits.

Shreyan, are you running the tests against the PR, or something else? Have you tried make clean (or git clean -fdx)?

> Are you two collaborating?

No, but any help is of course appreciated.

> Shreyan, are you running the tests against the PR, or something else? Have you tried make clean (or git clean -fdx)?

I always have a clean branch checked out but the last time I ran the tests it was unsuccessful. I'm re-running the tests now and hoping that the tests will be successful this time. However lot of tests have been fixed.

> I've not seen any failing tests for GH-25653 on the CI or on my computer.

That would be GH-25520. GH-25653 is the slightly related issue/PR. Both are passing CI, though.

Re-running tests complete. I'm attaching the results. If the errors are not reproducible then please tell. I'll then rebuild and rerun the tests once more.

I don't think I've ever successfully managed to complete the test on my
Windows 10 box either (not even in "production" mode -- you seem to be
using debug mode). I recommend using creating a new PR and having the CI
machinery run the tests. It's more reliable that way.

There are no errors in apply-to-all.diff because I've ran the tests both with and without the changes and both give me the same output. It's safe to proceed with apply-to-all.diff. The only problem is why are the tests failing at all? I ran the tests a few days earlier on this machine only (Windows 10) and it was successful then.

Since, these changes and the test failures are not related to each other at all, I'm opening a new issue addressing the test failures only. I'm adding everyone in this issue's nosy to the new issue's nosy as well.

See issue https://bugs.python.org/issue43955 for the test failures.

Was EVPtype_spec in _hashopenssl.c converted to heap type?

See msg391933, Shreyan. I think Christian will take care of his types :)

New changeset 3b52c8d66b25415f09478ab43f93d59a3547dc13 by Erlend Egeberg Aasland in branch 'master':
bpo-43908: Add Py_TPFLAGS_IMMUTABLETYPE flag (GH-25520)
https://github.com/python/cpython/commit/3b52c8d66b25415f09478ab43f93d59a3547dc13

Is it necessary to add tests just for testing type immutability? We don't have immutability tests for static types then why heap types?

> We don't have immutability tests for static types then why heap types?

Because all static types are immutable, and some have been converted to heap types changing the contract, therefore is a regression and therefore we need regression tests for those.

Erlend, wouldn't it be easy to apply all the changes in 1 PR?

Shreyan, see msg391954 earlier in this thread:
"I also think you should try to separately land small patches that add the IMMUTABLETYPE flag to a few very public types, e.g. array.array and the three in _sre (which are exported by re.py)."

ok.

I am not sure tests are needed here. Immutability of the type is mainly an implementation detail. We keep the types immutable because we don't want users to use the feature of mutating these types. I don't think anything bad will happen if in some Python implementation some of these types are Python classes and are mutable. It is not easy to make a Python class immutable. Users will not intentionally write a code depending on mutability of these types because it does not work in CPython.

> Immutability of the type is mainly an implementation detail

I kindly disagree and based on the title of this issue I think more people have a similar opinion.

In any case, that's my personal opinion of course.

What we should do is decide collectively if immutability is defined behaviour. If we decide that it is, we need regression tests. If we define that as "nice to have" but ultimately "implementation-defined" we don't.

In any case, I would argue that having tests is good and allow us to know when the behaviour flips without people intending it to, like what happened in this case.

New changeset 5daf70b22e72ea1a88c05975f69120b8c4e4927f by Erlend Egeberg Aasland in branch 'master':
bpo-43908: Make re types immutable (GH-25697)
https://github.com/python/cpython/commit/5daf70b22e72ea1a88c05975f69120b8c4e4927f

New changeset c6ad03fddf4b04c60dca4327140e59fb2dcca8e5 by Erlend Egeberg Aasland in branch 'master':
bpo-43908: Make array.array type immutable (GH-25696)
https://github.com/python/cpython/commit/c6ad03fddf4b04c60dca4327140e59fb2dcca8e5

The types converted to heap types in 3.10 are different from 3.9 therefore just backporting wouldn't help. Should we now try to apply the flags on other types? If yes then I'd like to volunteer to apply the flags on the types converted in 3.9.

> BTW, slightly related, AFAICT:
> - https://github.com/python/cpython/blob/e047239eafefe8b19725efffe7756443495cf78b/Objects/typeobject.c#L4620-L4668
> - bpo-24912

object_set_class() must be changed to check for Py_TPFLAGS_IMMUTABLETYPE flag, rather than Py_TPFLAGS_HEAPTYPE.

Erlend: Do you want to write such change?

> Erlend: Do you want to write such change?

Sure, I'll have a go at it. Thanks.

New changeset 5bd0619533ed6587ce76402e9b0b118eb4d4dd09 by Victor Stinner in branch 'master':
bpo-43908: Document Static Types in the C API (GH-25710)
https://github.com/python/cpython/commit/5bd0619533ed6587ce76402e9b0b118eb4d4dd09

check_set_special_type_attr() must also be updated to check for Py_TPFLAGS_IMMUTABLETYPE flag.

> Erlend: Do you want to write such change?

Erlend created bpo-43973 and PR 25714.

> check_set_special_type_attr() must also be updated to check for Py_TPFLAGS_IMMUTABLETYPE flag.

Ok, lets just use this issue for the PR. I can fix it in an hour or so.

> https://discuss.python.org/t/list-of-built-in-types-converted-to-heap-types/8403

In the past, I used _random.Random for manual tests to compare static type and heap types, check which one is mutable.

C type _random.Random is inherited by Python type random.Random which is mutable. Since _random.Random is not directly seen by developers, I don't think that it's worth it to make it immutable.

For the other types, I would not say that they are "built-in types" or that it would be really bad to modify them. I would say that for the other types, the "We are consenting adults" rule stands. You can hack a type for a very specific need, but in this case you are on your own.

For example, people love to hack AST. Maybe the fact that ast.AST became mutable in Python 3.9 will unlock some crazy hack?

check_set_special_type_attr() is used to prevent setting the following attributes:

* __name__
* __qualname__
* __module__
* __bases__
* __doc__

Right now, I cannot set the attribues on array.array type:

$ ./python
Python 3.10.0a7+ (heads/debug_doc2-dirty:0623fdb60d, Apr 29 2021, 12:06:18)
>>> import array

>>> array.array.__name__ = 'newname'
TypeError: can't set attributes of built-in/extension type 'array.array'

>>> array.array.__qualname__ = 'newname'
TypeError: can't set attributes of built-in/extension type 'array.array'

>>> array.array.__module__ = 'new module'
TypeError: can't set attributes of built-in/extension type 'array.array'

>>> array.array.__bases__ = (int,)
TypeError: can't set attributes of built-in/extension type 'array.array'

>>> array.array.__doc__ = 'doc'
TypeError: can't set attributes of built-in/extension type 'array.array'

I guess that type_setattro() is used and it checks for Py_TPFLAGS_IMMUTABLETYPE flag early.

That is as intended, right?

On Thu, Apr 29, 2021 at 07:59 STINNER Victor <report@bugs.python.org> wrote:

>
> STINNER Victor <vstinner@python.org> added the comment:
>
> check_set_special_type_attr() is used to prevent setting the following
> attributes:
>
> * __name__
> * __qualname__
> * __module__
> * __bases__
> * __doc__
>
> Right now, I cannot set the attribues on array.array type:
>
> $ ./python
> Python 3.10.0a7+ (heads/debug_doc2-dirty:0623fdb60d, Apr 29 2021, 12:06:18)
> >>> import array
>
> >>> array.array.__name__ = 'newname'
> TypeError: can't set attributes of built-in/extension type 'array.array'
>
> >>> array.array.__qualname__ = 'newname'
> TypeError: can't set attributes of built-in/extension type 'array.array'
>
> >>> array.array.__module__ = 'new module'
> TypeError: can't set attributes of built-in/extension type 'array.array'
>
> >>> array.array.__bases__ = (int,)
> TypeError: can't set attributes of built-in/extension type 'array.array'
>
> >>> array.array.__doc__ = 'doc'
> TypeError: can't set attributes of built-in/extension type 'array.array'
>
> I guess that type_setattro() is used and it checks for
> Py_TPFLAGS_IMMUTABLETYPE flag early.
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <https://bugs.python.org/issue43908>
> _______________________________________
>
-- 
--Guido (mobile)

I don’t think we’re waiting for more crazy hacks.

On Thu, Apr 29, 2021 at 07:27 STINNER Victor <report@bugs.python.org> wrote:

>
> STINNER Victor <vstinner@python.org> added the comment:
>
> >
> https://discuss.python.org/t/list-of-built-in-types-converted-to-heap-types/8403
>
> In the past, I used _random.Random for manual tests to compare static type
> and heap types, check which one is mutable.
>
> C type _random.Random is inherited by Python type random.Random which is
> mutable. Since _random.Random is not directly seen by developers, I don't
> think that it's worth it to make it immutable.
>
> For the other types, I would not say that they are "built-in types" or
> that it would be really bad to modify them. I would say that for the other
> types, the "We are consenting adults" rule stands. You can hack a type for
> a very specific need, but in this case you are on your own.
>
> For example, people love to hack AST. Maybe the fact that ast.AST became
> mutable in Python 3.9 will unlock some crazy hack?
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <https://bugs.python.org/issue43908>
> _______________________________________
>
-- 
--Guido (mobile)

Py_TPFLAGS_IMMUTABLETYPE has been applied to array.array by Erlend in https://github.com/python/cpython/pull/25696. Should Py_TPFLAGS_IMMUTABLETYPE be applied to other heap types also (the ones that were converted from static types)?

Victor:
> check_set_special_type_attr() is used to prevent setting the following attributes:
> [...]
> Right now, I cannot set the attribues on array.array type:
> [...]
> I guess that type_setattro() is used and it checks for Py_TPFLAGS_IMMUTABLETYPE flag early.

Is this always the case? If so, can we turn the check in check_set_special_type_attr() into an assert? In any case, Py_TPFLAGS_IMMUTABLETYPE should be used, not !Py_TPFLAGS_HEAPTYPE.

> Is this always the case? If so, can we turn the check in check_set_special_type_attr() into an assert? In any case, Py_TPFLAGS_IMMUTABLETYPE should be used, not !Py_TPFLAGS_HEAPTYPE.

I don't know. In case of doubt, I suggest to only replace !Py_TPFLAGS_HEAPTYPE with Py_TPFLAGS_IMMUTABLETYPE.

Yep, I agree. From my coverage output, it seems that this branch is never executed. BUT, that may of course be because there's no test that exercises this branch.

Hum, another function should be updated. Do you want to propose a fix Erlend?

static int
type_set_annotations(PyTypeObject *type, PyObject *value, void *context)
{
    if (!(type->tp_flags & Py_TPFLAGS_HEAPTYPE)) {
        PyErr_Format(PyExc_TypeError, "can't set attributes of built-in/extension type '%s'", type->tp_name);
        return -1;
    }

I just found it myself :) pushing out the PR now!

Included in GH-25743

See also bpo-24912 "The type of cached objects is mutable".

PyStdPrinter_Type implements tp_new, but tp_init always fail:

static int
stdprinter_init(PyObject *self, PyObject *args, PyObject *kwds)
{
    PyErr_SetString(PyExc_TypeError,
                    "cannot create 'stderrprinter' instances");
    return -1;
}

Maybe it should use the Py_TPFLAGS_IMMUTABLETYPE flag instead?

Instances must be created with PyFile_NewStdPrinter() which doesn't use tp_new nor tp_init.

Erlend: do you want to propose a PR?

https://docs.python.org/dev/c-api/typeobj.html#Py_TPFLAGS_IMMUTABLETYPE says:
"This bit is set for type objects that are immutable: type attributes cannot be set nor deleted."

Is it possible that a metatype (type subtype) overrides tp_setattro and ignores the Py_TPFLAGS_IMMUTABLETYPE flag? Should we suggest that metatypes implemented in C and overridding tp_setattro should take the Py_TPFLAGS_IMMUTABLETYPE flag in account?

When a type overrides tp_setattro, I understand that it's fine since it's only used to set attributes of its instances, not on the type itself.

--

See also bpo-43770 "Rework C types initialization": some types explicitly sets explicitly tp_setattro to PyObject_GenericSetAttr. But it's unrelated since it's used to set attributes of type instances.

Erlend: would you mind to document the new Py_TPFLAGS_IMMUTABLETYPE flag in the C API section of What's New in Python 3.10?
https://docs.python.org/dev/whatsnew/3.10.html#c-api-changes

In "New Features", something like:

* Add a new :c:data:`Py_TPFLAGS_IMMUTABLETYPE` type flag to (...).
  (Contributed by xxx in :issue:`43908`.)

And in the "Porting to Python 3.10", I don't know if we should suggest reviewing metatypes implemented in C which overrides tp_setattro, to take the new flag in account.

Maybe explain that Py_TPFLAGS_IMMUTABLETYPE should now be checked to decide if a type is mutable or not, rather than relying on Py_TPFLAGS_HEAPTYPE.

Yes, I’ll have a look at it this afternoon. (On mobile now.)

New changeset 64141382ecbad665d5738ff26d15505f3427c724 by Erlend Egeberg Aasland in branch 'master':
bpo-43908: check_set_special_type_attr() checks Py_TPFLAGS_IMMUTABLETYPE (GH-25743)
https://github.com/python/cpython/commit/64141382ecbad665d5738ff26d15505f3427c724

Currently, 4 types are declared with Py_TPFLAGS_IMMUTABLETYPE explicitly:

* array.array
* re.Pattern
* re.Match
* _sre.SRE_Scanner

Moreover, all static types get the Py_TPFLAGS_IMMUTABLETYPE flag automatically. For example, "./python -c pass" initializes 196 immutable static types:

* ArithmeticError
* AssertionError
* AttributeError
* BaseException
* BlockingIOError
* BrokenPipeError
* BufferError
* BytesWarning
* ChildProcessError
* ConnectionAbortedError
* ConnectionError
* ConnectionRefusedError
* ConnectionResetError
* DeprecationWarning
* EOFError
* EncodingMap
* EncodingWarning
* Exception
* FileExistsError
* FileNotFoundError
* FloatingPointError
* FutureWarning
* GeneratorExit
* ImportError
* ImportWarning
* IndentationError
* IndexError
* InterpreterID
* InterruptedError
* IsADirectoryError
* KeyError
* KeyboardInterrupt
* LookupError
* MemoryError
* ModuleNotFoundError
* NameError
* NoneType
* NotADirectoryError
* NotImplementedError
* NotImplementedType
* OSError
* OverflowError
* PendingDeprecationWarning
* PermissionError
* ProcessLookupError
* PyCapsule
* RecursionError
* ReferenceError
* ResourceWarning
* RuntimeError
* RuntimeWarning
* StopAsyncIteration
* StopIteration
* SyntaxError
* SyntaxWarning
* SystemError
* SystemExit
* TabError
* TimeoutError
* Token.MISSING
* TypeError
* UnboundLocalError
* UnicodeDecodeError
* UnicodeEncodeError
* UnicodeError
* UnicodeTranslateError
* UnicodeWarning
* UnraisableHookArgs
* UserWarning
* ValueError
* Warning
* ZeroDivisionError
* _contextvars.Context
* _contextvars.ContextVar
* _contextvars.Token
* _io.BufferedRWPair
* _io.BufferedRandom
* _io.BufferedReader
* _io.BufferedWriter
* _io.BytesIO
* _io.FileIO
* _io.IncrementalNewlineDecoder
* _io.StringIO
* _io.TextIOWrapper
* _io._BufferedIOBase
* _io._BytesIOBuffer
* _io._IOBase
* _io._RawIOBase
* _io._TextIOBase
* _thread._ExceptHookArgs
* anext_awaitable
* async_generator
* async_generator_asend
* async_generator_athrow
* async_generator_wrapped_value
* asyncgen_hooks
* bool
* builtin_function_or_method
* builtin_method
* bytearray
* bytearray_iterator
* bytes
* bytes_iterator
* callable_iterator
* cell
* classmethod
* classmethod_descriptor
* code
* collections.OrderedDict
* complex
* coroutine
* coroutine_wrapper
* dict
* dict_itemiterator
* dict_items
* dict_keyiterator
* dict_keys
* dict_reverseitemiterator
* dict_reversekeyiterator
* dict_reversevalueiterator
* dict_valueiterator
* dict_values
* ellipsis
* enumerate
* fieldnameiterator
* filter
* float
* formatteriterator
* frame
* frozenset
* function
* generator
* getset_descriptor
* hamt
* hamt_array_node
* hamt_bitmap_node
* hamt_collision_node
* instancemethod
* int
* items
* iterator
* keys
* list
* list_iterator
* list_reverseiterator
* longrange_iterator
* managedbuffer
* map
* mappingproxy
* member_descriptor
* memoryview
* method
* method-wrapper
* method_descriptor
* module
* moduledef
* object
* odict_items
* odict_iterator
* odict_keys
* odict_values
* pickle.PickleBuffer
* property
* range
* range_iterator
* reversed
* set
* set_iterator
* signal.struct_siginfo
* slice
* staticmethod
* stderrprinter
* str
* str_iterator
* super
* symtable entry
* sys.flags
* sys.float_info
* sys.hash_info
* sys.int_info
* sys.thread_info
* sys.version_info
* time.struct_time
* traceback
* tuple
* tuple_iterator
* type
* types.GenericAlias
* types.SimpleNamespace
* types.Union
* values
* weakcallableproxy
* weakproxy
* weakref
* wrapper_descriptor
* zip

Victor, the types, that Py_TPFLAGS_IMMUTABLETYPE flag is yet to be applied to, are different for 3.9 and 3.10. Should we proceed with that? (If yes I'd be happy to collaborate with Erlend to apply the flag on those types.)

> Victor, the types, that Py_TPFLAGS_IMMUTABLETYPE flag is yet to be applied to, are different for 3.9 and 3.10. Should we proceed with that? (If yes I'd be happy to collaborate with Erlend to apply the flag on those types.)

Please don't change the behavior in the 3.9 stable branch.

Ok

Ok

I am decreasing the priority of this to deferred blocker since https://bugs.python.org/issue43916 is mostly fixed

What's the plan for heap type exceptions? PyErr_NewException() and PyErr_NewExceptionWithDoc() doesn't accept flags.

New changeset 91554e4c5ca3c762998296522f854a7166ba84f0 by Christian Heimes in branch 'master':
bpo-43908: Mark ssl, hash, and hmac types as immutable (GH-25792)
https://github.com/python/cpython/commit/91554e4c5ca3c762998296522f854a7166ba84f0

Should I apply the flags on other newly converted heap types?

Victor:
> And in the "Porting to Python 3.10", I don't know if we should suggest reviewing metatypes implemented in C which overrides tp_setattro, to take the new flag in account.

I don't know either; perhaps someone else could chime in here. I've added a draft of a minimal What's New (PR comin' up in a few seconds).

New changeset 3222b25b2f55d3b3d1dab4547bf7b5adaa1d874f by Miss Islington (bot) in branch '3.10':
[3.10] bpo-43908: Add What's New entry for Py_TPFLAGS_IMMUTABLETYPE flag (GH-25816) (GH-26115)
https://github.com/python/cpython/commit/3222b25b2f55d3b3d1dab4547bf7b5adaa1d874f

main branch:

commit a09fc9c63f1b5980c62ff2712f67500bacb92b04
Author: Erlend Egeberg Aasland <erlend.aasland@innova.no>
Date:   Fri May 14 00:44:55 2021 +0200

    bpo-43908: Add What's New entry for Py_TPFLAGS_IMMUTABLETYPE flag (GH-25816)

Guido: "I don’t think we’re waiting for more crazy hacks."

Right now, _ast.AST is still mutable:

$ ./python
Python 3.11.0a0 (heads/subtype_dealloc-dirty:efd45ad788, May 21 2021
>>> import _ast
>>> _ast.AST.x=1
>>> _ast.AST.x
1

Notice this issue is marked as "deferred blocker" it won't block this beta release (beta 2) but it will block the next.

PR 26351 adds the Py_TPFLAGS_IMMUTABLETYPE type flag to all types converted to heap type during 3.10 development.

New changeset 00710e6346fd2394aa020b2dfae170093effac98 by Erlend Egeberg Aasland in branch 'main':
bpo-43908: Make heap types converted during 3.10 alpha immutable (GH-26351)
https://github.com/python/cpython/commit/00710e6346fd2394aa020b2dfae170093effac98

New changeset 7297d74251de3b1c02dcdb9ca281461cc7fb4535 by Miss Islington (bot) in branch '3.10':
bpo-43908: Make heap types converted during 3.10 alpha immutable (GH-26351) (GH-26766)
https://github.com/python/cpython/commit/7297d74251de3b1c02dcdb9ca281461cc7fb4535

In inherit_slots() in Objects/typeobject.c, Py_TPFLAGS_HAVE_VECTORCALL inheritance depends on if the base type is a heap type or not. This aligns with PEP 590[1]:

  Heap types never inherit the vectorcall protocol because that
  would not be safe (heap types can be changed dynamically).

AFAICS, inherit_slots() should now use Py_TPFLAGS_IMMUTABLETYPE to decide if Py_TPFLAGS_HAVE_VECTORCALL can be inherited, and the PEP should be updated.


- [1] https://www.python.org/dev/peps/pep-0590/#subclassing

+1--
--Guido (mobile)

New changeset a3739b207adb5d17e3b53347df05b54b1a8b87f0 by Erlend Egeberg Aasland in branch 'main':
bpo-43908: Immutable types inherit vectorcall (GH-27001)
https://github.com/python/cpython/commit/a3739b207adb5d17e3b53347df05b54b1a8b87f0

Is anything left here?

> Is anything left here?

Should the immutable flag also be applied to the heap types converted in and before Python 3.9 before closing this issue?

I would say yes. Do you have a compiled list of what's left?

These types now lack the Py_TPFLAGS_IMMUTABLETYPE flag:

## Types converted in Python 3.9

- _abc._abc_data
- _json.Encoder
- _json.Scanner
- _random.Random
- _struct.Struct
- _struct.unpack_iterator
- ast.AST
- posix.DirEntry
- posix.ScandirIterator
- select.devpoll
- select.epoll
- select.kevent
- select.kqueue
- select.poll
- zlib.Compress
- zlib.Decompress


## Types converted before Python 3.9

- _curses_panel.panel
- _tkinter.Tcl_Obj
- _tkinter.tkapp
- _tkinter.tktimertoken

Yeah, I think these should be changed, but lest give a bit of time for other core devs to mention what they think

> Should the immutable flag also be applied to the heap types converted in and before Python 3.9 before closing this issue?

I don't think that Python 3.9 should be changed. It's too late. IMO this issue is not important enough to introduce a new type flag in a minor Python 3.9.x bugfix release. I suggest to close this issue.

In Python 3.11, 68 heap types have the Py_TPFLAGS_IMMUTABLETYPE flag:

* _blake2.blake2b
* _blake2.blake2s
* _bz2.BZ2Compressor
* _bz2.BZ2Decompressor
* _csv.Dialect
* _csv.reader
* _csv.writer
* _dbm.dbm
* _gdbm.gdbm
* _hashlib.HASH
* _hashlib.HASHXOF
* _hashlib.HMAC
* _lsprof.Profiler
* _lzma.LZMACompressor
* _lzma.LZMADecompressor
* _md5.md5
* _multibytecodec.MultibyteCodec
* _multibytecodec.MultibyteIncrementalDecoder
* _multibytecodec.MultibyteIncrementalEncoder
* _multibytecodec.MultibyteStreamReader
* _multibytecodec.MultibyteStreamWriter
* _overlapped.Overlapped
* _queue.SimpleQueue
* _sha1.sha1
* _sha256.sha224
* _sha256.sha256
* _sha3.keccak_224
* _sha3.keccak_256
* _sha3.keccak_384
* _sha3.keccak_512
* _sha3.sha3_224
* _sha3.sha3_256
* _sha3.sha3_384
* _sha3.sha3_512
* _sha512.sha384
* _sha512.sha512
* _sre.SRE_Scanner
* _ssl.Certificate
* _ssl.MemoryBIO
* _ssl.SSLSession
* _ssl._SSLContext
* _ssl._SSLSocket
* ssl.SSLError
* _thread.RLock
* _thread._local
* _thread._localdummy
* _thread.lock
* _tokenize.TokenizerIter
* _winapi.Overlapped
* array.array
* array.arrayiterator
* functools.KeyWrapper
* functools._lru_cache_wrapper
* functools._lru_list_elem
* functools.partial
* mmap.mmap
* operator.attrgetter
* operator.itemgetter
* operator.methodcaller
* pyexpat.xmlparser
* re.Match
* re.Pattern
* sqlite3.Connection
* sqlite3.Cursor
* sqlite3.PrepareProtocol
* sqlite3.Row
* sqlite3.Statement
* unicodedata.UCD