New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove explicit type check from inspect.Signature.from_function() #61361
Comments
I can't see a reason why Signature.from_function() should explicitly check the type of the object being passed in. As long as the object has all required attributes, it should be accepted. This is specifically an issue with Cython compiled functions, which are not Python functions but look the same. |
This patch removes the type check from Signature.from_function() and cleans up the type tests in signature() to use whatever the inspect module defines as isfunction() and isbuiltin(), so that it becomes properly monkey-patchable. It also adds a test that makes sure the signature relies only on the four relevant special attributes, not on the type of the object being inspected. |
Patch looks good, but I’m worried about the change from TypeError to AttributeError in a stable version. Could you also make clear that all function-like objects are accepted in the doc? |
The method doesn't seem to be documented, and I'm not sure if the docstring really benefits from this lengthy addition. Anyway, here's a patch that includes the docstring update. The exception could be kept the same if we catch an AttributeError and explicitly raise a TypeError instead. |
My first concern is whether this is a behavior issue that can be fixed in the next 3.3 release or an enhancement request that should wait for 3.4. If Signature.from_function were documented in the manual with the current limitation, this issue would definitely be an enhancement request. But it is not even mentioned in the manual. I presume this is intentional as it is pretty clearly not meant to be called directly but only from signature(). Perhaps its should have been given a leading underscore to mark it as private. The docstring does say 'python function'. However, its type check is redundant with the guard in signature before its call. So in normal usage, it can never fail. Moreover, the limitation is completely unnecessary even for pure Python code, as shown the the ease of creating a funclike class for the test. It is contrary to Python policy and stdlib practice. So in a sense, it is a bug. My conclusion: the proposed change is an enhancement bugfix (or bugfix enhancement?) for an undocumented private function. So I think it ok for 3.3, but would not blame anyone who disagreed. --- --- |
Fine with me. Updated patch attached. |
Any more comments? Any objections to applying the last patch? Anyone ready to do it? |
I certainly think the patch is ok on the principle. I'll let someone else pronounce on the details :-) |
I am puzzled by this part of the patch:
With 3.3, I get "TypeError: 42 is not a callable object", which I consider correct, and which should not be changed by the two changes to signature(). So both the old test, missing '42' (how does it pass?) and the new version, with extraneous '.*' look wrong. I am also puzzled by the 'from None' part in While I remember that being in the pydev discussion and while "raise XyzError from None" executes, it does not seems to be documented for 3.3 in 7.8. The raise statement. (Should this be another issue?) In fact, that section says " if given, the second expression must be another exception class or instance", which excludes None. |
|
On Mon, Feb 11, 2013 at 11:46 AM, Terry J. Reedy <report@bugs.python.org> wrote:
That's a separate docs bug - it seems we missed the necessary language |
I opened separate issue bpo-17188: |
Updated patch to also fix a little typo in the docstring (lower case "python"). |
Stefan, is this one still relevant? If it is, I can review it (I have a couple of things to improve) and push in 3.4, as it doesn't really change any API. |
Yes, the situation didn't change. |
Stefan, I'm attaching a reviewed patch--was a bit easier to rebase it on the new code and fix the remaining issues. Please review. BTW, do cython functions have __name__ attribute? |
LGTM, thanks! And works nicely with Cython: """ def test_isfunction():
"""
>>> test_isfunction()
True
"""
return inspect.isfunction(test_isfunction)
def test_signature():
"""
>>> test_signature() # doctest: +ELLIPSIS
<inspect.Signature object ...>
"""
return inspect.signature(test_signature)
"""
Yes, they have everything that can possibly be emulated (although not |
Attached is a second patch (sig_func_ducktype_02.patch), with a bit improved tests (and a small bug fixed). Larry, do you think it's OK if I merge this one in 3.4? Technically, it's a feature that we start to support Cython functions in inspect.signature, but from the standpoint of the API everything is the same. |
pretty please? :) |
Tell you what. If this *specifically* is a fix for Cython, please add a some text saying that to the comment for the duck-type test function. If you do that, and assuming that test_inspect still passes, then you may check it in. |
Third version of the patch is attached (sig_func_ducktype_03.patch). Changes:
Stefan, please take a look. I want to make sure, that the fact that we won't accept classes that look like a function, is OK with you. |
Tried it, works for me. Explicitly rejecting classes is a good idea, IMHO, as is requiring that any function-like object must be callable, obviously. |
Yeah, I think it's good to restrict this duck-typing as much as possible. Committing the patch. |
New changeset 32a660a52aae by Yury Selivanov in branch 'default': |
OK, closing this one. |
Interesting, wasn't aware of that. Then let's wait what will happen when
What I'm saying is that the existing function introspection API would have
I don't think that's possible any more once it's out in the wild. The |
Yes. The "inheritance" of Cython's function type from PyCFunction is a pure Cython's functions are neither instances of PyFunction nor of PyCFunction. This change is redundant since BuiltinFunctionType (which isbuiltin() tests |
The code is more understandable, though. Also, please let's stop posting anything unrelated in this issue. |
What "existing function introspection API"? I wasn't aware there was an existing mechanism to provide signature metadata for builtin functions. |
Not for builtin functions, but it's unclear to me why the API of builtin I agree with Yury, however, that this discussion is unrelated to this ticket. |
I really don't follow you. You seem to be saying that __text_signature__ is a bad idea, and keep talking about existing Be specific. Let's say we remove __text_signature__. How do we If __text_signature__ is redundant with existing APIs, then we should remove it now before 3.4 ships. |
Python 3.4.0b3+ (default:19d81cc213d7, Feb 1 2014, 10:38:23)
[GCC 4.8.1] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> def test(a,b,c=None): pass
>>> set(dir(test)) - set(dir(len))
{'__get__', '__code__', '__globals__', '__dict__', '__defaults__', '__kwdefaults__', '__annotations__', '__closure__'}
>>> test.__kwdefaults__
>>> test.__defaults__
(None,)
>>> test.__annotations__
{}
>>> test.__code__
<code object test at ..., file "<stdin>", line 1>
>>> dir(test.__code__)
['__class__', '__delattr__', '__dir__', '__doc__', '__eq__', '__format__', '__ge__', '__getattribute__', '__gt__', '__hash__', '__init__', '__le__', '__lt__', '__ne__', '__new__', '__reduce__', '__reduce_ex__', '__repr__', '__setattr__', '__sizeof__', '__str__', '__subclasshook__', 'co_argcount', 'co_cellvars', 'co_code', 'co_consts', 'co_filename', 'co_firstlineno', 'co_flags', 'co_freevars', 'co_kwonlyargcount', 'co_lnotab', 'co_name', 'co_names', 'co_nlocals', 'co_stacksize', 'co_varnames']
>>> test.__code__.co_varnames
()
>>> test.__code__.co_varnames
()
>>> test.__code__.co_varnames
('a', 'b', 'c')
>>> test.__code__.co_kwonlyargcount
0
>>> test.__code__.co_name
'test' But again, this is not covered by the subject of this ticket. I also don't think it's a good idea to delay Py3.4 until this discrepancy is fixed. |
"""
>>> test.__code__.co_varnames
()
>>> test.__code__.co_varnames
()
>>> test.__code__.co_varnames
('a', 'b', 'c')
""" copy&pasto, please ignore the first two... :o) |
Yyou have just answered the question "How do you determine signature information for functions written in Python?". A shorter way to express this answer: functions written in Python are implemented as a "function object" (in C, PyFunctionObject), which internally has a reference to a "code object" (PyCodeObject). These two objects collectively contain all the information you'd need to determine the function's signature in Python. However, builtin functions don't use either of these objects. Builtin functions are implemented with a "builtin code object" (PyCFunctionObject) which doesn't have any of the metadata you cited. So that doesn't answer my question. Nor is it practical to implement a builtin function using a "function object" and a "code object". So I'll ask you again:
|
That exactly is the bug. I think we should take the further discussion offline, or move it to a new ticket. |
Stefan is suggesting that rather than emulating the Python signature line However, I'm not sure *how* generating and storing an assortment of hard to |
I understand Stefan to (reasonably) want 1 api instead of 2. He imagined that the only way to do that would be for everything to at least partially imitate the old methods. Larry and Nick are suggesting instead that everything should adopt the implementation-independent new method. Sounds right to me. So further discussion should be about PEP-457, elsewhere. |
Absolutely. However, I guess the underlying reasoning here is that there As soon as there is easy-to-use support for annotations in C implemented |
Oh, well... isbuiltin(cyfunction) *does* return False. However, ismethoddescriptor(cyfunction) returns True, because Cython's functions bind as methods and thus have a "__get__()" (but no __set__()). Therefore, _signature_is_builtin(cyfunction) returns True, and the Signature.from_builtin(cyfunction) code path is taken and fails. So, once again: If the intention is to test that a callable has a "__text_signature__", then the code should test for the existance of "__text_signature__". It should *not* try to test for types that may or may not have such an attribute, and then just fail if it surprisingly does not find it. |
BTW, ismethoddescriptor() is an exceedingly broad test, IMHO. I wonder if it should even be relied upon for anything in inspect.py. |
Since this is a problem in Cython, not in CPython, maybe you can fix it in Cython? |
Patch sig_cython_03.patch is still applicable (it probably won't apply to the current repo, but I can fix that). It should solve all these problems once and for all. If Larry agrees, I can still merge it in. |
Unfortunately this is something I do *not* want to change. |
I'm actually considering that. Now that Signature.from_function() allows |
Oh, sound like a big hack. Stefan, Larry, Please take a look at the new patch 'sig_cython_latest' The change is really minimal, I think we can still push this in 3.4. |
Well, it's certainly a bunch of overhead (even assuming that "inspect" will I just tried it, it adds some 20 lines of C code but works ok.
I'm certainly ok with it, given that I had already asked for that a while ago. |
New changeset d6aa3fa646e2 by Yury Selivanov in branch 'default': |
Stefan, I committed one more fix in signature -- now cython I'm not sure if Larry accepts this in 3.4.0 though, |
I'm closing this issue. Please do not reopen it, and create a new one if necessary. |
Stefan: does the checkin in d6aa3fa646e2 fix your problem? I'm willing to cherry-pick for this but I'd prefer to only have to do it once. |
I tested it and it works, so I could take the simple route now and say "yes, it fixes the problem", but it's actually no longer required because I already added a "__signature__" property to Cython's functions. However, as Yury noted, that's a hack because inspect.py can do the same thing way more efficiently with his latest change, so it allows me to reconsider and potentially get rid of it again. Long story short, it works and does the right thing, so I'm happy to see this change go into Py3.4. |
New changeset fa5127cdfe9d by Yury Selivanov in branch '3.4': |
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: