Code review fixes

pythonnet · lostmsu · Jan 7, 2021 · Nov 9, 2020 · Nov 9, 2020 · Nov 18, 2020
commit 62ae10749aea3512e0b8c45730e1a7692817d8b7
diff --git a/src/domain_tests/Python.DomainReloadTests.csproj b/src/domain_tests/Python.DomainReloadTests.csproj
@@ -23,8 +23,4 @@
     <PackageReference Include="Microsoft.NETFramework.ReferenceAssemblies" Version="1.0.0" />
   </ItemGroup>
 
-  <ItemGroup Condition="'$(TargetFramework)' == 'net40'">
-    <Reference Include="Microsoft.CSharp" />
-  </ItemGroup>
-
 </Project>
diff --git a/src/domain_tests/TestRunner.cs b/src/domain_tests/TestRunner.cs
@@ -913,8 +913,7 @@ public static int Main()
                 PythonEngine.Initialize(mode:{0});
                 using (Py.GIL())
                 {{
-                    // Because the generated assemblies are in the $TEMP folder, add it to the path
-                    var temp = Path.GetTempPath();
+                    var temp = AppDomain.CurrentDomain.BaseDirectory;
                     dynamic sys = Py.Import(""sys"");
                     sys.path.append(new PyString(temp));
                     dynamic test_mod = Py.Import(""domain_test_module.mod"");
@@ -938,6 +937,8 @@ public static int Main()
 ";
         readonly static string PythonDllLocation = Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "Python.Runtime.dll");
 
+        static string TestPath = null;
+
         public static int Main(string[] args)
         {
             TestCase testCase;
@@ -951,16 +952,11 @@ public static int Main(string[] args)
                 Console.WriteLine($"-- Looking for domain reload test case {testName}");
                 testCase = Cases.First(c => c.Name == testName);
             }
+
             Console.WriteLine($"-- Running domain reload test case: {testCase.Name}");
 
-            var tempFolderPython = Path.Combine(Path.GetTempPath(), "Python.Runtime.dll");
-            if (File.Exists(tempFolderPython))
-            {
-                File.Delete(tempFolderPython);
-            }
+            SetupTestFolder(testCase.Name);
 
-            File.Copy(PythonDllLocation, tempFolderPython);
-
             CreatePythonModule(testCase);
             {
                 var runnerAssembly = CreateCaseRunnerAssembly(verb:"before");
@@ -989,9 +985,32 @@ public static int Main(string[] args)
                     RunAndUnload(runnerDomain, runnerAssembly);
                 }
             }
+
+            // Don't delete unconditionally. It's sometimes useful to leave the
+            // folder behind to debug failing tests.
+            TeardownTestFolder();
+
             return 0;
         }
-#if !NETCOREAPP
+
+        static void SetupTestFolder(string testCaseName)
+        {
+            TestPath = Path.Combine(Path.GetTempPath(), $"Python.TestRunner.{testCaseName}");
+            if (Directory.Exists(TestPath))
+            {
+                Directory.Delete(TestPath, recursive: true);
+            }
+            Directory.CreateDirectory(TestPath);
+            File.Copy(PythonDllLocation, Path.Combine(TestPath, "Python.Runtime.dll"));
+        }
+
+        static void TeardownTestFolder()
+        {
+            if (Directory.Exists(TestPath))
+            {
+                Directory.Delete(TestPath, recursive: true);
+            }
+        }
 
         static void RunAndUnload(AppDomain domain, string assemblyPath)
         {
@@ -1027,7 +1046,7 @@ static string CreateAssembly(string name, string code, bool exe = false)
             CompilerParameters parameters = new CompilerParameters();
             parameters.GenerateExecutable = exe;
             var assemblyName = name;
-            var assemblyFullPath = Path.Combine(Path.GetTempPath(), assemblyName);
+            var assemblyFullPath = Path.Combine(TestPath, assemblyName);
             parameters.OutputAssembly = assemblyFullPath;
             parameters.ReferencedAssemblies.Add("System.dll");
             parameters.ReferencedAssemblies.Add("System.Core.dll");
@@ -1062,7 +1081,7 @@ static AppDomain CreateDomain(string name)
             var currentDomain = AppDomain.CurrentDomain;
             var domainsetup = new AppDomainSetup()
             {
-                ApplicationBase = Path.GetTempPath(),
+                ApplicationBase = TestPath,
                 ConfigurationFile = currentDomain.SetupInformation.ConfigurationFile,
                 LoaderOptimization = LoaderOptimization.SingleDomain,
                 PrivateBinPath = "."
@@ -1077,7 +1096,7 @@ static AppDomain CreateDomain(string name)
 
         static string CreatePythonModule(TestCase testCase)
         {
-            var modulePath = Path.Combine(Path.GetTempPath(), "domain_test_module");
+            var modulePath = Path.Combine(TestPath, "domain_test_module");
             if (Directory.Exists(modulePath))
             {
                 Directory.Delete(modulePath, recursive: true);
@@ -1092,7 +1111,5 @@ static string CreatePythonModule(TestCase testCase)
 
             return null;
         }
-#endif
-
     }
 }
diff --git a/src/runtime/Python.Runtime.csproj b/src/runtime/Python.Runtime.csproj
@@ -1,39 +1,39 @@
-<Project Sdk="Microsoft.NET.Sdk">
-  <PropertyGroup>
-    <TargetFrameworks>netstandard2.0</TargetFrameworks>
-    <Platforms>AnyCPU</Platforms>
-    <RootNamespace>Python.Runtime</RootNamespace>
-    <AssemblyName>Python.Runtime</AssemblyName>
-    <PackageId>pythonnet</PackageId>
-    <PackageLicenseUrl>https://github.com/pythonnet/pythonnet/blob/master/LICENSE</PackageLicenseUrl>
-    <RepositoryUrl>https://github.com/pythonnet/pythonnet</RepositoryUrl>
-    <RepositoryType>git</RepositoryType>
-    <PackageTags>python interop dynamic dlr Mono pinvoke</PackageTags>
-    <PackageIconUrl>https://raw.githubusercontent.com/pythonnet/pythonnet/master/src/console/python-clear.ico</PackageIconUrl>
-    <PackageProjectUrl>https://pythonnet.github.io/</PackageProjectUrl>
-    <NoWarn>1591;NU1701</NoWarn>
-    <AllowUnsafeBlocks>True</AllowUnsafeBlocks>
-  </PropertyGroup>
-
-  <PropertyGroup>
-    <DefineConstants>$(DefineConstants);$(ConfiguredConstants)</DefineConstants>
-  </PropertyGroup>
-
-  <ItemGroup Condition=" '$(PythonInteropFile)' != '' ">
-    <Compile Remove="interop*.cs" />
-    <Compile Include="interop.cs" />
-    <Compile Include="$(PythonInteropFile)" />
-  </ItemGroup>
-
-  <ItemGroup>
-    <None Remove="resources\clr.py" />
-    <EmbeddedResource Include="resources\clr.py">
-      <LogicalName>clr.py</LogicalName>
-    </EmbeddedResource>
-  </ItemGroup>
-
-  <ItemGroup>
-    <PackageReference Include="System.Security.Permissions" Version="4.4.0" />
-    <PackageReference Include="System.Reflection.Emit" Version="4.3.0" />
-  </ItemGroup>
-</Project>
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <TargetFrameworks>netstandard2.0</TargetFrameworks>
+    <Platforms>AnyCPU</Platforms>
+    <RootNamespace>Python.Runtime</RootNamespace>
+    <AssemblyName>Python.Runtime</AssemblyName>
+    <PackageId>pythonnet</PackageId>
+    <PackageLicenseUrl>https://github.com/pythonnet/pythonnet/blob/master/LICENSE</PackageLicenseUrl>
+    <RepositoryUrl>https://github.com/pythonnet/pythonnet</RepositoryUrl>
+    <RepositoryType>git</RepositoryType>
+    <PackageTags>python interop dynamic dlr Mono pinvoke</PackageTags>
+    <PackageIconUrl>https://raw.githubusercontent.com/pythonnet/pythonnet/master/src/console/python-clear.ico</PackageIconUrl>
+    <PackageProjectUrl>https://pythonnet.github.io/</PackageProjectUrl>
+    <NoWarn>1591;NU1701</NoWarn>
+    <AllowUnsafeBlocks>True</AllowUnsafeBlocks>
+  </PropertyGroup>
+
+  <PropertyGroup>
+    <DefineConstants>$(DefineConstants);$(ConfiguredConstants)</DefineConstants>
+  </PropertyGroup>
+
+  <ItemGroup Condition=" '$(PythonInteropFile)' != '' ">
+    <Compile Remove="interop*.cs" />
+    <Compile Include="interop.cs" />
+    <Compile Include="$(PythonInteropFile)" />
+  </ItemGroup>
+
+  <ItemGroup>
+    <None Remove="resources\clr.py" />
+    <EmbeddedResource Include="resources\clr.py">
+      <LogicalName>clr.py</LogicalName>
+    </EmbeddedResource>
+  </ItemGroup>
+
+  <ItemGroup>
+    <PackageReference Include="System.Security.Permissions" Version="4.4.0" />
+    <PackageReference Include="System.Reflection.Emit" Version="4.3.0" />
+  </ItemGroup>
+</Project>
diff --git a/src/runtime/StateSerialization/MaybeMemberInfo.cs b/src/runtime/StateSerialization/MaybeMemberInfo.cs
@@ -6,10 +6,10 @@
 
 namespace Python.Runtime
 {
-     [Serializable]
-    internal struct MaybeMemberInfo<T> : ISerializable where T: MemberInfo
+    [Serializable]
+    internal struct MaybeMemberInfo<T> : ISerializable where T : MemberInfo
     {
-        public static implicit operator MaybeMemberInfo<T> (T ob) => new MaybeMemberInfo<T>(ob);
+        public static implicit operator MaybeMemberInfo<T>(T ob) => new MaybeMemberInfo<T>(ob);
 
         // .ToString() of the serialized object
         const string SerializationName = "s";
@@ -22,7 +22,7 @@ internal struct MaybeMemberInfo<T> : ISerializable where T: MemberInfo
         [NonSerialized]
         Exception deserializationException;
 
-        public string DeletedMessage 
+        public string DeletedMessage
         {
             get
             {
@@ -42,7 +42,7 @@ public T Value
             }
         }
 
-        public string Name {get{return name;}}
+        public string Name => name;
         public bool Valid => info != null;
 
         public override string ToString()

diff --git a/src/runtime/StateSerialization/MaybeType.cs b/src/runtime/StateSerialization/MaybeType.cs
@@ -36,7 +36,7 @@ public Type Value
             }
         }
 
-        public string Name {get{return name;}}
+        public string Name => name;
         public bool Valid => type != null;
 
         public override string ToString()

diff --git a/src/runtime/classmanager.cs b/src/runtime/classmanager.cs
@@ -129,6 +129,10 @@ internal static void SaveRuntimeData(RuntimeDataStorage storage)
                         // raises an error. We don't care about it.
                         Runtime.PyErr_Clear();
                     }
+                    else if (Exceptions.ErrorOccurred())
+                    {
+                        throw new PythonException();
+                    }
                 }
                 // We modified the Type object, notify it we did.
                 Runtime.PyType_Modified(cls.Value.tpHandle);

diff --git a/src/runtime/methodbinder.cs b/src/runtime/methodbinder.cs
@@ -24,7 +24,8 @@ internal class MethodBinder
 
         [NonSerialized]
         public bool init = false;
-        public bool allow_threads = true;
+        public const bool DefaultAllowThreads = true;
+        public bool allow_threads = DefaultAllowThreads;
 
         internal MethodBinder()
         {
@@ -187,7 +188,7 @@ internal static int GetPrecedence(MethodBase mi)
         {
             if (mi == null)
             {
-                return -1;
+                return int.MaxValue;
             }
 
             ParameterInfo[] pi = mi.GetParameters();
@@ -864,8 +865,8 @@ internal class MethodSorter : IComparer<MaybeMethodBase>
     {
         int IComparer<MaybeMethodBase>.Compare(MaybeMethodBase m1, MaybeMethodBase m2)
         {
-            MethodBase me1 = m1.Valid ? m1.Value : null;
-            MethodBase me2 = m2.Valid ? m2.Value : null;
+            MethodBase me1 = m1.UnsafeValue;
+            MethodBase me2 = m2.UnsafeValue;
             if (me1 == null && me2 == null)
             {
                 return 0;

diff --git a/src/runtime/methodobject.cs b/src/runtime/methodobject.cs
@@ -28,8 +28,7 @@ internal class MethodObject : ExtensionType
         internal IntPtr doc;
         internal Type type;
 
-        //  `allow_threads = true`: True being the default value of MethodBinder.allow_threads
-        public MethodObject(Type type, string name, MethodInfo[] info, bool allow_threads = true)
+        public MethodObject(Type type, string name, MethodInfo[] info, bool allow_threads = MethodBinder.DefaultAllowThreads)
         {
             this.type = type;
             this.name = name;

diff --git a/src/runtime/moduleobject.cs b/src/runtime/moduleobject.cs
@@ -351,6 +351,10 @@ protected override void OnSave(InterDomainContext context)
                     // raises an error. We don't care about it.
                     Runtime.PyErr_Clear();
                 }
+                else if (Exceptions.ErrorOccurred())
+                {
+                    throw new PythonException();
+                }
                 pair.Value.DecrRefCount();
             }