Update name binding code and comments (#1066)

Author: BCSharp

Src/IronPython/Compiler/Ast/Comprehension.cs

@@ -251,7 +251,7 @@ internal override PythonVariable BindReference(PythonNameBinder binder, PythonRe
         }
 
         internal override Ast GetVariableExpression(PythonVariable variable) {
-            if (variable.IsGlobal) {
+            if (variable.Kind is VariableKind.Global) {
                 return GlobalParent.ModuleVariables[variable];
             }
 

Src/IronPython/Compiler/Ast/FlowChecker.cs

@@ -56,7 +56,7 @@
  * 
  * The bit arrays in the flow checker hold the state and upon encountering NameExpr we figure
  * out whether the name has not yet been initialized at all (in which case we need to emit the
- * first explicit assignment to Uninitialized.instance and guard the use with an inlined check
+ * first explicit assignment to Uninitialized.instance and guard the use with an inlined check)
  * or whether it is definitely assigned (we don't need to inline the check)
  * or whether it may be uninitialized, in which case we must only guard the use by inlining the Uninitialized check
  * 
@@ -97,7 +97,7 @@ public override bool Walk(IndexExpression node) {
             node.Walk(_fc);
             return false;
         }
-        
+
         public override bool Walk(ParenthesisExpression node) {
             return true;
         }
@@ -109,14 +109,13 @@ public override bool Walk(TupleExpression node) {
         public override bool Walk(ListExpression node) {
             return true;
         }
-        
+
         public override bool Walk(Parameter node) {
             _fc.Define(node.Name);
             return true;
         }
     }
 
-    // TODO: probably obsolete, PythonVariable.Deleted does the job better (across scopes)
     internal class FlowDeleter : PythonWalkerNonRecursive {
         private readonly FlowChecker _fc;
 

Src/IronPython/Compiler/Ast/PythonAst.cs

@@ -276,13 +276,20 @@ internal ModuleContext ModuleContext {
         /// </summary>
         internal PythonVariable/*!*/ EnsureGlobalVariable(string name) {
             PythonVariable variable;
-            if (!TryGetVariable(name, out variable)) {
+            if (TryGetVariable(name, out variable)) {
+                variable.LiftToGlobal();
+            } else {
                 variable = CreateVariable(name, VariableKind.Global);
             }
 
             return variable;
         }
 
+        internal override MSAst.Expression GetVariableExpression(PythonVariable variable) {
+            Debug.Assert(_globalVariables.ContainsKey(variable));
+            return _globalVariables[variable];
+        }
+
         #endregion
 
         #region MSASt.Expression Overrides

Src/IronPython/Compiler/Ast/PythonNameBinder.cs

@@ -11,28 +11,35 @@
 using Microsoft.Scripting.Runtime;
 using Microsoft.Scripting.Utils;
 
+using IronPython.Runtime;
+
 /*
  * The name binding:
  *
- * The name binding happens in 2 passes.
- * In the first pass (full recursive walk of the AST) we resolve locals.
- * The second pass uses the "processed" list of all context statements (functions and class
- * bodies) and has each context statement resolve its free variables to determine whether
- * they are globals or references to lexically enclosing scopes.
+ * The name binding happens in 4 passes.
+ *
+ * The first pass is a full recursive walk of the AST. During this walk, all scopes are established
+ * (created by functions, classes, comprehensions, generators, and the AST root),
+ * and in each scope all variables are collected that are defined (local or parameter)
+ * or declared (global or nonlocal) there. Also, for each scope, all references used in the
+ * scope are collected but kept unresolved as yet.
+ *
+ * The second pass uses the collected stack of all scopes and has each scope resolve its references.
+ * The references can be resolved locally within the scope or as free variables, 
+ * either as globals or as references to lexically enclosing scopes.
  *
- * The second pass happens in post-order (the context statement is added into the "processed"
- * list after processing its nested functions/statements). This way, when the function is
- * processing its free variables, it also knows already which of its locals are being lifted
- * to the closure and can report error if such closure variable is being deleted.
+ * The second pass happens in post-order (a scope is processed after processing all its nested scopes).
+ * Consequently, when the scope is processing its free variables, it also knows already
+ * which of its locals are being lifted to the closure.
  *
- * This is illegal in Python:
+ * The third pass goes over all the scopes again, this time in pre-order (except for the root AST).
+ * In this pass, all scopes build theirs closures, if needed. Becasue nested scopes are processed
+ * after their encompassing scope, scopes may use the already prepared closure
+ * from their parent.
  *
- * def f():
- *     x = 10
- *     if (cond): del x        # illegal because x is a closure variable
- *     def g():
- *         print x
- * TODO: the example above is no longer valid; also the description of the name binding algorithm does not match the code
+ * During the fourth pass, each scope is being inspected by the FlowChecker,
+ * which analyzes the data flow in the scope. Information collected during this analysis
+ * allows for some optimizations later on, when the expression trees are being constructed.
  */
 
 namespace IronPython.Compiler.Ast {
@@ -58,37 +65,6 @@ public override bool Walk(ListExpression node) {
         }
     }
 
-    internal class ParameterBinder : PythonWalkerNonRecursive {
-        private PythonNameBinder _binder;
-        public ParameterBinder(PythonNameBinder binder) {
-            _binder = binder;
-        }
-
-        public override bool Walk(Parameter node) {
-            node.Parent = _binder._currentScope;
-            node.PythonVariable = _binder.DefineParameter(node.Name);
-            return false;
-        }
-
-        // TODO: obsolete?
-        private void WalkTuple(TupleExpression tuple) {
-            tuple.Parent = _binder._currentScope;
-            foreach (Expression innerNode in tuple.Items) {
-                if (innerNode is NameExpression name) {
-                    _binder.DefineName(name.Name);
-                    name.Parent = _binder._currentScope;
-                    name.Reference = _binder.Reference(name.Name);
-                } else {
-                    WalkTuple((TupleExpression)innerNode);
-                }
-            }
-        }
-        public override bool Walk(TupleExpression node) {
-            node.Parent = _binder._currentScope;
-            return true;
-        }
-    }
-
     internal class DeleteBinder : PythonWalkerNonRecursive {
         private PythonNameBinder _binder;
         public DeleteBinder(PythonNameBinder binder) {
@@ -111,7 +87,6 @@ internal class PythonNameBinder : PythonWalker {
 
         private readonly DefineBinder _define;
         private readonly DeleteBinder _delete;
-        private readonly ParameterBinder _parameter;
 
         #endregion
 
@@ -120,7 +95,6 @@ internal class PythonNameBinder : PythonWalker {
         private PythonNameBinder(CompilerContext context) {
             _define = new DefineBinder(this);
             _delete = new DeleteBinder(this);
-            _parameter = new ParameterBinder(this);
             Context = context;
         }
 
@@ -197,12 +171,11 @@ internal PythonVariable DefineDeleted(string name) {
         }
 
         internal void ReportSyntaxWarning(string message, Node node) {
-            Context.Errors.Add(Context.SourceUnit, message, node.Span, -1, Severity.Warning);
+            Context.Errors.Add(Context.SourceUnit, message, node.Span, ErrorCodes.SyntaxError, Severity.Warning);
         }
 
         internal void ReportSyntaxError(string message, Node node) {
-            // TODO: Change the error code (-1)
-            Context.Errors.Add(Context.SourceUnit, message, node.Span, -1, Severity.FatalError);
+            Context.Errors.Add(Context.SourceUnit, message, node.Span, ErrorCodes.SyntaxError, Severity.FatalError);
         }
 
         #region AstBinder Overrides
@@ -635,12 +608,12 @@ public override bool Walk(FromImportStatement node) {
         // FunctionDefinition
         public override bool Walk(FunctionDefinition node) {
             node._nameVariable = _globalScope.EnsureGlobalVariable("__name__");
-            
+
             // Name is defined in the enclosing context
             if (!node.IsLambda) {
                 node.PythonVariable = DefineName(node.Name);
             }
-            
+
             // process the default arg values in the outer context
             foreach (Parameter p in node.Parameters) {
                 p.DefaultValue?.Walk(this);
@@ -658,7 +631,8 @@ public override bool Walk(FunctionDefinition node) {
             PushScope(node);
 
             foreach (Parameter p in node.Parameters) {
-                p.Walk(_parameter);
+                p.Parent = _currentScope;
+                p.PythonVariable = DefineParameter(p.Name);
             }
 
             node.Body.Walk(this);
@@ -705,9 +679,8 @@ public override bool Walk(GlobalStatement node) {
                     ReportSyntaxError($"name '{n}' is used prior to global declaration", node);
                 }
 
-                // Create the variable in the global context and mark it as global
+                // Create the variable in the global context or mark it as global
                 PythonVariable variable = _globalScope.EnsureGlobalVariable(n);
-                variable.Kind = VariableKind.Global;
 
                 if (conflict == null) {
                     // no previously defined variables, add it to the current scope

Src/IronPython/Compiler/Ast/PythonVariable.cs

@@ -21,6 +21,7 @@ public class PythonVariable {
 
         public PythonVariable(string name, VariableKind kind, ScopeStatement/*!*/ scope) {
             Assert.NotNull(scope);
+            Debug.Assert(kind != VariableKind.Global || scope.IsGlobal);
             Debug.Assert(kind != VariableKind.Nonlocal || !scope.IsGlobal);
             Name = name;
             Kind = kind;
@@ -32,18 +33,22 @@ public PythonVariable(string name, VariableKind kind, ScopeStatement/*!*/ scope)
         /// </summary>
         public string Name { get; }
 
-        public bool IsGlobal {
-            get {
-                return Kind == VariableKind.Global || Scope.IsGlobal;
-            }
-        }
-
         /// <summary>
         /// The original scope in which the variable is defined.
         /// </summary>
         public ScopeStatement Scope { get; }
 
-        public VariableKind Kind { get; set; } // TODO: make readonly
+        public VariableKind Kind { get; private set; }
+
+        /// <summary>
+        /// Transform a local kind of variable in a global scope (yes, they can be created)
+        /// into a global kind, hence explicitly accessible to nested local scopes.
+        /// </summary>
+        internal void LiftToGlobal() {
+            Debug.Assert(Scope.IsGlobal);
+            Debug.Assert(Kind is not VariableKind.Parameter and not VariableKind.Nonlocal);
+            Kind = VariableKind.Global;
+        }
 
         /// <summary>
         /// The actual variable represented by this variable instance.

Src/IronPython/Compiler/Ast/ScopeStatement.cs

@@ -384,8 +384,7 @@ internal virtual void FinishBind(PythonNameBinder binder) {
             if (Variables != null) {
                 foreach (PythonVariable variable in Variables.Values) {
                     if (!HasClosureVariable(closureVariables, variable) &&
-                        !variable.IsGlobal &&
-                        variable.Kind != VariableKind.Nonlocal &&
+                        variable.Kind is not VariableKind.Global and not VariableKind.Nonlocal &&
                         (variable.AccessedInNestedScope || ExposesLocalVariable(variable))) {
 
                         if (closureVariables == null) {
@@ -700,7 +699,7 @@ private MSAst.Expression GetClosureCell(ClosureInfo variable) {
 
         internal virtual MSAst.Expression GetVariableExpression(PythonVariable variable) {
             Assert.NotNull(variable?.LimitVariable);
-            if (variable.IsGlobal) {
+            if (variable.Kind is VariableKind.Global) {
                 return GlobalParent.ModuleVariables[variable];
             }
 

Src/IronPython/Compiler/Ast/SerializedScopeStatement.cs

@@ -15,6 +15,7 @@
 namespace IronPython.Compiler.Ast {
     /// <summary>
     /// Fake ScopeStatement for FunctionCode's to hold on to after we have deserialized pre-compiled code.
+    /// Will never be a subject of name binding.
     /// </summary>
     internal class SerializedScopeStatement : ScopeStatement {
         private readonly string _name;
@@ -74,29 +75,15 @@ public override void Walk(PythonWalker walker) {
             throw new InvalidOperationException();
         }
 
-        public override string Name {
-            get {
-                return _name;
-            }
-        }
+        public override string Name => _name;
 
-        internal override string Filename {
-            get {
-                return _filename;
-            }
-        }
+        internal override string Filename => _filename;
 
-        internal override FunctionAttributes Flags {
-            get {
-                return _flags;
-            }
-        }
+        internal override FunctionAttributes Flags => _flags;
 
-        internal override string[] ParameterNames {
-            get {
-                return _parameterNames;
-            }
-        }
+        internal override bool IsGlobal => true;
+
+        internal override string[] ParameterNames => _parameterNames;
 
         internal override int ArgCount => _argCount;
 

Tests/hosting/editor_svcs/test_errorlistener.py

@@ -137,7 +137,7 @@ def test_multiple_erroneous_statements(self):
 
     def test_warning(self):
         expected = [
-            ("name 'a' is assigned to before global declaration", "global a", -1, self.FatalError), # reports as error since 3.6
+            ("name 'a' is assigned to before global declaration", "global a", 0x10, self.FatalError), # reports as error since 3.6
         ]
         code = """\
 def foo():
@@ -148,8 +148,8 @@ def foo():
     def test_should_report_multiple_warnings_negative(self):
         "Bug #17541, http://www.codeplex.com/IronPython/WorkItem/View.aspx?WorkItemId=17541"
         expected = [
-            ("Variable a assigned before global declaration", "global a", -1, self.Warning),
-            ("Variable b assigned before global declaration", "global b", -1, self.Warning),
+            ("Variable a assigned before global declaration", "global a", 0x10, self.Warning),
+            ("Variable b assigned before global declaration", "global b", 0x10, self.Warning),
         ]
         code = """\
 def foo():
@@ -164,8 +164,8 @@ def bar():
     def test_should_report_both_errors_and_warnings_negative(self):
         "Bug #17541, http://www.codeplex.com/IronPython/WorkItem/View.aspx?WorkItemId=17541"
         expected = [
-            ("can't assign to keyword", "None", -1, self.Error),
-            ("Variable a assigned before global declaration", "global a", -1, self.Warning),
+            ("can't assign to keyword", "None", 0x10, self.Error),
+            ("Variable a assigned before global declaration", "global a", 0x10, self.Warning),
         ]
         code = """\
 None = 2

WhatsNewInPython30.md

@@ -46,7 +46,7 @@ New Syntax
 - [ ] [PEP 3107][]: Function argument and return value annotations. This provides a standardized way of annotating a function's parameters and return value. There are no semantics attached to such annotations except that they can be introspected at runtime using the `__annotations__` attribute. The intent is to encourage experimentation through metaclasses, decorators or frameworks.
 - [ ] [PEP 3102][]: Keyword-only arguments. Named parameters occurring after `*args` in the parameter list must be specified using keyword syntax in the call. You can also use a bare `*` in the parameter list to indicate that you don't accept a variable-length argument list, but you do have keyword-only arguments.
 - [ ] Keyword arguments are allowed after the list of base classes in a class definition. This is used by the new convention for specifying a metaclass (see next section), but can be used for other purposes as well, as long as the metaclass supports it.
-- [ ] [PEP 3104][]: `nonlocal` statement. Using `nonlocal x` you can now assign directly to a variable in an outer (but non-`global`) scope. `nonlocal` is a new reserved word.
+- [x] [PEP 3104][]: `nonlocal` statement. Using `nonlocal x` you can now assign directly to a variable in an outer (but non-`global`) scope. `nonlocal` is a new reserved word.
 - [ ] [PEP 3132][]: Extended Iterable Unpacking. You can now write things like `a, b, *rest = some_sequence`. And even `*rest, a = stuff`. The `rest` object is always a (possibly empty) list; the right-hand side may be any iterable
 - [x] Dictionary comprehensions: `{k: v for k, v in stuff}` means the same thing as `dict(stuff)` but is more flexible. (This is [PEP 0274][] vindicated. :-)
 - [x] Set literals, e.g. `{1, 2}`. Note that `{}` is an empty dictionary; use `set()` for an empty set. Set comprehensions are also supported; e.g., `{x for x in stuff}` means the same thing as `set(stuff)` but is more flexible.