Use CommonDictionaryStorage for class variables (#1531)

Author: BCSharp

Src/IronPython/Compiler/Ast/ClassDefinition.cs

@@ -5,6 +5,7 @@
 using System;
 using System.Collections.Generic;
 using System.Collections.ObjectModel;
+using System.Diagnostics;
 using System.Diagnostics.CodeAnalysis;
 using System.Linq;
 using System.Runtime.CompilerServices;
@@ -97,13 +98,16 @@ internal override bool HasLateBoundVariableSets {
             }
         }
 
-        internal override bool ExposesLocalVariable(PythonVariable variable) => true;
+        internal override bool ExposesLocalVariable(PythonVariable variable) {
+            Debug.Assert(variable.Name == "__class__");
+            return true;
+        }
 
 
         internal override bool TryBindOuter(ScopeStatement from, PythonReference reference, out PythonVariable variable) {
             if (reference.Name == "__class__") {
+                ClassVariable = variable = EnsureClassVariable();
                 ClassCellVariable = EnsureVariable("__classcell__");
-                ClassVariable = variable = EnsureVariable(reference.Name);
                 variable.AccessedInNestedScope = true;
                 from.AddFreeVariable(variable, true);
                 for (ScopeStatement scope = from.Parent; scope != this; scope = scope.Parent) {
@@ -122,18 +126,19 @@ internal override bool TryBindOuter(ScopeStatement from, PythonReference referen
             // Python semantics: The variables bound local in the class
             // scope are accessed by name - the dictionary behavior of classes
             if (TryGetVariable(reference.Name, out variable)) {
-                // TODO: This results in doing a dictionary lookup to get/set the local,
-                // when it should probably be an uninitialized check / global lookup for gets
-                // and a direct set
-                if (variable.Kind == VariableKind.Global) {
+                if (variable.Kind is VariableKind.Global) {
                     AddReferencedGlobal(reference.Name);
-                } else if (variable.Kind == VariableKind.Local) {
+                } else if (variable.Kind is VariableKind.Local) {
+                    // TODO: This results in doing a dictionary lookup to get/set the local,
+                    // when it should probably be an uninitialized check / global lookup for gets
+                    // and a direct set
                     return null;
-                }
-
-                if (variable.Kind != VariableKind.Nonlocal) {
+                } else if (variable.Kind is VariableKind.Attribute) {
+                    return null; // fall back on LookupName/SetName in local context dict, which is faster than LookupGlobalVariable
+                } else if (variable.Kind is VariableKind.Parameter) {
                     return variable;
                 }
+                // else NonLocal: continue binding
             }
 
             // Try to bind in outer scopes, if we have an unqualified exec we need to leave the
@@ -148,6 +153,20 @@ internal override bool TryBindOuter(ScopeStatement from, PythonReference referen
             return null;
         }
 
+        internal override PythonVariable EnsureVariable(string name) {
+            if (TryGetVariable(name, out PythonVariable variable)) {
+                return variable;
+            }
+            return CreateVariable(name, VariableKind.Attribute);
+        }
+
+        internal PythonVariable EnsureClassVariable() {
+            if (TryGetVariable("$__class__", out PythonVariable variable)) {
+                return variable;
+            }
+            return CreateVariable("__class__", VariableKind.Local, "$__class__");
+        }
+
         internal override Ast LookupVariableExpression(PythonVariable variable) {
             // Emulates opcode LOAD_CLASSDEREF
             return Ast.Call(
@@ -240,6 +259,9 @@ static MSAst.Expression UnpackKeywordsHelper(MSAst.Expression context, ReadOnlyS
             }
         }
 
+        private MSAst.Expression SetLocalName(string name, MSAst.Expression expression)
+            => Ast.Call(AstMethods.SetName, LocalContext, Ast.Constant(name), expression);
+
         private Microsoft.Scripting.Ast.LightExpression<Func<CodeContext, CodeContext>> MakeClassBody() {
             // we always need to create a nested context for class defs            
 
@@ -260,15 +282,15 @@ private Microsoft.Scripting.Ast.LightExpression<Func<CodeContext, CodeContext>>
             init.Add(Ast.Assign(LocalCodeContextVariable, createLocal));
 
             // __module__ = __name__
-            MSAst.Expression modStmt = AssignValue(GetVariableExpression(ModVariable!), GetVariableExpression(ModuleNameVariable!));
+            MSAst.Expression modStmt = SetLocalName("__module__", AstUtils.Convert(GetVariableExpression(ModuleNameVariable!), typeof(object)));
 
             // TODO: set __qualname__
 
             // __doc__ = """..."""
             MSAst.Expression? docStmt = null;
             string doc = GetDocumentation(Body);
             if (doc is not null) {
-                docStmt = AssignValue(GetVariableExpression(DocVariable!), AstUtils.Constant(doc));
+                docStmt = SetLocalName("__doc__", Ast.Constant(doc, typeof(object)));
             }
 
             // Create the body
@@ -280,9 +302,9 @@ private Microsoft.Scripting.Ast.LightExpression<Func<CodeContext, CodeContext>>
 
             // __classcell__ == ClosureCell(__class__)
             MSAst.Expression? assignClassCellStmt = null;
-            if (ClassCellVariable is not null) {
-                var exp = (ClosureExpression)GetVariableExpression(ClassVariable!);
-                assignClassCellStmt = AssignValue(GetVariableExpression(ClassCellVariable), exp.ClosureCell);
+            if (ClassVariable is not null) {
+                var exp = (ClosureExpression)GetVariableExpression(ClassVariable);
+                assignClassCellStmt = AssignValue(GetVariableExpression(ClassCellVariable!), exp.ClosureCell);
             }
 
             bodyStmt = WrapScopeStatements(

Src/IronPython/Compiler/Ast/ScopeStatement.cs

@@ -311,7 +311,7 @@ internal virtual MSAst.Expression GetParentClosureTuple() {
             throw new NotSupportedException();
         }
 
-        private bool TryGetAnyVariable(string name, out PythonVariable variable) {
+        internal bool TryGetVariable(string name, out PythonVariable variable) {
             if (Variables != null) {
                 return Variables.TryGetValue(name, out variable);
             } else {
@@ -320,15 +320,6 @@ private bool TryGetAnyVariable(string name, out PythonVariable variable) {
             }
         }
 
-        internal bool TryGetVariable(string name, out PythonVariable variable) {
-            if (TryGetAnyVariable(name, out variable)) {
-                return true;
-            } else {
-                variable = null;
-                return false;
-            }
-        }
-
         internal virtual bool TryBindOuter(ScopeStatement from, PythonReference reference, out PythonVariable variable) {
             // Hide scope contents by default (only functions expose their locals)
             variable = null;
@@ -381,7 +372,7 @@ internal virtual void FinishBind(PythonNameBinder binder) {
             if (Variables != null) {
                 foreach (PythonVariable variable in Variables.Values) {
                     if (!HasClosureVariable(closureVariables, variable) &&
-                        variable.Kind is not VariableKind.Global and not VariableKind.Nonlocal &&
+                        variable.Kind is VariableKind.Local or VariableKind.Parameter &&
                         (variable.AccessedInNestedScope || ExposesLocalVariable(variable))) {
 
                         if (closureVariables == null) {
@@ -398,6 +389,8 @@ variable.Kind is not VariableKind.Global and not VariableKind.Nonlocal &&
                         } else {
                             _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
                     }
                 }
             }
@@ -461,15 +454,15 @@ private bool TryGetNonlocalStatement(string name, out NonlocalStatement node) {
             return _nonlocalVars?.TryGetValue(name, out node) ?? false;
         }
 
-        internal PythonVariable/*!*/ CreateVariable(string name, VariableKind kind) {
+        internal PythonVariable/*!*/ CreateVariable(string name, VariableKind kind, string key = null) {
             EnsureVariables();
-            Debug.Assert(!Variables.ContainsKey(name));
+            Debug.Assert(!Variables.ContainsKey(key ?? name));
             PythonVariable variable;
-            Variables[name] = variable = new PythonVariable(name, kind, this);
+            Variables[key ?? name] = variable = new PythonVariable(name, kind, this);
             return variable;
         }
 
-        internal PythonVariable/*!*/ EnsureVariable(string name) {
+        internal virtual PythonVariable/*!*/ EnsureVariable(string name) {
             PythonVariable variable;
             if (!TryGetVariable(name, out variable)) {
                 return CreateVariable(name, VariableKind.Local);

Src/IronPython/Compiler/Ast/VariableKind.cs

@@ -35,6 +35,15 @@ public enum VariableKind {
         ///
         /// Provides a by-reference access to a local variable in an outer scope.
         /// </summary>
-        Nonlocal
+        Nonlocal,
+
+        /// <summary>
+        /// Attrribute variable.
+        ///
+        /// Like a local variable, but is stored directly in the context dictionary,
+        /// rather than a closure cell.
+        /// Should only appear in a class lambda.
+        /// </summary>
+        Attribute
     }
 }

Tests/test_metaclass.py

@@ -323,7 +323,7 @@ def getclass(self):
 
         if is_cli:
             # TODO: Use MyDict as the namespace for the class body lambda
-            self.assertEqual(attributes, ['__doc__', '__module__', 'getclass', '__classcell__'])
+            self.assertEqual(set(attributes), {'__module__', '__doc__', 'getclass', '__classcell__'})
         else:
             self.assertEqual(attributes, ['__module__', '__qualname__', '__doc__', 'getclass', '__classcell__'])
 

Tests/test_super.py

@@ -2,13 +2,14 @@
 # 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.
 #
-# Copyright (c) Michael van der Kolff
+# Copyright (c) Michael van der Kolff and contributors
 #
 
 ##
 ## Test whether super() behaves as expected
 ##
 
+import sys
 import warnings
 from test.support import check_warnings