classification
Title: Nonsensical exception message when calling `__new__` on non-instaniable objects
Type: behavior Stage: patch review
Components: Interpreter Core, Library (Lib) Versions: Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Vadim Pushtaev, ncoghlan, ppperry, serhiy.storchaka
Priority: low Keywords: patch

Created on 2018-07-30 20:05 by ppperry, last changed 2018-08-08 18:15 by Vadim Pushtaev.

Pull Requests
URL Status Linked Edit
PR 8576 open Vadim Pushtaev, 2018-07-30 23:41
Messages (16)
msg322688 - (view) Author: (ppperry) Date: 2018-07-30 20:05
> type(sys.flags).__new__(type(sys.flags))
Traceback (most recent call last):
  File "<pyshell#102>", line 1, in <module>
    type(sys.flags).__new__(type(sys.flags))
TypeError: tuple.__new__(sys.flags) is not safe, use sys.flags.__new__()

Ignoring the confusion caused by both the type and the instance being called "sys.flags", this error doesn't make sense. I am using "sys.flags" (the type).__new__, so why is it complaining?

"type(sys.flags)()" produces the standard "non-instantiable type" error. 

The same behavior also happens for "sys.version_info", but strangely not for any of the other sys constants.
msg322694 - (view) Author: Vadim Pushtaev (Vadim Pushtaev) * Date: 2018-07-30 22:18
I'm trying to do something about this. Let me know if you have some ideas.
msg322695 - (view) Author: Vadim Pushtaev (Vadim Pushtaev) * Date: 2018-07-30 23:40
Here is what I learned:

1) Nothing is wrong with that "tuple.__new__(sys.flags) is not safe" part. `__new__` is deleted from `type(sys.flags)`, so parent's `__new__` is called. `tuple` is indeed a base class for `type(sys.flags)`.

2) Another part where we recommend to use "sys.flags.__new__()" doesn't make much sense, so I choose to delete this advice if there is no `__new__` in a class.

3) This second part also may suggest a wrong class to call `__new__` from:


In [1]: from collections import namedtuple

In [2]: class A(namedtuple('A', 'z')):
   ...:     pass
   ...:

In [3]: object.__new__(A)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-3-24eacd9ea752> in <module>()
----> 1 object.__new__(A)

TypeError: object.__new__(A) is not safe, use tuple.__new__()


This should be A.__new__(), not tuple.__new__().
msg322699 - (view) Author: (ppperry) Date: 2018-07-30 23:54
Your PR is not an improvement:
1. In this case, this will produce the error "tuple.__new__(sys.flags) is not safe". But I didn't call "tuple.__new__", I called sys.flags.__new__, and type(X).__new__(type(X)) should always be safe
2. The change in error message for namedtuples (A) isn't related and (B) isn't correct. `tuple.__new__(NamedTuple)` works, and produces a namedtuple object, so tuple.__new__ is what the error should point to.
msg322700 - (view) Author: (ppperry) Date: 2018-07-30 23:55
Same thing happens for other objects, like `type(sys._getframe(0)).__new__(type(sys._getframe_))`, and presumably any object that one cannot instantiate by calling
msg322703 - (view) Author: Vadim Pushtaev (Vadim Pushtaev) * Date: 2018-07-31 00:09
> 1. In this case, this will produce the error "tuple.__new__(sys.flags) is not safe". But I didn't call "tuple.__new__", I called sys.flags.__new__, and type(X).__new__(type(X)) should always be safe

Should it? There is no sys.flags.__new__, tuple.__new__ is called and it has different __new__.

> 2. The change in error message for namedtuples (A) isn't related and (B) isn't correct. `tuple.__new__(NamedTuple)` works, and produces a namedtuple object, so tuple.__new__ is what the error should point to.

I believe you are right about (A), it's not related and should be discussed separately.
msg322704 - (view) Author: (ppperry) Date: 2018-07-31 00:17
The error I'm expecting here is "cannot create sys.flags objects". Anything else violates the fact that type(*args) is sugar for:

result = type.__new__(type, *args)
if isinstance(result, type):
    result.__init__(*args)

("type" in the above snippet is a variable name, not the actual builtin "type")
msg322705 - (view) Author: (ppperry) Date: 2018-07-31 00:18
Thus, I think the bug is that "type(sys.flags).__new__" is an alias for "tuple.__new__" and is not in the code for __new__ calls that your PR touches.
msg322715 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-07-31 03:19
See also issue31506.
msg322745 - (view) Author: Vadim Pushtaev (Vadim Pushtaev) * Date: 2018-07-31 08:17
> See also issue31506

Okay, I admit, reporting `tuple.__new__` instead of `sys.flags` is misleading.

But what about this?

> `tuple.__new__(NamedTuple)` works, and produces a namedtuple object, so tuple.__new__ is what the error should point to.

Isn't it the same? Why should we say anything about `tuple` if a user wants A? This looks similar to 31506:

>>> from collections import namedtuple
>>> class A(namedtuple('x', 'x')):
...     pass
...
>>> A.__new__(1, 2)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<string>", line 1, in __new__
TypeError: tuple.__new__(X): X is not a type object (int)
msg323110 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2018-08-04 15:44
Looking at the code in https://github.com/python/cpython/blob/e42b705188271da108de42b55d9344642170aa2b/Python/sysmodule.c#L2360, I think the underlying problem here is that the code to make these PyStructSequence subclasses uninstantiable isn't really right - rather than clearing those fields in the subclass (which still allows dynamic method lookup to fall back to the parent class, even though fast C level dispatch fails), it would be more consistent and reliable to replace them with implementations that always raise a TypeError with a suitable message.

(An alternative may be to implement the same behaviour as the builtin singleton types used for None and Ellipsis: replace __new__ with an implementation that always returns the original singleton instance. However, there isn't a compelling use case, so raising a descriptive exception along the lines of what ppperry suggests seems most appropriate)
msg323113 - (view) Author: (ppperry) Date: 2018-08-04 16:25
The problem doesn't just happen with `sys.flags`, though. It happens with all types that can't be created directly by python. Ex: frame objects, generators, cells, etc. The bug is that in types whose c-level tp_new is null, the python-level __new__ is inherited from the superclass (and since object defines a `__new__`, such a function always exists), instead of being defined to always raise an error, like calling the type directly does.
msg323127 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2018-08-05 01:53
Agreed, but it's still a definition time bug, as the types are only nulling out tp_new after creating the singleton instance, and not preventing __new__ from resolving.

If they *don't* null out tp_new, but instead set tp_new to a common helper function that raises "TypeError: Cannot create <type name> instances", that will both prevent __new__ from working, and also ensure that `type(obj)()` and `type(obj).__new__()` give the same error.

(Changing the applicable version, as this issues combines a relatively obscure error reporting issue with a relatively intrusive fix, so the risk/reward ratio pushes it towards being a Python 3.8 only change).
msg323226 - (view) Author: Vadim Pushtaev (Vadim Pushtaev) * Date: 2018-08-06 21:01
Sorry for the delay, I'm still working on a new PR.
msg323240 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-08-07 11:53
Issue5322 may has a relation to this.
msg323293 - (view) Author: Vadim Pushtaev (Vadim Pushtaev) * Date: 2018-08-08 18:15
Usually, tp_new==NULL means that __new__ is inherited, but not always. Here is the comment from typeobject.c:

/* 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__. */


So my current solution is to explicitly set __new__ to the common helper function that raises TypeError in that case.

---

Thanks a lot for your comments and ideas. In truth, I feel a little overwhelmed and definitely need further guidance for this issue.
History
Date User Action Args
2018-08-08 18:15:06Vadim Pushtaevsetmessages: + msg323293
2018-08-07 11:53:25serhiy.storchakasetmessages: + msg323240
2018-08-06 21:01:10Vadim Pushtaevsetmessages: + msg323226
2018-08-05 01:53:12ncoghlansetmessages: + msg323127
versions: - Python 3.7
2018-08-04 16:25:38ppperrysetmessages: + msg323113
2018-08-04 15:44:41ncoghlansetpriority: normal -> low

messages: + msg323110
2018-07-31 08:17:57Vadim Pushtaevsetmessages: + msg322745
2018-07-31 03:19:47serhiy.storchakasetnosy: + serhiy.storchaka, ncoghlan
messages: + msg322715
2018-07-31 00:18:41ppperrysetmessages: + msg322705
2018-07-31 00:17:16ppperrysetmessages: + msg322704
title: Nonsensical exception message when calling `__new__` on some sys objects -> Nonsensical exception message when calling `__new__` on non-instaniable objects
2018-07-31 00:09:04Vadim Pushtaevsetmessages: + msg322703
title: Nonsensical exception message when calling `__new__` on non-instaniable object -> Nonsensical exception message when calling `__new__` on some sys objects
2018-07-30 23:55:37ppperrysetmessages: + msg322700
title: Nonsensical exception message when calling `__new__` on some sys objects -> Nonsensical exception message when calling `__new__` on non-instaniable object
2018-07-30 23:54:28ppperrysetmessages: + msg322699
2018-07-30 23:41:11Vadim Pushtaevsetkeywords: + patch
stage: patch review
pull_requests: + pull_request8083
2018-07-30 23:40:12Vadim Pushtaevsetmessages: + msg322695
2018-07-30 22:18:06Vadim Pushtaevsetnosy: + Vadim Pushtaev
messages: + msg322694
2018-07-30 20:05:01ppperrycreate