Repository navigation
Argument Clinic doesn't support named constants as default values #64343
Description
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:
assignee = 'https://github.com/larryhastings'
closed_at = <Date 2014-01-07.20:06:20.892>
created_at = <Date 2014-01-06.14:23:13.718>
labels = ['build']
title = "Argument Clinic doesn't support named constants as default values"
updated_at = <Date 2014-01-07.21:05:39.979>
user = 'https://github.com/serhiy-storchaka'bugs.python.org fields:
activity = <Date 2014-01-07.21:05:39.979>
actor = 'serhiy.storchaka'
assignee = 'larry'
closed = True
closed_date = <Date 2014-01-07.20:06:20.892>
closer = 'larry'
components = ['Build']
creation = <Date 2014-01-06.14:23:13.718>
creator = 'serhiy.storchaka'
dependencies = []
files = ['33331', '33333', '33340', '33344', '33348']
hgrepos = []
issue_num = 20144
keywords = []
message_count = 21.0
messages = ['207435', '207444', '207449', '207451', '207453', '207454', '207463', '207467', '207471', '207475', '207476', '207479', '207483', '207484', '207526', '207527', '207547', '207565', '207585', '207586', '207601']
nosy_count = 6.0
nosy_names = ['barry', 'ncoghlan', 'larry', 'jkloth', 'python-dev', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue20144'
versions = ['Python 3.4']
Activity
serhiy-storchaka commentedon Jan 6, 2014
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.
larryhastings commentedon Jan 6, 2014
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.
larryhastings commentedon Jan 6, 2014
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:
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:
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.
serhiy-storchaka commentedon Jan 6, 2014
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.
larryhastings commentedon Jan 6, 2014
"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:
serhiy-storchaka commentedon Jan 6, 2014
Then perhaps these methods can not be converted to use Argument Clinic now.
larryhastings commentedon Jan 6, 2014
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.)
larryhastings commentedon Jan 6, 2014
Whoops, forgot to attach the file. here it is.
warsaw commentedon Jan 6, 2014
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 9223372036854775807larryhastings commentedon Jan 6, 2014
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?
warsaw commentedon Jan 6, 2014
On Jan 06, 2014, at 08:23 PM, Larry Hastings wrote:
For sure!
larryhastings commentedon Jan 6, 2014
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.
serhiy-storchaka commentedon Jan 6, 2014
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)'larryhastings commentedon Jan 6, 2014
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.
larryhastings commentedon Jan 7, 2014
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:
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.
larryhastings commentedon Jan 7, 2014
Oops! I forgot to actually attach the new patch. I'm dumb.
larryhastings commentedon Jan 7, 2014
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?
larryhastings commentedon Jan 7, 2014
Incorporated suggestions from Serhiy. Thanks, Serhiy!
python-dev commentedon Jan 7, 2014
New changeset c96dba33f019 by Larry Hastings in branch 'default':
Issue bpo-20144: Argument Clinic now supports simple constants as parameter
http://hg.python.org/cpython/rev/c96dba33f019
larryhastings commentedon Jan 7, 2014
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.
serhiy-storchaka commentedon Jan 7, 2014
Thank you Larry for all your fixes.