bpo-43916: Add Py_TPFLAGS_DISALLOW_INSTANTIATION type flag

[vstinner]

Add a new Py_TPFLAGS_DISALLOW_INSTANTIATION type flag to disallow
creating type instances: set tp_new to NULL and don't create the
"new" key in the type dictionary.

The flag is set automatically on static types if tp_base is NULL or
&PyBaseObject_Type and tp_new is NULL.

Use the flag on the following types:

• _curses.ncurses_version type
• _curses_panel.panel
• _tkinter.Tcl_Obj
• _tkinter.tkapp
• _tkinter.tktimertoken
• _xxsubinterpretersmodule.ChannelID
• sys.flags type
• sys.getwindowsversion() type
• sys.version_info type

Update MyStr example in the C API documentation to use
Py_TPFLAGS_DISALLOW_INSTANTIATION.

Add _PyStructSequence_InitType() function to create a structseq type
with the Py_TPFLAGS_DISALLOW_INSTANTIATION flag set.

https://bugs.python.org/issue43916

[vstinner]

This flag can be used outside CPython code base. For example, the pyrsistent project sets tp_new to NULL:

static PyObject* pyrsistent_pvectorc_moduleinit(void){... // Only allow creation/initialization through factory method pvec PVectorType.tp_init = NULL; PVectorType.tp_new = NULL; if (PyType_Ready(&PVectorType) < 0){return NULL} ... Py_INCREF(&PVectorType); PyModule_AddObject(m, "PVector", (PyObject *)&PVectorType); return m} 

[vstinner]

cc @erlend-aasland@tiran@serhiy-storchaka@pablogsal

[tiran]

Nice! Do you want to update _hashopenssl.c or should I update it later?

[vstinner]

Nice! Do you want to update _hashopenssl.c or should I update it later?

I prefer to keep the PR simple to make the review easier, and write a second PR to use the flag on heap types that should not be instanciated directly: rewrite PR #25653 with this flag (or let @erlend-aasland do it) if my PR is accepted.

[erlend-aasland]

Nice! Do you want to update _hashopenssl.c or should I update it later?

I prefer to keep the PR simple to make the review easier, and write a second PR to use the flag on heap types that should not be instanciated directly: rewrite PR #25653 with this flag (or let @erlend-aasland do it) if my PR is accepted.

I can adapt #25653 to use this flag.

[tiran]

@morzen CPython uses an external issue tracker. You can find more information in the top level README, https://github.com/python/cpython#issue-tracker-and-mailing-list

And please don't post pictures. They are neither searchable nor accessible.

[tiran]

Nice! Do you want to update _hashopenssl.c or should I update it later?

I prefer to keep the PR simple to make the review easier, and write a second PR to use the flag on heap types that should not be instanciated directly: rewrite PR #25653 with this flag (or let @erlend-aasland do it) if my PR is accepted.

Sounds good to me! I have created PR draft #25722.

[vstinner]

About the flag name, another name could be Py_TPFLAGS_DISALLOW_NEW. Or a different name.

[vstinner]

About the complexity of tp_new inheritance, here is an extract of tp_new_wrapper():

 /* Check that the use doesn't do something silly and unsafe like object.__new__(dict). To do this, we check that the most derived base that's not a heap type is this type. */ staticbase = subtype; while (staticbase && (staticbase->tp_new == slot_tp_new)) staticbase = staticbase->tp_base; /* If staticbase is NULL now, it is a really weird type. In the spirit of backwards compatibility (?), just shut up. */ if (staticbase && staticbase->tp_new != type->tp_new){PyErr_Format(PyExc_TypeError, "%s.__new__(%s) is not safe, use %s.__new__()", type->tp_name, subtype->tp_name, staticbase->tp_name); return NULL} 

[tiran]

Naming is hard. Both Py_TPFLAGS_DISABLE_NEW and Py_TPFLAGS_DISALLOW_NEW reflect how Python prevents creation instances, not what the flag actually accomplishes. How about something like Py_TPFLAGS_DISALLOW_INSTANTIATION?

[erlend-aasland]

I've adapted #25653 to use this flag. Regarding naming, I like @tiran's suggestion. I'll do a force push as soon as the name has landed :)

[vstinner]

I renamed the flag to Py_TPFLAGS_DISALLOW_INSTANTIATION. I also updated _xxsubinterpretersmodule.ChannelID to use the new flag.

"The creation of an instance is called instantiation." says Wikipedia: https://en.wikipedia.org/wiki/Instance_(computer_science)

I compared the usage of "disallow" versus "deny" in Include/ header files and in Doc/ documentation: "disallow" is more popular.

[vstinner]

@morzen CPython uses an external issue tracker. You can find more information in the top level README, https://github.com/python/cpython#issue-tracker-and-mailing-list

I removed the comment.

[vstinner]

@tiran@erlend-aasland: my PR uses the flag in 9 types (and 1 documentation example). @erlend-aasland's PR disallow instanciation in many types. Do you think that these usages are common enough to justify to add a type flag?

[vstinner]

This design doesn't prevent calling directly tp_new in C. I clarified the flag documentation:

/* Disallow creating instances of the type in Python. It remains possible to * call directly the tp_new function in C. */ 

[pablogsal]

I think this is a much more elegant solution, the only thing I am slightly wary of is to expose this publicly.

[serhiy-storchaka]

There is already a standard way to prevent creating a new instance: setting tp_new to NULL. No new type flag is needed. BTW, tp_new == NULL is one of checks for pickleability of the type.

[vstinner]

@serhiy-storchaka:

There is already a standard way to prevent creating a new instance: setting tp_new to NULL. No new type flag is needed. BTW, tp_new == NULL is one of checks for pickleability of the type.

If tp_new is set NULL after the type is created, creating an instance in Python fails with a weird error message. Example in Python 3.9:

$ python3.9 Python 3.9.4 (default, Apr 6 2021, 00:00:00) >>> import _curses_panel # Good >>> _curses_panel.panel() TypeError: cannot create '_curses_panel.panel' instances # Weird error >>> _curses_panel.panel.__new__(_curses_panel.panel) TypeError: object.__new__(_curses_panel.panel) is not safe, use _curses_panel.panel.__new__() >>> _curses_panel.panel.__new__ is object.__new__ True 

The problem is that the the type __dict__['__new__'] exists (wrapper to object.new). I would prefer to request in the type spec that instantiation is disallowed. Copy of my previous comment, extract of tp_new_wrapper():

 /* Check that the use doesn't do something silly and unsafe like object.__new__(dict). To do this, we check that the most derived base that's not a heap type is this type. */ staticbase = subtype; while (staticbase && (staticbase->tp_new == slot_tp_new)) staticbase = staticbase->tp_base; /* If staticbase is NULL now, it is a really weird type. In the spirit of backwards compatibility (?), just shut up. */ if (staticbase && staticbase->tp_new != type->tp_new){PyErr_Format(PyExc_TypeError, "%s.__new__(%s) is not safe, use %s.__new__()", type->tp_name, subtype->tp_name, staticbase->tp_name); return NULL} 

[vstinner]

Extract of inherit_special():

 /* The condition below could use some explanation. It appears that tp_new is not inherited for static types whose base class is 'object'; this seems to be a precaution so that old extension types don't suddenly become callable (object.__new__ wouldn't insure the invariants that the extension type's own factory function ensures). Heap types, of course, are under our control, so they do inherit tp_new; static extension types that specify some other built-in type as the default also inherit object.__new__. */ if (base != &PyBaseObject_Type || (type->tp_flags & Py_TPFLAGS_HEAPTYPE)){if (type->tp_new == NULL) type->tp_new = base->tp_new} 

PyType_FromSpec() creates a type and then calls PyType_Ready() on it. PyType_Ready() calls inherit_special() which inherits tp_new (since it's a heap type), and then creates tp_dict["__new__"] since tp_new is not NULL anymore.

[vstinner]

@serhiy-storchaka: I updated the PR. The flag now explicitly sets tp_new to NULL. So code checking if tp_new is NULL (like pickle) is not affected.

Advantages of using the new flag compared to setting tp_new to NULL:

• PyType_FromSpec() doesn't support tp_new=NULL and it would be tricky to support that.
• It ensures that __new__ is not created in the type dictionary. In Python, my_type.__new__ attribute doesn't exist.
• The same flag can be used on static types, heap types and with _PyStructSequence_InitType().
• The flag is known when the type is created and so PyType_Ready() can handle corner cases. For example, it doesn't create __new__ key in the type dictionary.

[vstinner]

I mark this PR as a draft. I'm trying to modify PyType_FromSpec() to support {Py_tp_new, NULL}, slot, so we can compare the three options (Erlend's PR, this PR, and the future PyType_FromSpec PR).

[vstinner]

Ok, there are now 3 API options to solve https://bugs.python.org/issue43916:

• A (PR bpo-43916: Add _PyType_DisabledNew to prevent new heap types from being created uninitialised #25653) sets tp_new to a new _PyType_DisabledNew() function
• B (this PR bpo-43916: Add Py_TPFLAGS_DISALLOW_INSTANTIATION type flag #25721) adds a public Py_TPFLAGS_DISALLOW_INSTANTIATION flag
• C (PR [WIP] bpo-43916: PyType_FromSpec() supports Py_tp_new=NULL slot #25733) modifies PyType_FromSpec() to support Py_tp_new=NULL slot

There are 3 main ways to create types

• Static types

  • A uses tp_new = _PyType_DisabledNew
  • B sets Py_TPFLAGS_DISALLOW_INSTANTIATION flag
  • C uses regular tp_new = NULL

• Heap types: PyType_Spec struct + PyFrom_FromSpec()

  • A uses {Py_tp_new, _PyType_DisabledNew} slot
  • B sets Py_TPFLAGS_DISALLOW_INSTANTIATION flag
  • C uses a new {Py_tp_new, NULL} slot

• structseq type: PyStructSequence_InitType2() or a new _PyStructSequence_InitType() function

  • A doesn't address this case (keep the current code which sets tp_new and remove the __new__ key afterwards)
  • B sets Py_TPFLAGS_DISALLOW_INSTANTIATION flag
  • C uses a new internal function to pass disallow_instantiation=1

A drawbacks:

• _PyType_DisabledNew() is only exposed in the internal C API and so needs to build more C extensions with the internal C API.
• Don't slove the structseq types case.

B drawbacks:

• Add a new public type flag.

C drawbacks:

• {Py_tp_new, NULL} is less explicit than _PyType_DisabledNew() and Py_TPFLAGS_DISALLOW_INSTANTIATION.
• New internal API are more specific to this problem than B internal functions.

[pablogsal]

My preference is B, even if we can manually set tp_new to NULL it has a bit of a "hacky" feeling. I find the flag the most simple and elegant of all, but is true that still feels just a bit weird that a flag will disallow instantiation.

My preference order is B-> C -> A

[erlend-aasland]

My preference order is B-> C -> A

Mine too, FWIW.

[vstinner]

@pablogsal:

My preference is B, even if we can manually set tp_new to NULL it has a bit of a "hacky" feeling. I find the flag the most simple and elegant of all, but is true that still feels just a bit weird that a flag will disallow instantiation.

In Python, there is a similar "hack" to mark a type as non hashable:

$ python3 Python 3.9.4 (default, Apr 6 2021, 00:00:00) >>> class A: ... pass >>> hash(A()) 8731944244974 >>> class B: ... __hash__ = None >>> hash(B()) TypeError: unhashable type: 'B' 

[vstinner]

In Python, setting __new__ to None prevents instantiation, but with a surprising error message ;-)

$ python3 Python 3.9.4 (default, Apr 6 2021, 00:00:00) >>> class A: ... __new__ = None >>> A() TypeError: 'NoneType' object is not callable 

[pablogsal]

In Python, setting __new__ to None prevents instantiation, but with a surprising error message ;-)

I wouldn't call that surprising TBH :)

[vstinner]

Another slice of type inheritance (tp_new) complexity. Extract of update_one_slot() which is called by type_new() after PyType_Ready():

 else if (Py_IS_TYPE(descr, &PyCFunction_Type) && PyCFunction_GET_FUNCTION(descr) == (PyCFunction)(void(*)(void))tp_new_wrapper && ptr == (void**)&type->tp_new){/* The __new__ wrapper is not a wrapper descriptor, so must be special-cased differently. If we don't do this, creating an instance will always use slot_tp_new which will look up __new__ in the MRO which will call tp_new_wrapper which will look through the base classes looking for a static base and call its tp_new (usually PyType_GenericNew), after performing various sanity checks and constructing a new argument list. Cut all that nonsense short -- this speeds up instance creation tremendously. */ specific = (void *)type->tp_new; /* XXX I'm not 100% sure that there isn't a hole in this reasoning that requires additional sanity checks. I'll buy the first person to point out a bug in this reasoning a beer. */ } 

[erlend-aasland]

I've converted #25653 to apply Py_TPFLAGS_DISALLOW_INSTANTIATION to the rest of the affected types: erlend-aasland@82d7a4b

Let me know if you want me to create a PR for it, @pablogsal.

[vstinner]

PR rebased to fix a conflict.

[vstinner]

I took @pablogsal and @serhiy-storchaka concerns in account. Pablo would prefer to avoid adding a public type, Serhiy would prefer to modify PyType_FromSpec() to accept tp_new=NULL. I implemented Serhiy's idea to compare the API with this Py_TPFLAGS_DISALLOW_INSTANTIATION PR. Most people prefer the explicit Py_TPFLAGS_DISALLOW_INSTANTIATION flag which is less surprising than tp_new=NULL. I also listed other advantages of the Py_TPFLAGS_DISALLOW_INSTANTIATION flag in my previous comments.

Usually, I would prefer to get a clear consensus, but we are out of time: Python 3.10 beta1 (feature freeze) is next Wednesday, and I will be in holiday for 1 week starting tonight. If there is still a disagreement, we can still revisit the API or the implementation before Python 3.10 final release ("3.10.0 final: Monday, 2021-10-04" says PEP 619).

I merged my PR to unblock https://bugs.python.org/issue43916 release blocker issue (and there are other open release blocker issues to be solved before Wednesday).

Anyway, thanks everyone for this constructive and interesting discussion ;-)

The final merged change also takes in account your remarks. For example, the flag now sets tp_new to NULL, to take Serhiy's comment in account.