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

Deprecate creating immutable types with mutable bases #95388

Closed
encukou opened this issue Jul 28, 2022 · 6 comments
Closed

Deprecate creating immutable types with mutable bases #95388

encukou opened this issue Jul 28, 2022 · 6 comments
Assignees
Labels
topic-C-API type-feature A feature request or enhancement

Comments

@encukou
Copy link
Member

encukou commented Jul 28, 2022

The C API currently allows creating immutable types from mutable bases. I can't find a case where this breaks things, but it's very hard to think about (and easy to skip thinking about) how this edge case affects things like cache invalidation.

I also don't immediately see a use case for this, so I think it's best to deprecate it, wait for 2 releases to see if anyone needs it, and remove it.

@encukou encukou added type-feature A feature request or enhancement topic-C-API labels Jul 28, 2022
@encukou encukou self-assigned this Jul 28, 2022
@rhettinger
Copy link
Contributor

How are you detecting mutability? My understanding is that mutability arises solely through the addition of mutating methods and that in the general case this isn't easily detected.

Also, isn't it a valid pattern to subclass a mutable class like dict and then override the mutating methods to disable them and create an immutable class? IIRC, there are frozendict implementations that do exactly this.

@encukou
Copy link
Member Author

encukou commented Jul 30, 2022

This is about type objects themselves, not instances – e.g. the str type is immutable so you can't set str.foo.
Immutable types have Py_TPFLAGS_IMMUTABLETYPE set.

@vstinner
Copy link
Member

vstinner commented Aug 1, 2022

The C API currently allows creating immutable types from mutable bases

Ah? Would you mind to elaborate? Do you have an example?

Are you talking about defining a heap type which inherits from a heap type or a static type?


type_ready_mro() implements the following check. Does it prevent the issue for static types at least?

    /* All bases of statically allocated type should be statically allocated */
    if (!(type->tp_flags & Py_TPFLAGS_HEAPTYPE)) {
        PyObject *mro = type->tp_mro;
        Py_ssize_t n = PyTuple_GET_SIZE(mro);
        for (Py_ssize_t i = 0; i < n; i++) {
            PyTypeObject *base = _PyType_CAST(PyTuple_GET_ITEM(mro, i));
            if (base->tp_flags & Py_TPFLAGS_HEAPTYPE) {
                PyErr_Format(PyExc_TypeError,
                             "type '%.100s' is not dynamically allocated but "
                             "its base type '%.100s' is dynamically allocated",
                             type->tp_name, base->tp_name);
                return -1;
            }
        }
    }

@encukou
Copy link
Member Author

encukou commented Aug 1, 2022

@vstinner, this test that can serve as an example: https://github.com/python/cpython/pull/95533/files#diff-a537c794ffb51531649e2b81f9ae6978c25077de6aa10c9134576ac628fcc345R629-R654

Yes, static types aren't affected. Only heap types can be mutable.

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Aug 4, 2022

Py_TPFLAGS_IMMUTABLETYPE was added to simplify the conversion of static types to heap types. Inheriting static types from heap types was forbidden (I do not remember exact reason but it is likely that it was the same reason as of this issue). Perhaps allowing inheriting type with a heap type was Py_TPFLAGS_IMMUTABLETYPE was a mistake. And since it is a new feature, should not we just disallow it instead of emitting a warning?

@encukou
Copy link
Member Author

encukou commented Aug 4, 2022

It's not a new feature any more. I'd rather go through the deprecation cycle than try to prove no one is using this.

erlend-aasland added a commit to erlend-aasland/cpython that referenced this issue Aug 5, 2022
…ith_mutable_base

When 3.14 kicks in, it'll be a RuntimeError;
the test will correctly fail then.
encukou pushed a commit that referenced this issue Aug 8, 2022
…table_base (GH-95728)

When 3.14 kicks in, it'll be a RuntimeError;
the test will correctly fail then.
iritkatriel pushed a commit to iritkatriel/cpython that referenced this issue Aug 11, 2022
…ith_mutable_base (pythonGH-95728)

When 3.14 kicks in, it'll be a RuntimeError;
the test will correctly fail then.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-C-API type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

4 participants