inspect.Signature doesn't recognize all builtin types #64388
Comments
|
Stefan added some docstring text signatures by hand, only to discover that inspect.Signature still didn't recognize them. Specifically, decimal.Decimal.compare was unrecognized. This is a method_descriptor object, which is a type that isn't even exposed in types. Rather than go on a search-and-destroy mission for all these different builtin types, I'm going to change inspect.Signature so as a fallback at the end it says "if it has a __call__ and a valid __text_signature__, just use that". |
larryhastings
added
stdlib
|
Okay, learned some things.
|
|
Here's a patch that adds __text_signature__ support for three more builtin types: The patch also modifies inspect.Signature so it recognizes these types. |
|
Thanks, this is working here for the parameters. Is there a way to |
|
Yes, it's just Python syntax, so you'd use "->". However, you are not permitted to according to PEP-8: "The Python standard library will not use function annotations as that would result in a premature commitment to a particular annotation style." |
I tried that, but it didn't filter through to inspect.signature().
Ah, too bad. Return annotations are nice. |
|
Larry, Congrats on the amazing job you did with the arguments clinic. |
|
Yury: Thanks! I don't need any help right now though--just a review on this patch ;-) |
|
Larry, just a small thing.. Could you please add something like "Parameter = cls._parameter_cls" in the "from_builtin" method? (see the discussion in bpo-17373) |
|
Okay, life has gotten even more complicated. In another issue (bpo-20172) Zachary Ware pointed out that Argument Clinic needs to generate "self" parameters in the text string. But this complicates life for inspect.Signature, which needs to not publish the "self" parameter when it's been bound. I'm busy hacking up clinic.py to fix this right now and hope to have a patch later today. |
That's already supported, isn't it? >>> str(inspect.signature(F.a))
'(self, a)'
>>> str(inspect.signature(F().a))
'(a)' |
|
Not for builtins. |
|
Another issue is that with the patch applied help() is broken for certain forms from decimal import *
>>> print(setcontext.__doc__)setcontext(c) - Set a new default context. >>> help(setcontext)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/home/stefan/hg/cpython/Lib/_sitebuiltins.py", line 99, in __call__
return pydoc.help(*args, **kwds)
File "/home/stefan/hg/cpython/Lib/pydoc.py", line 1792, in __call__
self.help(request)
File "/home/stefan/hg/cpython/Lib/pydoc.py", line 1842, in help
else: doc(request, 'Help on %s:', output=self._output)
File "/home/stefan/hg/cpython/Lib/pydoc.py", line 1578, in doc
pager(render_doc(thing, title, forceload))
File "/home/stefan/hg/cpython/Lib/pydoc.py", line 1571, in render_doc
return title % desc + '\n\n' + renderer.document(object, name)
File "/home/stefan/hg/cpython/Lib/pydoc.py", line 358, in document
if inspect.isroutine(object): return self.docroutine(*args)
File "/home/stefan/hg/cpython/Lib/pydoc.py", line 1323, in docroutine
signature = inspect.signature(object)
File "/home/stefan/hg/cpython/Lib/inspect.py", line 1551, in signature
raise ValueError(msg)
ValueError: no signature found for builtin function <built-in function setcontext>Perhaps this form of docstrings is discouraged (I used it because it looks nice |
|
Here's an updated patch. I tried to "do it right" which wound up being a huge amount of work in Clinic. The actual change to inspect.Signature was really easy, once I understood everything. The churn in the .c files is because Clinic now uses the self converter's type for the parsing function, and (obviously) because it's now generating "self" in the signatures as appropriate. Fun trivia: the "self" parameter to a builtin is always a positional-only parameter, even if all other argument processing for the function is PyArg_ParseTupleAndKeywords. I think this patch marks the first time inspect.Signature will ever mark a parameter as positional-only! |
Yeah. We discussed this briefly in bpo-19674. I wanted to use a marker that wasn't The Convention That People Have Used For Decades but I felt overruled. I want to revisit it for precisely the reason you cite. (I just realized, Sphinx autodoc is irrelevant, as if the string is legitimate it be stripped off the docstring anyway.) |
|
Dang it, I forgot to add the second patch. Here it is. |
|
Updated the patch. (Diff #2 apparently didn't apply cleanly, so we didn't get a review link.) Old-guard core devs: I'm *really* desperate for a review of this patch. You don't have to review everything thing, just these files: I can get a different reviewer for the other files. But I worry about touching these tender bits of the type system and I want to make sure that a) I haven't done something awful, and Just that part of the diff is 345 lines, and it's pretty regular. I'd be surprised if it took you a whole hour. If you have any questions email me, I'd be thrilled to answer 'em if it means I can get this patch checked in. Maintaining the patch is overhead that I just don't need during the Derby. |
|
Nick, could you maybe review this? |
|
Whoever does the review, could you post here? I feel bad enough asking y'all, maybe we don't need multiple people doing it ;-) |
|
Looking now. |
|
Larry clarified that the signature(min) change in this patch was actually restoring the Python 3.3 behaviour, so I think with the addition of some relevant test cases to the test suite, go for it :) |
|
Now looking. Note that a few parts of the patch no longer cleanly apply: hg import --no-c http://bugs.python.org/review/download/issue20189_10572.diff |
|
Updating / merging / resolving now, but it will take me a few minutes. |
|
Here's a fresh patch against trunk. It applies cleanly against current tip (725bc24f5492). |
|
I limited myself to the four files you mentioned, and they look totally fine. Together with Nick's view you should have enough core review now, right? |
|
I do. Thanks for your time! |
|
A few issues with this patch:
"""
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "P:\ath\to\cpython\lib\_sitebuiltins.py", line 99, in __call__
return pydoc.help(*args, **kwds)
File "P:\ath\to\cpython\lib\pydoc.py", line 1792, in __call__
self.help(request)
File "P:\ath\to\cpython\lib\pydoc.py", line 1842, in help
else: doc(request, 'Help on %s:', output=self._output)
File "P:\ath\to\cpython\lib\pydoc.py", line 1578, in doc
pager(render_doc(thing, title, forceload))
File "P:\ath\to\cpython\lib\pydoc.py", line 1571, in render_doc
return title % desc + '\n\n' + renderer.document(object, name)
File "P:\ath\to\cpython\lib\pydoc.py", line 356, in document
if inspect.ismodule(object): return self.docmodule(*args)
File "P:\ath\to\cpython\lib\pydoc.py", line 1142, in docmodule
contents.append(self.document(value, key, name))
File "P:\ath\to\cpython\lib\pydoc.py", line 358, in document
if inspect.isroutine(object): return self.docroutine(*args)
File "P:\ath\to\cpython\lib\pydoc.py", line 1323, in docroutine
signature = inspect.signature(object)
File "P:\ath\to\cpython\lib\inspect.py", line 1551, in signature
raise ValueError(msg)
ValueError: no signature found for builtin function <built-in function abort>
"""
"""
>>> help(pickle.dump)
Help on built-in function dump in module _pickle:dump(module, obj, file, protocol=None, *, fix_imports=True)
Write a pickled representation of obj to the open file object file.<etc.> >>> pickle.dump.__text_signature__
'(module, obj, file, protocol=None, *, fix_imports=True)'
"""
Clinic block: Return the fully-qualified path for the file that contains module. <etc> Traceback:
"""
P:\ath\to\cpython>PCbuild\python_d.exe Tools\clinic\clinic.py Modules\_winapi.c
Error in file "Modules\_winapi.c" on line 1299:
Exception raised during parsing:
Traceback (most recent call last):
File "Tools\clinic\clinic.py", line 1099, in parse
parser.parse(block)
File "Tools\clinic\clinic.py", line 2283, in parse
self.state(None)
File "Tools\clinic\clinic.py", line 3022, in state_terminal
self.function.docstring = self.format_docstring()
File "Tools\clinic\clinic.py", line 2847, in format_docstring
assert isinstance(parameters[0].converter, self_converter)
AssertionError
""" |
|
Problems 1 (ValueError from help(os)) and 2 ('module' as first param) were simple fixes: The attached patch fixes problem 1 (and is probably worth doing anyway, since inspect.signature has the ability to raise ValueError, and (IMO) help(<module>) should never raise an exception. Problem 2 is just a matter of adding self.show_in_signature = False to the first param of module-level functions. I've left a review comment at the right line. Problem 3 would also be fixed by re-adding 'module' to c_keywords, but since you want it to work without that, that's out, and I'm not sure what the proper fix would be otherwise. The current patch no longer applies cleanly after bpo-20287; I would just post an updated version of the entire patch with my changes as well, but the merge is not trivial and I don't want to screw it up :) |
|
Your fixes for #1 and #2 were fine, I've incorporated them into the patch. I'll update the diff after I've added the tests Nick suggested. The assertion failure in #3 will also be gone, replaced with a failure: You can't have two parameters named module! The problem is that we silently inserted a self converter for the first argument, and because this is a module-level function, that "self" parameter is naturally named "module". I have a fix in mind for this: basically to teach Argument Clinic that the parser function and impl function have different namespaces, and to map names in the first to the second. So, you could have a parameter named "args", and Clinic would notice, and rename the variable in the parser function "args_value" or something, and then pass it in in the right spot. Once I've done that, it'd be easy to make it also rename the secret self converter name to "_module" or something. Anyway, long story short, let's not try to fix #3 in this patch. |
|
Larry Hastings added the comment:
That sounds fine. _winapi is the only place I've seen that has a |
|
Here is the hopefully-final patch for this issue. I incorporated the suggested changes from Zachary Ware. Also I fixed some "cls" parameters that were leaking into the signatures. I think this is ready for checkin! |
|
Argh. I lost 1.5 day's worth of work on revision 6 of this patch last I have more C fixes by the way:
|
|
At last, my refreshed patch. Changes from the previous patch:
Boy am I emotionally ready to check this thing in. |
|
Ok, I found the source of the real issue alluded to in the misguided comment about the 'cls' -> 'type' change that I left on Rietveld. I was under the impression that with that change, 'help(datetime.datetime.now)' would show a signature of 'now(type, tz=None)'. In actual fact, 'str(inspect.signature(datetime.datetime.now))' (correctly) returns (tz=None), and that's what help (incorrectly) displays. To properly match the help output of Python-implemented methods, pydoc will need to add in the 'self' or 'cls' parameter somehow. However, I think that situation can be resolved in another issue in favor of getting this in, with the few issues I pointed out on Rietveld fixed. |
|
I'm happy to resolve it before checking in the patch. If people said "eww" then I'll back it out. Nobody said "eww" But let's talk about it a little! -- First, the name *is* visible in Python, if you examine the unbound >>> str(inspect.signature(_datetime.datetime.__dict__['now']))
`(<type>, tz=None)`
>>> help(_datetime.datetime.__dict__['now'])
# ... shows help, including <type> parameter in the signatureThe angle-brackets are Signature's way of denoting a positional-only (The ugly angle brackets are being addressed in another issue.) -- Second, I'm surprised at the behavior of help. I hadn't realized >>> class C:
... @classmethod
... def wife(cls, a, b):
... print(cls, a, b)
...
>>> help(C.wife)That shows "cls" as part of the signature. But inspect.signature >>> str(inspect.signature(C.wife))
'(a, b)'FWIW help on a callable bound using functools.partial shows you (help() only goes one level deep on this by the way. If you have Anyway, I think it's odd, but I'm not here to change the behavior -- Third, it's inconvenient to use "type" as an identifier in Python code, We don't have these restrictions in C. So actually "class" would -- Fourth, I already called the first parameter "type" for __new__ At the very least, I want Argument Clinic to use one name -- Fifth, up until Argument Clinic, most callables had "PyObject *self" I proposed generating "PyModuleDef *module" there instead, Guido said |
|
A little more on consistency and inconsistency. I count 109 tp_new callback functions in CPython, and they overwhelmingly call the first parameter "PyTypeObject *type" (93 instances). In second place is "PyObject *self" (9 instances), which is flat-out wrong. I count 21 METH_CLASS callback functions in CPython; they prefer calling the first parameter "PyObject *cls" (16 instances). In second place is "PyTypeObject *type" (3 instances). Both callbacks are class methods. And both callbacks are passed the *exact same object* for their first parameter, the PyTypeObject * representing that type. I can see no good reason why they should have different names in different callbacks. There's no practical or semantic difference between the two. I suspect it's something silly like legacy code / copying and pasting / force of habit, perhaps carried over from the days before type/class unification. |
|
Note that tp_new is a static method, not a class method (the type creation |
I admit I didn't know that. But from a practical perspective, surely you agree that tp_new walks and quacks like a class method? That I, as an author of an extension type, should think of it as such? |
|
It doesn't act like a class method, though, it acts like a static method: >>> int.__new__()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: int.__new__(): not enough arguments
>>> int.__new__(int)
0You have to *write* __new__ and tp_new as if they were class methods (because the type machinery expects you to do so), but you have to *call* them like static methods if you're invoking them directly for some reason. |
|
(Also, I can't give you a solid reason for *why* it's like that - Guido just wrote it that way, and the type machinery is hairy enough that I have no intentions of second guessing him on that one) |
|
Oh, yes, now I remember - it *has* to be that way, otherwise upcalls from subclass __new__ methods don't do the right thing (int.__new__(MyInt), etc), just as you need to pass the current type in explicitly for cooperative super calls. This is perhaps *the* most obscure design detail of the type system that I'm aware of - I have to go scratching around in my brain for the reason every time it comes up, which is fortunately almost never :) |
|
Okay, one more diff. I have high hopes for this, but then I had high hopes yesterday. Nick, could you review the PyTypeObject changes in this patch? Obviously I'd love a review of the whole thing, but if you can only make a little time, the crucial part is the "delta from patch set" 5 for typeobject.c. First thing: I must never have run the unit test suite before cutting the diff yesterday, because I did today and there were a bunch of problems. That's clowny and I apologize. But it's fixed now, and I assure you, there's no way I would have actually checked this in without running the test suite immediately before. Here's what changed today: Core:
Lib and tests:
Tools:
|
|
Scanned the whole patch, especially the type changes. This looks like a solid approach to me. For 3.5, PEP-457 might want to consider proposing a tp_sig slot and splitting the signature out at type creation time rather than on attribute lookup. The current dynamic approach is fine for 3.4, though. |
|
Okay, I'm checking this beast in. Hooray! Thanks for your reviews, everybody! -- I thought it was still possible to introduce objects into Python at runtime without calling PyType_Ready on their type. If that's true, Is that no longer allowed as of 3.4? Are all types required to be |
|
There are probably still ways to do it, but we don't *support* doing it (and I'm pretty sure we've fixed them all in the builtins and stdlib). However, yes, that's another good reason to be conservative in only doing the split into signature+doc at attribute lookup time. |
|
I just realized, I forgot to fix the bug Zach reported, where help(bound_thing) should still show the class or self parameter. |
|
New changeset 85710aa396ef by Larry Hastings in branch 'default': |
|
Phew! Thanks again, everybody! |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: