(Not for checkin) Fix convenience init to protocol extension init delegation #11635
Conversation
…elfDecl() NFC for now, but is required for correctness once a subsequent patch gives 'self' a DynamicSelfType in convenience initializers.
…ntReturnConversionExpr The UncheckedRefCast instruction supports optional-to-optional casts natively, so we can avoid the unnecessary conditionals here.
NFC for now, but becomes important once a subsequent patch gives 'self' a DynamicSelfType in convenience initializers.
…lfType We don't want to print 'Self' here; the actual concrete type is more useful. NFC for now, but becomes important once a subsequent patch gives 'self' a DynamicSelfType in convenience initializers.
…te to protocol extension initializers A protocol extension initializer creates a new instance of the static type of Self at the call site. However a convenience initializer in a class is expected to initialize an instance of the dynamic type of the 'self' value, because convenience initializers can be inherited by subclasses. This means that when a convenience initializer delegates to a protocol extension initializer, we have to substitute the 'Self' type in the protocol extension generic signature with the DynamicSelfType, and not the static type. Since the substitution is formed from the type of the 'self' parameter in the class convenience initializer, the solution is to change the type of 'self' in a class convenience initializer to DynamicSelfType, just like we do for methods that return 'Self'. This fixes cases where we allowed code to type check that should not type check (if the protocol extension initializer has 'Self' in contravariant position, and we pass in an instance of the static type). It also fixes a miscompile with valid code -- if the protocol extension initializer was implemented by calling 'Self()', it would again use the static type and not the dynamic type. Note that the SILGen change is necessary because Sema now creates CovariantReturnExprs that convert a static class type to DynamicSelfType, but the latter lowers to the former at the SIL level, so we have to peephole away unnecessary unchecked_ref_cast instructions in this case.
|
@swift-ci Please test source compatibility |
|
@swift-ci Please smoke test |
|
@parkera With my change swift-corelibs-foundation no longer builds: The new behavior is technically correct because this delegation is performed from a convenience initializer. Convenience initializers can be inherited, therefore it is not safe to replace 'self' with an 'NSNumber', since you might subclass 'NSNumber'. I can think of three possible fixes:
with something like This is the most general solution. It will allow NSNumber subclasses to be defined as before, but if they add extra stored properties you'll get undefined behavior at runtime. |
|
cc @jrose-apple |
|
Ok, I see you're defining a couple of NSNumber subclasses there, so making it final is a no-go. I'll try the generics hack. |
|
apple/swift-corelibs-foundation#1190 |
slavapestov
changed the title
|
cc @phausler |
Consider a class initializer that delegates to a protocol extension initializer, for example
The
self.init()inside the class initializer must pass a substitution for the Self type parameter to the protocol extension method. It was passing the static type, not the dynamic type. This is wrong because convenience initializers can be inherited, so calling an inherited initializer would use the wrong 'Self' type. This was a problem if the protocol extension method either had 'Self' in contravariant position (in which case we might type check invalid code) or if it constructed a new instance of 'Self' and returned it (in which case we would do the wrong thing at runtime).Fix the problem and add tests for both cases.
Also clean up some related code I looked at along the way.