classification
Title: Change the text signature format (again) to be more robust
Type: enhancement Stage: resolved
Components: Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: larry Nosy List: georg.brandl, jcea, larry, ncoghlan, python-dev, serhiy.storchaka, yselivanov, zach.ware
Priority: release blocker Keywords: patch

Created on 2014-02-06 14:09 by larry, last changed 2014-11-27 12:33 by larry. This issue is now closed.

Files
File name Uploaded Description Edit
larry.even.newerer.signature.syntax.1.diff larry, 2014-02-06 14:15 review
larry.even.newerer.signature.syntax.2.diff larry, 2014-02-07 06:44 review
larry.even.newerer.signature.syntax.3.diff yselivanov, 2014-02-07 18:55 review
larry.even.newerer.signature.syntax.4.diff yselivanov, 2014-02-07 19:10 review
issue20530_fixed_assertion.diff ncoghlan, 2014-02-09 03:56 Fixes an assertion failure in typeobject review
Messages (14)
msg210380 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-06 14:08
Following a new discussion of the format of the text signature, I decided to go with this:
    <name-of-function>(...)\n--\n\n

See here for the discussion:
   https://mail.python.org/pipermail/python-dev/2014-February/132271.html

Patch attached implementing this approach.  Can I get a review?  I want this in before tagging for rc1 on Saturday.

Other changes:

* clinic.py now generates the "/" marker in sigantures to denote
  positional-only parameters.

* clinic.py --make is a lot faster; it prescans the file for any
  clinic block signatures, and if it doesn't see any it skips the file.

* When generating the docstring for a function with optional groups,
  the signature generated is not intended to be machine-readable.
  So it omits the "$", the "/", and the "--" markers.  (See
  Modules/_cursesmodule.c for the one and only example.)
msg210382 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-06 14:15
Whoops, forgot to fix all the breakage in clinic_test.py.  It's good now.
msg210387 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-02-06 14:35
> Whoops, forgot to fix all the breakage in clinic_test.py.  It's good now.
Ah, you re-uploaded the patch and reset my code review comments. I'll repeat them here, just in case.

Reviewed the patch.

- In one of the C files I saw this signature: "(/)" -- probably should be just "()"

- signature for 'type' builtin was '(object_or_name, bases, dict)', which is a correct signature (minus the groups/optional parameters). In this patch it's a ValueError. Can we restore the prior behaviour? If not, then test_signature_on_class_without_init is incorrect now, as it should throw a ValueError for metaclasses.

- and some other minor stuff
msg210438 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-07 06:44
Thanks for noticing the "(/)", that's fixed.

Yes, the signature for type() was wrong.  type() can accept either one parameter or three parameters--in other words, it uses optional groups.  And we can't represent optional groups in an inspect.Signature signature in 3.4.  And I assert that it's better to not have a signature than to have a wrong signature.  So I've removed it.

I'm interested in your "other minor stuff".  I'll try and leave the patch alone this time :)

If you apply diff #2, you'll have a failure in test_inspect.  This is because there's a bug in inspect.Signature that it would take too long to fix, and I have to leave right now-ish.  I changed the tests so they expect the correct results, but they get the wrong results at the moment.
Hopefully we can fix this really quickly.

--

Yuri: This change uncovered a lurking bug in inspect.Signature that I'm hoping you can fix.  I could probably figure it out given enough time but I won't have time to look at it for about 24 hours.

Now that type() doesn't have a signature, I have discovered that some of the logic in Signature is wrong.  

   class C(type): pass
   print(str(inspect.signature(C)))

The signature of that *should* be the same as type().  But inspect.signature() reports the signature as '()'.

The reason this happens: inspect.Signature gets to the "for base in mro" case for handling classes.  The first base it tries is type(), but type() doesn't have a public signature so it keeps going.  The next class in the MRO is object(), which has a signature of "()", so it uses that.

It shouldn't keep going!  I'm 99% certain that the first entry in the MRO will always be callable.  (Is it possible to have a type in Python that isn't callable?)

I *think* what it should do is: simply try the first entry in the MRO.  If that has a signature, return it.  If it doesn't have a signature, inspect.Signature should raise ValueError.

I also *think* that all the code after this comment:
    # No '__text_signature__' was found for the 'obj' class.
should be removed.  If C doesn't define its own __new__ or __init__, and its base class doesn't have __call__, then the signature of C *is* the signature of its base class.  If inspect.Signature can't read that signature, then it can't return a signature for C.
msg210502 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-02-07 17:48
New patch still gives an assertion failure at line 153 of typeobject.c when trying to get __text_signature__ from anything that should have one (on Windows, at least).

Python 3.4.0b3+ (default, Feb  7 2014, 11:43:04) [MSC v.1600 32 bit (Intel)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> type.__text_signature__
>>> object.__text_signature__
Assertion failed: *end == ')', file ..\Objects\typeobject.c, line 153
msg210524 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-02-07 18:55
Larry,

I'm attaching a revised version of the patch -- larry.even.newerer.signature.syntax.3.diff.

Changes:

1. test_signature_on_class_without_init was fixed, 
inspect.signature was fixed as well. Now, for

   class Meta(type): pass

"inspect.signature(Meta)" will raise a ValueError.

> I also *think* that all the code after this comment:
>    # No '__text_signature__' was found for the 'obj' class.
> should be removed.

You are right, but I'd prefer to leave that code until 3.5, when we have correct signature for 'type'.

> The reason this happens: inspect.Signature gets to 
> the "for base in mro" case for handling classes.  
> The first base it tries is type(), but type() doesn't 
> have a public signature so it keeps going.  The next 
> class in the MRO is object(), which has a signature 
> of "()", so it uses that.

> It shouldn't keep going!  I'm 99% certain that the first 
> entry in the MRO will always be callable.  (Is it possible 
> to have a type in Python that isn't callable?)

Unfortunately, no, we need to traverse the MRO. See the
"test_signature_on_builtin_class" unit test, for instance.

The way I solved the current bug, is to fixing the code from
traversing the full MRO, to traversing the MRO without the
last item -- 'object' builtin.

2. _strip_non_python_syntax -> _signature_strip_non_python_syntax
Minor fixup, just to have all signature helper functions 
in inspect.py follow one notation.

3. test_strip_non_python_syntax -> was moved to another
test case -- TestSignaturePrivateHelpers

4. I also fixed one thing that Zachary found: unused 's' variable
in the '_signature_fromstr' method.

Here's a quick summary of changes (diff of the diffs,
omitting some changes in tests): 
https://gist.github.com/1st1/8869242
msg210529 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-02-07 19:10
One more patch (v4)

I fixed the commented out 'ThisWorksNow' test.
And also 'inspect.signature' now raises a bit
more coherent exception for that particular case.
msg210716 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-02-09 03:56
Updated patch fixes an assertion failure in typeobject.c (end doesn't point at the ")", it points at the newline immediately after it).
msg210719 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-09 04:27
Nick: I made the same fix on the plane, except I also added an assert that end - start >= 2.  ;-)

I'm going to go through everyone's feedback, and if there are any significant changes I'll post a new patch, otherwise I think this patch is basically ready.
msg210723 - (view) Author: Roundup Robot (python-dev) Date: 2014-02-09 06:16
New changeset 29d9638bf449 by Larry Hastings in branch 'default':
Issue #20530: Argument Clinic's signature format has been revised again.
http://hg.python.org/cpython/rev/29d9638bf449
msg210724 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-09 06:23
I made a couple final tweaks to what was essentially Nick's patch:

* Argument Clinic wraps parameters in the signature at 72 columns
  instead of 79 columns.  There are a couple extra characters that
  get emitted, so this ensures that the generated lines are never > 80.

* Added an assert to typeobject.c that end - start > 2, so that 
  the subsequent assert on end[-1] is always a legal memory access.

* inspect._strip_non_python_syntax() now emits a space after commas in
  the signature, enhancing readability on the off chance that someone
  needs to read them.

* Added a couple extra tests for _strip_non_python_syntax().

Thanks for all the reviews and the fixes, everybody!
msg210799 - (view) Author: Roundup Robot (python-dev) Date: 2014-02-10 06:23
New changeset 28aef6e22736 by Larry Hastings in branch 'default':
Issue #20530: The signatures for slot builtins have been updated
http://hg.python.org/cpython/rev/28aef6e22736
msg231584 - (view) Author: Jesús Cea Avión (jcea) * (Python committer) Date: 2014-11-24 00:48
Preparing a presentation about Python Magic methods I found something weird: (Python 3.4)

"""
>>> help(int.__lt__)
Help on wrapper_descriptor:

__lt__(self, value, /)  <- THIS!!
    Return self<value.
"""

I am amused about the "/)" suffix in the signature. It happens to all magic methods.
msg231758 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-11-27 12:33
Actually this is the wrong issue for that observation.  You want the issue where yselivanov added emitting the "/" to pydoc.
History
Date User Action Args
2014-11-27 12:33:20larrysetmessages: + msg231758
2014-11-24 00:48:14jceasetnosy: + jcea
messages: + msg231584
2014-02-10 06:23:21python-devsetmessages: + msg210799
2014-02-09 06:23:03larrysetstatus: open -> closed
resolution: fixed
messages: + msg210724

stage: patch review -> resolved
2014-02-09 06:16:10python-devsetnosy: + python-dev
messages: + msg210723
2014-02-09 04:27:45larrysetmessages: + msg210719
2014-02-09 03:56:54ncoghlansetfiles: + issue20530_fixed_assertion.diff

messages: + msg210716
2014-02-07 19:10:52yselivanovsetfiles: + larry.even.newerer.signature.syntax.4.diff

messages: + msg210529
2014-02-07 18:55:03yselivanovsetfiles: + larry.even.newerer.signature.syntax.3.diff

messages: + msg210524
2014-02-07 17:48:27zach.waresetmessages: + msg210502
2014-02-07 06:44:58larrysetfiles: + larry.even.newerer.signature.syntax.2.diff

messages: + msg210438
2014-02-06 14:35:08yselivanovsetmessages: + msg210387
2014-02-06 14:15:17larrysetfiles: + larry.even.newerer.signature.syntax.1.diff

messages: + msg210382
2014-02-06 14:14:40larrysetfiles: - larry.even.newerer.signature.syntax.1.diff
2014-02-06 14:09:10larrycreate