Roundup Tracker - Issues

Issue 2551026

classification
template variable not defined even though it is
Type: crash Severity: major
Components: Web interface Versions: devel
process
Status: fixed fixed
:
: rouilj : joseph_myers, rouilj, tekberg
Priority: high : patch, python3

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:31tekbergsetmessages: + msg6458
2019-03-27 00:41:17rouiljsetmessages: + msg6442
2019-03-26 22:09:34tekbergsetmessages: + msg6441
2019-03-26 21:38:02rouiljsetpriority: high
status: open -> fixed
messages: + msg6440
resolution: fixed
type: crash
2019-03-26 15:55:56tekbergsetfiles: - test_pythonexpr.py
2019-03-26 15:55:35tekbergsetmessages: + msg6437
2019-03-26 15:37:29tekbergsetfiles: + test_pythonexpr.py
messages: + msg6436
2019-03-25 23:28:50rouiljsetstatus: new -> open
assignee: rouilj
messages: + msg6434
2019-03-25 17:39:22tekbergsetfiles: - PythonExpr.py.diff
2019-03-25 17:39:14tekbergsetfiles: + PythonExpr.py.diff
messages: + msg6431
2019-03-25 17:36:04tekbergsetfiles: - PythonExpr.py.diff
2019-03-25 17:34:06tekbergsetfiles: + PythonExpr.py.diff
messages: + msg6430
2019-03-25 17:28:01tekbergsetfiles: + test_pythonexpr.py
messages: + msg6429
2019-03-25 15:23:58tekbergsetfiles: + python_bug.py
messages: + msg6425
2019-03-25 13:58:49tekbergsetmessages: + msg6424
2019-03-24 21:27:02joseph_myerssetmessages: + msg6423
2019-03-24 01:36:38rouiljsetkeywords: + python3
2019-03-24 01:16:56rouiljsetmessages: + msg6420
2019-03-24 01:14:18rouiljsetnosy: + joseph_myers
2019-03-04 22:59:34tekbergsetfiles: + PythonExpr.py.diff
keywords: + patch
messages: + msg6365
2019-03-04 21:04:58tekbergcreate