Unhelpful error message when one calls a subclass of type with a custom metaclass

[pppery]

BPO	27157
Nosy	@gvanrossum, @rhettinger, @stevendaprano, @bitdancer, @berkerpeksag, @serhiy-storchaka, @eryksun, @pppery, @Vgr255
Files	type_one_argument_1.patch type_one_argument_2.patch type_one_argument_3.patch type_one_argument_4.patch type_one_argument_5.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2016-08-19.08:04:09.342>
created_at = <Date 2016-05-29.22:46:08.510>
labels = ['interpreter-core', 'type-bug']
title = 'Unhelpful error message when one calls a subclass of type with a custom metaclass'
updated_at = <Date 2016-08-19.08:04:09.341>
user = 'https://github.com/pppery'

bugs.python.org fields:

activity = <Date 2016-08-19.08:04:09.341>
actor = 'berker.peksag'
assignee = 'none'
closed = True
closed_date = <Date 2016-08-19.08:04:09.342>
closer = 'berker.peksag'
components = ['Interpreter Core']
creation = <Date 2016-05-29.22:46:08.510>
creator = 'ppperry'
dependencies = []
files = ['43054', '43056', '43060', '43062', '44109']
hgrepos = []
issue_num = 27157
keywords = ['patch']
message_count = 31.0
messages = ['266643', '266645', '266649', '266650', '266651', '266652', '266653', '266654', '266660', '266662', '266663', '266666', '266667', '266673', '266676', '266677', '266684', '266693', '266701', '266702', '266705', '266706', '266712', '266722', '266726', '266728', '266729', '270348', '272697', '273089', '273090']
nosy_count = 10.0
nosy_names = ['gvanrossum', 'rhettinger', 'steven.daprano', 'r.david.murray', 'python-dev', 'berker.peksag', 'serhiy.storchaka', 'eryksun', 'ppperry', 'abarry']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue27157'
versions = ['Python 3.6']

[pppery]

>>>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.

[Vgr255]

Works for me:

>>> class X(type): pass
...
>>> X(X)
<class 'type'>

[pppery]

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]

[pppery]

Also happens on 2.7, although you have to declare the metaclass using __metaclass__ = meta instead.

[Vgr255]

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.

[pppery]

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.

[pppery]

Ignore the first part of my previous comment; I improperly tested that.

[Vgr255]

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.

[stevendaprano]

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.

[pppery]

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.

[bitdancer]

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 :)

[stevendaprano]

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.

[eryksun]

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

[Vgr255]

+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.

[rhettinger]

Guido, this is your code. Do you care to opine?

[gvanrossum]

I don't recall writing that any more, but that fix looks right. (Though why write metatype == &PyType_Type rather than PyType_CheckExact(metatype)?)

[eryksun]

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".

[Vgr255]

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

[gvanrossum]

OK, the patch looks fine. Please treat this as a new feature (just in case) and only apply it to 3.6.

[Vgr255]

Fair enough. Should this get a note in What's new? Possibly in the "Changes in Python API" section.

[gvanrossum]

No, just an entry in Misc/NEWS.

--Guido (mobile)

[serhiy-storchaka]

Needed tests and updating the documentation.

[Vgr255]

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).

[berkerpeksag]

Please don't mark your own patch as 'commit review'.

[bitdancer]

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...)

[berkerpeksag]

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? :)

[Vgr255]

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.

[pppery]

Ping

[Vgr255]

Rebased patch so that it applies cleanly again.

[python-dev]

New changeset c3498187f998 by Berker Peksag in branch 'default':
Issue bpo-27157: Make only type() itself accept the one-argument form
https://hg.python.org/cpython/rev/c3498187f998

[berkerpeksag]

Thanks!