Argument Clinic: expression default arguments broken

[zware]

BPO	20381
Nosy	@larryhastings, @zware, @serhiy-storchaka
Files	issue20381.diff issue20381.v2.diff larry.clinic.rollup.jan.24.diff.1.txt issue20381.v3.diff

^{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/zware'
closed_at = <Date 2014-01-25.04:53:35.563>
created_at = <Date 2014-01-24.16:02:56.969>
labels = ['type-bug']
title = 'Argument Clinic: expression default arguments broken'
updated_at = <Date 2014-01-25.07:12:49.672>
user = 'https://github.com/zware'

bugs.python.org fields:

activity = <Date 2014-01-25.07:12:49.672>
actor = 'larry'
assignee = 'zach.ware'
closed = True
closed_date = <Date 2014-01-25.04:53:35.563>
closer = 'zach.ware'
components = ['Demos and Tools']
creation = <Date 2014-01-24.16:02:56.969>
creator = 'zach.ware'
dependencies = []
files = ['33687', '33691', '33692', '33694']
hgrepos = []
issue_num = 20381
keywords = ['patch']
message_count = 16.0
messages = ['209094', '209103', '209120', '209121', '209123', '209129', '209133', '209135', '209142', '209146', '209147', '209149', '209150', '209154', '209155', '209160']
nosy_count = 4.0
nosy_names = ['larry', 'python-dev', 'zach.ware', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue20381'
versions = ['Python 3.4']

[zware]

The fix for bpo-20189 broke simple expression defaults. Specifically, with the latest bpo-20172 patch applied, Clinic fails on winreg.c at winreg line 1351 with this traceback:

Error in file "PC\winreg.c" on line 1351:
Exception raised during parsing:
Traceback (most recent call last):
  File "Tools\clinic\clinic.py", line 1541, in parse
    parser.parse(block)
  File "Tools\clinic\clinic.py", line 2942, in parse
    self.state(line)
  File "Tools\clinic\clinic.py", line 3482, in state_parameter_docstring
    return self.next(self.state_parameter, line)
  File "Tools\clinic\clinic.py", line 2975, in next
    self.state(line)
  File "Tools\clinic\clinic.py", line 3321, in state_parameter
    bad = default != repr(eval(default))
  File "<string>", line 1, in <module>
NameError: name 'winreg' is not defined

In this case, 'default' is 'winreg.KEY_WRITE'. The 'default != repr(eval(default))' check cannot succeed with such a default even if winreg were defined, as it will return an int, 131078.

[zware]

This also prevents a vanilla trunk build (as of 85710aa396ef) from succeeding at 'Tools/clinic/clinic.py --make':

Error in file ".\Modules\_sre.c" on line 574:
Unsupported expression as default value: 'sys.maxsize'

I think the only reasonable check we can do is to make sure there aren't any syntax errors in the default value (see patch), but there may well be something I'm missing here.

[larryhastings]

Remove the winreg.

On Jan 24, 2014 8:02 AM, Zachary Ware <report@bugs.python.org> wrote:
>
>
> New submission from Zachary Ware: 
>
> The fix for python/cpython#64388 broke simple expression defaults.  Specifically, with the latest python/cpython#64371 patch applied, Clinic fails on winreg.c at winreg line 1351 with this traceback: 
>
> Error in file "PC\winreg.c" on line 1351: 
> Exception raised during parsing: 
> Traceback (most recent call last): 
>   File "Tools\clinic\clinic.py", line 1541, in parse 
>     parser.parse(block) 
>   File "Tools\clinic\clinic.py", line 2942, in parse 
>     self.state(line) 
>   File "Tools\clinic\clinic.py", line 3482, in state_parameter_docstring 
>     return self.next(self.state_parameter, line) 
>   File "Tools\clinic\clinic.py", line 2975, in next 
>     self.state(line) 
>   File "Tools\clinic\clinic.py", line 3321, in state_parameter 
>     bad = default != repr(eval(default)) 
>   File "<string>", line 1, in <module> 
> NameError: name 'winreg' is not defined 
>
> In this case, 'default' is 'winreg.KEY_WRITE'.  The 'default != repr(eval(default))' check cannot succeed with such a default even if winreg were defined, as it will return an int, 131078. 
>
> ---------- 
> assignee: larry 
> components: Demos and Tools 
> messages: 209094 
> nosy: larry, zach.ware 
> priority: normal 
> severity: normal 
> stage: needs patch 
> status: open 
> title: Argument Clinic: expression default arguments broken 
> type: behavior 
> versions: Python 3.4 
>
> _______________________________________ 
> Python tracker <report@bugs.python.org> 
> <http://bugs.python.org/issue20381> 
> _______________________________________

[zware]

Removing 'winreg.' gives the very similar traceback:

Error in file "PC\winreg.c" on line 1351:
Exception raised during parsing:
Traceback (most recent call last):
  File "Tools\clinic\clinic.py", line 1541, in parse
    parser.parse(block)
  File "Tools\clinic\clinic.py", line 2942, in parse
    self.state(line)
  File "Tools\clinic\clinic.py", line 3482, in state_parameter_docstring
    return self.next(self.state_parameter, line)
  File "Tools\clinic\clinic.py", line 2975, in next
    self.state(line)
  File "Tools\clinic\clinic.py", line 3321, in state_parameter
    bad = default != repr(eval(default))
  File "<string>", line 1, in <module>
NameError: name 'KEY_WRITE' is not defined

[larryhastings]

Let's not catch Exception there. Does catching NameError make the problem go away?

And, bpo-20189 changed the rules for simple expressions. Now names you want from the local module should not have the name of the module in front. You wouldn't have to do this in foo.py:

IMPORTANT_SYMBOLIC_CONSTANT=83
def fn(a=foo.IMPORTANT_SYMBOLIC_CONSTANT): ...

So it was always a dumb idea to require it for Argument Clinic code too.

[zware]

How does this one strike you? Somewhat reversed logic compared to the first patch, but with this one we get either success or a nicely formatted fail message, no more tracebacks.

[larryhastings]

It's considered bad form to catch Exception. We should catch the minimum necessary. If there are exceptions we forgot, people will complain and then we'll add those.

Also, I want to make sure (if possible) that the value round-trips correctly through eval and repr. We need to repr the value, and if it doesn't round-trip properly the user needs to know.

On the other hand, I'm okay with starting with bad=False and going from there. The previous block (with DetectBadNodes()) already does that. Hang on for a patch.

[larryhastings]

How about this? I'm going to do a rollup patch today with three fixes: this, bpo-20385, and changing the default filename for the "file" destination.

[zware]

This will still cause issue in _sre.c; repr(sys.maxsize) == '9223372036854775807' != 'sys.maxsize'.

Yes, it's bad form to throw the net too wide, but for anything other than NameError, we fail out in the next block anyway. My (second) patch just makes for nicer output from the failure, though it does fail to say what the exception was.

What about something more along these lines?

bad = False
try:
    eval(default)
except NameError:
    pass # probably a named constant
except Exception as e:
    fail("Malformed expression given as default value\n"
         "{!r} raised {!r}".format(default, e))

[zware]

Here's a patch with my last suggestion, and the check for unspecified.

[larryhastings]

Yeah, that's a good approach.

[python-dev]

New changeset 93afa651e074 by Zachary Ware in branch 'default':
Issue bpo-20381: Fix sanity checking on default arguments when c_default is
http://hg.python.org/cpython/rev/93afa651e074

[zware]

Done, thanks for the review!