The third review of "SE-0177: Allow distinguishing between public access and public overridability" ran from Active review July 21...25. The proposal has been *accepted with revisions*.
This proposal was far better received by the community than previous versions of the proposal, and the “first design” was the favored path within it. However, there were some concerns raised about the complexity of the model, stemming from non-obvious combinations like “open private”. As such, the core team has requested that the proposal be revised to make “open” function as another access control specifier. “open” is now simply “more public than public”, providing a very simple and clean model.
John has already revised the proposal to the new model, I encourage you to read it if you haven’t already.
Thank you to John McCall and also Javier Soto for driving this discussion forward! John is already working on an implementation of this now.
I realize that there’s no review needed, but I actually wanted to give a hearty to the authors and commenters of this proposal, because I genuinely think we’ve reached something good in the result.
The selling point for me is this:
// This is allowed since the superclass is `open`.
class SubclassB : SubclassableParentClass {
// This is invalid because it overrides a method that is
// defined outside of the current module but is not `open'.
override func foo() { }
// This is allowed since the superclass's method is overridable.
// It does not need to be marked `open` because it is defined on
// an `internal` class.
override func bar() { }
}
This feels super-clean; it gives Library developers `open` for their APIs, without confusing app developers, and still requires that sub-classing Library developers think about `open`.
The third review of "SE-0177: Allow distinguishing between public access
and public overridability" ran from Active review July 21...25. The
proposal has been *accepted with revisions*.
The third review of "SE-0177: Allow distinguishing between public access
and public overridability"
The third review of "SE-0177
SE-0177
…did someone already buy the core team a barrel of whiskey and I missed the
chance to chip in?
:-P
I second everything Scott said: The final result looks great—excellent job
everyone, especially the core team!
Nevin
···
On Wed, Jul 27, 2016 at 6:06 PM, Chris Lattner via swift-evolution < swift-evolution@swift.org> wrote:
The third review of "SE-0177: Allow distinguishing between public access
and public overridability" ran from Active review July 21...25. The
proposal has been *accepted with revisions*.
This proposal was far better received by the community than previous
versions of the proposal, and the “first design” was the favored path
within it. However, there were some concerns raised about the complexity
of the model, stemming from non-obvious combinations like “open private”.
As such, the core team has requested that the proposal be revised to make
“open” function as another access control specifier. “open” is now simply
“more public than public”, providing a very simple and clean model.
John has already revised the proposal to the new model, I encourage you to
read it if you haven’t already.
Thank you to John McCall and also Javier Soto for driving this discussion
forward! John is already working on an implementation of this now.
Great news! Given that this was probably the most polarising proposal ever, I am very impressed with the patience and professionalism the core team has shown in handling the situation and ultimately converging on an elegant solution — all that under oppressive deadline! A barrel of whiskey would be more then appropriate :)
--T
···
On 28 Jul 2016, at 00:06, Chris Lattner via swift-evolution <swift-evolution@swift.org> wrote:
The third review of "SE-0177: Allow distinguishing between public access and public overridability" ran from Active review July 21...25. The proposal has been *accepted with revisions*.
This proposal was far better received by the community than previous versions of the proposal, and the “first design” was the favored path within it. However, there were some concerns raised about the complexity of the model, stemming from non-obvious combinations like “open private”. As such, the core team has requested that the proposal be revised to make “open” function as another access control specifier. “open” is now simply “more public than public”, providing a very simple and clean model.
John has already revised the proposal to the new model, I encourage you to read it if you haven’t already.
Thank you to John McCall and also Javier Soto for driving this discussion forward! John is already working on an implementation of this now.
I'm resurrecting this thread to point out some minor revisions / clarifications that came up during implementation.
The first is that the accepted proposal contained "temporary" restrictions that required (1) open classes to subclass open classes and (2) open methods to override open methods. (2) is actually inconsistent with our existing access-control rules about overrides: the overridden method doesn't even have to be public. Making a method open in a subclass shouldn't force you to expose it in your superclass. Therefore, we have lifted this restriction. (1) is consistent with the access control rule on subclasses, so it has been conservatively kept; we're still open to reconsidering this in the future.
The second is that the proposal wasn't clear about how open interacts with initializers. There are many things about our current class-initialization design that are unsatisfactory, but we are not going to fix them in Swift 3. However, as a general matter, the clearest mental model for initializers is that every class provides its own, independent interface for constructing complete-objects / base-subobjects of that class. The initializers in each class are formally distinct from the initializers in their subclasses, even when they happen to have the same signature; they are not in any real sense "overrides" (even if in some cases we do currently require the "override" keyword). This is true even for required initializers, which merely state a requirement that all subclasses must provide a complete-object initializer with a particular signature, rather than expressing a real relationship between those initializers at each level. Furthermore, it is understood that constructing an object of a subclass necessarily involves delegating to that subclass; subclasses must always be able to initialize their portions of constructed objects. For all of these reasons, it is correct for initializers to not participate in open checking: they cannot be declared open, but correspondingly there are no restrictions on what initializers can be defined in a subclass, even if they have the same signature as an initializer from a superclass.
The third review of "SE-0177: Allow distinguishing between public access and public overridability" ran from Active review July 21...25. The proposal has been *accepted with revisions*.
This proposal was far better received by the community than previous versions of the proposal, and the “first design” was the favored path within it. However, there were some concerns raised about the complexity of the model, stemming from non-obvious combinations like “open private”. As such, the core team has requested that the proposal be revised to make “open” function as another access control specifier. “open” is now simply “more public than public”, providing a very simple and clean model.
John has already revised the proposal to the new model, I encourage you to read it if you haven’t already.
Thank you to John McCall and also Javier Soto for driving this discussion forward! John is already working on an implementation of this now.
I brought this up in review, but there seems to be a serious testability problem here because of how special @testable is.
// This class is not subclassable outside of ModuleA.
public class NonSubclassableParentClass {
// This method is not overridable outside of ModuleA.
public func foo() {}
// This method is not overridable outside of ModuleA because
// its class restricts its access level.
// It is not invalid to declare it as `open`.
open func bar() {}
// The behavior of `final` methods remains unchanged.
public final func baz() {}
}
In a unit test, I *can* subclass `NonSubclassableParentClass`, the access level of `NonSubclassableParentClass` is irrelevant. There’s now no programatic way to ensure that I’m actually testing the contract that I had intended to provide to the consumers of my framework (that my class is subclassable). Is this really the intention?
The “fix”, on the authors end, is to create another target that consumes the output framework to ensure my contract is actually what I wanted (and make sure that it’s not a special test target!). No one is going to do this.
Sure, maybe a code review might catch it. Or I can write a custom linter to validate for this. Do we really want `open` members in non `open` types? The issue with `public` members on `internal` types is much less concerning as the `internal` type isn’t being exposed to others to begin with.
-David
···
On Jul 27, 2016, at 3:18 PM, Scott James Remnant via swift-evolution <swift-evolution@swift.org> wrote:
I realize that there’s no review needed, but I actually wanted to give a hearty to the authors and commenters of this proposal, because I genuinely think we’ve reached something good in the result.
The selling point for me is this:
// This is allowed since the superclass is `open`.
class SubclassB : SubclassableParentClass {
// This is invalid because it overrides a method that is
// defined outside of the current module but is not `open'.
override func foo() { }
// This is allowed since the superclass's method is overridable.
// It does not need to be marked `open` because it is defined on
// an `internal` class.
override func bar() { }
}
This feels super-clean; it gives Library developers `open` for their APIs, without confusing app developers, and still requires that sub-classing Library developers think about `open`.
Given that this was probably the most polarising proposal ever, I am very impressed with the patience and professionalism the core team has shown in handling the situation and ultimately converging on an elegant solution
… and at first sight, they even found a way to please opposers of SE-0117:
It looks like this proposal will never be officially accepted (or maybe it's just a typo caused by all the stress ;-) — I guess it's possible to find a group of sixty proposals that caused less chatter).
I brought this up in review, but there seems to be a serious testability problem here because of how special @testable is.
// This class is not subclassable outside of ModuleA.
public class NonSubclassableParentClass {
// This method is not overridable outside of ModuleA.
public func foo() {}
// This method is not overridable outside of ModuleA because
// its class restricts its access level.
// It is not invalid to declare it as `open`.
open func bar() {}
// The behavior of `final` methods remains unchanged.
public final func baz() {}
}
In a unit test, I *can* subclass `NonSubclassableParentClass`, the access level of `NonSubclassableParentClass` is irrelevant. There’s now no programatic way to ensure that I’m actually testing the contract that I had intended to provide to the consumers of my framework (that my class is subclassable). Is this really the intention?
A "black box" unit test emulating consumer behavior has no business using a @testable import. It should just use the external API of the library.
John.
···
On Jul 27, 2016, at 4:41 PM, David Owens II via swift-evolution <swift-evolution@swift.org> wrote:
The “fix”, on the authors end, is to create another target that consumes the output framework to ensure my contract is actually what I wanted (and make sure that it’s not a special test target!). No one is going to do this.
Sure, maybe a code review might catch it. Or I can write a custom linter to validate for this. Do we really want `open` members in non `open` types? The issue with `public` members on `internal` types is much less concerning as the `internal` type isn’t being exposed to others to begin with.
-David
On Jul 27, 2016, at 3:18 PM, Scott James Remnant via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:
I realize that there’s no review needed, but I actually wanted to give a hearty to the authors and commenters of this proposal, because I genuinely think we’ve reached something good in the result.
The selling point for me is this:
// This is allowed since the superclass is `open`.
class SubclassB : SubclassableParentClass {
// This is invalid because it overrides a method that is
// defined outside of the current module but is not `open'.
override func foo() { }
// This is allowed since the superclass's method is overridable.
// It does not need to be marked `open` because it is defined on
// an `internal` class.
override func bar() { }
}
This feels super-clean; it gives Library developers `open` for their APIs, without confusing app developers, and still requires that sub-classing Library developers think about `open`.
I could be wrong, but isn't `@testable import` applied per-file? So if you keep code users should actually be able to write in one file with a non-`@testable` import, and mocks and other testing hacks in a different file with an `@testable` import, the first file should fail to compile if you use anything that's not normally permitted.
In a future version of Swift, we might consider refining this by requiring people to apply `@testable` directly to declarations which treat something closed as if it's open, but it seems like even the current feature set does not make testing impossible.
···
On Jul 27, 2016, at 4:41 PM, David Owens II via swift-evolution <swift-evolution@swift.org> wrote:
I brought this up in review, but there seems to be a serious testability problem here because of how special @testable is.
// This class is not subclassable outside of ModuleA.
public class NonSubclassableParentClass {
// This method is not overridable outside of ModuleA.
public func foo() {}
// This method is not overridable outside of ModuleA because
// its class restricts its access level.
// It is not invalid to declare it as `open`.
open func bar() {}
// The behavior of `final` methods remains unchanged.
public final func baz() {}
}
In a unit test, I *can* subclass `NonSubclassableParentClass`, the access level of `NonSubclassableParentClass` is irrelevant. There’s now no programatic way to ensure that I’m actually testing the contract that I had intended to provide to the consumers of my framework (that my class is subclassable). Is this really the intention?
Since the result of breaching your subclassing contract is a compile-time error, how are you intending to test that using XCTest?
On the other hand, if you want to write a test that subclasses the class, and uses it as a Library consumer would, why would you use @testable? Wouldn’t you just import the module normally and verify the contract that way (and thus getting a compile failure if the open/public stuff is wrong).
That’s not a Unit Test, that’s a functional test, so I’d probably make that a separate test target anyway.
Scott
···
On Jul 27, 2016, at 4:41 PM, David Owens II <david@owensd.io> wrote:
I brought this up in review, but there seems to be a serious testability problem here because of how special @testable is.
// This class is not subclassable outside of ModuleA.
public class NonSubclassableParentClass {
// This method is not overridable outside of ModuleA.
public func foo() {}
// This method is not overridable outside of ModuleA because
// its class restricts its access level.
// It is not invalid to declare it as `open`.
open func bar() {}
// The behavior of `final` methods remains unchanged.
public final func baz() {}
}
In a unit test, I *can* subclass `NonSubclassableParentClass`, the access level of `NonSubclassableParentClass` is irrelevant. There’s now no programatic way to ensure that I’m actually testing the contract that I had intended to provide to the consumers of my framework (that my class is subclassable). Is this really the intention?
The “fix”, on the authors end, is to create another target that consumes the output framework to ensure my contract is actually what I wanted (and make sure that it’s not a special test target!). No one is going to do this.
Sure, maybe a code review might catch it. Or I can write a custom linter to validate for this. Do we really want `open` members in non `open` types? The issue with `public` members on `internal` types is much less concerning as the `internal` type isn’t being exposed to others to begin with.
-David
On Jul 27, 2016, at 3:18 PM, Scott James Remnant via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:
I realize that there’s no review needed, but I actually wanted to give a hearty to the authors and commenters of this proposal, because I genuinely think we’ve reached something good in the result.
The selling point for me is this:
// This is allowed since the superclass is `open`.
class SubclassB : SubclassableParentClass {
// This is invalid because it overrides a method that is
// defined outside of the current module but is not `open'.
override func foo() { }
// This is allowed since the superclass's method is overridable.
// It does not need to be marked `open` because it is defined on
// an `internal` class.
override func bar() { }
}
This feels super-clean; it gives Library developers `open` for their APIs, without confusing app developers, and still requires that sub-classing Library developers think about `open`.
While arguably true, that's a philosophical debate. There are plenty of reasons to use @testable, and if that's what people are using, then I think there is a valid concern here.
···
Sent from my iPhone
On Jul 27, 2016, at 5:22 PM, John McCall <rjmccall@apple.com> wrote:
On Jul 27, 2016, at 4:41 PM, David Owens II via swift-evolution <swift-evolution@swift.org> wrote:
I brought this up in review, but there seems to be a serious testability problem here because of how special @testable is.
// This class is not subclassable outside of ModuleA.
public class NonSubclassableParentClass {
// This method is not overridable outside of ModuleA.
public func foo() {}
// This method is not overridable outside of ModuleA because
// its class restricts its access level.
// It is not invalid to declare it as `open`.
open func bar() {}
// The behavior of `final` methods remains unchanged.
public final func baz() {}
}
In a unit test, I *can* subclass `NonSubclassableParentClass`, the access level of `NonSubclassableParentClass` is irrelevant. There’s now no programatic way to ensure that I’m actually testing the contract that I had intended to provide to the consumers of my framework (that my class is subclassable). Is this really the intention?
A "black box" unit test emulating consumer behavior has no business using a @testable import. It should just use the external API of the library.
John.
The “fix”, on the authors end, is to create another target that consumes the output framework to ensure my contract is actually what I wanted (and make sure that it’s not a special test target!). No one is going to do this.
Sure, maybe a code review might catch it. Or I can write a custom linter to validate for this. Do we really want `open` members in non `open` types? The issue with `public` members on `internal` types is much less concerning as the `internal` type isn’t being exposed to others to begin with.
-David
On Jul 27, 2016, at 3:18 PM, Scott James Remnant via swift-evolution <swift-evolution@swift.org> wrote:
I realize that there’s no review needed, but I actually wanted to give a hearty to the authors and commenters of this proposal, because I genuinely think we’ve reached something good in the result.
The selling point for me is this:
// This is allowed since the superclass is `open`.
class SubclassB : SubclassableParentClass {
// This is invalid because it overrides a method that is
// defined outside of the current module but is not `open'.
override func foo() { }
// This is allowed since the superclass's method is overridable.
// It does not need to be marked `open` because it is defined on
// an `internal` class.
override func bar() { }
}
This feels super-clean; it gives Library developers `open` for their APIs, without confusing app developers, and still requires that sub-classing Library developers think about `open`.
Yes, it’s per file. It’s also added in the initial template that Xcode creates with your project. In addition, it’s recommended by many that talk about “how to unit test in Swift.” So, to someone that is not paying scrupulous attention, there is no mechanism to prevent against this today.
This also assumes that people write tests… which, well, we know is not the case at all.
I guess time will tell if this should be a warning/error or not depending on the usage of the various Swift linters that I’m sure will start to become prevalent. I know it’s something I’ll be enabling in my code.
It seems this design is acceptable and by-design.
-David
···
On Jul 27, 2016, at 6:36 PM, Brent Royal-Gordon <brent@architechies.com> wrote:
On Jul 27, 2016, at 4:41 PM, David Owens II via swift-evolution <swift-evolution@swift.org> wrote:
I brought this up in review, but there seems to be a serious testability problem here because of how special @testable is.
// This class is not subclassable outside of ModuleA.
public class NonSubclassableParentClass {
// This method is not overridable outside of ModuleA.
public func foo() {}
// This method is not overridable outside of ModuleA because
// its class restricts its access level.
// It is not invalid to declare it as `open`.
open func bar() {}
// The behavior of `final` methods remains unchanged.
public final func baz() {}
}
In a unit test, I *can* subclass `NonSubclassableParentClass`, the access level of `NonSubclassableParentClass` is irrelevant. There’s now no programatic way to ensure that I’m actually testing the contract that I had intended to provide to the consumers of my framework (that my class is subclassable). Is this really the intention?
I could be wrong, but isn't `@testable import` applied per-file? So if you keep code users should actually be able to write in one file with a non-`@testable` import, and mocks and other testing hacks in a different file with an `@testable` import, the first file should fail to compile if you use anything that's not normally permitted.
In a future version of Swift, we might consider refining this by requiring people to apply `@testable` directly to declarations which treat something closed as if it's open, but it seems like even the current feature set does not make testing impossible.
I brought this up in review, but there seems to be a serious testability problem here because of how special @testable is.
// This class is not subclassable outside of ModuleA.
public class NonSubclassableParentClass {
// This method is not overridable outside of ModuleA.
public func foo() {}
// This method is not overridable outside of ModuleA because
// its class restricts its access level.
// It is not invalid to declare it as `open`.
open func bar() {}
// The behavior of `final` methods remains unchanged.
public final func baz() {}
}
In a unit test, I *can* subclass `NonSubclassableParentClass`, the access level of `NonSubclassableParentClass` is irrelevant. There’s now no programatic way to ensure that I’m actually testing the contract that I had intended to provide to the consumers of my framework (that my class is subclassable). Is this really the intention?
I could be wrong, but isn't `@testable import` applied per-file? So if you keep code users should actually be able to write in one file with a non-`@testable` import, and mocks and other testing hacks in a different file with an `@testable` import, the first file should fail to compile if you use anything that's not normally permitted.
In a future version of Swift, we might consider refining this by requiring people to apply `@testable` directly to declarations which treat something closed as if it's open, but it seems like even the current feature set does not make testing impossible.
+1 to @testable on declarations. I really do not like making things internal when they should be private just because a test needs to inspect state.
···
On Jul 27, 2016, at 8:36 PM, Brent Royal-Gordon via swift-evolution <swift-evolution@swift.org> wrote:
On Jul 27, 2016, at 4:41 PM, David Owens II via swift-evolution <swift-evolution@swift.org> wrote:
Other than warning on open methods in non-open classes, I can’t think of a good way to avoid this and still preserve the rest of the testing model. After all, @testable allows you to override internal methods too.
(Just to make things clear, @testable only applies to the current file, so it’s possible to have a single test bundle with both black-box and glass-box tests, as long as they’re in different files.)
Jordan
···
On Jul 27, 2016, at 17:40, David Owens II via swift-evolution <swift-evolution@swift.org> wrote:
While arguably true, that's a philosophical debate. There are plenty of reasons to use @testable, and if that's what people are using, then I think there is a valid concern here.
Sent from my iPhone
On Jul 27, 2016, at 5:22 PM, John McCall <rjmccall@apple.com <mailto:rjmccall@apple.com>> wrote:
On Jul 27, 2016, at 4:41 PM, David Owens II via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:
I brought this up in review, but there seems to be a serious testability problem here because of how special @testable is.
// This class is not subclassable outside of ModuleA.
public class NonSubclassableParentClass {
// This method is not overridable outside of ModuleA.
public func foo() {}
// This method is not overridable outside of ModuleA because
// its class restricts its access level.
// It is not invalid to declare it as `open`.
open func bar() {}
// The behavior of `final` methods remains unchanged.
public final func baz() {}
}
In a unit test, I *can* subclass `NonSubclassableParentClass`, the access level of `NonSubclassableParentClass` is irrelevant. There’s now no programatic way to ensure that I’m actually testing the contract that I had intended to provide to the consumers of my framework (that my class is subclassable). Is this really the intention?
A "black box" unit test emulating consumer behavior has no business using a @testable import. It should just use the external API of the library.
John.
The “fix”, on the authors end, is to create another target that consumes the output framework to ensure my contract is actually what I wanted (and make sure that it’s not a special test target!). No one is going to do this.
Sure, maybe a code review might catch it. Or I can write a custom linter to validate for this. Do we really want `open` members in non `open` types? The issue with `public` members on `internal` types is much less concerning as the `internal` type isn’t being exposed to others to begin with.
-David
On Jul 27, 2016, at 3:18 PM, Scott James Remnant via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:
I realize that there’s no review needed, but I actually wanted to give a hearty to the authors and commenters of this proposal, because I genuinely think we’ve reached something good in the result.
The selling point for me is this:
// This is allowed since the superclass is `open`.
class SubclassB : SubclassableParentClass {
// This is invalid because it overrides a method that is
// defined outside of the current module but is not `open'.
override func foo() { }
// This is allowed since the superclass's method is overridable.
// It does not need to be marked `open` because it is defined on
// an `internal` class.
override func bar() { }
}
This feels super-clean; it gives Library developers `open` for their APIs, without confusing app developers, and still requires that sub-classing Library developers think about `open`.
Whoa, that wasn't what I was suggesting at all. I was suggesting that the *test suite* should mark the forbidden subclass/override with `@testable`.
···
On Jul 27, 2016, at 6:43 PM, Matthew Johnson <matthew@anandabits.com> wrote:
In a future version of Swift, we might consider refining this by requiring people to apply `@testable` directly to declarations which treat something closed as if it's open, but it seems like even the current feature set does not make testing impossible.
+1 to @testable on declarations. I really do not like making things internal when they should be private just because a test needs to inspect state.
While arguably true, that's a philosophical debate. There are plenty of reasons to use @testable, and if that's what people are using, then I think there is a valid concern here.
There are absolutely plenty of reasons to use @testable. But if you're a library developer, and you're writing a test that specifically is validating that a third party will be able to use your library the way you want it to be used, that is the very definition of black-box testing, and you should not be using a @testable import in that specific test. The compiler's standard behavior if you don't use a @testable import is *exactly* the custom linter that you're talking about writing.
Like Jordan said, @testable imports are file-specific, so you can freely mix tests that use them with tests that don't. If you have common testing infrastructure that needs internal access to your library, that's pretty easy to put in its own file, too.
John.
···
On Jul 27, 2016, at 5:40 PM, David Owens II <david@owensd.io> wrote:
Sent from my iPhone
On Jul 27, 2016, at 5:22 PM, John McCall <rjmccall@apple.com <mailto:rjmccall@apple.com>> wrote:
On Jul 27, 2016, at 4:41 PM, David Owens II via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:
I brought this up in review, but there seems to be a serious testability problem here because of how special @testable is.
// This class is not subclassable outside of ModuleA.
public class NonSubclassableParentClass {
// This method is not overridable outside of ModuleA.
public func foo() {}
// This method is not overridable outside of ModuleA because
// its class restricts its access level.
// It is not invalid to declare it as `open`.
open func bar() {}
// The behavior of `final` methods remains unchanged.
public final func baz() {}
}
In a unit test, I *can* subclass `NonSubclassableParentClass`, the access level of `NonSubclassableParentClass` is irrelevant. There’s now no programatic way to ensure that I’m actually testing the contract that I had intended to provide to the consumers of my framework (that my class is subclassable). Is this really the intention?
A "black box" unit test emulating consumer behavior has no business using a @testable import. It should just use the external API of the library.
John.
The “fix”, on the authors end, is to create another target that consumes the output framework to ensure my contract is actually what I wanted (and make sure that it’s not a special test target!). No one is going to do this.
Sure, maybe a code review might catch it. Or I can write a custom linter to validate for this. Do we really want `open` members in non `open` types? The issue with `public` members on `internal` types is much less concerning as the `internal` type isn’t being exposed to others to begin with.
-David
On Jul 27, 2016, at 3:18 PM, Scott James Remnant via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:
I realize that there’s no review needed, but I actually wanted to give a hearty to the authors and commenters of this proposal, because I genuinely think we’ve reached something good in the result.
The selling point for me is this:
// This is allowed since the superclass is `open`.
class SubclassB : SubclassableParentClass {
// This is invalid because it overrides a method that is
// defined outside of the current module but is not `open'.
override func foo() { }
// This is allowed since the superclass's method is overridable.
// It does not need to be marked `open` because it is defined on
// an `internal` class.
override func bar() { }
}
This feels super-clean; it gives Library developers `open` for their APIs, without confusing app developers, and still requires that sub-classing Library developers think about `open`.
Yes, it’s per file. It’s also added in the initial template that Xcode creates with your project. In addition, it’s recommended by many that talk about “how to unit test in Swift.” So, to someone that is not paying scrupulous attention, there is no mechanism to prevent against this today.
Perhaps we're not doing a good job of messaging this.
The Xcode template is the way it is because @testable imports are the right default for a *program* written in Swift. A lot of the code in an application or command-line program isn't really suitable for independent, in-process black-box testing, because it isn't really presenting an integrated API. If you break it down into components that can be independently tested, that's awesome, and at that point you can switch the imports in your tests over to non-@testable. But we don't want the test template to create any obstacles to testing, because as you say, people already don't write enough tests.
In contrast, a library developer needs to be more conscientious about the difference between black-box and white-box testing. In fact, they should really be leaning towards making black-box tests whenever that's reasonably practical. We don't currently have Xcode templates around making Swift libraries, though; I think we all agree that there's a lot of room for tools improvement there.
John.
···
On Jul 27, 2016, at 6:55 PM, David Owens II via swift-evolution <swift-evolution@swift.org> wrote:
This also assumes that people write tests… which, well, we know is not the case at all.
I guess time will tell if this should be a warning/error or not depending on the usage of the various Swift linters that I’m sure will start to become prevalent. I know it’s something I’ll be enabling in my code.
It seems this design is acceptable and by-design.
-David
On Jul 27, 2016, at 6:36 PM, Brent Royal-Gordon <brent@architechies.com> wrote:
On Jul 27, 2016, at 4:41 PM, David Owens II via swift-evolution <swift-evolution@swift.org> wrote:
I brought this up in review, but there seems to be a serious testability problem here because of how special @testable is.
// This class is not subclassable outside of ModuleA.
public class NonSubclassableParentClass {
// This method is not overridable outside of ModuleA.
public func foo() {}
// This method is not overridable outside of ModuleA because
// its class restricts its access level.
// It is not invalid to declare it as `open`.
open func bar() {}
// The behavior of `final` methods remains unchanged.
public final func baz() {}
}
In a unit test, I *can* subclass `NonSubclassableParentClass`, the access level of `NonSubclassableParentClass` is irrelevant. There’s now no programatic way to ensure that I’m actually testing the contract that I had intended to provide to the consumers of my framework (that my class is subclassable). Is this really the intention?
I could be wrong, but isn't `@testable import` applied per-file? So if you keep code users should actually be able to write in one file with a non-`@testable` import, and mocks and other testing hacks in a different file with an `@testable` import, the first file should fail to compile if you use anything that's not normally permitted.
In a future version of Swift, we might consider refining this by requiring people to apply `@testable` directly to declarations which treat something closed as if it's open, but it seems like even the current feature set does not make testing impossible.
In a future version of Swift, we might consider refining this by requiring people to apply `@testable` directly to declarations which treat something closed as if it's open, but it seems like even the current feature set does not make testing impossible.
+1 to @testable on declarations. I really do not like making things internal when they should be private just because a test needs to inspect state.
Whoa, that wasn't what I was suggesting at all. I was suggesting that the *test suite* should mark the forbidden subclass/override with `@testable`.
Sorry, I misread that. But I still stand by the enhancement to @testable I mentioned. There are times when access control is wider than necessary to support tests and it always sucks doing that.
···
On Jul 27, 2016, at 8:47 PM, Brent Royal-Gordon <brent@architechies.com> wrote:
On Jul 27, 2016, at 6:43 PM, Matthew Johnson <matthew@anandabits.com> wrote:
Yes, it’s per file. It’s also added in the initial template that Xcode creates with your project. In addition, it’s recommended by many that talk about “how to unit test in Swift.” So, to someone that is not paying scrupulous attention, there is no mechanism to prevent against this today.
Perhaps we're not doing a good job of messaging this.
The Xcode template is the way it is because @testable imports are the right default for a *program* written in Swift. A lot of the code in an application or command-line program isn't really suitable for independent, in-process black-box testing, because it isn't really presenting an integrated API. If you break it down into components that can be independently tested, that's awesome, and at that point you can switch the imports in your tests over to non-@testable. But we don't want the test template to create any obstacles to testing, because as you say, people already don't write enough tests.
My primary concern is that it’s easy to think you’ve done the right thing and push out a release because all of your testing shows it’s good, only to find you messed up in a way that it’s easy for a tool to validate. Writing new code is probably not going to be the primary source of this, but refactoring a `public` class to an `open` one, where there are already existing tests for that class, probably in a single file and using `@testable`, it’s easy to say, “looks good, tests passed, integrations look good.”
In contrast, a library developer needs to be more conscientious about the difference between black-box and white-box testing. In fact, they should really be leaning towards making black-box tests whenever that's reasonably practical. We don't currently have Xcode templates around making Swift libraries, though; I think we all agree that there's a lot of room for tools improvement there.
Just FYI: The default Frameworks target also uses @testable for the Swift unit tests.
Regardless, there are mitigations that can turn this potential (maybe it’s not even an issue in practice) test-time error into a near compile-time error (using a linter).
-David
···
On Jul 27, 2016, at 7:18 PM, John McCall <rjmccall@apple.com> wrote:
On Jul 27, 2016, at 6:55 PM, David Owens II via swift-evolution <swift-evolution@swift.org> wrote:
>
>
>>
>> Yes, it’s per file. It’s also added in the initial template that Xcode
creates with your project. In addition, it’s recommended by many that talk
about “how to unit test in Swift.” So, to someone that is not paying
scrupulous attention, there is no mechanism to prevent against this today.
>
> Perhaps we're not doing a good job of messaging this.
>
> The Xcode template is the way it is because @testable imports are the
right default for a *program* written in Swift. A lot of the code in an
application or command-line program isn't really suitable for independent,
in-process black-box testing, because it isn't really presenting an
integrated API. If you break it down into components that can be
independently tested, that's awesome, and at that point you can switch the
imports in your tests over to non-@testable. But we don't want the test
template to create any obstacles to testing, because as you say, people
already don't write enough tests.
My primary concern is that it’s easy to think you’ve done the right thing
and push out a release because all of your testing shows it’s good, only to
find you messed up in a way that it’s easy for a tool to validate. Writing
new code is probably not going to be the primary source of this, but
refactoring a `public` class to an `open` one, where there are already
existing tests for that class, probably in a single file and using
`@testable`, it’s easy to say, “looks good, tests passed, integrations look
good.”
If you're refactoring `public` classes into `open` ones, though, you'd be
more likely to have forgotten to open a method you intend to make
overridable, no? And there's no way to change the rules here to have
`@testable` pick that up...
···
On Wed, Jul 27, 2016 at 10:30 PM, David Owens II via swift-evolution < swift-evolution@swift.org> wrote:
> On Jul 27, 2016, at 7:18 PM, John McCall <rjmccall@apple.com> wrote:
>> On Jul 27, 2016, at 6:55 PM, David Owens II via swift-evolution < > swift-evolution@swift.org> wrote:
In contrast, a library developer needs to be more conscientious about the
difference between black-box and white-box testing. In fact, they should
really be leaning towards making black-box tests whenever that's reasonably
practical. We don't currently have Xcode templates around making Swift
libraries, though; I think we all agree that there's a lot of room for
tools improvement there.
Just FYI: The default Frameworks target also uses @testable for the Swift
unit tests.
Regardless, there are mitigations that can turn this potential (maybe it’s
not even an issue in practice) test-time error into a near compile-time
error (using a linter).
to the authors and commenters of this proposal, because I genuinely think we’ve reached something good in the result.