Improve handling of __classcell__ (#1520)

BCSharp · web-flow · commit 0df498fc26e6 · 2022-08-02T20:21:04.000-07:00
* Copy class namespace when using metaclass

* Fix preconditions for type constructor

* Improve handling of __classcell__

* Reorder class __doc__ and __module__

* Add tests

* Change nullabiliy of CallPrepare return type

* Pass __classcell__ tests from StdLib 3.6

* Move __classcell__ tests from test_class to test_super
diff --git a/Src/IronPython/Compiler/Ast/ClassDefinition.cs b/Src/IronPython/Compiler/Ast/ClassDefinition.cs
@@ -99,11 +99,9 @@ internal override bool HasLateBoundVariableSets {
 
         internal override bool ExposesLocalVariable(PythonVariable variable) => true;
 
-        private bool needClassCell { get; set; }
 
         internal override bool TryBindOuter(ScopeStatement from, PythonReference reference, out PythonVariable variable) {
             if (reference.Name == "__class__") {
-                needClassCell = true;
                 ClassCellVariable = EnsureVariable("__classcell__");
                 ClassVariable = variable = EnsureVariable(reference.Name);
                 variable.AccessedInNestedScope = true;
@@ -260,44 +258,41 @@ private Microsoft.Scripting.Ast.LightExpression<Func<CodeContext, CodeContext>>
             var createLocal = CreateLocalContext(_parentContextParam);
 
             init.Add(Ast.Assign(LocalCodeContextVariable, createLocal));
-            // __classcell__ == ClosureCell(__class__)
-            if (needClassCell) {
-                var exp = (ClosureExpression) GetVariableExpression(ClassVariable!);
-                MSAst.Expression assignClassCell = AssignValue(GetVariableExpression(ClassCellVariable!), exp.ClosureCell);
-                init.Add(assignClassCell);
-            }
-
-            List<MSAst.Expression> statements = new List<MSAst.Expression>();
-            // Create the body
-            MSAst.Expression bodyStmt = Body;
-
 
             // __module__ = __name__
             MSAst.Expression modStmt = AssignValue(GetVariableExpression(ModVariable!), GetVariableExpression(ModuleNameVariable!));
 
+            // TODO: set __qualname__
+
+            // __doc__ = """..."""
+            MSAst.Expression? docStmt = null;
             string doc = GetDocumentation(Body);
-            if (doc != null) {
-                statements.Add(
-                    AssignValue(
-                        GetVariableExpression(DocVariable!),
-                        AstUtils.Constant(doc)
-                    )
-                );
+            if (doc is not null) {
+                docStmt = AssignValue(GetVariableExpression(DocVariable!), AstUtils.Constant(doc));
             }
 
+            // Create the body
+            MSAst.Expression bodyStmt = Body;
             if (Body.CanThrow && GlobalParent.PyContext.PythonOptions.Frames) {
                 bodyStmt = AddFrame(LocalContext, FuncCodeExpr, bodyStmt);
                 locals.Add(FunctionStackVariable);
             }
 
+            // __classcell__ == ClosureCell(__class__)
+            MSAst.Expression? assignClassCellStmt = null;
+            if (ClassCellVariable is not null) {
+                var exp = (ClosureExpression)GetVariableExpression(ClassVariable!);
+                assignClassCellStmt = AssignValue(GetVariableExpression(ClassCellVariable), exp.ClosureCell);
+            }
+
             bodyStmt = WrapScopeStatements(
                 Ast.Block(
                     Ast.Block(init),
-                    statements.Count == 0 ?
-                        EmptyBlock :
-                        Ast.Block(new ReadOnlyCollection<MSAst.Expression>(statements)),
                     modStmt,
+                    // __qualname__
+                    docStmt is not null ? docStmt : AstUtils.Empty(),
                     bodyStmt,
+                    assignClassCellStmt is not null ? assignClassCellStmt : AstUtils.Empty(),
                     LocalContext
                 ),
                 Body.CanThrow
diff --git a/Src/IronPython/Compiler/Ast/PythonNameBinder.cs b/Src/IronPython/Compiler/Ast/PythonNameBinder.cs
@@ -118,20 +118,20 @@ private void Bind(PythonAst unboundAst) {
             // Find all scopes and variables
             unboundAst.Walk(this);
 
-            // Bind
+            // Bind scopes
             foreach (ScopeStatement scope in _scopes) {
                 scope.Bind(this);
             }
 
-            // Finish the globals
+            // Bind globals
             unboundAst.Bind(this);
 
-            // Finish Binding w/ outer most scopes first.
+            // Finish binding w/ outer most scopes first.
             for (int i = _scopes.Count - 1; i >= 0; i--) {
                 _scopes[i].FinishBind(this);
             }
 
-            // Finish the globals
+            // Finish globals
             unboundAst.FinishBind(this);
 
             // Run flow checker
diff --git a/Src/IronPython/Runtime/Operations/PythonOps.cs b/Src/IronPython/Runtime/Operations/PythonOps.cs
@@ -1414,7 +1414,7 @@ public static void InitializeForFinalization(CodeContext/*!*/ context, object ne
             iwr.SetFinalizer(new WeakRefTracker(iwr, nif, nif));
         }
 
-        public static object MakeClass(FunctionCode funcCode, Func<CodeContext, CodeContext> body, CodeContext/*!*/ parentContext, string name, PythonTuple bases, PythonDictionary? keywords, string selfNames) {
+        public static object? MakeClass(FunctionCode funcCode, Func<CodeContext, CodeContext> body, CodeContext/*!*/ parentContext, string name, PythonTuple bases, PythonDictionary? keywords, string selfNames) {
             Func<CodeContext, CodeContext> func = GetClassCode(parentContext, funcCode, body);
 
             // Check and normalize bases
@@ -1458,6 +1458,14 @@ public static object MakeClass(FunctionCode funcCode, Func<CodeContext, CodeCont
                 }
             } // else metaclass is expected to be a callable and overrides any inherited metaclass through any bases
 
+            // Call class body lambda
+            CodeContext classContext = func(parentContext);
+            PythonDictionary vars = classContext.Dict;
+
+            // Prepare classdict
+            // TODO: prepared classdict should be used by `func` (PEP 3115)
+            object classdict = CallPrepare(parentContext, metaclass, name, bases, keywords, vars);
+
             // Fasttrack for metaclass == `type`
             if (metaclass is null) {
                 if (keywords != null && keywords.Count > 0) {
@@ -1467,26 +1475,18 @@ public static object MakeClass(FunctionCode funcCode, Func<CodeContext, CodeCont
                 if (bases.Count == 0) {
                     bases = PythonTuple.MakeTuple(TypeCache.Object);
                 }
-                PythonDictionary vars = func(parentContext).Dict;
-                return PythonType.__new__(parentContext, TypeCache.PythonType, name, bases, vars, selfNames);
+                
+                return PythonType.__new__(parentContext, TypeCache.PythonType, name, bases, vars, selfNames)!;
             }
 
-            CodeContext classContext = func(parentContext);
-            // If __classcell__ is defined, verify later that it makes all the way to type.__new__
-            var classCell = (ClosureCell?)classContext.Dict.get("__classcell__");
-
-            // Prepare classdict
-            // TODO: prepared classdict should be used by `func` (PEP 3115)
-            object? classdict = CallPrepare(parentContext, metaclass, name, bases, keywords, classContext.Dict);
-
             // Dispatch to the metaclass to do class creation and initialization
             // metaclass could be simply a callable, eg:
             // >>> def foo(*args): print(args)
             // >>> class bar(metaclass=foo): pass
             // calls our function...
             PythonContext pc = parentContext.LanguageContext;
 
-            object obj = pc.MetaClassCallSite.Target(
+            object? obj = pc.MetaClassCallSite.Target(
                 pc.MetaClassCallSite,
                 parentContext,
                 metaclass,
@@ -1496,11 +1496,24 @@ public static object MakeClass(FunctionCode funcCode, Func<CodeContext, CodeCont
                 keywords ?? MakeEmptyDict()
             );
 
-            if (classCell is not null && classCell.Value == Uninitialized.Instance) {
-                // Python 3.8: RuntimeError
-                Warn(parentContext, PythonExceptions.DeprecationWarning,
-                    "__class__ not set defining '{0}' as {1}. Was __classcell__ propagated to type.__new__?", name, Repr(parentContext, obj));
-                classCell.Value = obj;
+            // If __class__ is used, verify that it has been set
+            if (vars._storage is RuntimeVariablesDictionaryStorage storage) {
+                int pos = Array.IndexOf(storage.Names, "__class__");
+                if (pos >= 0) {
+                    ClosureCell classCell = storage.GetCell(pos);
+                    if (!ReferenceEquals(classCell.Value, obj)) {
+                        if (classCell.Value == Uninitialized.Instance) {
+                            if (obj is not null) {
+                                // Python 3.8: RuntimeError
+                                Warn(parentContext, PythonExceptions.DeprecationWarning,
+                                    "__class__ not set defining '{0}' as {1}. Was __classcell__ propagated to type.__new__?", name, Repr(parentContext, obj));
+                            }
+                            classCell.Value = obj; // Fill in the cell, since type.__new__ didn't do it
+                        } else {
+                            throw TypeError("__class__ set to {2} defining '{0}' as {1}", name, Repr(parentContext, obj), Repr(parentContext, classCell.Value));
+                        }
+                    }
+                }
             }
 
             // Ensure the class derives from `object`
@@ -1526,8 +1539,10 @@ static Func<CodeContext, CodeContext> GetClassCode(CodeContext/*!*/ context, Fun
                 }
             }
 
-            static object? CallPrepare(CodeContext/*!*/ context, object meta, string name, PythonTuple bases, PythonDictionary? keywords, PythonDictionary dict) {
-                object? classdict = dict;
+            static object CallPrepare(CodeContext/*!*/ context, object? meta, string name, PythonTuple bases, PythonDictionary? keywords, PythonDictionary dict) {
+                if (meta is null) return dict;
+
+                object? classdict = null;
 
                 object? prepareFunc = null;
                 // if available, call the __prepare__ method to get the classdict (PEP 3115)
@@ -1552,7 +1567,7 @@ static Func<CodeContext, CodeContext> GetClassCode(CodeContext/*!*/ context, Fun
                         context.LanguageContext.SetIndex(classdict, pair.Key, pair.Value);
                 }
 
-                return classdict;
+                return classdict ?? new PythonDictionary(dict);
             }
         }
 
diff --git a/Src/IronPython/Runtime/Types/PythonType.cs b/Src/IronPython/Runtime/Types/PythonType.cs
@@ -245,25 +245,18 @@ internal BuiltinFunction Ctor {
 
         #region Public API
 
-        public static object __new__(CodeContext/*!*/ context, PythonType cls, string name, PythonTuple bases, PythonDictionary dict) {
+#nullable enable
+
+        public static object? __new__(CodeContext/*!*/ context, [NotNone] PythonType cls, [NotNone] string name, [NotNone] PythonTuple bases, [NotNone] PythonDictionary dict) {
             return __new__(context, cls, name, bases, dict, string.Empty);
         }
 
-        internal static object __new__(CodeContext/*!*/ context, PythonType cls, string name, PythonTuple bases, PythonDictionary dict, string selfNames) {
-            if (name == null) {
-                throw PythonOps.TypeError("type() argument 1 must be string, not None");
-            }
-            if (bases == null) {
-                throw PythonOps.TypeError("type() argument 2 must be tuple, not None");
-            }
-            if (dict == null) {
-                throw PythonOps.TypeError("TypeError: type() argument 3 must be dict, not None");
-            }
+        internal static object? __new__(CodeContext/*!*/ context, PythonType cls, string name, PythonTuple bases, PythonDictionary dict, string selfNames) {
 
             EnsureModule(context, dict);
 
             PythonType meta = FindMetaClass(cls, bases);
-            object type;
+            object? type;
 
             if (meta != TypeCache.PythonType) {
                 if (meta != cls // the user has a custom __new__ which picked the wrong meta class, call the correct metaclass __new__
@@ -280,16 +273,20 @@ internal static object __new__(CodeContext/*!*/ context, PythonType cls, string
                 // no custom user type for __new__
                 type = new PythonType(context, name, bases, dict, selfNames);
             }
-            object cell = dict.get("__classcell__");
-            if (cell is ClosureCell pycell)
-            {
-                pycell.Value = type;
-                dict.RemoveDirect("__classcell__");
+
+            // set class cell, if any
+            if (dict.TryGetValueNoMissing("__classcell__", out object? classCellObject)) {
+                if (classCellObject is not ClosureCell classCell) {
+                    throw PythonOps.TypeErrorForBadInstance("__classcell__ must be a nonlocal cell, not <class '{0}'>", classCellObject);
+                }
+                classCell.Value = type;
             }
-            return type;
 
+            return type;
         }
 
+#nullable restore
+
         [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Performance", "CA1822:MarkMembersAsStatic")]
         public void __init__(string name, PythonTuple bases, PythonDictionary dict, [ParamDictionary] IDictionary<object, object> kwargs) {
         }
@@ -2096,6 +2093,7 @@ private void PopulateDictionary(CodeContext/*!*/ context, string name, PythonTup
 
             List<string> slots = GetSlots(vars);
             if (slots != null) {
+                slots.Remove("__classcell__");
                 _slots = slots.ToArray();
                 
                 int index = _originalSlotCount;
@@ -2142,8 +2140,8 @@ private void PopulateDictionary(CodeContext/*!*/ context, string name, PythonTup
             }
 
             foreach (var kvp in vars) {
-                if (kvp.Key is string) {
-                    PopulateSlot((string)kvp.Key, kvp.Value);
+                if (kvp.Key is string skey && skey != "__classcell__") {
+                    PopulateSlot(skey, kvp.Value);
                 }
             }
 
diff --git a/Tests/test_class.py b/Tests/test_class.py
@@ -6,7 +6,6 @@
 import os
 import sys
 import unittest
-import warnings
 
 from iptest import IronPythonTestCase, is_cli, is_mono, myint, big, run_test, skipUnlessIronPython
 
@@ -2622,104 +2621,6 @@ class y(object): pass
         self.assertEqual(x.__bases__, (object, ))
         self.assertEqual(x.__name__, 'x')
 
-    def test_class_attribute(self):
-        class C:
-            def f(self):
-                return self.__class__
-            def g(self):
-                return __class__
-            def h():
-                return __class__
-            @staticmethod
-            def j():
-                return __class__
-            @classmethod
-            def k(cls):
-                return __class__
-
-        self.assertEqual(C().f(), C)
-        self.assertEqual(C().g(), C)
-        self.assertEqual(C.h(), C)
-        self.assertEqual(C.j(), C)
-        self.assertEqual(C.k(), C)
-        self.assertEqual(C().k(), C)
-
-        # Test that a metaclass implemented as a function sets __class__ at a proper moment
-        def makeclass(name, bases, attrs):
-            attrNames = set(attrs.keys())
-            self.assertRaisesMessage(NameError, "free variable '__class__' referenced before assignment in enclosing scope", attrs['getclass'], None)
-            if (is_cli or sys.version_info >= (3, 6)):
-                self.assertIn('__classcell__', attrNames)
-
-            t = type(name, bases, attrs)
-
-            if (is_cli or sys.version_info >= (3, 6)):
-                self.assertEqual(t.getclass(None), t) # __class__ is set right after the type is created
-            else: # CPython 3.5-
-                self.assertRaisesMessage(NameError, "free variable '__class__' referenced before assignment in enclosing scope", attrs['getclass'], None)
-            if not is_cli:
-                self.assertEqual(set(attrs.keys()), attrNames) # set of attrs is not modified by type creation
-            else:
-                # TODO: prevent modification of attrs in IronPython
-                self.assertEqual(set(attrs.keys()) | {'__classcell__'}, attrNames | {'__class__'})
-
-            return t
-
-        class A(metaclass=makeclass):
-            def getclass(self):
-                return __class__
-
-        self.assertEquals(A.getclass(None), A)
-        self.assertEquals(A().getclass(), A)
-
-        dirA = dir(A)
-        self.assertIn('getclass', dirA)
-        self.assertIn('getclass', A.__dict__)
-        self.assertIn('__class__', dirA)
-        self.assertNotIn('__class__', A.__dict__)
-        if not is_cli:
-            self.assertNotIn('__classcell__', dirA)
-            self.assertNotIn('__classcell__', A.__dict__)
-        else:
-            # TODO: filter out __classcell__ in IronPython
-            self.assertIn('__classcell__', dirA)
-            self.assertIn('__classcell__', A.__dict__)
-
-    def test_classcell_propagation(self):
-        with warnings.catch_warnings(record=True) as ws:
-            warnings.simplefilter("always")
-
-            with self.assertWarnsRegex(DeprecationWarning, r"^__class__ not set defining 'MyClass' as <class '.*\.MyClass'>\. Was __classcell__ propagated to type\.__new__\?$"):
-                class MyDict(dict):
-                    def __setitem__(self, key, value):
-                        pass
-
-                class MetaClass(type):
-                    @classmethod
-                    def __prepare__(metacls, name, bases):
-                        return MyDict()
-
-                class MyClass(metaclass=MetaClass):
-                    def test(self):
-                        return __class__
-
-        self.assertEqual(len(ws), 0) # no unchecked warnings
-
-        with warnings.catch_warnings(record=True) as ws:
-            warnings.simplefilter("always")
-
-            with self.assertWarnsRegex(DeprecationWarning, r"^__class__ not set defining 'bar' as <class '.*\.gez'>\. Was __classcell__ propagated to type\.__new__\?$"):
-                class gez: pass
-
-                def foo(*args):
-                    return gez
-
-                class bar(metaclass=foo):
-                    def barfun(self):
-                        return __class__
-
-        self.assertEqual(len(ws), 0) # no unchecked warnings
-
     def test_issubclass(self):
         # first argument doesn't need to be new-style or old-style class if it defines __bases__
         class C(object):
diff --git a/Tests/test_metaclass.py b/Tests/test_metaclass.py
diff --git a/Tests/test_super.py b/Tests/test_super.py
