Issue 2551026
Created on 2019-03-04 21:04 by tekberg, last changed 2019-04-08 20:12 by tekberg.
Files | ||||
---|---|---|---|---|
File name | Uploaded | Description | Edit | Remove |
python_bug.py | tekberg, 2019-03-25 15:23 | Simple python program that illustrates this problem. | ||
PythonExpr.py.diff | tekberg, 2019-03-25 17:39 | Patch file for PythonExpr.py | ||
test_pythonexpr.py | tekberg, 2019-03-26 15:37 | Updated test case for PythonExpr. |
Messages | |||
---|---|---|---|
msg6364 | Author: [hidden] (tekberg) | Date: 2019-03-04 21:04 | |
When using python 3.5 (probably fails with all python 3s), references to tal variables sometimes fail. To reproduce, create the demo tracker. Change html/issue.item.html Locate the <table> element near line 42. Change it to this: <table class="form" tal:define="user_realnames python:sorted('Kirk,Spock,Uhura, McCoy,Scotty,Sulu,Chekov,Chapel'.split(',')); bad_expression python:[x for x in context.assignedto if x.realname not in user_realnames]"> To make things simpler, in the config.ini file, set web.debug=yes. Do whatever you need to do to deploy these two changes. Go to the browser and bring up the demo tracker. Click on 'Create New' in the sidebar, you will get a 'Templating Error' with this detail at the top of the page: <class 'NameError'>: name 'user_realnames' is not defined If you do a browser search for 'user_realnames' it will match the name in the above error message, with the last match matching the variable which is actually defined with the correct value. The problem is that roundup/cgi/PageTemplates/PythonExpr.py tries to obtain variables referenced in the python expression using this expression at line 43: self._f.__code__.co_names With python 3.5 this expression does not return all variables in the expression ('context', 'assignedto'). The same code for python 2.7 works fine and returns all variables in the expression ('context', 'assignedto', 'realname', 'user_realnames'). I am working on a fix for this using the symtable package. I'll post it here when I'm finished. My proof of concept test code works for both python 2.7 and 3.5. |
|||
msg6365 | Author: [hidden] (tekberg) | Date: 2019-03-04 22:59 | |
Here is a patch file generated using: $ hg diff roundup/cgi/PageTemplates/PythonExpr.py It works for me with Python 3.5. I assume it works for Python 2.7 since my proof of concept works with Python 2.7. Hopefully someone can verify that. It might be nice if someone could review my code too. |
|||
msg6420 | Author: [hidden] (rouilj) | Date: 2019-03-24 01:16 | |
Joseph, this looks like a python 3 issue. Can you take a look at the patch and see if it looks ok for you. I am kind of lost as to what magic is happening there. There may be a better way to patch it and get rid of the problem Tom found. Tom do you have any test cases that we can run that will show the problem and that go away when this patch is applied? The fact that this slipped though indicates we need to enhance the test/test_templating.py cases. -- rouilj |
|||
msg6423 | Author: [hidden] (joseph_myers) | Date: 2019-03-24 21:27 | |
I have no information about any changes to co_names in Python 3. Thus, while I don't know anything wrong with the patch, I also don't know what the actual change in Python 3 is that should require such a change. |
|||
msg6424 | Author: [hidden] (tekberg) | Date: 2019-03-25 13:58 | |
I'll investigate to see if there a documented reason for this change from Python 2.7 to 3.5. I signed up to the python-porting mailing list. Perhaps someone there has knowledge of this change. |
|||
msg6425 | Author: [hidden] (tekberg) | Date: 2019-03-25 15:23 | |
Re msg6423, you can duplicate this problem by following what I described in msg6364. If you'd rather not create a new tracker, I attached a simple Python program that can be used to verify the difference between Python 2.7 and Python 3.5. Whether this is documented or not, the problem exists. I sent an email to python-porting but this appears to be a very low volume mailing list. If I don't receive a response I will file a bug report for python 3.5. I read all release python 3.5 and after and there were no references to co_names. Even if this bug report is accepted and corrected, the fix probably won't be available until Python 3.8, or after. In the meantime others using roundup may experience this problem. |
|||
msg6429 | Author: [hidden] (tekberg) | Date: 2019-03-25 17:28 | |
At John's request I wrote a test case. With the non-working version running the test generates this output: ~/src/tracker-new/roundup-code$ python run_tests.py test/test_pythonexpr.py ====================================== test session starts ======================================= platform linux -- Python 3.5.2, pytest-2.9.2, py-1.4.31, pluggy-0.3.1 rootdir: /home/tekberg/src/tracker-new/roundup-code, inifile: collected 1 items test/test_pythonexpr.py F ============================================ FAILURES ============================================ _______________________________________ ExprTest.testExpr ________________________________________ self = <test.test_pythonexpr.ExprTest testMethod=testExpr> def testExpr(self): expr = '[x for x in context.assignedto if x.realname not in user_realnames]' pe = PythonExprClass('test', expr, None) expected_names = ['context', 'user_realnames', 'x'] got_names = pe._f_varnames > self.assertCountEqual(expected_names, got_names) E AssertionError: Element counts were not equal: E First has 1, Second has 0: 'user_realnames' E First has 1, Second has 0: 'x' E First has 0, Second has 1: 'assignedto' test/test_pythonexpr.py:25: AssertionError ==================================== 1 failed in 0.07 seconds ==================================== With the fix, running the test generates this output: ~/src/tracker-new/roundup-code$ python run_tests.py test/test_pythonexpr.py ====================================== test session starts ======================================= platform linux -- Python 3.5.2, pytest-2.9.2, py-1.4.31, pluggy-0.3.1 rootdir: /home/tekberg/src/tracker-new/roundup-code, inifile: collected 1 items test/test_pythonexpr.py . ==================================== 1 passed in 0.04 seconds ==================================== I ran pychecker on the changes and the test case. No errors were generated. |
|||
msg6430 | Author: [hidden] (tekberg) | Date: 2019-03-25 17:34 | |
Updated the patch due to messages generated by pychecker. |
|||
msg6431 | Author: [hidden] (tekberg) | Date: 2019-03-25 17:39 | |
Removed debug code from patch. I used it to switch between old and new code. |
|||
msg6434 | Author: [hidden] (rouilj) | Date: 2019-03-25 23:28 | |
Hi Tom: Thanks for the test case. I had to make a change to get it to work under python2. The code now reads: ... expected_names = ['context', 'user_realnames', 'x'] got_names = pe._f_varnames try: self.assertItemsEqual(expected_names, got_names) except AttributeError: self.assertCountEqual(expected_names, got_names) however I expected this to pass when run as: python2 ./run_tests.py test/test_pythonexpr.py without any patches to the roundup code since this problem doesn't happen under python2. But it fails with: ... try: > self.assertItemsEqual(expected_names, got_names) E AssertionError: Element counts were not equal: E First has 1, Second has 0: 'x' E First has 0, Second has 1: 'assignedto' E First has 0, Second has 1: 'realname' It also fails under python 3 with: self.assertItemsEqual(expected_names, got_names) except AttributeError: > self.assertCountEqual(expected_names, got_names) E AssertionError: Element counts were not equal: E First has 1, Second has 0: 'user_realnames' E First has 1, Second has 0: 'x' E First has 0, Second has 1: 'assignedto' Does the indicate we have had issues under python2 and just never noticed? With your patch to PythonExpr.py both test cases pass. If you can shed a little light on the unexpected python 2 failure without the patch I would appreciate it. -- rouilj |
|||
msg6436 | Author: [hidden] (tekberg) | Date: 2019-03-26 15:37 | |
Updated test case to work for python2 and python3. |
|||
msg6437 | Author: [hidden] (tekberg) | Date: 2019-03-26 15:55 | |
John, Thanks for testing the test case in python2. I didn't think of it. To help me write the test case I came up with this matrix for the names in the expression: Name Python3 w/o fix Python2 Fix context Y Y Y x Y user_realnames Y Y assignedto Y Y realname Y If you look at the expression you will realize that only context and user_realnames are external variables, and so are required. It doesn't matter if x is there or not since it is local and initialized, and assignedto and realname are members of context and x, respectively. I updated and ran the test case with these results: ~/src/tracker-new/roundup-code$ python2 run_tests.py test/test_pythonexpr.py ====================================== test session starts ======================================= platform linux2 -- Python 2.7.12, pytest-2.9.2, py-1.4.31, pluggy-0.3.1 rootdir: /home/tekberg/src/tracker-new/roundup-code, inifile: collected 1 items test/test_pythonexpr.py . ==================================== 1 passed in 0.02 seconds ==================================== ~/src/tracker-new/roundup-code$ python run_tests.py test/test_pythonexpr.py ====================================== test session starts ======================================= platform linux -- Python 3.5.2, pytest-2.9.2, py-1.4.31, pluggy-0.3.1 rootdir: /home/tekberg/src/tracker-new/roundup-code, inifile: collected 1 items test/test_pythonexpr.py . ==================================== 1 passed in 0.04 seconds ==================================== |
|||
msg6440 | Author: [hidden] (rouilj) | Date: 2019-03-26 21:38 | |
Hi Tom: I didn't quite follow the explanation for your solution matrix. However my take away is that the patch does not need to be applied to the maint-1.6.0 branch as it is not needed for python 2.7. Also the new tests work for both 2.7 and 3.x (so will not break the CI environment) and are good to push. Pushed in hg5676:e70885fe72a4 |
|||
msg6441 | Author: [hidden] (tekberg) | Date: 2019-03-26 22:09 | |
John, The matrix reads better in the tracker (msg6437). My emailer used a variable width font (probably yours too) which made columns not line up. The idea is that there are 3 cases. The original case for Python3 which doesn't recognize enough names as variables. I put a 'Y' for both of the names that were recognized as variables. The middle case is the same thing for Python2, which already worked. The third case is the names recognized as variables with the patch applied (same value for Python2 and Python3). By looking at these I could determine what names need to appear for the test case to pass: context and user_realnames. The name x is a local variable, and the other two names, assignedto and realname, are not variables but are object instance variables. Hopefully that makes more sense. |
|||
msg6442 | Author: [hidden] (rouilj) | Date: 2019-03-27 00:41 | |
In message <1553638174.91.0.733605524818.issue2551026@roundup.psfhosted.org>, Tom Ekberg writes: >The matrix reads better in the tracker (msg6437). ... I didn't understand why x, assignedto and realname were special (not included in the fix. Got it now. Thanks. |
|||
msg6458 | Author: [hidden] (tekberg) | Date: 2019-04-08 20:12 | |
Sent a query to python-porting@python.org and got back a reasonable response from Piet van Oostrum explaining the situation. Here is the body of his response: Tom Ekberg wrote: > > My question to this group: > > Do you have knowledge of the different contents of __code__.co_names between Python 2.7 and Python > 3.5? I'm expecting that this is a difference that is documented somewhere. > The difference is that in Python 3 a (list) comprehension is compiled into an anonymous function, to prevent the leaking of the control variable (x in this case). The base from which the list is constructed (i.e. the expression after 'in') is given as the argument to this function, so any variable in that part appears in co_names. But the rest of the comprehension is part of that anonymous function, so in this case the names 'realname' and 'user_realnames' are not local to your function 'f', but of the anonymous function. In Python 2, the list comprehension was compiled inline. |
History | |||
---|---|---|---|
Date | User | Action | Args |
2019-04-08 20:12:31 | tekberg | set | messages: + msg6458 |
2019-03-27 00:41:17 | rouilj | set | messages: + msg6442 |
2019-03-26 22:09:34 | tekberg | set | messages: + msg6441 |
2019-03-26 21:38:02 | rouilj | set | priority: high status: open -> fixed messages: + msg6440 resolution: fixed type: crash |
2019-03-26 15:55:56 | tekberg | set | files: - test_pythonexpr.py |
2019-03-26 15:55:35 | tekberg | set | messages: + msg6437 |
2019-03-26 15:37:29 | tekberg | set | files:
+ test_pythonexpr.py messages: + msg6436 |
2019-03-25 23:28:50 | rouilj | set | status: new -> open assignee: rouilj messages: + msg6434 |
2019-03-25 17:39:22 | tekberg | set | files: - PythonExpr.py.diff |
2019-03-25 17:39:14 | tekberg | set | files:
+ PythonExpr.py.diff messages: + msg6431 |
2019-03-25 17:36:04 | tekberg | set | files: - PythonExpr.py.diff |
2019-03-25 17:34:06 | tekberg | set | files:
+ PythonExpr.py.diff messages: + msg6430 |
2019-03-25 17:28:01 | tekberg | set | files:
+ test_pythonexpr.py messages: + msg6429 |
2019-03-25 15:23:58 | tekberg | set | files:
+ python_bug.py messages: + msg6425 |
2019-03-25 13:58:49 | tekberg | set | messages: + msg6424 |
2019-03-24 21:27:02 | joseph_myers | set | messages: + msg6423 |
2019-03-24 01:36:38 | rouilj | set | keywords: + python3 |
2019-03-24 01:16:56 | rouilj | set | messages: + msg6420 |
2019-03-24 01:14:18 | rouilj | set | nosy: + joseph_myers |
2019-03-04 22:59:34 | tekberg | set | files:
+ PythonExpr.py.diff keywords: + patch messages: + msg6365 |
2019-03-04 21:04:58 | tekberg | create |