classification
Title: inspect.Signature no longer handles builtin classes correctly
Type: behavior Stage: needs patch
Components: Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: yselivanov Nosy List: larry, ncoghlan, python-dev, yselivanov
Priority: normal Keywords: patch

Created on 2014-02-01 03:42 by larry, last changed 2014-02-03 14:22 by larry. This issue is now closed.

Files
File name Uploaded Description Edit
sig_builtincls_01.patch yselivanov, 2014-02-02 02:21 review
sig_builtincls_02.patch yselivanov, 2014-02-02 05:57 fixed per Nick's review review
Messages (21)
msg209871 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-01 03:42
Yury: In revision 9433b380ad33 you changed inspect.Signature so that it cannot handle builtin classes.  Please fix it.

   >>> import _pickle
   >>> import inspect
   >>> str(inspect.signature(_pickle.Pickler))
   '()'
   >>> _pickle.Pickler.__text_signature__
   '(file, protocol=None, fix_imports=True)'

Those two strings should be the same.

I don't know any guaranteed way to tell a builtin class from a
user class.  So if you pass in a class, the best approach is to
do what it used to do: try from_builtin, and if it fails fail over
to the isinstance(obj, type) code.  You changed it to

    if _signature_is_builtin(obj):
        return Signature.from_builtin(obj)

This unambiguously returns the result from from_builtin.  Either
find a way that you can determine a class is a builtin 100%
reliably, or change this to *try* from_builtin but only return its
result if it's successful.

Your changes might have also caused #20471; that wasn't failing before.  I'm still investigating.
msg209879 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-02-01 08:44
OK, I'll take a look tomorrow. Don't think it's related  to #20471 though, as in there, if fails to find a signature for the 'builtin.object' (but I may be wrong).
msg209881 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-02-01 10:04
OK, there was no unit-test for this... looking into the problem, the first questing is: why is __text_signature__ is on the '_pickle.Pickler' object, not on '_pickle.Pickler.__init__'?
msg209883 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-01 10:18
Because slots like tp_init and tp_call don't have docstrings.  So it has to go in the class's docstring.
msg209886 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-01 11:20
I meant to say "slots like tp_new and tp_init".  But fwiw it's true of tp_call too.
msg209887 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-01 11:22
And, I don't see how your changes to inspect.py could have caused the failures on the buildbot either.  But, then, I don't see how *anything* could cause the failures on the buildbot.  And your changes to inspect.py happened at (I think) roughly the same time.  Correlation isn't causation, but it's all I have to go on right now.
msg209890 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-01 12:06
Stefan Krah suggests that the failure in 20473 is because that platform builds without docstrings, and the test requires them.  So that should be an easy fix.
msg209906 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-02-01 18:29
> Stefan Krah suggests that the failure in 20473 is because that platform builds without docstrings, and the test requires them.  So that should be an easy fix.

Good news ;) OK, I'll decorate the test.

> Because slots like tp_init and tp_call don't have docstrings.  So it has to go in the class's docstring.

So what's going on and why is it broken:

- from_builtin wasn't checking the incoming 'func' argument's type, it started to work with its '__text_signature__' right away. That was fixed.

- in 'inspect.signature' we checked 

     (isinstance(obj, _NonUserDefinedCallables) or ismethoddescriptor(obj) or
            isinstance(obj, type))

  so *classes* were tried in 'from_builtin' too.

And that's why it worked.  Any class (even used-defined in pure Python) that had '__text_signature__' was passed to the 'Signature.from_builtin' and it did the right job.

Now, I don't want to completely rollback my commits, as I still think that 'from_builtin' should strictly check what object is it working with, and raise appropriate exceptions. What we need to do is to separate parsing of '__text_signature__' into a private inspect module helper, so that 'from_builtin' becomes a tiny wrapper.  Then, in 'signature.inspect' we need to find a way of testing if the given object is a builtin class, and call the parsing helper on it if it has the '__text_signature__' attribute.

I'm looking into this.

BTW, are you sure we can't somehow add '__text_signature__' to builtin class' '__init__'?
msg209923 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-02-01 22:46
Those are C level descriptors, so we'd have to add new fields to the structs, and that's not going to happen at this stage of the release cycle.

However, there's also the fact that tp_new and tp_init are required to have the *same* signature, and those override the apparent signature of __call__ on the metaclass, so it's actually better to just document it once on the class.
msg209940 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-02-02 02:21
Please review the attached patch (sig_builtins_01.patch).

Some details:

- All parsing code from Signature.from_builtin was moved
in a separate helper '_signature_fromstr'

- Signature.from_builtin calls '_signature_fromstr'. All
its validation logic is untouched.

- 'inspect.signature' was tweaked a bit: when it's certain
that the object is a class and there is no user-defined
__init__ or __new__ or its meta's __call__, it traverses
the MRO to find non-empty __text_signature__. If it finds
one -- it returns with _signature_fromstr(). If not, it checks
if __init__ is type.__init__ or object.__init__. The last
check is a bit tricky -- the only way of doing that check
(I think) is to use __qualname__.

Since the patch is non-trivial, any review/comments would
be greatly appreciated.
msg209951 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-02-02 06:05
Second patch attached: sig_builtincls_02.patch, with a fix, that Nick suggested. Larry, I'd like you to take a quick look at it as well, before I commit it.
msg210005 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-02-02 18:09
Larry,

Quoting your reply from #17159:

> Also also, I remember specifically that the isinstance(type) code would fail builtin classes.

Could you please find an example of this?
msg210087 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2014-02-03 07:48
New changeset c19f5e4fdbe0 by Yury Selivanov in branch 'default':
inspect.signature: Add (restore) support for builtin classes #20473
http://hg.python.org/cpython/rev/c19f5e4fdbe0
msg210088 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-02-03 07:51
Committed.

We'll likely need to modify the code again in 3.5, once we settle the exact semantics of __text_signature__.
msg210089 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-03 07:54
What semantics are unsettled?
msg210090 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-02-03 07:58
For instance if __text_signature__ and __signature__ are present simultaneously which one should be used, or the use of '($' to specify bound-methods first parameters etc.
msg210091 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-02-03 08:01
And also right now, inspect.signature looks for '__text_signature__' when no used-defined __init__ was found. That's also going to be changed, but again, when __text_signature__ becomes a public documented API.
msg210092 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-03 08:03
I don't think __text_signature__ should ever be a documented public API.
msg210093 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-02-03 08:04
FWIW, I think the same.
msg210094 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-02-03 08:09
And also, reading Stefan in another issue, I'm a bit worried that
it may forcibly become a public API. Users tend to start using APIs 
before they are public, and that's especially true for python dunder 
attributes.

Maybe we should document '__text_signature__' and 'sig=', and 
explicitly state that it's a part of CPython private API, that likely 
to have some semantics/syntax changed in 3.5?
msg210128 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-03 14:22
> > Also also, I remember specifically that the isinstance(type) code
> > would fail builtin classes.
>
> Could you please find an example of this?

This was during the development of the original feature.  I changed the if statement for the from_builtin() call so it'd accept type objects too.  But it never got a chance to see any, because the check for type objects above it would see that it was a type, see that it was a builtin type, and raise an exception.  That's why I moved the if statement with the from_builtin() call to the top of the function, so it would get the first chance to examine the callable.

This was just historical context, and I'm sure you already solved the problem in an equivalent way.
History
Date User Action Args
2014-02-03 14:22:25larrysetmessages: + msg210128
2014-02-03 08:09:33yselivanovsetmessages: + msg210094
2014-02-03 08:04:17yselivanovsetmessages: + msg210093
2014-02-03 08:03:14larrysetmessages: + msg210092
2014-02-03 08:01:27yselivanovsetmessages: + msg210091
2014-02-03 07:58:01yselivanovsetmessages: + msg210090
2014-02-03 07:54:00larrysetmessages: + msg210089
2014-02-03 07:51:13yselivanovsetstatus: open -> closed
resolution: fixed
messages: + msg210088
2014-02-03 07:48:25python-devsetnosy: + python-dev
messages: + msg210087
2014-02-02 18:09:31yselivanovsetmessages: + msg210005
2014-02-02 06:05:59yselivanovsetmessages: + msg209951
2014-02-02 05:57:28yselivanovsetfiles: + sig_builtincls_02.patch
2014-02-02 02:21:23yselivanovsetfiles: + sig_builtincls_01.patch
keywords: + patch
messages: + msg209940
2014-02-01 22:46:41ncoghlansetmessages: + msg209923
2014-02-01 18:58:01yselivanovsetnosy: + ncoghlan
2014-02-01 18:29:27yselivanovsetmessages: + msg209906
2014-02-01 12:06:00larrysetmessages: + msg209890
2014-02-01 11:22:57larrysetmessages: + msg209887
2014-02-01 11:20:58larrysetmessages: + msg209886
2014-02-01 10:18:33larrysetmessages: + msg209883
2014-02-01 10:04:22yselivanovsetmessages: + msg209881
2014-02-01 08:44:38yselivanovsetmessages: + msg209879
2014-02-01 03:42:17larrycreate