WIP SR-1715: require superclass to have a designated initializer #14024
Conversation
Tests should probably live somewhere else, not satisfied with options I have found so far. Handles case where super has initializers but not designated initializers, throws a better error. (This might be possible to collapse with other code already throwing this error. Have not yet pursued that.) Numerous tests also need updating to have valid superclasses, those changes follow in a separate commit. Explicitly checks super invalidity. Worth asking - should that bool be set by/from the circularity check somehow, since it replaces a super with an error type?
Non-Swift classes need a valid init method to be subclassable from Swift. New error handling pointed out problems with these tests. The new error will also require new tests ensure that Obj-C types have an init method if they are subclassed in Swift. There are a few failing tests still, but this commit fixes up most failures caught by the new error.
| if (!isInvalidSuperclass && !Super->isInvalid()) { | ||
| bool superIsConstructible = false; // can be satisfied by convenience initializers | ||
| bool superHasDesignatedInitializer = false; // is subclassable | ||
| for (auto member : Super->getMembers()) { |
There was a problem hiding this comment.
There was a problem hiding this comment.
Might work. I'd like to find a refactor that cleans this up for sure.
| @@ -2002,6 +2002,9 @@ ERROR(inheritance_from_cf_class,none, | |||
| ERROR(inheritance_from_objc_runtime_visible_class,none, | |||
| "cannot inherit from class %0 because it is only visible via the " | |||
| "Objective-C runtime", (Identifier)) | |||
| ERROR(inheritance_from_class_with_no_designated_initializers,none, | |||
| "cannot inherit from class %0 because it has no designated initializers", | |||
There was a problem hiding this comment.
Drive-by nit: spaces, not tabs, are used in this file; please align this line as shown above.
| // Require the superclass to have an available designated initializer, | ||
| // unless we have already determined the class or superclass is invalid. | ||
| if (!isInvalidSuperclass && !Super->isInvalid()) { | ||
| bool superIsConstructible = false; // can be satisfied by convenience initializers |
There was a problem hiding this comment.
Another nit: all comments are capitalized and punctuated, and this file respects the 80-character limit for each line.
|
@randomstep Are you still interested in this patch? If so, please rebase it and address the review comments. Otherwise, we can close it out and I can update the SR to point another contributor at this work to see if they can take it over the goal line. Thanks! |
|
Alright, there hasn't been any movement or response here in a while. I'm going to close this out and discharge the SR assignment. If you wish to pursue this change further, please reopen this pull request, rebase it, and address the review feedback. Thanks! |
Description
After checking the superclass in other ways, this PR verifies that the superclass has at least one designated initializer. Classes like NSProxy do not have a designated init, and thus are not sub-classable / instantiatable from Swift. The current error is lacking, and this PR provides a more targeted error.
Bug Link
Resolves SR-1715.
Questions before running CI
Before CI is run, I'd like any PR feedback. Specifically:
Aclass in the test version of Foundation (by adding an init), and that may have changed some assumptions. I am not sure where to even start figuring this out. It does appear to be Swift 2 syntax still in the test, so I could at least clean that up (duplicate labels and similar).