This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Unhelpful error message when one calls a subclass of type with a custom metaclass
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: abarry, berker.peksag, eryksun, gvanrossum, ppperry, python-dev, r.david.murray, rhettinger, serhiy.storchaka, steven.daprano
Priority: normal Keywords: patch

Created on 2016-05-29 22:46 by ppperry, last changed 2022-04-11 14:58 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
type_one_argument_1.patch abarry, 2016-05-30 03:57 review
type_one_argument_2.patch abarry, 2016-05-30 13:37 review
type_one_argument_3.patch abarry, 2016-05-30 17:57 review
type_one_argument_4.patch abarry, 2016-05-31 02:13 review
type_one_argument_5.patch abarry, 2016-08-14 22:00 review
Messages (31)
msg266643 - (view) Author: (ppperry) Date: 2016-05-29 22:46
>>>class x(type):pass
>>> x(x)
Traceback (most recent call last):
  File "<pyshell#335>", line 1, in <module>
    x(x)
TypeError: type() takes 1 or 3 arguments

I am giving it one argument, and yet it's complaining that `type` expects one or three arguments. How unhelpful.
msg266645 - (view) Author: Anilyka Barry (abarry) * (Python triager) Date: 2016-05-29 22:55
Works for me:

>>> class X(type): pass
...
>>> X(X)
<class 'type'>
msg266649 - (view) Author: (ppperry) Date: 2016-05-29 23:23
This issue only happens when the type in question has a custom metaclass:

>>> class meta(type):pass
>>> class X(type,metaclass=meta):pass
>>> X(X)
[Same unhelpful TypeError]
msg266650 - (view) Author: (ppperry) Date: 2016-05-29 23:26
Also happens on 2.7, although you have to declare the metaclass using  `__metaclass__ = meta` instead.
msg266651 - (view) Author: Anilyka Barry (abarry) * (Python triager) Date: 2016-05-30 00:12
In Objects/typeobject.c#L2290, the code checks that the (meta?)class is exactly `type` and that there's one argument, then returns `Py_TYPE(x)`. Subclasses of `type` allowing a single argument is a side-effect of not overriding __new__, not a documented feature.

Changing the call from `PyType_CheckExact` to `PyType_Check` makes it work, but I'm assuming there may be something I didn't think of. Or maybe there isn't, but either way, I don't consider that this is worth fixing -- if you want to call your subclass with only one argument, override __new__ and do the logic in there. And if you want the type of an object, use type directly.

Also, there may be performance concerns here. `type` is heavily optimized in many places; I thought that `PyType_Check` called Python code, but after checking the macro definitions and testing a bit, it turns out I'm wrong. But if there *is* a negative performance impact, this probably can't go in -- this check runs everytime that type() is called, no matter how many arguments, and including in class creation; that's also probably why `PyType_CheckExact` was chosen to begin with.
msg266652 - (view) Author: (ppperry) Date: 2016-05-30 00:25
Further testing reveals that this issue has nothing to do with metaclasses:

>>>class X(type):pass
>>>X()(X)

raises the same TypeError.
Even if the possibly dubious feature of being able to call instances of subclasses of type with one argument is rejected, the error message should definitely be improved. It is ridiculously unhelpful.
msg266653 - (view) Author: (ppperry) Date: 2016-05-30 00:27
Ignore the first part of my previous comment; I improperly tested that.
msg266654 - (view) Author: Anilyka Barry (abarry) * (Python triager) Date: 2016-05-30 00:39
Yes, that would be preferable. The error message is at Objects/typeobject.c#l2301, but keep in mind that this message is shown for both faulty calls to type() as well as any of its subclasses that don't override __new__, and I'm lukewarm on adding e.g. a PyType_Check call before that; might as well replace the PyType_CheckExact call and make this work. I'm not knowledgeable enough in that field though, you'll need a core dev's advice.
msg266660 - (view) Author: Steven D'Aprano (steven.daprano) * (Python committer) Date: 2016-05-30 01:29
I am unable to replicate this in Python 2.7, 3.3 or 3.6. I haven't bothered to test against other versions, because I think that this is a PyShell issue, not a Python issue. (I think you are using PyShell, based on the traceback given.)

Before reporting bugs in the interpreter, please test using the vanilla Python interactive interpreter, and not in an enhanced shell or IDE (iPython, PyShell, Spyder, etc).

I'm closing the task, please don't re-open unless you can demonstrate the issue in the plain Python interpreter. But please do report it to the PyShell maintainers.
msg266662 - (view) Author: (ppperry) Date: 2016-05-30 01:43
steven.daprano, you don't appear to have properly read the issue comments. I originally underspecified the conditions necessary to reproduce this, producing Emanuel Barry's closure. I then added a proper reproducer in the third comment, which does work in the vanilla python interpreter.
msg266663 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2016-05-30 01:51
When I hit this recently I assumed that that error message meant I'd screwed up the metaclass definition.  But if there's a way to improve the error message that would be great.

I'll add this to the list of bugs I'm making for the sprints, perhaps one of the sprinters can convince a knowlegable core dev to look at it and render an opinion :)
msg266666 - (view) Author: Steven D'Aprano (steven.daprano) * (Python committer) Date: 2016-05-30 03:01
On Mon, May 30, 2016 at 01:43:22AM +0000, ppperry wrote:
> steven.daprano, you don't appear to have properly read the issue 
> comments.

Ack; I saw your comment about the metaclass, then saw your retraction 
of the metaclass issue, then misinterpreted your retraction of the 
retraction.
msg266667 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2016-05-30 03:17
All types are instances of `type`, so the single argument case makes sense to me as a 'constructor'. It always returns an instance of `type`, just not a new instance. 

    >>> X = type('X', (type,), {})
    >>> type(X)
    <class 'type'>
    >>> isinstance(type(X), type)
    True

OTOH, implementing this for subclasses of `type` doesn't generally make sense to me. That this is allowed (sometimes) is I think a mistake:

    >>> X(X)
    <class 'type'>
    >>> isinstance(X(X), X)
    False

PyType_CheckExact(metatype) isn't checking that metatype is `type`. It's checking that the type of metatype is exactly `type`, which is true for `type` and immediate instances of type, i.e. normal metaclasses. But it's not the case for a metaclass that's an instance of another metaclass:

    >>> Y = X('Y', (X,), {})
    >>> Y(Y)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: type() takes 1 or 3 arguments

Maybe I'm missing something, but it makes more sense to me if metatype is required to be *exactly* `type`, i.e. metatype == &PyType_Type, and that this check is used to gate the entire special case:

    /* Special case: type(x) should return x->ob_type */
    if (metatype == &PyType_Type) {
        const Py_ssize_t nargs = PyTuple_GET_SIZE(args);
        const Py_ssize_t nkwds = kwds == NULL ? 0 : PyDict_Size(kwds);

        if (nargs == 1 && nkwds == 0) {
            PyObject *x = PyTuple_GET_ITEM(args, 0);
            Py_INCREF(Py_TYPE(x));
            return (PyObject *) Py_TYPE(x);
        }

        /* SF bug 475327 -- if that didn't trigger, we need 3
           arguments. but PyArg_ParseTupleAndKeywords below may give
           a msg saying type() needs exactly 3. */
        if (nargs + nkwds != 3) {
            PyErr_SetString(PyExc_TypeError,
                            "type() takes 1 or 3 arguments");
            return NULL;
        }
    }

This change yields the following behavior:

    >>> X = type('X', (type,), {})

    >>> type(X)
    <class 'type'>

    >>> type(1, 2)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: type() takes 1 or 3 arguments

    >>> X()
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: Required argument 'name' (pos 1) not found

    >>> X(X)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: type() argument 1 must be str, not type
msg266673 - (view) Author: Anilyka Barry (abarry) * (Python triager) Date: 2016-05-30 03:57
+1. I prefer that change, as using subclasses of `type` as if they were type itself never made sense to me. This shouldn't break existing code, but if it does, it was either a concealed bug or a very bad idea to begin with, and should be fixed either way.

Attached patch implements Eryk's suggestion. I haven't found any tests that checked for subclasses of type specifically (except tests testing for metaclass stuff), and I haven't added any either.
msg266676 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2016-05-30 05:41
Guido, this is your code.  Do you care to opine?
msg266677 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2016-05-30 05:52
I don't recall writing that any more, but that fix looks right. (Though why write `metatype == &PyType_Type` rather than `PyType_CheckExact(metatype)`?)
msg266684 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2016-05-30 08:08
> why write `metatype == &PyType_Type` rather than 
> PyType_CheckExact(metatype)`?

If only `type` should implement this special case, then it needs to be `metatype == &PyType_Type`. This was actually how it was implemented in 2.2a3:

https://hg.python.org/cpython/file/v2.2a3/Objects/typeobject.c#l631

I don't know why the final release of 2.2 switched to using PyType_CheckExact, which is true for most metaclasses. That's why I feel like I'm missing something here.

Probably it used PyType_CheckExact instead of PyType_Check to ensure PyType_IsSubtype wouldn't be called. Nowadays that's optimized away via PyType_FastSubclass and the Py_TPFLAGS_TYPE_SUBCLASS flag (set up in inherit_special). If it's decided to retain this special case for metaclasses other than `type`, then I think it should use PyType_Check to consistently implement it for all metaclasses. Also, the error message should be more generic, e.g. maybe "__new__() takes 1 or 3 arguments".
msg266693 - (view) Author: Anilyka Barry (abarry) * (Python triager) Date: 2016-05-30 13:37
Yes, `metatype == &PyType_Type` makes sure that only `type` itself is valid for the one-argument part, whereas subclasses can also do so right now. I clarified that in a comment in the new patch, so that someone doesn't accidentally revert this, thinking PyType_CheckExact is fine.

Before the patch:

>>> type(1, 2, 3)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: type() argument 1 must be str, not int

After the patch:

>>> type(1, 2, 3)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: type.__new__() argument 1 must be str, not int
>>> class X(type): pass
...
>>> X(1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: type.__new__() argument 1 must be str, not int
msg266701 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2016-05-30 16:06
OK, the patch looks fine. Please treat this as a new feature (just in case) and only apply it to 3.6.
msg266702 - (view) Author: Anilyka Barry (abarry) * (Python triager) Date: 2016-05-30 16:10
Fair enough. Should this get a note in What's new? Possibly in the "Changes in Python API" section.
msg266705 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2016-05-30 16:23
No, just an entry in Misc/NEWS.

--Guido (mobile)
msg266706 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-05-30 16:28
Needed tests and updating the documentation.
msg266712 - (view) Author: Anilyka Barry (abarry) * (Python triager) Date: 2016-05-30 17:57
New patch with tests and documentation. I didn't really know where to put the tests; test_types seemed reasonable to me (other option was test_metaclass).
msg266722 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-05-31 00:08
Please don't mark your own patch as 'commit review'.
msg266726 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2016-05-31 00:42
Berker, I don't mind if people mark their own patches for commit review *when they think it is ready for commit*.  (Just as with reviewers, if they start being consistently right, they are ready for commit privs :)  However, a non-committer setting a patch to commit review is waving a hand for attention from a core dev, and that should only be done *after* a patch has been *reviewed* and judged ready for final review before commit.

(I broke my own rule the other day and got in trouble, though...I set a patch to commit review to remind myself to review it rather than because I thought it was commit ready already, and it got committed...)
msg266728 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-05-31 01:59
Thanks for the patch, Emanuel. I left some comments about Sphinx markup on Rietveld.

> Berker, I don't mind if people mark their own patches for commit review *when they think it is ready for commit*.

Is there any person who really thinks that their own patch is *not* ready for commit review? :)
msg266729 - (view) Author: Anilyka Barry (abarry) * (Python triager) Date: 2016-05-31 02:13
New patch with Berker's comments. I'm really not used to Sphinx markup so thanks for that!

> Is there any person who really thinks that their own patch is *not* ready for commit review? :)

Partial patches aren't that uncommon in some projects. I also sometimes don't trust that I got everything right (especially if it's in a very large codebase like CPython), and having a few more pair of eyes taking a look at it helps.
msg270348 - (view) Author: (ppperry) Date: 2016-07-13 22:06
Ping
msg272697 - (view) Author: Anilyka Barry (abarry) * (Python triager) Date: 2016-08-14 22:00
Rebased patch so that it applies cleanly again.
msg273089 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-08-19 08:03
New changeset c3498187f998 by Berker Peksag in branch 'default':
Issue #27157: Make only type() itself accept the one-argument form
https://hg.python.org/cpython/rev/c3498187f998
msg273090 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-08-19 08:04
Thanks!
History
Date User Action Args
2022-04-11 14:58:31adminsetgithub: 71344
2016-08-19 08:04:09berker.peksagsetstatus: open -> closed
resolution: fixed
messages: + msg273090

stage: patch review -> resolved
2016-08-19 08:03:35python-devsetnosy: + python-dev
messages: + msg273089
2016-08-14 22:00:38abarrysetfiles: + type_one_argument_5.patch

messages: + msg272697
2016-07-13 22:06:28ppperrysetmessages: + msg270348
2016-05-31 02:13:04abarrysetfiles: + type_one_argument_4.patch

messages: + msg266729
2016-05-31 01:59:01berker.peksagsetmessages: + msg266728
2016-05-31 00:42:10r.david.murraysetmessages: + msg266726
2016-05-31 00:08:25berker.peksagsetnosy: + berker.peksag

messages: + msg266722
stage: commit review -> patch review
2016-05-30 17:57:46abarrysetfiles: + type_one_argument_3.patch
2016-05-30 17:57:36abarrysetmessages: + msg266712
stage: test needed -> commit review
2016-05-30 17:32:43abarrysetmessages: - msg266711
2016-05-30 17:31:19eryksunsetmessages: - msg266710
2016-05-30 17:26:58abarrysetmessages: + msg266711
2016-05-30 17:23:42eryksunsetmessages: + msg266710
2016-05-30 16:28:13serhiy.storchakasetnosy: + serhiy.storchaka

messages: + msg266706
stage: commit review -> test needed
2016-05-30 16:23:38gvanrossumsetmessages: + msg266705
2016-05-30 16:10:47abarrysetstage: patch review -> commit review
messages: + msg266702
versions: + Python 3.6, - Python 2.7, Python 3.4
2016-05-30 16:06:43gvanrossumsetmessages: + msg266701
2016-05-30 13:37:39abarrysetfiles: + type_one_argument_2.patch

messages: + msg266693
2016-05-30 08:08:57eryksunsetmessages: + msg266684
2016-05-30 05:52:10gvanrossumsetmessages: + msg266677
2016-05-30 05:41:01rhettingersetnosy: + rhettinger, gvanrossum
messages: + msg266676
2016-05-30 03:58:31abarrysetstage: needs patch -> patch review
2016-05-30 03:57:45abarrysetfiles: + type_one_argument_1.patch
keywords: + patch
messages: + msg266673

stage: needs patch
2016-05-30 03:17:17eryksunsetnosy: + eryksun
messages: + msg266667
2016-05-30 03:01:26steven.dapranosetmessages: + msg266666
2016-05-30 01:51:02r.david.murraysetnosy: + r.david.murray

messages: + msg266663
stage: resolved -> (no value)
2016-05-30 01:43:22ppperrysetstatus: closed -> open
resolution: works for me -> (no value)
messages: + msg266662
2016-05-30 01:29:05steven.dapranosetstatus: open -> closed

nosy: + steven.daprano
messages: + msg266660

resolution: works for me
stage: resolved
2016-05-30 00:39:08abarrysetmessages: + msg266654
stage: resolved -> (no value)
2016-05-30 00:27:53ppperrysetmessages: + msg266653
title: Unhelpful error message when one calls an instance of a subclass of type -> Unhelpful error message when one calls a subclass of type with a custom metaclass
2016-05-30 00:25:39ppperrysetmessages: + msg266652
title: Unhelpful error message when one calls a subclass of type with a custom metaclass -> Unhelpful error message when one calls an instance of a subclass of type
2016-05-30 00:12:08abarrysetmessages: + msg266651
2016-05-29 23:26:03ppperrysetmessages: + msg266650
versions: + Python 2.7
2016-05-29 23:23:40ppperrysetstatus: closed -> open
resolution: works for me -> (no value)
messages: + msg266649

title: Unhelpful error message when one calls a subclass of type -> Unhelpful error message when one calls a subclass of type with a custom metaclass
2016-05-29 22:55:05abarrysetstatus: open -> closed

nosy: + abarry
messages: + msg266645

resolution: works for me
stage: resolved
2016-05-29 22:46:08ppperrycreate