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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
Date: 2016-05-30 05:41 |
Guido, this is your code. Do you care to opine?
|
msg266677 - (view) |
Author: Guido van Rossum (gvanrossum) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
Date: 2016-05-30 16:23 |
No, just an entry in Misc/NEWS.
--Guido (mobile)
|
msg266706 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) *  |
Date: 2016-05-30 16:28 |
Needed tests and updating the documentation.
|
msg266712 - (view) |
Author: Anilyka Barry (abarry) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
Date: 2016-08-14 22:00 |
Rebased patch so that it applies cleanly again.
|
msg273089 - (view) |
Author: Roundup Robot (python-dev)  |
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) *  |
Date: 2016-08-19 08:04 |
Thanks!
|
|
Date |
User |
Action |
Args |
2022-04-11 14:58:31 | admin | set | github: 71344 |
2016-08-19 08:04:09 | berker.peksag | set | status: open -> closed resolution: fixed messages:
+ msg273090
stage: patch review -> resolved |
2016-08-19 08:03:35 | python-dev | set | nosy:
+ python-dev messages:
+ msg273089
|
2016-08-14 22:00:38 | abarry | set | files:
+ type_one_argument_5.patch
messages:
+ msg272697 |
2016-07-13 22:06:28 | ppperry | set | messages:
+ msg270348 |
2016-05-31 02:13:04 | abarry | set | files:
+ type_one_argument_4.patch
messages:
+ msg266729 |
2016-05-31 01:59:01 | berker.peksag | set | messages:
+ msg266728 |
2016-05-31 00:42:10 | r.david.murray | set | messages:
+ msg266726 |
2016-05-31 00:08:25 | berker.peksag | set | nosy:
+ berker.peksag
messages:
+ msg266722 stage: commit review -> patch review |
2016-05-30 17:57:46 | abarry | set | files:
+ type_one_argument_3.patch |
2016-05-30 17:57:36 | abarry | set | messages:
+ msg266712 stage: test needed -> commit review |
2016-05-30 17:32:43 | abarry | set | messages:
- msg266711 |
2016-05-30 17:31:19 | eryksun | set | messages:
- msg266710 |
2016-05-30 17:26:58 | abarry | set | messages:
+ msg266711 |
2016-05-30 17:23:42 | eryksun | set | messages:
+ msg266710 |
2016-05-30 16:28:13 | serhiy.storchaka | set | nosy:
+ serhiy.storchaka
messages:
+ msg266706 stage: commit review -> test needed |
2016-05-30 16:23:38 | gvanrossum | set | messages:
+ msg266705 |
2016-05-30 16:10:47 | abarry | set | stage: patch review -> commit review messages:
+ msg266702 versions:
+ Python 3.6, - Python 2.7, Python 3.4 |
2016-05-30 16:06:43 | gvanrossum | set | messages:
+ msg266701 |
2016-05-30 13:37:39 | abarry | set | files:
+ type_one_argument_2.patch
messages:
+ msg266693 |
2016-05-30 08:08:57 | eryksun | set | messages:
+ msg266684 |
2016-05-30 05:52:10 | gvanrossum | set | messages:
+ msg266677 |
2016-05-30 05:41:01 | rhettinger | set | nosy:
+ rhettinger, gvanrossum messages:
+ msg266676
|
2016-05-30 03:58:31 | abarry | set | stage: needs patch -> patch review |
2016-05-30 03:57:45 | abarry | set | files:
+ type_one_argument_1.patch keywords:
+ patch messages:
+ msg266673
stage: needs patch |
2016-05-30 03:17:17 | eryksun | set | nosy:
+ eryksun messages:
+ msg266667
|
2016-05-30 03:01:26 | steven.daprano | set | messages:
+ msg266666 |
2016-05-30 01:51:02 | r.david.murray | set | nosy:
+ r.david.murray
messages:
+ msg266663 stage: resolved -> (no value) |
2016-05-30 01:43:22 | ppperry | set | status: closed -> open resolution: works for me -> (no value) messages:
+ msg266662
|
2016-05-30 01:29:05 | steven.daprano | set | status: open -> closed
nosy:
+ steven.daprano messages:
+ msg266660
resolution: works for me stage: resolved |
2016-05-30 00:39:08 | abarry | set | messages:
+ msg266654 stage: resolved -> (no value) |
2016-05-30 00:27:53 | ppperry | set | messages:
+ 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:39 | ppperry | set | messages:
+ 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:08 | abarry | set | messages:
+ msg266651 |
2016-05-29 23:26:03 | ppperry | set | messages:
+ msg266650 versions:
+ Python 2.7 |
2016-05-29 23:23:40 | ppperry | set | status: 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:05 | abarry | set | status: open -> closed
nosy:
+ abarry messages:
+ msg266645
resolution: works for me stage: resolved |
2016-05-29 22:46:08 | ppperry | create | |