PyIter: do not force dispose previous object upon moving to the next one

lostmsu · lostmsu · commit cebc51536201 · 2020-12-18T12:21:30.000-08:00
also added exception handling for PyIter_Next

PyObject now implements `IEnumerable&lt;PyObject&gt;`
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -26,6 +26,7 @@ details about the cause of the failure
 -   BREAKING: Parameters marked with `ParameterAttributes.Out` are no longer returned in addition
      to the regular method return value (unless they are passed with `ref` or `out` keyword).
 -   BREAKING: Drop support for the long-deprecated CLR.* prefix.
+-   `PyObject` now implements `IEnumerable<PyObject>` in addition to `IEnumerable`
 
 ### Fixed
 
@@ -40,6 +41,7 @@ details about the cause of the failure
 -    Fixed a bug where indexers could not be used if they were inherited
 -    Made it possible to use `__len__` also on `ICollection<>` interface objects
 -    Made it possible to call `ToString`, `GetHashCode`, and `GetType` on inteface objects
+-    Fixed objects returned by enumerating `PyObject` being disposed too soon
 
 ### Removed
 
diff --git a/src/embed_tests/TestPyIter.cs b/src/embed_tests/TestPyIter.cs
@@ -0,0 +1,37 @@
+using System.Linq;
+using System.Text;
+
+using NUnit.Framework;
+
+using Python.Runtime;
+
+namespace Python.EmbeddingTest
+{
+    class TestPyIter
+    {
+        [OneTimeSetUp]
+        public void SetUp()
+        {
+            PythonEngine.Initialize();
+        }
+
+        [OneTimeTearDown]
+        public void Dispose()
+        {
+            PythonEngine.Shutdown();
+        }
+
+        [Test]
+        public void KeepOldObjects()
+        {
+            using (Py.GIL())
+            using (var testString = new PyString("hello world! !$%&/()=?"))
+            {
+                PyObject[] chars = testString.ToArray();
+                Assert.IsTrue(chars.Length > 1);
+                string reconstructed = string.Concat(chars.Select(c => c.As<string>()));
+                Assert.AreEqual(testString.As<string>(), reconstructed);
+            }
+        }
+    }
+}
diff --git a/src/runtime/pyiter.cs b/src/runtime/pyiter.cs
@@ -9,7 +9,7 @@ namespace Python.Runtime
     /// PY3: https://docs.python.org/3/c-api/iterator.html
     /// for details.
     /// </summary>
-    public class PyIter : PyObject, IEnumerator<object>
+    public class PyIter : PyObject, IEnumerator<PyObject>
     {
         private PyObject _current;
 
@@ -46,41 +46,35 @@ public static PyIter GetIter(PyObject iterable)
 
         protected override void Dispose(bool disposing)
         {
-            if (null != _current)
-            {
-                _current.Dispose();
-                _current = null;
-            }
+            _current = null;
             base.Dispose(disposing);
         }
 
         public bool MoveNext()
         {
-            // dispose of the previous object, if there was one
-            if (null != _current)
+            NewReference next = Runtime.PyIter_Next(Reference);
+            if (next.IsNull())
             {
-                _current.Dispose();
-                _current = null;
-            }
+                if (Exceptions.ErrorOccurred())
+                {
+                    throw new PythonException();
+                }
 
-            IntPtr next = Runtime.PyIter_Next(obj);
-            if (next == IntPtr.Zero)
-            {
+                // stop holding the previous object, if there was one
+                _current = null;
                 return false;
             }
 
-            _current = new PyObject(next);
+            _current = next.MoveToPyObject();
             return true;
         }
 
         public void Reset()
         {
-            //Not supported in python.
+            throw new NotSupportedException();
         }
 
-        public object Current
-        {
-            get { return _current; }
-        }
+        public PyObject Current => _current;
+        object System.Collections.IEnumerator.Current => _current;
     }
 }
diff --git a/src/runtime/pyobject.cs b/src/runtime/pyobject.cs
@@ -16,7 +16,7 @@ namespace Python.Runtime
     /// for details.
     /// </summary>
     [Serializable]
-    public partial class PyObject : DynamicObject, IEnumerable, IDisposable
+    public partial class PyObject : DynamicObject, IEnumerable<PyObject>, IDisposable
     {
 #if TRACE_ALLOC
         /// <summary>
@@ -705,10 +705,11 @@ public PyObject GetIterator()
         /// python object to be iterated over in C#. A PythonException will be
         /// raised if the object is not iterable.
         /// </remarks>
-        public IEnumerator GetEnumerator()
+        public IEnumerator<PyObject> GetEnumerator()
         {
             return PyIter.GetIter(this);
         }
+        IEnumerator IEnumerable.GetEnumerator() => this.GetEnumerator();
 
 
         /// <summary>
diff --git a/src/runtime/runtime.cs b/src/runtime/runtime.cs
@@ -1854,6 +1854,8 @@ internal static bool PyIter_Check(IntPtr pointer)
 
         [DllImport(_PythonDll, CallingConvention = CallingConvention.Cdecl)]
         internal static extern IntPtr PyIter_Next(IntPtr pointer);
+        [DllImport(_PythonDll, CallingConvention = CallingConvention.Cdecl)]
+        internal static extern NewReference PyIter_Next(BorrowedReference pointer);
 
 
         //====================================================================

Original file line number	Diff line number	Diff line change
`@@ -9,7 +9,7 @@ namespace Python.Runtime`
`9`	`9`	`/// PY3: https://docs.python.org/3/c-api/iterator.html`
`10`	`10`	`/// for details.`
`11`	`11`	`/// </summary>`
`12`		`- public class PyIter : PyObject, IEnumerator<object>`
	`12`	`+ public class PyIter : PyObject, IEnumerator<PyObject>`
`13`	`13`	`{`
`14`	`14`	`private PyObject _current;`
`15`	`15`
`@@ -46,41 +46,35 @@ public static PyIter GetIter(PyObject iterable)`
`46`	`46`
`47`	`47`	`protected override void Dispose(bool disposing)`
`48`	`48`	`{`
`49`		`- if (null != _current)`
`50`		`- {`
`51`		`- _current.Dispose();`
`52`		`- _current = null;`
`53`		`- }`
	`49`	`+ _current = null;`
`54`	`50`	`base.Dispose(disposing);`
`55`	`51`	`}`
`56`	`52`
`57`	`53`	`public bool MoveNext()`
`58`	`54`	`{`
`59`		`- // dispose of the previous object, if there was one`
`60`		`- if (null != _current)`
	`55`	`+ NewReference next = Runtime.PyIter_Next(Reference);`
	`56`	`+ if (next.IsNull())`
`61`	`57`	`{`
`62`		`- _current.Dispose();`
`63`		`- _current = null;`
`64`		`- }`
	`58`	`+ if (Exceptions.ErrorOccurred())`
	`59`	`+ {`
	`60`	`+ throw new PythonException();`
	`61`	`+ }`
`65`	`62`
`66`		`- IntPtr next = Runtime.PyIter_Next(obj);`
`67`		`- if (next == IntPtr.Zero)`
`68`		`- {`
	`63`	`+ // stop holding the previous object, if there was one`
	`64`	`+ _current = null;`
`69`	`65`	`return false;`
`70`	`66`	`}`
`71`	`67`
`72`		`- _current = new PyObject(next);`
	`68`	`+ _current = next.MoveToPyObject();`
`73`	`69`	`return true;`
`74`	`70`	`}`
`75`	`71`
`76`	`72`	`public void Reset()`
`77`	`73`	`{`
`78`		`- //Not supported in python.`
	`74`	`+ throw new NotSupportedException();`
`79`	`75`	`}`
`80`	`76`
`81`		`- public object Current`
`82`		`- {`
`83`		`- get { return _current; }`
`84`		`- }`
	`77`	`+ public PyObject Current => _current;`
	`78`	`+ object System.Collections.IEnumerator.Current => _current;`
`85`	`79`	`}`
`86`	`80`	`}`
Original file line number	Diff line number	Diff line change
`@@ -16,7 +16,7 @@ namespace Python.Runtime`
`16`	`16`	`/// for details.`
`17`	`17`	`/// </summary>`
`18`	`18`	`[Serializable]`
`19`		`- public partial class PyObject : DynamicObject, IEnumerable, IDisposable`
	`19`	`+ public partial class PyObject : DynamicObject, IEnumerable<PyObject>, IDisposable`
`20`	`20`	`{`
`21`	`21`	`#if TRACE_ALLOC`
`22`	`22`	`/// <summary>`
`@@ -705,10 +705,11 @@ public PyObject GetIterator()`
`705`	`705`	`/// python object to be iterated over in C#. A PythonException will be`
`706`	`706`	`/// raised if the object is not iterable.`
`707`	`707`	`/// </remarks>`
`708`		`- public IEnumerator GetEnumerator()`
	`708`	`+ public IEnumerator<PyObject> GetEnumerator()`
`709`	`709`	`{`
`710`	`710`	`return PyIter.GetIter(this);`
`711`	`711`	`}`
	`712`	`+ IEnumerator IEnumerable.GetEnumerator() => this.GetEnumerator();`
`712`	`713`
`713`	`714`
`714`	`715`	`/// <summary>`