Resolve obscure super errors (#1535)

BCSharp · web-flow · commit 894df46b20f4 · 2022-08-28T18:35:51.000-07:00
* Resolve obscure super errors

* Add classcell generation tests

* Add obscure super tests

* Fix some typos

* Improve perfomance

* Trigger class cell creation on any super reference
diff --git a/Src/IronPython/Compiler/Ast/AstMethods.cs b/Src/IronPython/Compiler/Ast/AstMethods.cs
@@ -72,7 +72,7 @@ internal static class AstMethods {
         public static readonly MethodInfo MakeSet = GetMethod((Func<object[], SetCollection>)PythonOps.MakeSet);
         public static readonly MethodInfo MakeEmptySet = GetMethod((Func<SetCollection>)PythonOps.MakeEmptySet);
         public static readonly MethodInfo MakeHomogeneousDictFromItems = GetMethod((Func<object[], PythonDictionary>)PythonOps.MakeHomogeneousDictFromItems);
-        public static readonly MethodInfo CreateLocalContext = GetMethod((Func<CodeContext, MutableTuple, string[], CodeContext>)PythonOps.CreateLocalContext);
+        public static readonly MethodInfo CreateLocalContext = GetMethod((Func<CodeContext, MutableTuple, string[], int, int, CodeContext>)PythonOps.CreateLocalContext);
         public static readonly MethodInfo UpdateStackTrace = GetMethod((Action<Exception, CodeContext, FunctionCode, int>)PythonOps.UpdateStackTrace);
         public static readonly MethodInfo ForLoopDispose = GetMethod((Action<KeyValuePair<IEnumerator, IDisposable>>)PythonOps.ForLoopDispose);
         public static readonly MethodInfo GetClosureTupleFromContext = GetMethod((Func<CodeContext, MutableTuple>)PythonOps.GetClosureTupleFromContext);
diff --git a/Src/IronPython/Compiler/Ast/CallExpression.cs b/Src/IronPython/Compiler/Ast/CallExpression.cs
@@ -24,7 +24,6 @@ namespace IronPython.Compiler.Ast {
     public class CallExpression : Expression, IInstructionProvider {
         private readonly Expression[] _args;
         private readonly Keyword[] _kwargs;
-        private Expression[]? _implicitArgs;
 
         public CallExpression(Expression target, IReadOnlyList<Expression>? args, IReadOnlyList<Keyword>? kwargs) {
             Target = target;
@@ -38,10 +37,6 @@ public CallExpression(Expression target, IReadOnlyList<Expression>? args, IReadO
 
         public IReadOnlyList<Keyword> Kwargs => _kwargs;
 
-        internal void SetImplicitArgs(params Expression[] args) {
-            _implicitArgs = args;
-        }
-
         public bool NeedsLocalsDictionary() {
             if (!(Target is NameExpression nameExpr)) return false;
 
@@ -61,10 +56,6 @@ public override MSAst.Expression Reduce() {
             ReadOnlySpan<Expression> args = _args;
             ReadOnlySpan<Keyword> kwargs = _kwargs;
 
-            if (args.Length == 0 && _implicitArgs != null) {
-                args = _implicitArgs;
-            }
-
             // count splatted list args and find the lowest index of a list argument, if any
             ScanArgs(args, out var numList, out int firstListPos);
             Debug.Assert(numList == 0 || firstListPos < args.Length);
@@ -188,9 +179,6 @@ static MSAst.Expression UnpackDictHelper(MSAst.Expression context, ReadOnlySpan<
 
         void IInstructionProvider.AddInstructions(LightCompiler compiler) {
             ReadOnlySpan<Expression> args = _args;
-            if (args.Length == 0 && _implicitArgs != null) {
-                args = _implicitArgs;
-            }
 
             if (Kwargs.Count > 0) {
                 compiler.Compile(Reduce());
@@ -430,11 +418,6 @@ public override int Run(InterpretedFrame frame) {
         public override void Walk(PythonWalker walker) {
             if (walker.Walk(this)) {
                 Target?.Walk(walker);
-                if (_implicitArgs is not null) {
-                    foreach (var arg in _implicitArgs) {
-                        arg.Walk(walker);
-                    }
-                }
                 foreach (var arg in _args) {
                     arg.Walk(walker);
                 }
diff --git a/Src/IronPython/Compiler/Ast/FunctionDefinition.cs b/Src/IronPython/Compiler/Ast/FunctionDefinition.cs
@@ -145,7 +145,10 @@ internal override int KwOnlyArgCount {
         internal PythonVariable PythonVariable { get; set; }
 
         internal override bool ExposesLocalVariable(PythonVariable variable) {
-            return NeedsLocalsDictionary; 
+            return NeedsLocalsDictionary
+                || (ContainsSuperCall && variable.Kind is VariableKind.Parameter
+                        && _parameters is not null && _parameters.Length > 0
+                        && _parameters[0].PythonVariable == variable);
         }
 
         internal override FunctionAttributes Flags {
@@ -258,7 +261,7 @@ internal override void Bind(PythonNameBinder binder) {
         
         internal override void FinishBind(PythonNameBinder binder) {
             foreach (var param in _parameters) {
-                _variableMapping[param.PythonVariable] = param.FinishBind(NeedsLocalsDictionary);
+                _variableMapping[param.PythonVariable] = param.FinishBind(forceClosureCell: ExposesLocalVariable(param.PythonVariable));
             }
             base.FinishBind(binder);
         }
diff --git a/Src/IronPython/Compiler/Ast/Parameter.cs b/Src/IronPython/Compiler/Ast/Parameter.cs
@@ -60,8 +60,8 @@ public Expression DefaultValue {
 
         internal PythonVariable PythonVariable { get; set; }
 
-        internal MSAst.Expression FinishBind(bool needsLocalsDictionary) {
-            if (PythonVariable.AccessedInNestedScope || needsLocalsDictionary) {
+        internal MSAst.Expression FinishBind(bool forceClosureCell) {
+            if (PythonVariable.AccessedInNestedScope || forceClosureCell) {
                 ParameterExpression = Expression.Parameter(typeof(object), Name);
                 var cell = Ast.Parameter(typeof(ClosureCell), Name);
                 return new ClosureExpression(PythonVariable, cell, ParameterExpression);
diff --git a/Src/IronPython/Compiler/Ast/PythonNameBinder.cs b/Src/IronPython/Compiler/Ast/PythonNameBinder.cs
@@ -851,13 +851,13 @@ public override bool Walk(ComprehensionFor node) {
         public override bool Walk(CallExpression node) {
             node.Parent = _currentScope;
 
-            if (node.Target is NameExpression nameExpr && nameExpr.Name == "super" && _currentScope is FunctionDefinition func) {
-                _currentScope.Reference("__class__");
-                if (node.Args.Count == 0 && node.Kwargs.Count == 0 && func.ParameterNames.Length > 0) {
-                    node.SetImplicitArgs(new NameExpression("__class__"), new NameExpression(func.ParameterNames[0]));
-                }
+            if (node.Target is NameExpression nameExpr && nameExpr.Name == "super"
+                && _currentScope is not ClassDefinition
+                && node.Args.Count == 0 && node.Kwargs.Count == 0) {
+
+                _currentScope.ContainsSuperCall = true;
             }
-            return base.Walk(node);
+            return true;
         }
 
         #endregion
diff --git a/Src/IronPython/Compiler/Ast/PythonReference.cs b/Src/IronPython/Compiler/Ast/PythonReference.cs
@@ -2,6 +2,8 @@
 // The .NET Foundation licenses this file to you under the Apache 2.0 License.
 // See the LICENSE file in the project root for more information.
 
+#nullable enable
+
 namespace IronPython.Compiler.Ast {
     /// <summary>
     /// Represents a reference to a name.  A PythonReference is created for each name
@@ -14,6 +16,6 @@ public PythonReference(string name) {
 
         public string Name { get; }
 
-        internal PythonVariable PythonVariable { get; set; }
+        internal PythonVariable? PythonVariable { get; set; }
     }
 }
diff --git a/Src/IronPython/Compiler/Ast/ScopeStatement.cs b/Src/IronPython/Compiler/Ast/ScopeStatement.cs
@@ -93,6 +93,13 @@ internal bool IsClosure {
         /// </summary>
         internal bool NeedsLocalsDictionary { get; set; }
 
+        /// <summary>
+        /// True if this scope contains a parameterless call to super().
+        ///
+        /// It is used to ensure that the first argument is accessible through a closure cell.
+        /// </summary>
+        internal bool ContainsSuperCall{ get; set; }
+
         public virtual string Name {
             get {
                 return "<unknown>";
@@ -124,7 +131,7 @@ internal virtual bool IsGlobal {
 
         internal bool NeedsLocalContext {
             get {
-                return NeedsLocalsDictionary || ContainsNestedFreeVariables;
+                return NeedsLocalsDictionary || ContainsNestedFreeVariables || ContainsSuperCall;
             }
         }
 
@@ -350,6 +357,7 @@ internal virtual void FinishBind(PythonNameBinder binder) {
                 Debug.Assert(parentClosure != null);
 
                 foreach (var variable in FreeVariables) {
+                    Debug.Assert(!HasClosureVariable(closureVariables, variable));
                     for (int i = 0; i < parentClosure.Length; i++) {
                         if (parentClosure[i].Variable == variable) {
                             Ast prop = LocalParentTuple;
@@ -362,22 +370,19 @@ internal virtual void FinishBind(PythonNameBinder binder) {
                     }
                     Debug.Assert(_variableMapping.ContainsKey(variable));
 
-                    if (closureVariables == null) {
-                        closureVariables = new List<ClosureInfo>();
-                    }
-                    closureVariables.Add(new ClosureInfo(variable, !(this is ClassDefinition)));
+                    closureVariables ??= new List<ClosureInfo>();
+                    closureVariables.Add(new ClosureInfo(variable, this is not ClassDefinition));
                 }
             }
 
             if (Variables != null) {
                 foreach (PythonVariable variable in Variables.Values) {
-                    if (!HasClosureVariable(closureVariables, variable) &&
-                        variable.Kind is VariableKind.Local or VariableKind.Parameter &&
+                    if (variable.Kind is VariableKind.Local or VariableKind.Parameter &&
                         (variable.AccessedInNestedScope || ExposesLocalVariable(variable))) {
 
-                        if (closureVariables == null) {
-                            closureVariables = new List<ClosureInfo>();
-                        }
+                        Debug.Assert(!HasClosureVariable(closureVariables, variable));
+
+                        closureVariables ??= new List<ClosureInfo>();
                         closureVariables.Add(new ClosureInfo(variable, true));
                     }
 
@@ -390,7 +395,9 @@ variable.Kind is VariableKind.Local or VariableKind.Parameter &&
                             _variableMapping[variable] = Ast.Parameter(typeof(object), variable.Name);
                         }
                     } else if (variable.Kind == VariableKind.Attribute) {
-                        _variableMapping[variable] = new LookupGlobalVariable(LocalContext, variable.Name, isLocal: true); // TODO: If no user-supplied dictionary is in place, optimize to use more efficient access, see CollectableCompilationMode
+                        // If no user-supplied dictionary is in place, a more efficient access is possible, see CollectableCompilationMode
+                        // However, this would probably have a negligible effect in this case.
+                        _variableMapping[variable] = new LookupGlobalVariable(LocalContext, variable.Name, isLocal: true);
                     }
                 }
             }
@@ -430,12 +437,12 @@ internal void AddGlobalVariable(PythonVariable variable) {
         }
 
         internal PythonReference Reference(string name) {
-            if (_references == null) {
-                _references = new Dictionary<string, PythonReference>(StringComparer.Ordinal);
-            }
-            PythonReference reference;
-            if (!_references.TryGetValue(name, out reference)) {
+            _references ??= new Dictionary<string, PythonReference>(StringComparer.Ordinal);
+            if (!_references.TryGetValue(name, out PythonReference reference)) {
                 _references[name] = reference = new PythonReference(name);
+                if (name == "super" && this is not ClassDefinition) {
+                    Reference("__class__");
+                }
             }
             return reference;
         }
@@ -671,15 +678,26 @@ internal MSAst.Expression FuncCodeExpr {
         }
 
         internal MSAst.MethodCallExpression CreateLocalContext(MSAst.Expression parentContext) {
-            var closureVariables = _closureVariables;
-            if (_closureVariables == null) {
-                closureVariables = new ClosureInfo[0];
+            var closureVariables = _closureVariables ?? Array.Empty<ClosureInfo>();
+            
+            int numFreeVars = FreeVariables?.Count ?? 0;
+            int firstArgIdx = -1;
+            if ((NeedsLocalsDictionary || ContainsSuperCall) && ArgCount > 0) {
+                for (int idx = numFreeVars; idx < closureVariables.Length; idx++) {
+                    if (closureVariables[idx].Variable.Kind == VariableKind.Parameter) {
+                        firstArgIdx = idx;
+                        break;
+                    }
+                }
             }
+
             return Ast.Call(
                 AstMethods.CreateLocalContext,
                 parentContext,
                 MutableTuple.Create(ArrayUtils.ConvertAll(closureVariables, x => GetClosureCell(x))),
-                Ast.Constant(ArrayUtils.ConvertAll(closureVariables, x => x.AccessedInScope ? x.Variable.Name : null))
+                Ast.Constant(ArrayUtils.ConvertAll(closureVariables, x => x.AccessedInScope ? x.Variable.Name : null)),
+                AstUtils.Constant(numFreeVars),
+                AstUtils.Constant(firstArgIdx)
             );
         }
 
diff --git a/Src/IronPython/Runtime/Operations/PythonOps.cs b/Src/IronPython/Runtime/Operations/PythonOps.cs
@@ -3622,10 +3622,10 @@ public static object SetName(CodeContext/*!*/ context, string name, object value
 
         #region Global Access
 
-        public static CodeContext/*!*/ CreateLocalContext(CodeContext/*!*/ outerContext, MutableTuple boxes, string[] args) {
+        public static CodeContext/*!*/ CreateLocalContext(CodeContext/*!*/ outerContext, MutableTuple boxes, string[] args, int numFreeVars, int arg0Idx) {
             return new CodeContext(
                 new PythonDictionary(
-                    new RuntimeVariablesDictionaryStorage(boxes, args)
+                    new RuntimeVariablesDictionaryStorage(boxes, args, numFreeVars, arg0Idx)
                 ),
                 outerContext.ModuleContext
             );
diff --git a/Src/IronPython/Runtime/RuntimeVariablesDictionaryStorage.cs b/Src/IronPython/Runtime/RuntimeVariablesDictionaryStorage.cs
@@ -4,30 +4,54 @@
 
 using System;
 using System.Collections.Generic;
+using System.Diagnostics;
+
 using Microsoft.Scripting;
 using Microsoft.Scripting.Runtime;
 
 namespace IronPython.Runtime {
     internal class RuntimeVariablesDictionaryStorage : CustomDictionaryStorage {
         private readonly MutableTuple _boxes;
         private readonly string[] _args;
+        private readonly int _numFreeVars;
+        private readonly int _arg0Idx;
+
+        public RuntimeVariablesDictionaryStorage(MutableTuple boxes, string[] args, int numFreeVars, int arg0Idx) {
+            Debug.Assert(0 <= numFreeVars && numFreeVars <= args.Length);
+            Debug.Assert(arg0Idx == -1 || numFreeVars <= arg0Idx && arg0Idx < args.Length);
 
-        public RuntimeVariablesDictionaryStorage(MutableTuple boxes, string[] args) {
             _boxes = boxes;
             _args = args;
+            _numFreeVars = numFreeVars;
+            _arg0Idx = arg0Idx;
         }
 
-        internal MutableTuple Tuple {
-            get {
-                return _boxes;
-            }
-        }
+        /// <summary>
+        /// Closure tuple.
+        /// </summary>
+        internal MutableTuple Tuple => _boxes;
 
-        internal string[] Names {
-            get {
-                return _args;
-            }
-        }
+        /// <summary>
+        /// Names of the variables in the closure.
+        /// Cell variables (not accessed in current scope) have null names.
+        /// </summary>
+        internal string[] Names => _args;
+
+        /// <summary>
+        /// Number of free variables in the closure.
+        /// If non-zero, cells for free variables are at the beginning of <see cref="Tuple"/>.
+        /// </summary>
+        internal int NumFreeVars => _numFreeVars;
+
+        /// <summary>
+        /// Index of the cell of the first positional parameter of the function call.
+        /// Value -1 means that information is not available.
+        /// </summary>
+        /// <remarks>
+        /// This information is intended to be consumed by a parameterless super() call.
+        /// For performance reasons, Arg0Idx is only tracked when deemed necessary to support super().
+        /// </remarks>
+        internal int Arg0Idx => _arg0Idx;
 
         protected override IEnumerable<KeyValuePair<string, object>> GetExtraItems() {
             for (int i = 0; i < _args.Length; i++) {
diff --git a/Src/IronPython/Runtime/Super.cs b/Src/IronPython/Runtime/Super.cs
@@ -29,8 +29,34 @@ public Super() {
 
         #region Python Public API Surface
 
-        public void __init__() {
-            throw PythonOps.RuntimeError("super(): no arguments");
+        public void __init__(CodeContext context) {
+            var vars = context.Dict._storage as RuntimeVariablesDictionaryStorage;
+
+            // Step 1: access arg[0]
+            if (vars is null || vars.Arg0Idx < 0) {
+                throw PythonOps.RuntimeError("super(): no arguments");
+            }
+
+            object? arg0 = vars.GetCell(vars.Arg0Idx).Value;
+            if (arg0 == Uninitialized.Instance) {
+                throw PythonOps.RuntimeError("super(): arg[0] deleted");
+            }
+
+            // Step 2: access __class__ cell
+            int idx = Array.IndexOf(vars.Names, "__class__");
+            if (idx < 0 || idx >= vars.NumFreeVars) {
+                throw PythonOps.RuntimeError("super(): __class__ cell not found");
+            }
+
+            object? cls = vars.GetCell(idx).Value;
+            if (cls == Uninitialized.Instance) {
+                throw PythonOps.RuntimeError("super(): empty __class__ cell");
+            }
+            if (cls is not PythonType type) {
+                throw PythonOps.RuntimeError("super(): __class__ is not a type ({0})", PythonOps.GetPythonTypeName(cls));
+            }
+
+            __init__(type, arg0);
         }
 
         public void __init__([NotNone] PythonType type) {
diff --git a/Tests/test_inheritance.py b/Tests/test_inheritance.py
@@ -1367,8 +1367,8 @@ def __new__(cls):
 
         c = C()
 
-    def test_super_errors(self):
-        # Test that TyypeError is raised, rather than pass or SystemError: Object reference not set to an instance of an object.
+    def test_super_type_errors(self):
+        # Test that TypeError is raised, rather than pass or SystemError: Object reference not set to an instance of an object.
 
         with self.assertRaises(TypeError):
             super(None)
diff --git a/Tests/test_super.py b/Tests/test_super.py

Original file line number	Diff line number	Diff line change
`@@ -145,7 +145,10 @@ internal override int KwOnlyArgCount {`
`145`	`145`	`internal PythonVariable PythonVariable { get; set; }`
`146`	`146`
`147`	`147`	`internal override bool ExposesLocalVariable(PythonVariable variable) {`
`148`		`- return NeedsLocalsDictionary;`
	`148`	`+ return NeedsLocalsDictionary`
	`149`	`+ \|\| (ContainsSuperCall && variable.Kind is VariableKind.Parameter`
	`150`	`+ && _parameters is not null && _parameters.Length > 0`
	`151`	`+ && _parameters[0].PythonVariable == variable);`
`149`	`152`	`}`
`150`	`153`
`151`	`154`	`internal override FunctionAttributes Flags {`
`@@ -258,7 +261,7 @@ internal override void Bind(PythonNameBinder binder) {`
`258`	`261`
`259`	`262`	`internal override void FinishBind(PythonNameBinder binder) {`
`260`	`263`	`foreach (var param in _parameters) {`
`261`		`- _variableMapping[param.PythonVariable] = param.FinishBind(NeedsLocalsDictionary);`
	`264`	`+ _variableMapping[param.PythonVariable] = param.FinishBind(forceClosureCell: ExposesLocalVariable(param.PythonVariable));`
`262`	`265`	`}`
`263`	`266`	`base.FinishBind(binder);`
`264`	`267`	`}`
Original file line number	Diff line number	Diff line change
`@@ -2,6 +2,8 @@`
`2`	`2`	`// The .NET Foundation licenses this file to you under the Apache 2.0 License.`
`3`	`3`	`// See the LICENSE file in the project root for more information.`
`4`	`4`
	`5`	`+#nullable enable`
	`6`	`+`
`5`	`7`	`namespace IronPython.Compiler.Ast {`
`6`	`8`	`/// <summary>`
`7`	`9`	`/// Represents a reference to a name. A PythonReference is created for each name`
`@@ -14,6 +16,6 @@ public PythonReference(string name) {`
`14`	`16`
`15`	`17`	`public string Name { get; }`
`16`	`18`
`17`		`- internal PythonVariable PythonVariable { get; set; }`
	`19`	`+ internal PythonVariable? PythonVariable { get; set; }`
`18`	`20`	`}`
`19`	`21`	`}`