- Notifications
You must be signed in to change notification settings - Fork748
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
1783 Implement Interface And Inherit Class#2028
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
src/runtime/Types/MetaType.cs Outdated
| { | ||
| 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 comment
The reason will be displayed to describe this comment to others.Learn more.
IMHO, it is better to enforce the order to beBaseClass, IInterface1, IInterface2, etc.
I believe the current code has a bug down below because it usesbase_type which is not theBaseClass if the order is different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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 comment
The reason will be displayed to describe this comment to others.Learn more.
Please, rebase before posting a new iteration.
P.S. sorry for the late response
| if (classBaseIt.type.Valid && classBaseIt.type.Value.IsInterface) | ||
| interfaces.Add(classBaseIt.type.Value); | ||
| else baseTypes.Add(classBaseIt); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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 comment
The reason will be displayed to describe this comment to others.Learn more.
Added exceptions here in the latest version.
src/runtime/Types/MetaType.cs Outdated
| 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 comment
The reason will be displayed to describe this comment to others.Learn more.
If the base class has a parameterless constructor
??? 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 comment
The 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.
| } | ||
| } | ||
| var base_type = Runtime.PyTuple_GetItem(bases, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
bases here can be empty since you removed check above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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.
| // 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 comment
The 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 comment
The reason will be displayed to describe this comment to others.Learn more.
Test added.
src/runtime/Types/MetaType.cs Outdated
| // Multiple inheritance is not supported, unless the other types are interfaces | ||
| if (baseTypes.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 comment
The 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 comment
The reason will be displayed to describe this comment to others.Learn more.
Ok, I'll include this.
src/runtime/Types/MetaType.cs Outdated
| if (GetManagedObject(baseTypeIt) is ClassBase classBaseIt) | ||
| { | ||
| if (classBaseIt.type.Valid && classBaseIt.type.Value.IsInterface) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
InvalidClassBase instances should also raise an error regardless of anything else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
got it, adding exceptions.
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.
- Added exceptions during unintended use of superclasses.- Removed invalid comment.- Improved exceptions from MetaType slightly.
cf4d59a to7acbb22CompareThanks for doing the review. I have rebased and fixed things you mentioned. Will you mark the conversations resolved where appropriate? |
Thank you for the contribution! |
* 1783 Implement Interface And Inherit ClassAdded 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.
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.
Close#1783
Checklist
Check all those that are applicable and complete.
AUTHORSCHANGELOG