Created on 2016-05-29 22:46 by ppperry, last changed 2016-08-19 08:04 by berker.peksag. This issue is now closed.
>>>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.
Works for me: >>> class X(type): pass ... >>> X(X) <class 'type'>
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]
Also happens on 2.7, although you have to declare the metaclass using `__metaclass__ = meta` instead.
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.
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.
Ignore the first part of my previous comment; I improperly tested that.
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.
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.
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.
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 :)
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.
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
+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.
Guido, this is your code. Do you care to opine?
I don't recall writing that any more, but that fix looks right. (Though why write `metatype == &PyType_Type` rather than `PyType_CheckExact(metatype)`?)
> 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".
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
OK, the patch looks fine. Please treat this as a new feature (just in case) and only apply it to 3.6.
Fair enough. Should this get a note in What's new? Possibly in the "Changes in Python API" section.
No, just an entry in Misc/NEWS. --Guido (mobile)
Needed tests and updating the documentation.
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).
Please don't mark your own patch as 'commit review'.
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...)
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? :)
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.
Ping
Rebased patch so that it applies cleanly again.
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
Thanks!
stage: patch review -> resolved
messages: + msg272697
messages: + msg266729
messages:
+ msg266722
stage: commit review -> patch review
messages:
+ msg266706
stage: commit review -> test needed
messages: + msg266693
stage: needs patch
messages:
+ msg266663
stage: resolved -> (no value)
nosy:
+ steven.daprano
messages:
+ msg266660
resolution: works for me
stage: resolved
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
nosy:
+ abarry
messages:
+ msg266645
resolution: works for me
stage: resolved