Code review fixes

pythonnet · lostmsu · Jan 7, 2021 · Nov 9, 2020 · Nov 9, 2020 · Nov 18, 2020
commit 79516f1984ae310d412c62cf6cb4c01655ea7c49
diff --git a/src/domain_tests/TestRunner.cs b/src/domain_tests/TestRunner.cs
@@ -16,22 +16,22 @@ namespace Python.DomainReloadTests
     /// 3. This class at runtime creates a directory that has both C# and
     ///    python code, and compiles the C#.
     /// 4. This class then runs the C# code.
-    ///
-    /// But wait there's more indirection. The C# code that's run -- known as
-    /// the test runner --
-    /// This class compiles a DLL that contains the class which code will change
-    /// and a runner executable that will run Python code referencing the class.
-    /// Each test case:
+    /// 
+    /// But there's a bit more indirection. This class compiles a DLL that
+    /// contains code that will change.
+    /// Then, the test case:
     /// * Compiles some code, loads it into a domain, runs python that refers to it.
-    /// * Unload the domain.
-    /// * Compile a new piece of code, load it into a domain, run a new piece of python that accesses the code.
+    /// * Unload the domain, re-runs the domain to make sure domain reload happens correctly.
+    /// * Compile a new piece of code, load it into a new domain, run a new piece of 
+    ///   Python code to test the objects after they've been deleted or modified in C#.
     /// * Unload the domain. Reload the domain, run the same python again.
+    /// 
     /// This class gets built into an executable which takes one argument:
     /// which test case to run. That's because pytest assumes we'll run
     /// everything in one process, but we really want a clean process on each
-    /// test case to test the init/reload/teardown parts of the domain reload
-    /// code.
+    /// test case to test the init/reload/teardown parts of the domain reload.
     /// </summary>
+    /// 
     class TestRunner
     {
         const string TestAssemblyName = "DomainTests";
@@ -347,12 +347,6 @@ assert called is True
     called = False
     Cls.Call()
     assert called is False
-    #try:
-    #    assert 2 == Cls.Before
-    #except TypeError:
-    #    print('Caught expected exception')
-    #else:
-    #    raise AssertionError('Failed to throw exception')
 ",
             },
 
@@ -398,7 +392,6 @@ def before_reload():
 def after_reload():
      try:
         TestNamespace.Cls(2)
-        sys.my_cls.Member()
      except AttributeError:
          print('Caught expected exception')
      else:
@@ -845,7 +838,7 @@ raise AssertionError('failed to raise')
     # foo should have changed
     assert foo.num == 7
     assert bar.num == 7
-    # Pythonnet also returns a new object with `ref`-quialified parameters
+    # Pythonnet also returns a new object with `ref`-qualified parameters
     assert foo is not bar
                     ",
             },
@@ -1029,7 +1022,6 @@ static string CreateCaseRunnerAssembly(string verb, string shutdownMode = "Shutd
         }
         static string CreateAssembly(string name, string code, bool exe = false)
         {
-            // Console.WriteLine(code);
             // Never return or hold the Assembly instance. This will cause
             // the assembly to be loaded into the current domain and this
             // interferes with the tests. The Domain can execute fine from a 

diff --git a/src/runtime/StateSerialization/MaybeMethodBase.cs b/src/runtime/StateSerialization/MaybeMethodBase.cs
@@ -105,7 +105,7 @@ internal MaybeMethodBase(SerializationInfo serializationInfo, StreamingContext c
             deserializationException = null;
             try
             {
-                // Retrive the reflected type of the method;
+                // Retrieve the reflected type of the method;
                 var typeName = serializationInfo.GetString(SerializationType);
                 var tp = Type.GetType(typeName);
                 if (tp == null)
@@ -144,7 +144,7 @@ internal MaybeMethodBase(SerializationInfo serializationInfo, StreamingContext c
 
                 if (mb != null && hasRefType)
                 {
-                    CheckRefTypes(mb, param);
+                    mb = CheckRefTypes(mb, param);
                 }
 
                 // Do like in ClassManager.GetClassInfo
@@ -165,7 +165,7 @@ MethodBase CheckRefTypes(MethodBase mb, ParameterHelper[] ph)
             // void MyFn (ref int a)
             // to:
             // void MyFn (out int a)
-            // will still find the fucntion correctly as, `in`, `out` and `ref`
+            // will still find the function correctly as, `in`, `out` and `ref`
             // are all represented as a reference type. Query the method we got
             // and validate the parameters
             if (ph.Length != 0)

diff --git a/src/runtime/arrayobject.cs b/src/runtime/arrayobject.cs
@@ -34,6 +34,7 @@ public static IntPtr tp_new(IntPtr tpRaw, IntPtr args, IntPtr kw)
             {
                 return Exceptions.RaiseTypeError(self.type.DeletedMessage);
             }
+            Type arrType = self.type.Value;
 
             long[] dimensions = new long[Runtime.PyTuple_Size(args)];
             if (dimensions.Length == 0)
@@ -42,7 +43,7 @@ public static IntPtr tp_new(IntPtr tpRaw, IntPtr args, IntPtr kw)
             }
             if (dimensions.Length != 1)
             {
-                return CreateMultidimensional(self.type.Value.GetElementType(), dimensions,
+                return CreateMultidimensional(arrType.GetElementType(), dimensions,
                          shapeTuple: new BorrowedReference(args),
                          pyType: tp)
                        .DangerousMoveToPointerOrNull();
@@ -60,14 +61,14 @@ public static IntPtr tp_new(IntPtr tpRaw, IntPtr args, IntPtr kw)
                 }
                 else
                 {
-                    return NewInstance(self.type.Value.GetElementType(), tp, dimensions)
+                    return NewInstance(arrType.GetElementType(), tp, dimensions)
                            .DangerousMoveToPointerOrNull();
                 }
             }
             object result;
 
             // this implements casting to Array[T]
-            if (!Converter.ToManaged(op, self.type.Value, out result, true))
+            if (!Converter.ToManaged(op, arrType, out result, true))
             {
                 return IntPtr.Zero;
             }

diff --git a/src/runtime/constructorbinder.cs b/src/runtime/constructorbinder.cs
@@ -56,9 +56,10 @@ internal object InvokeRaw(IntPtr inst, IntPtr args, IntPtr kw, MethodBase info)
                 return Exceptions.RaiseTypeError(_containingType.DeletedMessage);
             }
             object result;
+            Type tp = _containingType.Value;
 
-            if (_containingType.Value.IsValueType && !_containingType.Value.IsPrimitive &&
-                !_containingType.Value.IsEnum && _containingType.Value != typeof(decimal) &&
+            if (tp.IsValueType && !tp.IsPrimitive &&
+                !tp.IsEnum && tp != typeof(decimal) &&
                 Runtime.PyTuple_Size(args) == 0)
             {
                 // If you are trying to construct an instance of a struct by
@@ -68,7 +69,7 @@ internal object InvokeRaw(IntPtr inst, IntPtr args, IntPtr kw, MethodBase info)
                 // Activator.CreateInstance().
                 try
                 {
-                    result = Activator.CreateInstance(_containingType.Value);
+                    result = Activator.CreateInstance(tp);
                 }
                 catch (Exception e)
                 {

diff --git a/src/runtime/constructorbinding.cs b/src/runtime/constructorbinding.cs
@@ -96,6 +96,7 @@ public static IntPtr mp_subscript(IntPtr op, IntPtr key)
             {
                 return Exceptions.RaiseTypeError(self.type.DeletedMessage);
             }
+            Type tp = self.type.Value;
 
             Type[] types = Runtime.PythonArgsToTypeArray(key);
             if (types == null)
@@ -104,12 +105,12 @@ public static IntPtr mp_subscript(IntPtr op, IntPtr key)
             }
             //MethodBase[] methBaseArray = self.ctorBinder.GetMethods();
             //MethodBase ci = MatchSignature(methBaseArray, types);
-            ConstructorInfo ci = self.type.Value.GetConstructor(types);
+            ConstructorInfo ci = tp.GetConstructor(types);
             if (ci == null)
             {
                 return Exceptions.RaiseTypeError("No match found for constructor signature");
             }
-            var boundCtor = new BoundContructor(self.type.Value, self.pyTypeHndl, self.ctorBinder, ci);
+            var boundCtor = new BoundContructor(tp, self.pyTypeHndl, self.ctorBinder, ci);
 
             return boundCtor.pyHandle;
         }
@@ -126,6 +127,11 @@ public static IntPtr tp_repr(IntPtr ob)
                 return self.repr;
             }
             MethodBase[] methods = self.ctorBinder.GetMethods();
+
+            if (!self.type.Valid)
+            {
+                return Exceptions.RaiseTypeError(self.type.DeletedMessage);
+            }
             string name = self.type.Value.FullName;
             var doc = "";
             foreach (MethodBase t in methods)

diff --git a/src/runtime/converter.cs b/src/runtime/converter.cs
@@ -344,7 +344,13 @@ internal static bool ToManagedValue(IntPtr value, Type obType,
                 }
                 if (mt is ClassBase)
                 {
-                    result = ((ClassBase)mt).type.Value;
+                    var cb = (ClassBase)mt;
+                    if (!cb.type.Valid)
+                    {
+                        Exceptions.SetError(Exceptions.TypeError, cb.type.DeletedMessage);
+                        return false;
+                    }
+                    result = cb.type.Value;
                     return true;
                 }
                 // shouldn't happen

diff --git a/src/runtime/delegateobject.cs b/src/runtime/delegateobject.cs
@@ -56,6 +56,7 @@ public static IntPtr tp_new(IntPtr tp, IntPtr args, IntPtr kw)
             {
                 return Exceptions.RaiseTypeError(self.type.DeletedMessage);
             }
+            Type type = self.type.Value;
 
             if (Runtime.PyTuple_Size(args) != 1)
             {
@@ -69,7 +70,7 @@ public static IntPtr tp_new(IntPtr tp, IntPtr args, IntPtr kw)
                 return Exceptions.RaiseTypeError("argument must be callable");
             }
 
-            Delegate d = PythonEngine.DelegateManager.GetDelegate(self.type.Value, method);
+            Delegate d = PythonEngine.DelegateManager.GetDelegate(type, method);
             return CLRObject.GetInstHandle(d, self.pyHandle);
         }
 

diff --git a/src/runtime/methodbinding.cs b/src/runtime/methodbinding.cs
@@ -114,13 +114,14 @@ public static IntPtr tp_call(IntPtr ob, IntPtr args, IntPtr kw)
             // for example this method in the tests: string Overloaded<T>(int arg1, int arg2, string arg3)
             if (self.info.Valid)
             {
-                if (self.info.Value.IsGenericMethod)
+                var info = self.info.Value;
+                if (info.IsGenericMethod)
                 {
                     var len = Runtime.PyTuple_Size(args); //FIXME: Never used
                     Type[] sigTp = Runtime.PythonArgsToTypeArray(args, true);
                     if (sigTp != null)
                     {
-                        Type[] genericTp = self.info.Value.GetGenericArguments();
+                        Type[] genericTp = info.GetGenericArguments();
                         MethodInfo betterMatch = MethodBinder.MatchSignatureAndParameters(self.m.info, genericTp, sigTp);
                         if (betterMatch != null)
                         {

diff --git a/src/runtime/moduleobject.cs b/src/runtime/moduleobject.cs
@@ -346,7 +346,7 @@ protected override void OnSave(InterDomainContext context)
             {
                 Runtime.PyDict_DelItemString(dict, pair.Key);
                 pair.Value.DecrRefCount();
-                 if (Exceptions.ExceptionMatches(Exceptions.KeyError))
+                if (Exceptions.ExceptionMatches(Exceptions.KeyError))
                 {
                     // Trying to remove a key that's not in the dictionary 
                     // raises an error. We don't care about it.

diff --git a/src/runtime/propertyobject.cs b/src/runtime/propertyobject.cs
@@ -36,7 +36,8 @@ public static IntPtr tp_descr_get(IntPtr ds, IntPtr ob, IntPtr tp)
             {
                 return Exceptions.RaiseTypeError(self.info.DeletedMessage);
             }
-            MethodInfo getter = self.getter.Value;
+            var info = self.info.Value;
+            MethodInfo getter = self.getter.UnsafeValue;
             object result;
 
 
@@ -56,8 +57,8 @@ public static IntPtr tp_descr_get(IntPtr ds, IntPtr ob, IntPtr tp)
 
                 try
                 {
-                    result = self.info.Value.GetValue(null, null);
-                    return Converter.ToPython(result, self.info.Value.PropertyType);
+                    result = info.GetValue(null, null);
+                    return Converter.ToPython(result, info.PropertyType);
                 }
                 catch (Exception e)
                 {
@@ -73,8 +74,8 @@ public static IntPtr tp_descr_get(IntPtr ds, IntPtr ob, IntPtr tp)
 
             try
             {
-                result = self.info.Value.GetValue(co.inst, null);
-                return Converter.ToPython(result, self.info.Value.PropertyType);
+                result = info.GetValue(co.inst, null);
+                return Converter.ToPython(result, info.PropertyType);
             }
             catch (Exception e)
             {
@@ -101,8 +102,9 @@ public static IntPtr tp_descr_get(IntPtr ds, IntPtr ob, IntPtr tp)
                 Exceptions.RaiseTypeError(self.info.DeletedMessage);
                 return -1;
             }
+            var info = self.info.Value;
 
-            MethodInfo setter = self.setter.Value;
+            MethodInfo setter = self.setter.UnsafeValue;
             object newval;
 
             if (val == IntPtr.Zero)
@@ -118,7 +120,7 @@ public static IntPtr tp_descr_get(IntPtr ds, IntPtr ob, IntPtr tp)
             }
 
 
-            if (!Converter.ToManaged(val, self.info.Value.PropertyType, out newval, true))
+            if (!Converter.ToManaged(val, info.PropertyType, out newval, true))
             {
                 return -1;
             }
@@ -144,11 +146,11 @@ public static IntPtr tp_descr_get(IntPtr ds, IntPtr ob, IntPtr tp)
                         Exceptions.RaiseTypeError("invalid target");
                         return -1;
                     }
-                    self.info.Value.SetValue(co.inst, newval, null);
+                    info.SetValue(co.inst, newval, null);
                 }
                 else
                 {
-                    self.info.Value.SetValue(null, newval, null);
+                    info.SetValue(null, newval, null);
                 }
                 return 0;
             }