Skip to content

Mark subclasses of Dimension final. #1761

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 13, 2018
Merged

Mark subclasses of Dimension final. #1761

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6b4c3fb
Mark subclasses of Dimension final.
tkremenek Nov 13, 2018
4148d8b
Add back 'public' access level to Dimension subclasses.
tkremenek Nov 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 21 additions & 21 deletions Foundation/Unit.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ open class Dimension : Unit {
}
}

open class UnitAcceleration : Dimension {
final class UnitAcceleration : Dimension {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can take this patch? These subclasses are open in ObjC, and we want as-close-as-possible source parity.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@tkremenek tkremenek Nov 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, and Swift is just highlighting it. :(

I'll circle back before EOD.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that sounds like potentially a reasonable language extension that does not exist today.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, to unblock Swift 5 adoption, we will take this patch.


/*
Base unit - metersPerSecondSquared
Expand Down Expand Up @@ -297,7 +297,7 @@ open class UnitAcceleration : Dimension {
}
}

open class UnitAngle : Dimension {
final class UnitAngle : Dimension {

/*
Base unit - degrees
Expand Down Expand Up @@ -378,7 +378,7 @@ open class UnitAngle : Dimension {
}
}

open class UnitArea : Dimension {
final class UnitArea : Dimension {

/*
Base unit - squareMeters
Expand Down Expand Up @@ -523,7 +523,7 @@ open class UnitArea : Dimension {
}
}

open class UnitConcentrationMass : Dimension {
final class UnitConcentrationMass : Dimension {

/*
Base unit - gramsPerLiter
Expand Down Expand Up @@ -578,7 +578,7 @@ open class UnitConcentrationMass : Dimension {
}
}

open class UnitDispersion : Dimension {
final class UnitDispersion : Dimension {

/*
Base unit - partsPerMillion
Expand Down Expand Up @@ -619,7 +619,7 @@ open class UnitDispersion : Dimension {
}
}

open class UnitDuration : Dimension {
final class UnitDuration : Dimension {

/*
Base unit - seconds
Expand Down Expand Up @@ -676,7 +676,7 @@ open class UnitDuration : Dimension {
}
}

open class UnitElectricCharge : Dimension {
final class UnitElectricCharge : Dimension {
/*
Base unit - coulombs
*/
Expand Down Expand Up @@ -756,7 +756,7 @@ open class UnitElectricCharge : Dimension {
}
}

open class UnitElectricCurrent : Dimension {
final class UnitElectricCurrent : Dimension {

/*
Base unit - amperes
Expand Down Expand Up @@ -830,7 +830,7 @@ open class UnitElectricCurrent : Dimension {
}
}

open class UnitElectricPotentialDifference : Dimension {
final class UnitElectricPotentialDifference : Dimension {

/*
Base unit - volts
Expand Down Expand Up @@ -904,7 +904,7 @@ open class UnitElectricPotentialDifference : Dimension {
}
}

open class UnitElectricResistance : Dimension {
final class UnitElectricResistance : Dimension {

/*
Base unit - ohms
Expand Down Expand Up @@ -978,7 +978,7 @@ open class UnitElectricResistance : Dimension {
}
}

open class UnitEnergy : Dimension {
final class UnitEnergy : Dimension {

/*
Base unit - joules
Expand Down Expand Up @@ -1052,7 +1052,7 @@ open class UnitEnergy : Dimension {
}
}

open class UnitFrequency : Dimension {
final class UnitFrequency : Dimension {

/*
Base unit - hertz
Expand Down Expand Up @@ -1149,7 +1149,7 @@ open class UnitFrequency : Dimension {
}
}

open class UnitFuelEfficiency : Dimension {
final class UnitFuelEfficiency : Dimension {

/*
Base unit - litersPer100Kilometers
Expand Down Expand Up @@ -1206,7 +1206,7 @@ open class UnitFuelEfficiency : Dimension {
}
}

open class UnitLength : Dimension {
final class UnitLength : Dimension {

/*
Base unit - meters
Expand Down Expand Up @@ -1415,7 +1415,7 @@ open class UnitLength : Dimension {
}
}

open class UnitIlluminance : Dimension {
final class UnitIlluminance : Dimension {

/*
Base unit - lux
Expand Down Expand Up @@ -1456,7 +1456,7 @@ open class UnitIlluminance : Dimension {
}
}

open class UnitMass : Dimension {
final class UnitMass : Dimension {

/*
Base unit - kilograms
Expand Down Expand Up @@ -1617,7 +1617,7 @@ open class UnitMass : Dimension {
}
}

open class UnitPower : Dimension {
final class UnitPower : Dimension {

/*
Base unit - watts
Expand Down Expand Up @@ -1738,7 +1738,7 @@ open class UnitPower : Dimension {
}
}

open class UnitPressure : Dimension {
final class UnitPressure : Dimension {

/*
Base unit - newtonsPerMetersSquared (equivalent to 1 pascal)
Expand Down Expand Up @@ -1851,7 +1851,7 @@ open class UnitPressure : Dimension {
}
}

open class UnitSpeed : Dimension {
final class UnitSpeed : Dimension {

/*
Base unit - metersPerSecond
Expand Down Expand Up @@ -1916,7 +1916,7 @@ open class UnitSpeed : Dimension {
}
}

open class UnitTemperature : Dimension {
final class UnitTemperature : Dimension {

/*
Base unit - kelvin
Expand Down Expand Up @@ -1979,7 +1979,7 @@ open class UnitTemperature : Dimension {
}
}

open class UnitVolume : Dimension {
final class UnitVolume : Dimension {

/*
Base unit - liters
Expand Down