-
Notifications
You must be signed in to change notification settings - Fork 752
1783 Implement Interface And Inherit Class #2028
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.
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
Added support for multiple inheritance when inheriting from one base class and/or multiple interfaces. Added a unit test verifying that it works with a simple class and interface. This unit test would previously have failed since multiple types are in the class super class list.
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,8 @@ | ||
using System; | ||
using System.Collections.Generic; | ||
using System.Diagnostics; | ||
using System.Linq; | ||
using System.Reflection; | ||
using System.Runtime.InteropServices; | ||
using System.Runtime.Serialization; | ||
|
||
|
@@ -79,49 +82,56 @@ public static NewReference tp_new(BorrowedReference tp, BorrowedReference args, | |
BorrowedReference bases = Runtime.PyTuple_GetItem(args, 1); | ||
BorrowedReference dict = Runtime.PyTuple_GetItem(args, 2); | ||
|
||
// We do not support multiple inheritance, so the bases argument | ||
// should be a 1-item tuple containing the type we are subtyping. | ||
// That type must itself have a managed implementation. We check | ||
// that by making sure its metatype is the CLR metatype. | ||
// Extract interface types and base class types. | ||
var interfaces = new List<Type>(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMHO, repeating the same base interface multiple times should be an error. And it might be, add a test. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test added. |
||
var baseType = new List<ClassBase>(); | ||
rmadsen-ks marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
if (Runtime.PyTuple_Size(bases) != 1) | ||
var cnt = Runtime.PyTuple_GetSize(bases); | ||
rmadsen-ks marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
for (uint i = 0; i < cnt; i++) | ||
{ | ||
return Exceptions.RaiseTypeError("cannot use multiple inheritance with managed classes"); | ||
var base_type2 = Runtime.PyTuple_GetItem(bases, (int)i); | ||
var cb2 = (ClassBase) GetManagedObject(base_type2); | ||
rmadsen-ks marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (cb2 != null) | ||
rmadsen-ks marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
if (cb2.type.Valid && cb2.type.Value.IsInterface) | ||
interfaces.Add(cb2.type.Value); | ||
else baseType.Add(cb2); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMHO, it is better to enforce the order to be I believe the current code has a bug down below because it uses There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have only added this functionality for types which goes through TypeManager.CreateSubType, so it should only affect that branch. for code flows not invoking CreateSubType, the behavior willl be exactly the same as before. |
||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if it is not a .NET class or interface? You can't simply ignore it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added exceptions here in the latest version. |
||
} | ||
|
||
BorrowedReference base_type = Runtime.PyTuple_GetItem(bases, 0); | ||
BorrowedReference mt = Runtime.PyObject_TYPE(base_type); | ||
|
||
if (!(mt == PyCLRMetaType || mt == Runtime.PyTypeType)) | ||
// if the base type count is 0, there might still be interfaces to implement. | ||
if (baseType.Count == 0) | ||
{ | ||
return Exceptions.RaiseTypeError("invalid metatype"); | ||
baseType.Add(new ClassBase(typeof(object))); | ||
} | ||
|
||
// Ensure that the reflected type is appropriate for subclassing, | ||
// disallowing subclassing of delegates, enums and array types. | ||
// Multiple inheritance is not supported, unless the other types are interfaces | ||
if (baseType.Count > 1) | ||
{ | ||
return Exceptions.RaiseTypeError("cannot use multiple inheritance with managed classes"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since you collected multiple base types, perhaps it would be useful to list them in the error message in case user did not notice which ones are classes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I'll include this. |
||
} | ||
|
||
if (GetManagedObject(base_type) is ClassBase cb) | ||
var cb = baseType[0]; | ||
try | ||
{ | ||
try | ||
{ | ||
if (!cb.CanSubclass()) | ||
{ | ||
return Exceptions.RaiseTypeError("delegates, enums and array types cannot be subclassed"); | ||
} | ||
} | ||
catch (SerializationException) | ||
if (!cb.CanSubclass()) | ||
{ | ||
return Exceptions.RaiseTypeError($"Underlying C# Base class {cb.type} has been deleted"); | ||
return Exceptions.RaiseTypeError("delegates, enums and array types cannot be subclassed"); | ||
} | ||
} | ||
catch (SerializationException) | ||
{ | ||
return Exceptions.RaiseTypeError($"Underlying C# Base class {cb.type} has been deleted"); | ||
} | ||
|
||
BorrowedReference slots = Runtime.PyDict_GetItem(dict, PyIdentifier.__slots__); | ||
if (slots != null) | ||
{ | ||
return Exceptions.RaiseTypeError("subclasses of managed classes do not support __slots__"); | ||
} | ||
|
||
// If __assembly__ or __namespace__ are in the class dictionary then create | ||
// If the base class has a parameterless constructor, or | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
??? What is this for? If that's right, it seems breaking to me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I forget, but that does not actually seem to be included with these changes. It is in my fork though. |
||
// if __assembly__ or __namespace__ are in the class dictionary then create | ||
// a managed sub type. | ||
// This creates a new managed type that can be used from .net to call back | ||
// into python. | ||
|
@@ -130,10 +140,12 @@ public static NewReference tp_new(BorrowedReference tp, BorrowedReference args, | |
using var clsDict = new PyDict(dict); | ||
if (clsDict.HasKey("__assembly__") || clsDict.HasKey("__namespace__")) | ||
{ | ||
return TypeManager.CreateSubType(name, base_type, clsDict); | ||
return TypeManager.CreateSubType(name, baseType, interfaces, clsDict); | ||
} | ||
} | ||
|
||
var base_type = Runtime.PyTuple_GetItem(bases, 0); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How can bases be length 0? This should only be called when a C# type is subclassed, right? In this case, it should at least be length 1. |
||
|
||
// otherwise just create a basic type without reflecting back into the managed side. | ||
IntPtr func = Util.ReadIntPtr(Runtime.PyTypeType, TypeOffset.tp_new); | ||
NewReference type = NativeCall.Call_3(func, tp, args, kw); | ||
|
Uh oh!
There was an error while loading. Please reload this page.