classification
Title: Argument Clinic doesn't support named constants as default values
Type: Stage: resolved
Components: Build Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: larry Nosy List: barry, jkloth, larry, ncoghlan, python-dev, serhiy.storchaka
Priority: normal Keywords:

Created on 2014-01-06 14:23 by serhiy.storchaka, last changed 2014-01-07 21:05 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
larry.simple.named.constants.in.text.signature.example larry, 2014-01-06 18:45 review
larry.simple.symbolic.constant.default.values.diff.1.txt larry, 2014-01-06 20:51 review
larry.simple.symbolic.constant.default.values.diff.2.txt larry, 2014-01-07 12:00 HURR DURR SECOND DERP review
larry.simple.symbolic.constant.default.values.diff.3.txt larry, 2014-01-07 15:38 review
larry.simple.symbolic.constant.default.values.diff.4.txt larry, 2014-01-07 17:16 review
Messages (21)
msg207435 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-06 14:23
For example it doesn't support PY_SSIZE_T_MAX as default value for Py_ssize_t argument. Is there any way to specify it? This is required for converting the _sre module.
msg207444 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-06 15:50
Here's the problem.  Let's say I gave you a way of specifying a symbolic constant for the default value for C.  What value should we use for the default value in Python?  Keep in mind, it has to be expressed as a static value that can be stored as a string as part of the __text_signature__ at the front of the docstring.

If you can answer that question, we can solve the problem.
msg207449 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-06 16:11
As an example, consider pattern_match() in _sre.c.  This implements the match method on a pattern object; in other words, re.compile().match().  The third parameter, endpos, defaults to PY_SSIZE_T_MAX in C.  What should inspect.Signature() report as the default value for endpos?  We can't look up the proper value for PY_SSIZE_T_MAX, because the default value inspect.Signature() uses was written out as text when Argument Clinic processed the function.

As a hint, the documentation for match() dodges this problem by outright lying.  It claims that the prototype for the function is:

    match(string[, pos[, endpos]])

which is a lie.  pattern_match() parses its arguments by calling PyArg_ParseTupleAndKeywords() with a format string of "O|nn".  which means, for example, you could call:

    match("abc", endpos=5)

The documentation claims this is invalid but it works fine.

In other words, it's a sticky problem, and the people who had it before us didn't try to solve it properly.
msg207451 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-06 16:18
Not always default value can be expressed in Python. Current docstring for the match() method says "match(string[, pos[, endpos]])". For this particular case it can be written as "match(string, pos=0, endpos=sys.maxsize)". And this is very common case (see also str.find(sub[, start[, end]]), etc). Perhaps there are cases in which the default value is not PY_SSIZE_T_MAX and None is not accepted (as for dir_fd parameter in os functions).

So as minimum Argument Clinic can support sys.maxsize, and as maximum, it should support arbitrary values.
msg207453 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-06 16:38
"sys.maxsize" won't work.  inspect.Signature parses the __text_signature__ using ast.parse, and it only recognizes constant values and named constants (True/False/None) for the default value for parameters.  Playing with ast, it looks like we'd have to support a tree of Attribute nodes, then look up sys (or whatever) and recursively getattr on it.  Which would be new code.

Anyway it seems like a bad idea:

    import inspect
    import re
    import sys

    sys.maxsize = 3
    inspect.signature(re.compile(".*").match)
msg207454 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-06 16:43
Then perhaps these methods can not be converted to use Argument Clinic now.
msg207463 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-06 18:27
Attached is an example of how we could support simple named constants in __text_signature__.  The change to posixmodule.c is just a hack to exercise the code in inspect.Signature; I didn't have a good example handy.

When I apply the patch and run "x.py", I see:
    (path, *, dir_fd=None, follow_symlinks=True, fake=9223372036854775807)

This patch is just an example; I don't propose to check it in in this state.  (We'd still need to modify Argument Clinic to support these named constants, too.)
msg207467 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-06 18:45
Whoops, forgot to attach the file.  here it is.
msg207471 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2014-01-06 19:40
It seems a little unfortunate that you lose the symbolic default, especially since the expanded number just appears random.

>>> print(inspect.signature(os.stat))
(path, *, dir_fd=None, follow_symlinks=True, fake=9223372036854775807)
>>> sys.maxsize
9223372036854775807
msg207475 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-06 20:23
It is unfortunate, but on the other hand that's what happens in Python too:

    >>> import sys
    >>> import inspect
    >>> def foo(blah=sys.maxsize): pass
    ... 
    >>> str(inspect.signature(foo))
    '(blah=9223372036854775807)'

Nick proposed something we could use to fix these (I think he called them "named constants") but it's not in the language yet.  Anyway propagating those all the way from Argument Clinic to inspect.Signature would be tricky.

Could you live with this being checked in to 3.4?
msg207476 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2014-01-06 20:29
On Jan 06, 2014, at 08:23 PM, Larry Hastings wrote:

>Could you live with this being checked in to 3.4?

For sure!
msg207479 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-06 20:51
Attached is a patch supporting simple symbolic constants.  It works from beginning to end--you specify it in the Argument Clinic input and it shows up in the inspect.Signature and uses the constant in the generated C code.

One complication: when using one of these constants, I force you to specify a c_default.  Which is now automatically supported on all converters, as is py_default.

I converted pattern_match in _sre.c as a sample.  I won't check that part of the patch in; it's just to give you an idea of what it looks like for the user.
msg207483 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-06 21:04
I proposed contrary approach. Allow docstring to fake signature.

    >>> import sys
    >>> import inspect
    >>> def foo(blah=sys.maxsize):
            "foo(blah=sys.maxsize)"
            pass
    ... 
    >>> str(inspect.signature(foo))
    '(blah=sys.maxsize)'
msg207484 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-06 21:09
storchaka: You propose a result, not an approach.  How do you propose we do that?

In any case, I think making *that* work would be way too big a change for 3.4.  Whatever you proposed would only be appropriate for 3.5.
msg207526 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-07 11:52
Here's a second patch; I think this is ready to go in

I cleaned up the node parsing a lot.  It now knows how to parse the following types of default values:
  * Number (this applies to both ints and floats)
  * String ('hello')
  * Attribute (sys.maxsize)
  * Named constant (None, True, False)
As long as I'm fixing this, I want to add support for any other values we're going to want.  Did I miss any?  Note that we can't really support container types, because initializing them on the C side isn't practical.

Also, the tests don't exercise all of the above possible node types, just because I couldn't remember builtins that used those types.  We'll fill in the remaining tests as we port new functions to Argument Clinic.

If I can get a quick LGTM review, I'll check this in.
msg207527 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-07 12:00
Oops!  I forgot to actually attach the new patch.  I'm dumb.
msg207547 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-07 15:38
At Antoine's suggestion, I added a custom function to testcapi that exercises all the different possible types for default values in a text signature.  Also the docs have been updated.  LGTU?
msg207565 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-07 17:16
Incorporated suggestions from Serhiy.  Thanks, Serhiy!
msg207585 - (view) Author: Roundup Robot (python-dev) Date: 2014-01-07 19:53
New changeset c96dba33f019 by Larry Hastings in branch 'default':
Issue #20144: Argument Clinic now supports simple constants as parameter
http://hg.python.org/cpython/rev/c96dba33f019
msg207586 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-07 20:06
Argument Clinic now supports simple constants like "sys.maxsize" as default values for arguments for builtins.  I'm assuming this gets you basically what you wanted; if this isn't sufficient please open a new issue.
msg207601 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-07 21:05
Thank you Larry for all your fixes.
History
Date User Action Args
2014-01-07 21:05:39serhiy.storchakasetmessages: + msg207601
2014-01-07 20:06:20larrysetstatus: open -> closed
messages: + msg207586

assignee: larry
resolution: fixed
stage: resolved
2014-01-07 19:53:54python-devsetnosy: + python-dev
messages: + msg207585
2014-01-07 17:16:52larrysetfiles: + larry.simple.symbolic.constant.default.values.diff.4.txt

messages: + msg207565
2014-01-07 15:39:00larrysetfiles: + larry.simple.symbolic.constant.default.values.diff.3.txt

messages: + msg207547
2014-01-07 12:00:55larrysetfiles: + larry.simple.symbolic.constant.default.values.diff.2.txt

messages: + msg207527
2014-01-07 11:52:15larrysetmessages: + msg207526
2014-01-06 21:09:07larrysetmessages: + msg207484
2014-01-06 21:04:12serhiy.storchakasetmessages: + msg207483
2014-01-06 20:57:55pitrousetnosy: + ncoghlan
2014-01-06 20:51:24larrysetfiles: + larry.simple.symbolic.constant.default.values.diff.1.txt

messages: + msg207479
2014-01-06 20:29:15barrysetmessages: + msg207476
2014-01-06 20:23:55larrysetmessages: + msg207475
2014-01-06 19:40:09barrysetnosy: + barry
messages: + msg207471
2014-01-06 18:45:21larrysetfiles: + larry.simple.named.constants.in.text.signature.example

messages: + msg207467
2014-01-06 18:27:36larrysetmessages: + msg207463
2014-01-06 17:08:14serhiy.storchakalinkissue20148 dependencies
2014-01-06 16:48:06jklothsetnosy: + jkloth
2014-01-06 16:43:53serhiy.storchakasetmessages: + msg207454
2014-01-06 16:38:52larrysetmessages: + msg207453
2014-01-06 16:18:52serhiy.storchakasetmessages: + msg207451
2014-01-06 16:11:17larrysetmessages: + msg207449
2014-01-06 15:50:12larrysetmessages: + msg207444
2014-01-06 14:23:13serhiy.storchakacreate