diff --git a/src/embed_tests/TestIndexerDelete.cs b/src/embed_tests/TestIndexerDelete.cs new file mode 100644 index 000000000..9a10009d2 --- /dev/null +++ b/src/embed_tests/TestIndexerDelete.cs @@ -0,0 +1,101 @@ +using System.Collections.Concurrent; +using System.Collections.Generic; + +using NUnit.Framework; + +using Python.Runtime; + +namespace Python.EmbeddingTest +{ + /// + /// `del ob[key]` reaches mp_ass_subscript with a null value. It must raise a catchable Python + /// exception (or delete, for IDictionary/IList types) instead of aborting the process. + /// + [TestFixture] + public class TestIndexerDelete + { + [OneTimeSetUp] + public void SetUp() + { + PythonEngine.Initialize(); + } + + [OneTimeTearDown] + public void Dispose() + { + PythonEngine.Shutdown(); + } + + public class SettableIndexer + { + private readonly Dictionary _items = new(); + + public string this[int key] + { + get => _items[key]; + set => _items[key] = value; + } + + public string Marker => "alive"; + } + + [Test] + public void DelOnSettableIndexerRaisesTypeError() + { + using (Py.GIL()) + { + using var scope = Py.CreateScope(); + scope.Set("ob", new SettableIndexer().ToPython()); + scope.Exec(@" +ob[1] = 'one' +raised = None +try: + del ob[1] +except TypeError as e: + raised = e +"); + using var raised = scope.Get("raised"); + Assert.IsFalse(raised.IsNone(), "del must raise TypeError"); + Assert.AreEqual("alive", scope.Eval("ob.Marker").As()); + Assert.AreEqual("one", scope.Eval("ob[1]").As()); + } + } + + [Test] + public void DelOnConcurrentDictionaryRemovesKey() + { + using (Py.GIL()) + { + using var scope = Py.CreateScope(); + var dict = new ConcurrentDictionary(); + dict["MyKey"] = "MyValue"; + scope.Set("d", dict.ToPython()); + + scope.Exec("del d['MyKey']"); + + Assert.IsFalse(dict.ContainsKey("MyKey")); + Assert.AreEqual(0, scope.Eval("d.Count").As()); + } + } + + [Test] + public void DelOnDictionaryMissingKeyRaisesKeyError() + { + using (Py.GIL()) + { + using var scope = Py.CreateScope(); + scope.Set("d", new Dictionary { ["a"] = 1 }.ToPython()); + scope.Exec(@" +raised = None +try: + del d['missing'] +except KeyError as e: + raised = e +"); + using var raised = scope.Get("raised"); + Assert.IsFalse(raised.IsNone(), "del of a missing key must raise KeyError"); + Assert.AreEqual(1, scope.Eval("d.Count").As()); + } + } + } +} diff --git a/src/runtime/ClassManager.cs b/src/runtime/ClassManager.cs index b88a6a6b6..5b2460108 100644 --- a/src/runtime/ClassManager.cs +++ b/src/runtime/ClassManager.cs @@ -681,6 +681,8 @@ void AddMember(string name, string snakeCasedName, bool isStaticReadonlyCallable } } + ci.indexer?.ResolveDeleter(type); + return ci; } diff --git a/src/runtime/Types/ArrayObject.cs b/src/runtime/Types/ArrayObject.cs index 3ca09ddce..0a03cf22b 100644 --- a/src/runtime/Types/ArrayObject.cs +++ b/src/runtime/Types/ArrayObject.cs @@ -245,6 +245,13 @@ public static NewReference mp_subscript(BorrowedReference ob, BorrowedReference /// public static int mp_ass_subscript(BorrowedReference ob, BorrowedReference idx, BorrowedReference v) { + // `del arr[i]` arrives here with a null value; arrays are fixed-size, so refuse it up front. + if (v.IsNull) + { + Exceptions.RaiseTypeError("array does not support item deletion"); + return -1; + } + var obj = (CLRObject)GetManagedObject(ob)!; var items = (Array)obj.inst; Type itemType = obj.inst.GetType().GetElementType(); diff --git a/src/runtime/Types/ClassBase.cs b/src/runtime/Types/ClassBase.cs index ed1659789..de0503db8 100644 --- a/src/runtime/Types/ClassBase.cs +++ b/src/runtime/Types/ClassBase.cs @@ -507,6 +507,13 @@ static int mp_ass_subscript_impl(BorrowedReference ob, BorrowedReference idx, Bo BorrowedReference tp = Runtime.PyObject_TYPE(ob); var cls = (ClassBase)GetManagedObject(tp)!; + // CPython routes `del ob[key]` through this same slot with a null value. None of the + // assignment code below can take a null, so deletion must be handled before anything else. + if (v.IsNull) + { + return DeleteItemImpl(cls, ob, idx); + } + if (cls.indexer == null || !cls.indexer.CanSet) { Exceptions.SetError(Exceptions.TypeError, "object doesn't support item assignment"); @@ -560,6 +567,44 @@ static int mp_ass_subscript_impl(BorrowedReference ob, BorrowedReference idx, Bo return 0; } + /// + /// Implements __delitem__ (del ob[key]) for reflected classes: IDictionary<K,V>.Remove or + /// IList<T>.RemoveAt through the binder, TypeError for everything else. + /// + static int DeleteItemImpl(ClassBase cls, BorrowedReference ob, BorrowedReference idx) + { + if (cls.indexer == null || !cls.indexer.CanDelete) + { + Exceptions.SetError(Exceptions.TypeError, "object doesn't support item deletion"); + return -1; + } + + if (Runtime.PyTuple_Check(idx)) + { + Exceptions.SetError(Exceptions.TypeError, "object doesn't support multi-index item deletion"); + return -1; + } + + using var args = Runtime.PyTuple_New(1); + Runtime.PyTuple_SetItem(args.Borrow(), 0, idx); + + // The binder converts the key and turns a managed exception into a Python error. + using var result = cls.indexer.DeleteItem(ob, args.Borrow()); + if (result.IsNull() || Exceptions.ErrorOccurred()) + { + return -1; + } + + // IDictionary.Remove reports a missing key by returning false; match dict semantics. + if (result.Borrow() == Runtime.PyFalse) + { + Exceptions.SetError(Exceptions.KeyError, idx); + return -1; + } + + return 0; + } + static NewReference tp_call_impl(BorrowedReference ob, BorrowedReference args, BorrowedReference kw) { BorrowedReference tp = Runtime.PyObject_TYPE(ob); diff --git a/src/runtime/Types/Indexer.cs b/src/runtime/Types/Indexer.cs index 2ef079710..3d7dacc60 100644 --- a/src/runtime/Types/Indexer.cs +++ b/src/runtime/Types/Indexer.cs @@ -1,4 +1,6 @@ using System; +using System.Collections.Generic; +using System.Linq; using System.Reflection; namespace Python.Runtime @@ -11,11 +13,13 @@ internal class Indexer { public MethodBinder GetterBinder; public MethodBinder SetterBinder; + public MethodBinder DeleterBinder; public Indexer() { GetterBinder = new MethodBinder(); SetterBinder = new MethodBinder(); + DeleterBinder = new MethodBinder(); } @@ -29,6 +33,11 @@ public bool CanSet get { return SetterBinder.Count > 0; } } + public bool CanDelete + { + get { return DeleterBinder?.Count > 0; } + } + public void AddProperty(PropertyInfo pi) { @@ -55,6 +64,54 @@ internal void SetItem(BorrowedReference inst, BorrowedReference args) SetterBinder.Invoke(inst, args, null); } + /// + /// Resolves the method behind del ob[key]: IDictionary<K,V>.Remove(K), else + /// IList<T>.RemoveAt(int). Types with neither don't support item deletion. + /// + internal void ResolveDeleter(Type type) + { + // Bind the interface method itself, not a member looked up by name: explicit implementations + // (e.g. ConcurrentDictionary.Remove, which only exposes TryRemove publicly) are reached this way. + var interfaces = type.GetInterfaces().AsEnumerable(); + if (type.IsInterface) + { + interfaces = interfaces.Prepend(type); + } + + foreach (var iface in interfaces) + { + if (iface.IsConstructedGenericType && iface.GetGenericTypeDefinition() == typeof(IDictionary<,>)) + { + var remove = iface.GetMethod(nameof(IDictionary.Remove), new[] { iface.GetGenericArguments()[0] }); + if (remove != null) + { + DeleterBinder.AddMethod(remove, true); + } + } + } + if (CanDelete) + { + return; + } + + foreach (var iface in interfaces) + { + if (iface.IsConstructedGenericType && iface.GetGenericTypeDefinition() == typeof(IList<>)) + { + var removeAt = iface.GetMethod(nameof(IList.RemoveAt), new[] { typeof(int) }); + if (removeAt != null) + { + DeleterBinder.AddMethod(removeAt, true); + } + } + } + } + + internal NewReference DeleteItem(BorrowedReference inst, BorrowedReference args) + { + return DeleterBinder.Invoke(inst, args, null); + } + internal bool NeedsDefaultArgs(BorrowedReference args) { var pynargs = Runtime.PyTuple_Size(args); diff --git a/src/testing/indexertest.cs b/src/testing/indexertest.cs index 08e6ad053..2088a5ad8 100644 --- a/src/testing/indexertest.cs +++ b/src/testing/indexertest.cs @@ -1,4 +1,6 @@ +using System; using System.Collections; +using System.Collections.Generic; namespace Python.Test { @@ -412,6 +414,38 @@ public MultiDefaultKeyIndexerTest() : base() } } + /// + /// IDictionary whose Remove throws: `del ob[key]` must surface it as a catchable Python error. + /// + public class ThrowingRemoveDictionary : IDictionary + { + private readonly Dictionary _items = new Dictionary(); + + public string Marker => "alive"; + + public string this[string key] + { + get { return _items[key]; } + set { _items[key] = value; } + } + + public ICollection Keys => _items.Keys; + public ICollection Values => _items.Values; + public int Count => _items.Count; + public bool IsReadOnly => false; + public void Add(string key, string value) => _items.Add(key, value); + public void Add(KeyValuePair item) => _items.Add(item.Key, item.Value); + public void Clear() => _items.Clear(); + public bool Contains(KeyValuePair item) => _items.ContainsKey(item.Key); + public bool ContainsKey(string key) => _items.ContainsKey(key); + public void CopyTo(KeyValuePair[] array, int arrayIndex) { } + public IEnumerator> GetEnumerator() => _items.GetEnumerator(); + IEnumerator IEnumerable.GetEnumerator() => _items.GetEnumerator(); + public bool Remove(string key) => throw new InvalidOperationException("remove failed"); + public bool Remove(KeyValuePair item) => throw new InvalidOperationException("remove failed"); + public bool TryGetValue(string key, out string value) => _items.TryGetValue(key, out value); + } + public class PublicInheritedIndexerTest : PublicIndexerTest { } public class ProtectedInheritedIndexerTest : ProtectedIndexerTest { } diff --git a/tests/test_indexer.py b/tests/test_indexer.py index 7db68df3e..aac6fbfe0 100644 --- a/tests/test_indexer.py +++ b/tests/test_indexer.py @@ -642,3 +642,128 @@ def test_public_inherited_overloaded_indexer(): with pytest.raises(TypeError): ob[[]] + + +def test_del_settable_indexer_raises_type_error(): + """`del ob[key]` on a type with a settable indexer but no delete support must raise + a catchable TypeError, not crash the process (PythonnetEnterprise GH #167).""" + ob = Test.PublicIndexerTest() + ob[0] = "zero" + + with pytest.raises(TypeError): + del ob[0] + + # The interpreter is alive and the object is untouched and still usable. + assert ob[0] == "zero" + ob[0] = "one" + assert ob[0] == "one" + + +def test_del_multi_arg_indexer_raises_type_error(): + """Tuple-key counterpart: the multi-parameter setter path must not see the null value.""" + ob = Test.MultiArgIndexerTest() + ob[0, 1] = "zero-one" + + with pytest.raises(TypeError): + del ob[0, 1] + + assert ob[0, 1] == "zero-one" + + +def test_del_dictionary_item(): + """`del d[key]` removes the key via IDictionary.Remove; a missing key is a KeyError.""" + from System.Collections.Generic import Dictionary + + d = Dictionary[str, str]() + d["MyKey"] = "MyValue" + + with pytest.raises(KeyError): + del d["missing"] + assert d.Count == 1 + + del d["MyKey"] + assert d.Count == 0 + assert not d.ContainsKey("MyKey") + + with pytest.raises(KeyError): + del d["MyKey"] + + +def test_del_dictionary_wrong_key_type(): + from System.Collections.Generic import Dictionary + + d = Dictionary[str, str]() + d["a"] = "b" + + with pytest.raises(TypeError): + del d[1] + + assert d.Count == 1 + + +def test_del_concurrent_dictionary_item(): + """ConcurrentDictionary implements IDictionary.Remove explicitly (only TryRemove is a + public member). It is the type behind QCAlgorithm.RuntimeStatistics in GH #167.""" + from System.Collections.Concurrent import ConcurrentDictionary + + d = ConcurrentDictionary[str, str]() + d["MyKey"] = "MyValue" + assert d["MyKey"] == "MyValue" + + del d["MyKey"] + + assert d.Count == 0 + assert not d.ContainsKey("MyKey") + + with pytest.raises(KeyError): + del d["MyKey"] + + +def test_del_list_item(): + """`del l[i]` removes the element via IList.RemoveAt; out of range surfaces the .NET error.""" + from System import ArgumentOutOfRangeException + from System.Collections.Generic import List + + l = List[str]() + l.Add("a") + l.Add("b") + + with pytest.raises(ArgumentOutOfRangeException): + del l[5] + assert l.Count == 2 + + del l[0] + assert l.Count == 1 + assert l[0] == "b" + + +def test_del_array_item_raises_type_error(): + from System import Array + + a = Array[int]([1, 2, 3]) + + with pytest.raises(TypeError): + del a[0] + + assert a[0] == 1 + + +def test_del_on_object_without_indexer_raises_type_error(): + from System import Uri + + with pytest.raises(TypeError): + del Uri("http://www.example.com")[0] + + +def test_throwing_remove_does_not_crash(): + """A managed Remove that throws must raise a catchable Python exception and leave the + interpreter and the object usable.""" + ob = Test.ThrowingRemoveDictionary() + ob["k"] = "v" + + with pytest.raises(Exception) as excinfo: + del ob["k"] + assert "InvalidOperationException" in type(excinfo.value).__name__ + + assert ob.Marker == "alive" + assert ob["k"] == "v"