Mark subclasses of Dimension final. #1761
Conversation
|
@swift-ci test |
Foundation/Unit.swift
Outdated
| @@ -248,7 +248,7 @@ open class Dimension : Unit { | |||
| } | |||
| } | |||
|
|
|||
| open class UnitAcceleration : Dimension { | |||
| final class UnitAcceleration : Dimension { | |||
There was a problem hiding this comment.
I don't think we can take this patch? These subclasses are open in ObjC, and we want as-close-as-possible source parity.
There was a problem hiding this comment.
(I assume we're trying to work around the fact that Swift 5 prevents us from having a pure Swift class that has the equivalent of + (instancetype)baseUnit?)
There was a problem hiding this comment.
At this point I'm testing the option for consideration. If we took it there are other things to consider, such as parity with Objective-C and expectations for users of the APIs.
There was a problem hiding this comment.
IIRC, subclassing is part of the contract of this API. (I will check tomorrow.)
I have a patch I can PR that moves .baseUnit() to be of Dimension type everywhere, erasing the type but allowing the build to work. I will discuss it with @parkera tomorrow morning.
There was a problem hiding this comment.
I suspect the type erasure approach will be possibly source-breaking. Clients using the class method directly on subclasses would get Dimension instead of the more-specific type, and some code may already depend on having a more specific UnitX type as the result of calling baseUnit.
An alternative is to add a runtime check within the baseUnit override implementations to make the type safety provided by a dynamic runtime check. I'm not sure if that is in the contact of the Objective-C version either. Fundamentally, the current baseUnit API isn't type-safe as declared in both Foundation in Objective-C and corelibs-Foundation as a subclass of a subclass of Dimension that does not override baseUnit will be declared as returning Self but won't — it will return an instance of the parent class. That's a type safety hole.
There was a problem hiding this comment.
Yeah, and Swift is just highlighting it. :(
I'll circle back before EOD.
There was a problem hiding this comment.
(It feels like Swift could allow Self class methods if they were similar to required initializers, wherein it is not valid to express a class that doesn't override the method.)
There was a problem hiding this comment.
Yes, that sounds like potentially a reasonable language extension that does not exist today.
There was a problem hiding this comment.
For now, to unblock Swift 5 adoption, we will take this patch.
|
@swift-ci test |
1 similar comment
|
@swift-ci test |
|
We had a side discussion between me, @tkremenek and @parkera and decided that the right way forward is to require subclasses to be made |
Possible resolution of blocking adoption of
-swift-version 5:apple/swift#20374