classification
Title: Argument Clinic: expression default arguments broken
Type: behavior Stage: resolved
Components: Demos and Tools Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: zach.ware Nosy List: larry, python-dev, serhiy.storchaka, zach.ware
Priority: normal Keywords: patch

Created on 2014-01-24 16:02 by zach.ware, last changed 2014-01-25 07:12 by larry. This issue is now closed.

Files
File name Uploaded Description Edit
issue20381.diff zach.ware, 2014-01-24 17:35 review
issue20381.v2.diff zach.ware, 2014-01-24 22:24 review
larry.clinic.rollup.jan.24.diff.1.txt larry, 2014-01-24 22:49 review
issue20381.v3.diff zach.ware, 2014-01-25 04:24 review
Messages (16)
msg209094 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-01-24 16:02
The fix for #20189 broke simple expression defaults.  Specifically, with the latest #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.
msg209103 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-01-24 17:35
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.
msg209120 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-24 21:26
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 #20189 broke simple expression defaults.  Specifically, with the latest #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. 
>
> ---------- 
> 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> 
> _______________________________________
msg209121 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-01-24 21:31
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
msg209123 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-24 21:41
Let's not catch Exception there.  Does catching NameError make the problem go away?

And, #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.
msg209129 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-01-24 22:24
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.
msg209133 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-24 22:41
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.
msg209135 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-24 22:49
How about this?  I'm going to do a rollup patch today with three fixes: this, #20385, and changing the default filename for the "file" destination.
msg209142 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-01-25 04:12
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))
msg209146 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-01-25 04:24
Here's a patch with my last suggestion, and the check for unspecified.
msg209147 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-25 04:24
Yeah, that's a good approach.
msg209149 - (view) Author: Roundup Robot (python-dev) Date: 2014-01-25 04:52
New changeset 93afa651e074 by Zachary Ware in branch 'default':
Issue #20381: Fix sanity checking on default arguments when c_default is
http://hg.python.org/cpython/rev/93afa651e074
msg209150 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-01-25 04:53
Done, thanks for the review!
msg209154 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-25 06:20
I think we should specify globals (=sys.modules) and locals (=module.__dict__) arguments to eval.
msg209155 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-25 06:23
Hmm, no, we shouldn't use sys.modules, it's content is depended on modules used in Argument Clinic.
msg209160 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-25 07:12
Right.  So my advice is: only use local symbols and symbols in preloaded modules.
History
Date User Action Args
2014-01-25 07:12:49larrysetmessages: + msg209160
2014-01-25 06:23:31serhiy.storchakasetmessages: + msg209155
2014-01-25 06:20:05serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg209154
2014-01-25 04:53:35zach.waresetstatus: open -> closed
messages: + msg209150

assignee: larry -> zach.ware
resolution: fixed
stage: needs patch -> resolved
2014-01-25 04:52:47python-devsetnosy: + python-dev
messages: + msg209149
2014-01-25 04:24:58larrysetmessages: + msg209147
2014-01-25 04:24:40zach.waresetfiles: + issue20381.v3.diff

messages: + msg209146
2014-01-25 04:12:53zach.waresetmessages: + msg209142
2014-01-24 22:49:12larrysetfiles: + larry.clinic.rollup.jan.24.diff.1.txt

messages: + msg209135
2014-01-24 22:41:54larrysetmessages: + msg209133
2014-01-24 22:24:06zach.waresetfiles: + issue20381.v2.diff

messages: + msg209129
2014-01-24 21:41:47larrysetmessages: + msg209123
2014-01-24 21:31:13zach.waresetmessages: + msg209121
2014-01-24 21:26:19larrysetmessages: + msg209120
2014-01-24 17:35:27zach.waresetfiles: + issue20381.diff
keywords: + patch
messages: + msg209103
2014-01-24 16:02:56zach.warecreate