Skip to content

Hard crash when deleting from Dictionary #2530

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
DiegoBaldassarMilleuno opened this issue Dec 13, 2024 · 5 comments · Fixed by #2533
Closed

Hard crash when deleting from Dictionary #2530

DiegoBaldassarMilleuno opened this issue Dec 13, 2024 · 5 comments · Fixed by #2533
Labels
bug

Comments

@DiegoBaldassarMilleuno
Copy link

DiegoBaldassarMilleuno commented Dec 13, 2024 •
edited
Loading

Environment

  • Pythonnet version: 3.0.5
  • Python version: both 3.12 and 3.13
  • Operating System: MacOS Sequoia 15.1.1
  • .NET Runtime: both 7.0.410 and 8.0.403

Details

  • Describe what you were trying to get done.

Remove an element from a .NET Dictionary using the del operator (__delitem__).

I reduced the problem to the following example, simply run from the command line:

import pythonnet
pythonnet.load('coreclr')
print(pythonnet.get_runtime_info(), end='\n\n')

import clr

from System.Collections.Generic import Dictionary
from System import Int32, String

DT = Dictionary[Int32, String]
d = DT()
d[10] = '10'
d[20] = '20'
print('__delitem__ is implemented by {}'.format(
    next(iter(cls for cls in DT.mro() if hasattr(cls, '__delitem__')))
))
del d[20]  # crash
  • If there was a crash, please include the traceback here.

The program crashed with the following output:

Runtime: CoreCLR
=============
  Version:      <undefined>
  Initialized:  True
  Shut down:    False
  Properties:
    HOST_RUNTIME_CONTRACT = 0x600003a840c8
    PROBING_DIRECTORIES =
    RUNTIME_IDENTIFIER = osx-arm64
    FX_DEPS_FILE = /usr/local/share/dotnet/shared/Microsoft.NETCore.App…
    APP_CONTEXT_DEPS_FILES = /usr/local/share/dotnet/shared/Microsoft.N…
    APP_CONTEXT_BASE_DIRECTORY =
    PLATFORM_RESOURCE_ROOTS = :
    NATIVE_DLL_SEARCH_DIRECTORIES = :/usr/local/share/dotnet/shared/Mic…
    TRUSTED_PLATFORM_ASSEMBLIES = /usr/local/share/dotnet/shared/Micros…

__delitem__ is implemented by <class 'System.Collections.Generic.Dictionary[Int32,String]'>
Unhandled exception. System.NullReferenceException: Object reference not set to an instance of an object.
   at Python.Runtime.BorrowedReference.DangerousGetAddress() in /home/benedikt/.cache/uv/sdists-v6/.tmpkW03e6/pythonnet-3.0.5/src/runtime/Native/BorrowedReference.cs:line 18
   at Python.Runtime.NewReference..ctor(BorrowedReference reference, Boolean canBeNull) in /home/benedikt/.cache/uv/sdists-v6/.tmpkW03e6/pythonnet-3.0.5/src/runtime/Native/NewReference.cs:line 20
   at Python.Runtime.Runtime.PyTuple_SetItem(BorrowedReference pointer, IntPtr index, BorrowedReference value) in /home/benedikt/.cache/uv/sdists-v6/.tmpkW03e6/pythonnet-3.0.5/src/runtime/Runtime.cs:line 1482
   at Python.Runtime.ClassBase.mp_ass_subscript_impl(BorrowedReference ob, BorrowedReference idx, BorrowedReference v) in /home/benedikt/.cache/uv/sdists-v6/.tmpkW03e6/pythonnet-3.0.5/src/runtime/Types/ClassBase.cs:line 498
[1]    56976 abort      python3.12 /tmp/ramdisk/delme_test_pythonnet.py

Notice that __delitem__ is also implemented by the MutableMappingMixin class here, which simply calls the Remove() method, but Dictionary has its own implementation (that I can't find), which takes precedence.
The former would have worked correctly (tested), as Remove() works as usual.
@filmor
Copy link
Member

filmor commented Dec 13, 2024

Thank you for the reproducible example, I will look into it. If you want to have a go at it, you are more than welcome though.

Side note: I have checked that this is not a regression in today's release, the same error happens on all 3.* versions.

@filmor filmor added the bug label Dec 13, 2024
@filmor
Copy link
Member

filmor commented Dec 13, 2024

My first guess is that we simply don't implement the deletion case. For __delitem__, Python passes a null to the value parameter of mp_ass_subscript.

mp_ass_subscript is implemented via Indexers which does not have an equivalent to __delitem__.

So, we have the following options:

  • Throw an exception (that can be caught), I think this is a good first step
  • For ICollection, IDictionary, IList, and classes that implement these, special-case scalar deletions to use the respective remove functions

@lostmsu
Copy link
Member

lostmsu commented Dec 13, 2024 •
edited
Loading

Hm, this actually should work due to

pythonnet/src/runtime/Mixins/collections.py

Line 58 in dfd746b

def __delitem__(self, key):

Maybe it is not enabled by default? It might not be enabled due to scenarios where you don't want these methods to appear in any kind of reflection (from Python).

@DiegoBaldassarMilleuno
Copy link
Author

And it would work, except that the Dictionary class provides its own implementation of __delitem__ that overrides the one in MutableMappingMixin:

>>> import pythonnet; pythonnet.load('coreclr'); import clr
>>> import System
>>> DictType = System.Collections.Generic.Dictionary[System.String, System.Int32]
>>> d = DictType()
>>> d['a'] = 1
>>> d['b'] = 2
>>> d['c'] = 3
>>> for cls in DictType.mro():  # print the classes that implement __delitem__
...     if hasattr(cls, '__delitem__'):
...         print('@@@', cls)
@@@ <class 'System.Collections.Generic.Dictionary[String,Int32]'>
@@@ <class 'clr._extras.collections.MutableMappingMixin'>
@@@ <class 'collections.abc.MutableMapping'>
>>> MutableMappingMixin = DictType.mro()[2]
>>> MutableMappingMixin
<class 'clr._extras.collections.MutableMappingMixin'>
>>> dict(d)
{'a': 1, 'b': 2, 'c': 3}
>>> MutableMappingMixin.__delitem__(d, 'c')  # Calling directly using base class, works OK
>>> dict(d)
{'a': 1, 'b': 2}
>>> super(DictType, d).__delitem__('b')  # Calling on superclass also works OK
>>> dict(d)
{'a': 1}
>>> DictType.__delitem__(d, 'a')  # Equivalent to d.__delitem('a') and del d['a'], crashes
Unhandled exception. System.NullReferenceException: Object reference not set to an instance of an object.
   at Python.Runtime.BorrowedReference.DangerousGetAddress() in /home/benedikt/.cache/uv/sdists-v6/.tmpkW03e6/pythonnet-3.0.5/src/runtime/Native/BorrowedReference.cs:line 18
   at Python.Runtime.NewReference..ctor(BorrowedReference reference, Boolean canBeNull) in /home/benedikt/.cache/uv/sdists-v6/.tmpkW03e6/pythonnet-3.0.5/src/runtime/Native/NewReference.cs:line 20
   at Python.Runtime.Runtime.PyTuple_SetItem(BorrowedReference pointer, IntPtr index, BorrowedReference value) in /home/benedikt/.cache/uv/sdists-v6/.tmpkW03e6/pythonnet-3.0.5/src/runtime/Runtime.cs:line 1482
   at Python.Runtime.ClassBase.mp_ass_subscript_impl(BorrowedReference ob, BorrowedReference idx, BorrowedReference v) in /home/benedikt/.cache/uv/sdists-v6/.tmpkW03e6/pythonnet-3.0.5/src/runtime/Types/ClassBase.cs:line 498

Open
@filmor
Copy link
Member

filmor commented Dec 17, 2024

Well, the direct implementation always wins, and Python, counts mp_ass_subscript as an implementation for __setitem__ and __delitem__. We could go through the __mro__ to find the mixin, but at that point it might make more sense to just extend the mp_ass_subscript implementation to handle the deletion itself.

lostmsu added a commit to losttech/pythonnet that referenced this issue Dec 18, 2024
@lostmsu
implemented __delitem__ for IDictionary<K,V> and IList<T>
2df6e6a 
fixed crash for all other types (now properly throws TypeError)

fixes pythonnet#2530
lostmsu added a commit to losttech/pythonnet that referenced this issue Dec 18, 2024
@lostmsu
implemented __delitem__ for IDictionary<K,V> and IList<T>
7fd0e26 
fixed crash for all other types (now properly throws TypeError)

fixes pythonnet#2530
@lostmsu lostmsu mentioned this issue Dec 18, 2024
Merged
5 tasks
lostmsu added a commit to losttech/pythonnet that referenced this issue Dec 18, 2024
@lostmsu
implemented __delitem__ for IDictionary<K,V> and IList<T>
5f962be 
fixed crash for all other types (now properly throws TypeError)

fixes pythonnet#2530
lostmsu added a commit to losttech/pythonnet that referenced this issue Apr 11, 2025
@lostmsu
implemented __delitem__ for IDictionary<K,V> and IList<T>
9d72d87 
fixed crash for all other types (now properly throws TypeError)

fixes pythonnet#2530
lostmsu added a commit to losttech/pythonnet that referenced this issue Apr 11, 2025
@lostmsu
implemented __delitem__ for IDictionary<K,V> and IList<T>
a14d303 
fixed crash for all other types (now properly throws TypeError)

fixes pythonnet#2530
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

__delitem__ for IList<T> and IDictionary<K,V> losttech/pythonnet
3 participants
@filmor @lostmsu @DiegoBaldassarMilleuno