Commit 25a94e4
committed
reworked the way .NET objects are constructed from Python
was: tp_new implementation would call .NET constructor and return a fully constructed object
now:
Except for some special .NET types tp_new creates uninitialized .NET object, which is later initialized by calling __init__.
__init__ is set using type dictionary to a MethodObject, that contains ConstructorInfo[] instead of MethodInfo[]
This allows Python to:
1) freely override __init__
2) when deriving from .NET types call base __init__ (e.g. .NET constructor), and choose the overload as needed
fixes #2381 parent 8d61215 commit 25a94e4
21 files changed
Lines changed: 294 additions & 481 deletions
File tree
- src
- embed_tests/StateSerialization
- runtime
- testing
- tests
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -46,6 +46,9 @@ details about the cause of the failure | |||
| 46 | 46 | - floating point values passed from Python are no longer silently truncated | |
| 47 | 47 | when .NET expects an integer [#1342][i1342] | |
| 48 | 48 | - More specific error messages for method argument mismatch | |
| 49 | + - BREAKING: when inheriting from .NET types in Python if you override `__init__` you | ||
| 50 | + must explicitly call base constructor using `super().__init__(.....)`. Not doing so will lead | ||
| 51 | + to undefined behavior. | ||
| 49 | 52 | - BREAKING: most `PyScope` methods will never return `null`. Instead, `PyObject` `None` will be returned. | |
| 50 | 53 | - BREAKING: `PyScope` was renamed to `PyModule` | |
| 51 | 54 | - BREAKING: Methods with `ref` or `out` parameters and void return type return a tuple of only the `ref` and `out` parameters. | |
@@ -85,6 +88,7 @@ Instead, `PyIterable` does that. | |||
| 85 | 88 | ### Fixed | |
| 86 | 89 | ||
| 87 | 90 | - Fix incorrect dereference of wrapper object in `tp_repr`, which may result in a program crash | |
| 91 | + - Fixed parameterless .NET constructor being silently called when a matching constructor overload is not found ([#238][i238]) | ||
| 88 | 92 | - Fix incorrect dereference in params array handling | |
| 89 | 93 | - Fixes issue with function resolution when calling overloaded function with keyword arguments from python ([#1097][i1097]) | |
| 90 | 94 | - Fix `object[]` parameters taking precedence when should not in overload resolution | |
@@ -874,3 +878,4 @@ This version improves performance on benchmarks significantly compared to 2.3. | |||
| 874 | 878 | [p534]: https://github.com/pythonnet/pythonnet/pull/534 | |
| 875 | 879 | [i449]: https://github.com/pythonnet/pythonnet/issues/449 | |
| 876 | 880 | [i1342]: https://github.com/pythonnet/pythonnet/issues/1342 | |
| 881 | + [i238]: https://github.com/pythonnet/pythonnet/issues/238 | ||
Lines changed: 11 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -19,6 +19,16 @@ public void GenericRoundtrip() | |||
| 19 | 19 | Assert.AreEqual(method, restored.Value); | |
| 20 | 20 | } | |
| 21 | 21 | ||
| 22 | + [Test] | ||
| 23 | + public void ConstrctorRoundtrip() | ||
| 24 | + { | ||
| 25 | + var ctor = typeof(MethodTestHost).GetConstructor(new[] { typeof(int) }); | ||
| 26 | + var maybeConstructor = new MaybeMethodBase<MethodBase>(ctor); | ||
| 27 | + var restored = SerializationRoundtrip(maybeConstructor); | ||
| 28 | + Assert.IsTrue(restored.Valid); | ||
| 29 | + Assert.AreEqual(ctor, restored.Value); | ||
| 30 | + } | ||
| 31 | + | ||
| 22 | 32 | static T SerializationRoundtrip<T>(T item) | |
| 23 | 33 | { | |
| 24 | 34 | using var buf = new MemoryStream(); | |
@@ -31,5 +41,6 @@ static T SerializationRoundtrip<T>(T item) | |||
| 31 | 41 | ||
| 32 | 42 | public class MethodTestHost | |
| 33 | 43 | { | |
| 44 | + public MethodTestHost(int _) { } | ||
| 34 | 45 | public void Generic<T>(T item, T[] array, ref T @ref) { } | |
| 35 | 46 | } | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -41,6 +41,17 @@ public PyObject MoveToPyObject() | |||
| 41 | 41 | return new PyObject(this.StealNullable()); | |
| 42 | 42 | } | |
| 43 | 43 | ||
| 44 | + /// <summary> | ||
| 45 | + /// Creates new instance of <see cref="NewReference"/> which now owns the pointer. | ||
| 46 | + /// Sets the original reference to <c>null</c>, as it no longer owns the pointer. | ||
| 47 | + /// </summary> | ||
| 48 | + public NewReference Move() | ||
| 49 | + { | ||
| 50 | + var result = new NewReference(this); | ||
| 51 | + this.pointer = default; | ||
| 52 | + return result; | ||
| 53 | + } | ||
| 54 | + | ||
| 44 | 55 | /// <summary>Moves ownership of this instance to unmanged pointer</summary> | |
| 45 | 56 | public IntPtr DangerousMoveToPointer() | |
| 46 | 57 | { | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -556,6 +556,17 @@ public virtual void InitializeSlots(BorrowedReference pyType, SlotsHolder slotsH | |||
| 556 | 556 | } | |
| 557 | 557 | } | |
| 558 | 558 | ||
| 559 | + public virtual bool HasCustomNew() => this.GetType().GetMethod("tp_new") is not null; | ||
| 560 | + | ||
| 561 | + public override bool Init(BorrowedReference obj, BorrowedReference args, BorrowedReference kw) | ||
| 562 | + { | ||
| 563 | + if (this.HasCustomNew()) | ||
| 564 | + // initialization must be done in tp_new | ||
| 565 | + return true; | ||
| 566 | + | ||
| 567 | + return base.Init(obj, args, kw); | ||
| 568 | + } | ||
| 569 | + | ||
| 559 | 570 | protected virtual void OnDeserialization(object sender) | |
| 560 | 571 | { | |
| 561 | 572 | this.dotNetMembers = new List<string>(); | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -50,23 +50,26 @@ internal ClassDerivedObject(Type tp) : base(tp) | |||
| 50 | 50 | { | |
| 51 | 51 | } | |
| 52 | 52 | ||
| 53 | - /// <summary> | ||
| 54 | - /// Implements __new__ for derived classes of reflected classes. | ||
| 55 | - /// </summary> | ||
| 56 | - public new static NewReference tp_new(BorrowedReference tp, BorrowedReference args, BorrowedReference kw) | ||
| 53 | + protected override NewReference NewObjectToPython(object obj, BorrowedReference tp) | ||
| 57 | 54 | { | |
| 58 | - var cls = (ClassDerivedObject)GetManagedObject(tp)!; | ||
| 55 | + var self = base.NewObjectToPython(obj, tp); | ||
| 59 | 56 | ||
| 60 | - // call the managed constructor | ||
| 61 | - object? obj = cls.binder.InvokeRaw(null, args, kw); | ||
| 62 | - if (obj == null) | ||
| 57 | + SetPyObj((IPythonDerivedType)obj, self.Borrow()); | ||
| 58 | + | ||
| 59 | + // Decrement the python object's reference count. | ||
| 60 | + // This doesn't actually destroy the object, it just sets the reference to this object | ||
| 61 | + // to be a weak reference and it will be destroyed when the C# object is destroyed. | ||
| 62 | + if (!self.IsNull()) | ||
| 63 | 63 | { | |
| 64 | - return default; | ||
| 64 | + Runtime.XDecref(self.Steal()); | ||
| 65 | 65 | } | |
| 66 | 66 | ||
| 67 | - // return the pointer to the python object | ||
| 68 | - // (this indirectly calls ClassDerivedObject.ToPython) | ||
| 69 | - return Converter.ToPython(obj, cls.GetType()); | ||
| 67 | + return Converter.ToPython(obj, type.Value); | ||
| 68 | + } | ||
| 69 | + | ||
| 70 | + protected override void SetTypeNewSlot(BorrowedReference pyType, SlotsHolder slotsHolder) | ||
| 71 | + { | ||
| 72 | + // Python derived types rely on base tp_new and overridden __init__ | ||
| 70 | 73 | } | |
| 71 | 74 | ||
| 72 | 75 | public new static void tp_dealloc(NewReference ob) | |
@@ -824,37 +827,24 @@ public static void InvokeSetProperty<T>(IPythonDerivedType obj, string propertyN | |||
| 824 | 827 | ||
| 825 | 828 | public static void InvokeCtor(IPythonDerivedType obj, string origCtorName, object[] args) | |
| 826 | 829 | { | |
| 830 | + var selfRef = GetPyObj(obj); | ||
| 831 | + if (selfRef.Ref == null) | ||
| 832 | + { | ||
| 833 | + // this might happen when the object is created from .NET | ||
| 834 | + using var _ = Py.GIL(); | ||
| 835 | + // In the end we decrement the python object's reference count. | ||
| 836 | + // This doesn't actually destroy the object, it just sets the reference to this object | ||
| 837 | + // to be a weak reference and it will be destroyed when the C# object is destroyed. | ||
| 838 | + using var self = CLRObject.GetReference(obj, obj.GetType()); | ||
| 839 | + SetPyObj(obj, self.Borrow()); | ||
| 840 | + } | ||
| 841 | + | ||
| 827 | 842 | // call the base constructor | |
| 828 | 843 | obj.GetType().InvokeMember(origCtorName, | |
| 829 | 844 | BindingFlags.InvokeMethod, | |
| 830 | 845 | null, | |
| 831 | 846 | obj, | |
| 832 | 847 | args); | |
| 833 | - | ||
| 834 | - NewReference self = default; | ||
| 835 | - PyGILState gs = Runtime.PyGILState_Ensure(); | ||
| 836 | - try | ||
| 837 | - { | ||
| 838 | - // create the python object | ||
| 839 | - var type = ClassManager.GetClass(obj.GetType()); | ||
| 840 | - self = CLRObject.GetReference(obj, type); | ||
| 841 | - | ||
| 842 | - // set __pyobj__ to self and deref the python object which will allow this | ||
| 843 | - // object to be collected. | ||
| 844 | - SetPyObj(obj, self.Borrow()); | ||
| 845 | - } | ||
| 846 | - finally | ||
| 847 | - { | ||
| 848 | - // Decrement the python object's reference count. | ||
| 849 | - // This doesn't actually destroy the object, it just sets the reference to this object | ||
| 850 | - // to be a weak reference and it will be destroyed when the C# object is destroyed. | ||
| 851 | - if (!self.IsNull()) | ||
| 852 | - { | ||
| 853 | - Runtime.XDecref(self.Steal()); | ||
| 854 | - } | ||
| 855 | - | ||
| 856 | - Runtime.PyGILState_Release(gs); | ||
| 857 | - } | ||
| 858 | 848 | } | |
| 859 | 849 | ||
| 860 | 850 | public static void PyFinalize(IPythonDerivedType obj) | |
@@ -890,7 +880,7 @@ internal static UnsafeReferenceWithRun GetPyObj(IPythonDerivedType obj) | |||
| 890 | 880 | return (UnsafeReferenceWithRun)fi.GetValue(obj); | |
| 891 | 881 | } | |
| 892 | 882 | ||
| 893 | - static void SetPyObj(IPythonDerivedType obj, BorrowedReference pyObj) | ||
| 883 | + internal static void SetPyObj(IPythonDerivedType obj, BorrowedReference pyObj) | ||
| 894 | 884 | { | |
| 895 | 885 | FieldInfo fi = GetPyObjField(obj.GetType())!; | |
| 896 | 886 | fi.SetValue(obj, new UnsafeReferenceWithRun(pyObj)); | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -210,7 +210,7 @@ internal static void InitClassBase(Type type, ClassBase impl, ReflectedClrType p | |||
| 210 | 210 | // information, including generating the member descriptors | |
| 211 | 211 | // that we'll be putting in the Python class __dict__. | |
| 212 | 212 | ||
| 213 | - ClassInfo info = GetClassInfo(type); | ||
| 213 | + ClassInfo info = GetClassInfo(type, impl); | ||
| 214 | 214 | ||
| 215 | 215 | impl.indexer = info.indexer; | |
| 216 | 216 | impl.richcompare.Clear(); | |
@@ -252,16 +252,17 @@ internal static void InitClassBase(Type type, ClassBase impl, ReflectedClrType p | |||
| 252 | 252 | // required that the ClassObject.ctors be changed to internal | |
| 253 | 253 | if (co != null) | |
| 254 | 254 | { | |
| 255 | - if (co.NumCtors > 0) | ||
| 255 | + if (co.NumCtors > 0 && !co.HasCustomNew()) | ||
| 256 | 256 | { | |
| 257 | 257 | // Implement Overloads on the class object | |
| 258 | 258 | if (!CLRModule._SuppressOverloads) | |
| 259 | 259 | { | |
| 260 | - using var ctors = new ConstructorBinding(type, pyType, co.binder).Alloc(); | ||
| 261 | - // ExtensionType types are untracked, so don't Incref() them. | ||
| 260 | + // HACK: __init__ points to instance constructors. | ||
| 261 | + // When unbound they fully instantiate object, so we get overloads for free from MethodBinding. | ||
| 262 | + var init = info.members["__init__"]; | ||
| 262 | 263 | // TODO: deprecate __overloads__ soon... | |
| 263 | - Runtime.PyDict_SetItem(dict, PyIdentifier.__overloads__, ctors.Borrow()); | ||
| 264 | - Runtime.PyDict_SetItem(dict, PyIdentifier.Overloads, ctors.Borrow()); | ||
| 264 | + Runtime.PyDict_SetItem(dict, PyIdentifier.__overloads__, init); | ||
| 265 | + Runtime.PyDict_SetItem(dict, PyIdentifier.Overloads, init); | ||
| 265 | 266 | } | |
| 266 | 267 | ||
| 267 | 268 | // don't generate the docstring if one was already set from a DocStringAttribute. | |
@@ -320,10 +321,10 @@ internal static bool ShouldBindEvent(EventInfo ei) | |||
| 320 | 321 | return ShouldBindMethod(ei.GetAddMethod(true)); | |
| 321 | 322 | } | |
| 322 | 323 | ||
| 323 | - private static ClassInfo GetClassInfo(Type type) | ||
| 324 | + private static ClassInfo GetClassInfo(Type type, ClassBase impl) | ||
| 324 | 325 | { | |
| 325 | 326 | var ci = new ClassInfo(); | |
| 326 | - var methods = new Dictionary<string, List<MethodInfo>>(); | ||
| 327 | + var methods = new Dictionary<string, List<MethodBase>>(); | ||
| 327 | 328 | MethodInfo meth; | |
| 328 | 329 | ExtensionType ob; | |
| 329 | 330 | string name; | |
@@ -420,13 +421,33 @@ private static ClassInfo GetClassInfo(Type type) | |||
| 420 | 421 | continue; | |
| 421 | 422 | } | |
| 422 | 423 | name = meth.Name; | |
| 424 | + | ||
| 425 | + //TODO mangle? | ||
| 426 | + if (name == "__init__" && !impl.HasCustomNew()) | ||
| 427 | + continue; | ||
| 428 | + | ||
| 423 | 429 | if (!methods.TryGetValue(name, out var methodList)) | |
| 424 | 430 | { | |
| 425 | - methodList = methods[name] = new List<MethodInfo>(); | ||
| 431 | + methodList = methods[name] = new List<MethodBase>(); | ||
| 426 | 432 | } | |
| 427 | 433 | methodList.Add(meth); | |
| 428 | 434 | continue; | |
| 429 | 435 | ||
| 436 | + case MemberTypes.Constructor when !impl.HasCustomNew(): | ||
| 437 | + var ctor = (ConstructorInfo)mi; | ||
| 438 | + if (ctor.IsStatic) | ||
| 439 | + { | ||
| 440 | + continue; | ||
| 441 | + } | ||
| 442 | + | ||
| 443 | + name = "__init__"; | ||
| 444 | + if (!methods.TryGetValue(name, out methodList)) | ||
| 445 | + { | ||
| 446 | + methodList = methods[name] = new List<MethodBase>(); | ||
| 447 | + } | ||
| 448 | + methodList.Add(ctor); | ||
| 449 | + continue; | ||
| 450 | + | ||
| 430 | 451 | case MemberTypes.Property: | |
| 431 | 452 | var pi = (PropertyInfo)mi; | |
| 432 | 453 | ||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -1,6 +1,8 @@ | |||
| 1 | 1 | using System; | |
| 2 | + using System.Diagnostics; | ||
| 2 | 3 | using System.Linq; | |
| 3 | 4 | using System.Reflection; | |
| 5 | + using System.Runtime.Serialization; | ||
| 4 | 6 | ||
| 5 | 7 | namespace Python.Runtime | |
| 6 | 8 | { | |
@@ -13,18 +15,12 @@ namespace Python.Runtime | |||
| 13 | 15 | [Serializable] | |
| 14 | 16 | internal class ClassObject : ClassBase | |
| 15 | 17 | { | |
| 16 | - internal ConstructorBinder binder; | ||
| 17 | - internal int NumCtors = 0; | ||
| 18 | + internal readonly int NumCtors = 0; | ||
| 18 | 19 | ||
| 19 | 20 | internal ClassObject(Type tp) : base(tp) | |
| 20 | 21 | { | |
| 21 | 22 | var _ctors = type.Value.GetConstructors(); | |
| 22 | 23 | NumCtors = _ctors.Length; | |
| 23 | - binder = new ConstructorBinder(type.Value); | ||
| 24 | - foreach (ConstructorInfo t in _ctors) | ||
| 25 | - { | ||
| 26 | - binder.AddMethod(t); | ||
| 27 | - } | ||
| 28 | 24 | } | |
| 29 | 25 | ||
| 30 | 26 | ||
@@ -33,7 +29,12 @@ internal ClassObject(Type tp) : base(tp) | |||
| 33 | 29 | /// </summary> | |
| 34 | 30 | internal NewReference GetDocString() | |
| 35 | 31 | { | |
| 36 | - MethodBase[] methods = binder.GetMethods(); | ||
| 32 | + if (!type.Valid) | ||
| 33 | + { | ||
| 34 | + return Exceptions.RaiseTypeError(type.DeletedMessage); | ||
| 35 | + } | ||
| 36 | + | ||
| 37 | + MethodBase[] methods = type.Value.GetConstructors(); | ||
| 37 | 38 | var str = ""; | |
| 38 | 39 | foreach (MethodBase t in methods) | |
| 39 | 40 | { | |
@@ -50,7 +51,7 @@ internal NewReference GetDocString() | |||
| 50 | 51 | /// <summary> | |
| 51 | 52 | /// Implements __new__ for reflected classes and value types. | |
| 52 | 53 | /// </summary> | |
| 53 | - public static NewReference tp_new(BorrowedReference tp, BorrowedReference args, BorrowedReference kw) | ||
| 54 | + static NewReference tp_new_impl(BorrowedReference tp, BorrowedReference args, BorrowedReference kw) | ||
| 54 | 55 | { | |
| 55 | 56 | var self = GetManagedObject(tp) as ClassObject; | |
| 56 | 57 | ||
@@ -100,15 +101,49 @@ public static NewReference tp_new(BorrowedReference tp, BorrowedReference args, | |||
| 100 | 101 | return NewEnum(type, args, tp); | |
| 101 | 102 | } | |
| 102 | 103 | ||
| 103 | - object? obj = self.binder.InvokeRaw(null, args, kw); | ||
| 104 | - if (obj == null) | ||
| 104 | + if (IsGenericNullable(type)) | ||
| 105 | 105 | { | |
| 106 | - return default; | ||
| 106 | + // Nullable<T> has special handling in .NET runtime. | ||
| 107 | + // Invoking its constructor via reflection on an uninitialized instance | ||
| 108 | + // does not actually set the object fields. | ||
| 109 | + return NewNullable(type, args, kw, tp); | ||
| 107 | 110 | } | |
| 108 | 111 | ||
| 109 | - return CLRObject.GetReference(obj, tp); | ||
| 112 | + object obj = FormatterServices.GetUninitializedObject(type); | ||
| 113 | + | ||
| 114 | + return self.NewObjectToPython(obj, tp); | ||
| 115 | + } | ||
| 116 | + | ||
| 117 | + protected virtual void SetTypeNewSlot(BorrowedReference pyType, SlotsHolder slotsHolder) | ||
| 118 | + { | ||
| 119 | + TypeManager.InitializeSlotIfEmpty(pyType, TypeOffset.tp_new, new Interop.BBB_N(tp_new_impl), slotsHolder); | ||
| 120 | + } | ||
| 121 | + | ||
| 122 | + public override bool HasCustomNew() | ||
| 123 | + { | ||
| 124 | + if (base.HasCustomNew()) return true; | ||
| 125 | + | ||
| 126 | + Type clrType = type.Value; | ||
| 127 | + return clrType.IsPrimitive | ||
| 128 | + || clrType.IsEnum | ||
| 129 | + || clrType == typeof(string) | ||
| 130 | + || IsGenericNullable(clrType); | ||
| 110 | 131 | } | |
| 111 | 132 | ||
| 133 | + static bool IsGenericNullable(Type type) | ||
| 134 | + => type.IsValueType && type.IsGenericType | ||
| 135 | + && type.GetGenericTypeDefinition() == typeof(Nullable<>); | ||
| 136 | + | ||
| 137 | + public override void InitializeSlots(BorrowedReference pyType, SlotsHolder slotsHolder) | ||
| 138 | + { | ||
| 139 | + base.InitializeSlots(pyType, slotsHolder); | ||
| 140 | + | ||
| 141 | + this.SetTypeNewSlot(pyType, slotsHolder); | ||
| 142 | + } | ||
| 143 | + | ||
| 144 | + protected virtual NewReference NewObjectToPython(object obj, BorrowedReference tp) | ||
| 145 | + => CLRObject.GetReference(obj, tp); | ||
| 146 | + | ||
| 112 | 147 | private static NewReference NewEnum(Type type, BorrowedReference args, BorrowedReference tp) | |
| 113 | 148 | { | |
| 114 | 149 | nint argCount = Runtime.PyTuple_Size(args); | |
@@ -146,6 +181,28 @@ private static NewReference NewEnum(Type type, BorrowedReference args, BorrowedR | |||
| 146 | 181 | return CLRObject.GetReference(enumValue, tp); | |
| 147 | 182 | } | |
| 148 | 183 | ||
| 184 | + private static NewReference NewNullable(Type type, BorrowedReference args, BorrowedReference kw, BorrowedReference tp) | ||
| 185 | + { | ||
| 186 | + Debug.Assert(IsGenericNullable(type)); | ||
| 187 | + | ||
| 188 | + if (kw != null) | ||
| 189 | + { | ||
| 190 | + return Exceptions.RaiseTypeError("System.Nullable<T> constructor does not support keyword arguments"); | ||
| 191 | + } | ||
| 192 | + | ||
| 193 | + nint argsCount = Runtime.PyTuple_Size(args); | ||
| 194 | + if (argsCount != 1) | ||
| 195 | + { | ||
| 196 | + return Exceptions.RaiseTypeError("System.Nullable<T> constructor expects 1 argument, got " + (int)argsCount); | ||
| 197 | + } | ||
| 198 | + | ||
| 199 | + var value = Runtime.PyTuple_GetItem(args, 0); | ||
| 200 | + var elementType = type.GetGenericArguments()[0]; | ||
| 201 | + return Converter.ToManaged(value, elementType, out var result, setError: true) | ||
| 202 | + ? CLRObject.GetReference(result!, tp) | ||
| 203 | + : default; | ||
| 204 | + } | ||
| 205 | + | ||
| 149 | 206 | ||
| 150 | 207 | /// <summary> | |
| 151 | 208 | /// Implementation of [] semantics for reflected types. This exists | |
0 commit comments