Properly bind references in comprehension scopes (#1142)

* Properly bind references in comprehension scopes

* Add tests

* Make Comprehension.Iterators a read-only list

Author: BCSharp

Src/IronPython/Compiler/Ast/Comprehension.cs

@@ -25,7 +25,7 @@ public abstract class ComprehensionIterator : Node {
     }
 
     public abstract class Comprehension : Expression {
-        public abstract IList<ComprehensionIterator> Iterators { get; }
+        public abstract IReadOnlyList<ComprehensionIterator> Iterators { get; }
         public abstract override string NodeName { get; }
 
         protected abstract MSAst.ParameterExpression MakeParameter();
@@ -85,7 +85,7 @@ public ListComprehension(Expression item, ComprehensionIterator[] iterators) {
 
         public Expression Item { get; }
 
-        public override IList<ComprehensionIterator> Iterators => _iterators;
+        public override IReadOnlyList<ComprehensionIterator> Iterators => _iterators;
 
         protected override MSAst.ParameterExpression MakeParameter()
             => Ast.Parameter(typeof(PythonList), "list_comprehension_list");
@@ -134,7 +134,7 @@ public SetComprehension(Expression item, ComprehensionIterator[] iterators) {
 
         public Expression Item { get; }
 
-        public override IList<ComprehensionIterator> Iterators => _iterators;
+        public override IReadOnlyList<ComprehensionIterator> Iterators => _iterators;
 
         protected override MSAst.ParameterExpression MakeParameter()
             => Ast.Parameter(typeof(SetCollection), "set_comprehension_set");
@@ -186,7 +186,7 @@ public DictionaryComprehension(Expression key, Expression value, ComprehensionIt
 
         public Expression Value { get; }
 
-        public override IList<ComprehensionIterator> Iterators => _iterators;
+        public override IReadOnlyList<ComprehensionIterator> Iterators => _iterators;
 
         protected override MSAst.ParameterExpression MakeParameter()
             => Ast.Parameter(typeof(PythonDictionary), "dict_comprehension_dict");
@@ -280,6 +280,7 @@ internal override bool TryBindOuter(ScopeStatement from, PythonReference referen
         }
 
         internal override PythonVariable BindReference(PythonNameBinder binder, PythonReference reference) {
+            // First try variables local to this scope
             if (TryGetVariable(reference.Name, out PythonVariable variable)) {
                 if (variable.Kind == VariableKind.Global) {
                     AddReferencedGlobal(reference.Name);
@@ -288,8 +289,14 @@ internal override PythonVariable BindReference(PythonNameBinder binder, PythonRe
                 return variable;
             }
 
-            // then bind in our parent scope
-            return Parent.BindReference(binder, reference);
+            // then try to bind in outer scopes
+            for (ScopeStatement parent = Parent; parent != null; parent = parent.Parent) {
+                if (parent.TryBindOuter(this, reference, out variable)) {
+                    return variable;
+                }
+            }
+
+            return null;
         }
 
         internal override Ast GetVariableExpression(PythonVariable variable) {

Src/IronPython/Modules/_ast.cs

@@ -353,7 +353,7 @@ internal static AST Convert(Node node) {
                 return ast;
             }
 
-            internal static PythonList Convert(IList<ComprehensionIterator> iters) {
+            internal static PythonList Convert(IReadOnlyList<ComprehensionIterator> iters) {
                 var cfCollector = new List<ComprehensionFor>();
                 var cifCollector = new List<List<ComprehensionIf>>();
                 List<ComprehensionIf> cif = null;

Tests/test_listcomp.py

@@ -4,7 +4,7 @@
 
 import unittest
 
-from iptest import run_test, is_cli
+from iptest import run_test, is_cpython
 
 class ListCompTest(unittest.TestCase):
     def test_positive(self):
@@ -79,12 +79,9 @@ def test_ipy3_gh809(self):
         # first iterable is captured and subsequent assignments do not change it
         self.maxDiff = None
         x, y, z = range(1), range(2), range(3)
-        if is_cli:
-            _dir = ['x', 'y']
-            # TODO: should be: _dir = ['x', 'y', 'z']
-            # See: https://github.com/IronLanguages/ironpython3/issues/1132
-        else:
-            _dir = ['.0', 'x', 'y', 'z'] # adds implementation-level variable '.0'
+        _dir = ['x', 'y', 'z']
+        if is_cpython:
+            _dir.insert(0, '.0') # adds implementation-level variable '.0'
         # below, x and first/third y is local, second y and z is from the outer scope
         self.assertEqual([(x, y, z, dir()) for x in y for y in z],
                          [(0, 0, range(3), _dir),
@@ -106,4 +103,26 @@ def apply(f, i): return f(i)
         res = [x for x in [y for y in apply(lambda i: range(i+x), x)]]
         self.assertEqual(res, [0, 1, 2, 3])
 
+    def test_ipy3_gh1132(self):
+        """https://github.com/IronLanguages/ironpython3/issues/1132"""
+
+        global global_test_list
+        global_test_list = [1, 2]
+        local_list = [3, 4]
+
+        res = [dir() for x in global_test_list for y in local_list]
+        assert 'global_test_list' not in res[0]
+        assert 'local_list' in res[0]
+
+    def test_ipy3_gh1082(self):
+        """https://github.com/IronLanguages/ironpython3/issues/1082"""
+
+        def foo():
+            [x for y in range(2)]
+            x = 2
+        with self.assertRaises(NameError) as cm:
+            foo()
+        self.assertNotIsInstance(cm.exception, UnboundLocalError)
+        self.assertEqual(cm.exception.args, ("free variable 'x' referenced before assignment in enclosing scope",))
+
 run_test(__name__)