[Review #3] SE-0117: Allow distinguishing between public access and public overridability


(Chris Lattner) #1

Hello Swift community,

The third review of "SE-0117: Allow distinguishing between public access and public overridability" begins now and runs through July 25. The proposal is available here:

  https://github.com/apple/swift-evolution/blob/master/proposals/0117-non-public-subclassable-by-default.md

Reviews are an important part of the Swift evolution process. All reviews should be sent to the swift-evolution mailing list at

  https://lists.swift.org/mailman/listinfo/swift-evolution

or, if you would like to keep your feedback private, directly to the review manager.

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and contribute to the direction of Swift. When writing your review, here are some questions you might want to answer in your review:

  * What is your evaluation of the proposal?
  * Is the problem being addressed significant enough to warrant a change to Swift?
  * Does this proposal fit well with the feel and direction of Swift?
  * If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
  * How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

More information about the Swift evolution process is available at

  https://github.com/apple/swift-evolution/blob/master/process.md

Thank you,

-Chris Lattner
Review Manager


(David Owens II) #2

What is your evaluation of the proposal?
I think the philosophical direction of the proposal is good, though I still don’t like the actual implementation details. There are two concepts here:
the ability to subclass
the ability to override members

Having both of these concepts annotated with `open` is confusing as they do different things. Marking a class as open does **not** provide the ability to override. However, marking a member as `open` would provide the ability to override. These are two related but different concepts.

I see no reason why `virtual` should not be used to mark members that are inheritable and overridable. That concept is what is taught in schools, that concept is what is easily searchable, and that term is well known in the computer science field for this very concept. Just do a Google search between “open function” and “virtual function”. Deviating from this seems to be counter to one of Swift goals of being approachable. Having to learn `open` here is just mental noise that makes the coder have to think, “right… this is just a virtual method”.

To me, the better approach is:
change `open` to `virtual`
`virtual` is only allowed on inheritable and overridable members (i.e. `var`, `func`, and `subscript`)
`virtual` is not permitted on declarations that are explicitly `final` or `dynamic`. (Note that it's okay if one or both of the modifiers are implicitly inferred.)
A class is made inheritable outside of the module the **only** when the class is marked as both `public` and one or members are marked as `virtual`

Lastly, it’s almost certainly a bug in your code if you mark a member as `open` on your public class but forget to annotate your class also with `open`. Worse, this error will **only** be caught if you attempt to use this type **outside** the context of tests because `@testable` will grant your module the `open` class state because `@testable` is special.

The only scenario that this change doesn’t allow would be the ability to subclass a class that has no virtual members. In my opinion, this scenario is better handled via extensions anyway.

However, **if** this scenario really was desirable, this could later be added by allowing `virtual` (or `inheritable` or `open`...) on a class definition. However, I would still make the argument that once you have a `virtual` member on a type, that the class is implicitly marked as `virtual` as well (see reasoning above).

Is the problem being addressed significant enough to warrant a change to Swift?
Yes

Does this proposal fit well with the feel and direction of Swift?
Yes

If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
Yes, many. This proposal’s philosophical direction is more resilient by default.

How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
I’ve read the posts, thought about it, experimented, blogged about it.

-David


(Xiaodi Wu) #3

Hello Swift community,

The third review of "SE-0117: Allow distinguishing between public access
and public overridability" begins now and runs through July 25. The
proposal is available here:

https://github.com/apple/swift-evolution/blob/master/proposals/0117-non-public-subclassable-by-default.md

Reviews are an important part of the Swift evolution process. All reviews
should be sent to the swift-evolution mailing list at

        https://lists.swift.org/mailman/listinfo/swift-evolution

or, if you would like to keep your feedback private, directly to the
review manager.

What goes into a review?

The goal of the review process is to improve the proposal under review
through constructive criticism and contribute to the direction of Swift.
When writing your review, here are some questions you might want to answer
in your review:

        * What is your evaluation of the proposal?

Much improved. I would be happy with either of the alternatives about open
classes.

        * Is the problem being addressed significant enough to warrant a
change to Swift?

(See previous review.)

        * Does this proposal fit well with the feel and direction of Swift?

Yes, almost entirely. One more nit. Now that `open` is proposed as a
standalone modifier (as opposed to `public open`), it should *not* be
composable with `internal`, etc. Here's why:

If I make an `internal` subclass of an `open` class, I should not be able
to have a `private open override`. In a [public] open subclass, I'd be
required to have a [public] open override. So too for an internal subclass
it doesn't make sense that the override might have less visibility than the
type itself. Forbidding accessory access control modifiers would have the
desired effect of enforcing the same visibility for an overriding open
member as for its containing type.

Moreover, from a learnability standpoint, seeing `private open` and
`internal open` sets up the expectation that `public open` might be
meaningful and not redundant, and that `open` might mean `internal open`
just as in (nearly) all other circumstances the absence of `private`,
`fileprivate`, and `public` implies `internal`. Since this is not true, and
since accessory access control modifiers should never be necessary as
discussed above, I'd advocate for forbidding the use of `open` in
conjunction with an explicit access level.

        * If you have used other languages or libraries with a similar

feature, how do you feel that this proposal compares to those?
        * How much effort did you put into your review? A glance, a quick
reading, or an in-depth study?

(See previous review.)

···

On Thu, Jul 21, 2016 at 10:33 AM, Chris Lattner via swift-evolution < swift-evolution@swift.org> wrote:

More information about the Swift evolution process is available at

        https://github.com/apple/swift-evolution/blob/master/process.md

Thank you,

-Chris Lattner
Review Manager

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(TJ Usiyan) #4

* What is your evaluation of the proposal?
+1 for the first design listed. Simplicity is nice but the ability to vend
non-final classes that cannot be publicly subclassed is worth the
complexity.
        * Is the problem being addressed significant enough to warrant a
change to Swift?
Yes.
        * Does this proposal fit well with the feel and direction of Swift?
Yes.
        * If you have used other languages or libraries with a similar
feature, how do you feel that this proposal compares to those?
I haven't
        * How much effort did you put into your review? A glance, a quick
reading, or an in-depth study?
I have followed the discussion and I gave a quick reading of this version.

···

On Thu, Jul 21, 2016 at 11:33 AM, Chris Lattner via swift-evolution < swift-evolution@swift.org> wrote:

Hello Swift community,

The third review of "SE-0117: Allow distinguishing between public access
and public overridability" begins now and runs through July 25. The
proposal is available here:

https://github.com/apple/swift-evolution/blob/master/proposals/0117-non-public-subclassable-by-default.md

Reviews are an important part of the Swift evolution process. All reviews
should be sent to the swift-evolution mailing list at

        https://lists.swift.org/mailman/listinfo/swift-evolution

or, if you would like to keep your feedback private, directly to the
review manager.

What goes into a review?

The goal of the review process is to improve the proposal under review
through constructive criticism and contribute to the direction of Swift.
When writing your review, here are some questions you might want to answer
in your review:

        * What is your evaluation of the proposal?
        * Is the problem being addressed significant enough to warrant a
change to Swift?
        * Does this proposal fit well with the feel and direction of Swift?
        * If you have used other languages or libraries with a similar
feature, how do you feel that this proposal compares to those?
        * How much effort did you put into your review? A glance, a quick
reading, or an in-depth study?

More information about the Swift evolution process is available at

        https://github.com/apple/swift-evolution/blob/master/process.md

Thank you,

-Chris Lattner
Review Manager

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Matthew Johnson) #5

  * What is your evaluation of the proposal?

+1 to the first design. I think this is a great solution that balances the many considerations that have been raised on all sides of this issue. `open` is 2 characters shorter than `public` so complaints about boilerplate are no longer valid. `internal` is the “default” - neither `public` nor `open` are privileged as a “default” for publishing API outside of a module.

I am interested in language enhancements such as exhaustive pattern matching on classes and protocols which rely on knowledge of the full class hierarchy. Such enhancements will be far more useful if the language supports non-open, non-final classes.

There are design techniques that would require additional boilerplate if we cannot have non-open, non-final classes.

Most importantly, requiring library authors to choose `public` or `open` provides important documentation value. Users of the library will know whether the author intends to support subclasses or not. The second design requires the use of comments and / or documentation to communicate this information. I prefer important semantic considerations like this to be encoded in the language itself.

I would prefer to see better language support for composition to support compositional subclasses rather than choosing the second design here. Language support for composition could be designed to work with structs as well as classes and would encourage the use of protocols rather than class hierarchies for polymorphism. This feels like a much Swiftier direction to me.

  * Is the problem being addressed significant enough to warrant a change to Swift?

Yes. Library authors are currently locked into “open” designs unless they are able to make the design `final` and remember to do so in the initial release.

  * Does this proposal fit well with the feel and direction of Swift?

Very much so. It seeks an optimal middle ground that keeps boilerplate to a minimum while providing explicit API contracts.

  * If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

The closest is the requirement to mark methods as `virtual` in C++ and C#. This solution is superior in a number of ways. A couple of items come to mind immediately:

1. It avoids boilerplate within a module allowing for easy rapid prototyping and evolution of a design within a module.
2. It requires an explicit choice to be made when publishing an API which means that a library author can’t accidentally “forget” to include the `open` modifier if that is their intent.

  * How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

In-depth participation in many of the discussion threads and the initial review thread as well as in-depth study of all relevant proposals.

···

More information about the Swift evolution process is available at

  https://github.com/apple/swift-evolution/blob/master/process.md

Thank you,

-Chris Lattner
Review Manager

_______________________________________________
swift-evolution-announce mailing list
swift-evolution-announce@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution-announce


(Chéyo Jiménez) #6

  * What is your evaluation of the proposal?

+1 as before but for the first implementation.

I want to like implementation 2 but I really don’t see the need for it because extensions.

The only reason for 2 is if the core team thinks that we will never get stored properties can be added via extensions.

  * Is the problem being addressed significant enough to warrant a change to Swift?

yes

  * Does this proposal fit well with the feel and direction of Swift?

yes

  * If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

n/1

  * How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

reviewed the last two iterations.


#7

*What is your evaluation of the proposal?*

Strong +1 to either design, and neutral between them.

In the prior review I advocated that `open` should *only* apply to classes,
not members. This new proposal turns that on its head and is *vastly
superior*. Tagging open members with *just* the word `open`, eliding
`public` entirely, is a phenomenal improvement.

Kudos to the core team both for crafting this solution, and more
importantly for conducting the (lengthy!) review process with such aplomb.
It would have been easy (and tempting!) to end the first review by
“accepting with revision”, leaving us with needless boilerplate and a
suboptimal design. By instead inviting the community to bikeshed a second
time *with the constraint that `sealed` would be the default*, and then
meeting afterward to thoroughly discuss the possibilities, the core team
has truly demonstrated how an *ahem* open, public design process ought to
be carried out.

*Is the problem being addressed significant enough to warrant a change to

Swift?*

Honestly I am not in a position to say.

*Does this proposal fit well with the feel and direction of Swift?*

Well, the one aspect I am not sure about is, “A class member that overrides
an open class member must be explicitly declared open unless it is
explicitly final or it is a member of a final or non-public class. In any
case, it is considered open.”

This seems like an unnecessary restriction. Perhaps I want to override an
`open` member with a `public` one. As in,

open class Base { open var x: Int { return 6 } }
open class Sub: Base { public var x: Int { return 7 } }
open class Grand: Sub { final var x: Int { return 8 } }

Note that “Sub” might be defined in a separate module from “Base”, a module
that wants to publish Sub and Grand without letting its own clients
override `x` from them.

*If you have used other languages or libraries with a similar feature, how

do you feel that this proposal compares to those?*

Only languages with run-of-the-mill access control, nothing like this!

*How much effort did you put into your review? A glance, a quick reading,

or an in-depth study?*

I read all messages in the previous draft and review threads, and the
entire new proposal except for the 8-paragraph, 886-word “Motivation”
section. Seriously, that is 3½ pages of motivation!

Nevin

···

On Thu, Jul 21, 2016 at 11:33 AM, Chris Lattner via swift-evolution < swift-evolution@swift.org> wrote:

Hello Swift community,

The third review of "SE-0117: Allow distinguishing between public access
and public overridability" begins now and runs through July 25. The
proposal is available here:

https://github.com/apple/swift-evolution/blob/master/proposals/0117-non-public-subclassable-by-default.md

Reviews are an important part of the Swift evolution process. All reviews
should be sent to the swift-evolution mailing list at

        https://lists.swift.org/mailman/listinfo/swift-evolution

or, if you would like to keep your feedback private, directly to the
review manager.

What goes into a review?

The goal of the review process is to improve the proposal under review
through constructive criticism and contribute to the direction of Swift.
When writing your review, here are some questions you might want to answer
in your review:

        * What is your evaluation of the proposal?
        * Is the problem being addressed significant enough to warrant a
change to Swift?
        * Does this proposal fit well with the feel and direction of Swift?
        * If you have used other languages or libraries with a similar
feature, how do you feel that this proposal compares to those?
        * How much effort did you put into your review? A glance, a quick
reading, or an in-depth study?

More information about the Swift evolution process is available at

        https://github.com/apple/swift-evolution/blob/master/process.md

Thank you,

-Chris Lattner
Review Manager

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Leonardo Pessoa) #8

Hello Swift community,

The third review of "SE-0117: Allow distinguishing between public access and public overridability" begins now and runs through July 25. The proposal is available here:

        https://github.com/apple/swift-evolution/blob/master/proposals/0117-non-public-subclassable-by-default.md

        * What is your evaluation of the proposal?

I like the design with open and have no issues with it implicitly
meaning public. After a little confabulation I also believe the
original intent can be achieved by allowing subclassing to be open and
method overriding to be sealed by default, thus providing ground for
composition with no replacing of behaviour as exemplified in the
proposal, so I have no issues with it as long as methods are sealed by
default.

As for the openness of overriden methods, I still believe there should
be a way other than final to reseal the method. Sure there are means
to implement the same without it but not as elegantly IMO.

        * Is the problem being addressed significant enough to warrant a change to Swift?

Yes.

        * Does this proposal fit well with the feel and direction of Swift?

Yes.

        * If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

No.

        * How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

I've read the proposal and discussed the issue a lot on the threads.

···

On 21 July 2016 at 12:33, Chris Lattner via swift-evolution <swift-evolution@swift.org> wrote:


(Paul Cantrell) #9

• What is your evaluation of the proposal?

Breaking it down in parts:

+1 to the basic principle of making the external subclassing a conscious, explicit API design decision.

+1 to providing a design state for subclassability that leaves the most room for non-breaking future changes. Those who think about API design know that allowing subclassing is a non-retractable promise, but the fact that “final” will _also_ be non-retractible in Swift when we have ABI stability is a slam-dunk argument for the existence of “open.”

+1 to the word “open.” I went back and forth on this a little, pondering “subclassable,” “overridable,” etc., but those terms are misleading: they imply “not final,” and don’t capture the fact that module boundaries are an important ingredient of the concept at hand here.

-1 to “open” implying “public.” I continue to feel that I want to see the word “public” on every element that is part of my public API. I don’t mind “public open,” and the rationale in the Alternatives section didn’t sway me. However, I don’t consider this point a dealbreaker, and am still in favor of this proposal overall.

Without having a very strong feeling about it, I’m in favor of the “first design,” i.e. the existence of non-open, non-final classes. It’s the less surprising model, somehow. I also like the idea of a library being able to make assumptions about a closed set of subtypes, just as enums allow assumptions about a closed set of values. (A future construct could even apply enum-like code path analysis when switching on a supertype with a known set of subtypes.)

Despite that, I would accept the second design (no such thing as “open class”) if it consensus favored it.

• Is the problem being addressed significant enough to warrant a change to Swift?

Yes. It would be on good API design principles alone, but the coming ABI stability problems around final, at least in my dim understanding of them, makes this crucial.

• Does this proposal fit well with the feel and direction of Swift?

It does.

Swift favors making consequential design decisions explicit, and “non-final by default” currently breaks with that principle.

“Subclassable within a module by default” is consistent with Swift’s “internal by default” attitude.

• If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

Other languages have solved these same API design concerns with “final by default.” I prefer this alternative.

• How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

I’ve loosely followed the conversation from the beginning, though I confess I did not count every sling and arrow in the long message threads.

Cheers,

Paul


(Tony Allevato) #10

Hello Swift community,

The third review of "SE-0117: Allow distinguishing between public access
and public overridability" begins now and runs through July 25. The
proposal is available here:

https://github.com/apple/swift-evolution/blob/master/proposals/0117-non-public-subclassable-by-default.md

Reviews are an important part of the Swift evolution process. All reviews
should be sent to the swift-evolution mailing list at

        https://lists.swift.org/mailman/listinfo/swift-evolution

or, if you would like to keep your feedback private, directly to the
review manager.

What goes into a review?

The goal of the review process is to improve the proposal under review
through constructive criticism and contribute to the direction of Swift.
When writing your review, here are some questions you might want to answer
in your review:

        * What is your evaluation of the proposal?

+1 to the first design. I wouldn't be upset if the second design was what
got accepted, but I strongly prefer the first.

One of the benefits of this proposal has been formalizing some software
design notions related to type extensibility, and I feel very strongly that
API authors should be forced to make a decision one way or the other at the
time that they design a class whether it is extensible or not. It's true
that under design #2 you get a closed class (module extending with data)
without the explicitness of the keyword. However, being able to look at a
class definition and know immediately whether it's intended to be open or
closed is even more important. The performance improvements also seem
compelling.

While the formal side of me really liked keeping orthogonal concepts
(visibility vs. extensibility) separate, I can appreciate the desire to
eliminate boilerplate. Having "open" imply "public" unless stated otherwise
seems like a reasonable compromise.

Likewise, open-within-a-module-and-closed-outside as the default for
"public" is entirely aligned with Swift's other default visibility,
internal-by-default. The credo is essentially "don't protect users from
themselves and don't make coding more difficult for the common case of
within-app/module development, but the minute you decide to expose
something for other users, you must think about these things that affect
how others can and will use it."

        * Is the problem being addressed significant enough to warrant a
change to Swift?

Yes. More mainstream languages need to do more to encourage authors to
think about their designs and to allow those who care deeply about API
design to express them well.

        * Does this proposal fit well with the feel and direction of Swift?

Yes.

        * If you have used other languages or libraries with a similar
feature, how do you feel that this proposal compares to those?

        * How much effort did you put into your review? A glance, a quick

reading, or an in-depth study?

Read the original and latest proposals and followed the discussions.

···

On Thu, Jul 21, 2016 at 8:33 AM Chris Lattner via swift-evolution < swift-evolution@swift.org> wrote:

More information about the Swift evolution process is available at

        https://github.com/apple/swift-evolution/blob/master/process.md

Thank you,

-Chris Lattner
Review Manager

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Shawn Erickson) #11

(Google inbox won't let me inline my comments)

Thanks for the effort on iterating and refining this proposal.

+1 on the proposal, more strongly favoring the first design.

If my brain is working correctly it seems like we could start with the
first design and if somehow problematic move to the the second model in the
future without (much?) disruption. I didn't sit down and reason it out in
much depth (watching my 2.5 year old at the moment) however so I will leave
that to others to ponder.

I think it aligns well with Swifts principles in terms of favoring external
to module API to be intentional / explicit. The latest revisions - I think
- has also sufficiently reduced the burden to module developers which is
also another important aspect of Swift.

I haven't used a language the has this exact type of feature but I have
often desired to haven his type of capability when I was authoring
libraries for others (often in company). I am a believer in being very
explicit in API contract to both confine what I have to design for/test
when authoring a library as well as having more clarity when consuming a
library.

-Shawn

···

On Thu, Jul 21, 2016 at 11:33 AM Chris Lattner <clattner@apple.com> wrote:

Hello Swift community,

The third review of "SE-0117: Allow distinguishing between public access
and public overridability" begins now and runs through July 25. The
proposal is available here:

https://github.com/apple/swift-evolution/blob/master/proposals/0117-non-public-subclassable-by-default.md

Reviews are an important part of the Swift evolution process. All reviews
should be sent to the swift-evolution mailing list at

        https://lists.swift.org/mailman/listinfo/swift-evolution

or, if you would like to keep your feedback private, directly to the
review manager.

What goes into a review?

The goal of the review process is to improve the proposal under review
through constructive criticism and contribute to the direction of Swift.
When writing your review, here are some questions you might want to answer
in your review:

        * What is your evaluation of the proposal?
        * Is the problem being addressed significant enough to warrant a
change to Swift?
        * Does this proposal fit well with the feel and direction of Swift?
        * If you have used other languages or libraries with a similar
feature, how do you feel that this proposal compares to those?
        * How much effort did you put into your review? A glance, a quick
reading, or an in-depth study?

More information about the Swift evolution process is available at

        https://github.com/apple/swift-evolution/blob/master/process.md

Thank you,

-Chris Lattner
Review Manager

_______________________________________________
swift-evolution-announce mailing list
swift-evolution-announce@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution-announce


(Karl) #12

Just posted in the Review #2 thread. I read the updated proposal, and I have another idea besides making “final” default:

I would be okay with the inferred situation for classes being a semantic “final”. That is, that they cannot be subclassed, but they won’t be automatically optimised in a fragile way either. We could call it “sealed” if you want to explicitly specify it - the difference is that it doesn’t only apply at the module boundary, and it’s just an annotation for the type-checker (both yours and that of the third-party developer) that it shouldn’t allow this - it’s not license for library module’s compiler to give up flexibility. So:

public class Foo {} // Implicitly “sealed”. Cannot be subclassed anywhere. Does not provide optimiser guarantees of “final”.
public(sealed) class Foo {} // as above

public final class Foo {} // Implicitly “sealed”. Cannot be subclassed anywhere. Allows resilience-breaking optimisations.
public(sealed) final class Foo {} // as above

public internal(open) class Foo {} // “open” overrides “sealed” for the internal scope. Cannot be subclassed externally; may be subclassed internally. Does not provide optimiser guarantees of “final”.
public open(internal) class Foo {} // another idea: flipping the order, so it becomes open(internal) rather than internal(open). This looks nicer, is the opposite of the property accessor scope syntax - "public internal(set) ..."
public(sealed) internal(open) class Foo {} // as above

public(sealed) internal(open) final class Foo {} // Error: A class cannot be both open and final

I believe that would meet the goals of:

- Not allowing subclassing from external modules unless explicitly allowed (the original goal)
- Making classes which are internally-subclassed easier to locally reason about (my nice-to-have)
- Maintain binary compatibility
- Do not give up binary flexibility unless the user explicitly asks for it (a goal in the LibraryEvolution docs, however current or not they may be)

Is there anything I missed?

Karl

···

On 21 Jul 2016, at 17:33, Chris Lattner via swift-evolution <swift-evolution@swift.org> wrote:

Hello Swift community,

The third review of "SE-0117: Allow distinguishing between public access and public overridability" begins now and runs through July 25. The proposal is available here:

  https://github.com/apple/swift-evolution/blob/master/proposals/0117-non-public-subclassable-by-default.md

Reviews are an important part of the Swift evolution process. All reviews should be sent to the swift-evolution mailing list at

  https://lists.swift.org/mailman/listinfo/swift-evolution

or, if you would like to keep your feedback private, directly to the review manager.

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and contribute to the direction of Swift. When writing your review, here are some questions you might want to answer in your review:

  * What is your evaluation of the proposal?
  * Is the problem being addressed significant enough to warrant a change to Swift?
  * Does this proposal fit well with the feel and direction of Swift?
  * If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
  * How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

More information about the Swift evolution process is available at

  https://github.com/apple/swift-evolution/blob/master/process.md

Thank you,

-Chris Lattner
Review Manager

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Vladimir) #13

Hello Swift community,

The third review of "SE-0117: Allow distinguishing between public access and public overridability" begins now and runs through July 25. The proposal is available here:

  https://github.com/apple/swift-evolution/blob/master/proposals/0117-non-public-subclassable-by-default.md

+1 for *second* approach because :

* Simple mental model. It is adding `open` keyword for methods only. It is very easy to understand/remember just one *new* rule : only `open` methods/props could be overridden outside of the module. Easy to describe this to anyone new in Swift.
* More flexible. You still can subclass the `public` class. But you can't "hurt" the logic inside the module while you can have additional state and methods in new class for your own logic. So IMO second approach fixes the exact reason why we have this proposal *without* adding unnecessary limitations and additional complexity.
* As I understand, for second approach there is no this IMO strange limitation : "The superclass of an open class must be open." (while yes, this still exists: "The overridden declaration of an open override must be open.")
* I feel this approach more Swifty - free to subclass if you want for your logic but safe for internal module's logic.
* 'open class' adds complexity to access model, given we already have public, internal, private, fileprivate
* Second approach unifies the syntax : structs and classes will have the same syntax for public declaration (only difference : classes can have 'open' for methods/props)

···

On 21.07.2016 18:33, Chris Lattner via swift-evolution wrote:

Reviews are an important part of the Swift evolution process. All reviews should be sent to the swift-evolution mailing list at

  https://lists.swift.org/mailman/listinfo/swift-evolution

or, if you would like to keep your feedback private, directly to the review manager.

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and contribute to the direction of Swift. When writing your review, here are some questions you might want to answer in your review:

  * What is your evaluation of the proposal?
  * Is the problem being addressed significant enough to warrant a change to Swift?
  * Does this proposal fit well with the feel and direction of Swift?
  * If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
  * How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

More information about the Swift evolution process is available at

  https://github.com/apple/swift-evolution/blob/master/process.md

Thank you,

-Chris Lattner
Review Manager

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Rod Brown) #14

Proposal Link: https://github.com/apple/swift-evolution/blob/master/proposals/0117-non-public-subclassable-by-default.md

- What is your evaluation of the proposal?

+1 to the second proposal, if we were to drop the concept of a final class. I feel like by blocking subclasses in preference to extensions, we would need to be 100% sure we could add stored properties in extensions, otherwise adding a stored property would require a subclass. I think that final fully on a class tends to be superfluous if we have final and/or open on methods. This would allow for composition with little penalty for optimisations.

If we have final classes as a whole, I recommend we go with option 1.

I personally prefer Option #2. I think that a lot of us are going “that’s not swifty” and using that as a justification. Lets be honest, we’re deliberately limiting ourselves here against composition via subclassing, without writing lots of frameworks and libraries where this would directly impact us and we’d learn our lessons. Why block composition via subclassing? Opinion, based on “this can all be done by extensions and that feels swifty”. Well it currently can’t be done via extensions. Perhaps we should be more open about this and allow users to choose their composition methodology… I feel like we’re blocking something unnecessarily.

Blocking all the members of a class can still be done, but we could theoretically allow composition on classes that are otherwise final or sealed. Why not?

I love the concept of clear subclassing with “Open” as an extension of the access level, showing public *and* subclassable. This is actually how I assumed this would work in the first place.

- Is the problem being addressed significant enough to warrant a change to Swift?
Yes

- Does this proposal fit well with the feel and direction of Swift?
Yes. I think Option 2 however slightly changes that direction, and I think that’s a good thing.

- If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
N/A

- How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
Continued interest in the discussion. Thorough read.

-Rod

···

On 22 Jul 2016, at 1:33 AM, Chris Lattner via swift-evolution <swift-evolution@swift.org> wrote:

Hello Swift community,

The third review of "SE-0117: Allow distinguishing between public access and public overridability" begins now and runs through July 25. The proposal is available here:

Reviews are an important part of the Swift evolution process. All reviews should be sent to the swift-evolution mailing list at

  https://lists.swift.org/mailman/listinfo/swift-evolution

or, if you would like to keep your feedback private, directly to the review manager.

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and contribute to the direction of Swift. When writing your review, here are some questions you might want to answer in your review:

  * What is your evaluation of the proposal?
  * Is the problem being addressed significant enough to warrant a change to Swift?
  * Does this proposal fit well with the feel and direction of Swift?
  * If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
  * How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

More information about the Swift evolution process is available at

  https://github.com/apple/swift-evolution/blob/master/process.md

Thank you,

-Chris Lattner
Review Manager

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Karoy Lorentey) #15

  * What is your evaluation of the proposal?

I gave enthusiastic thumbs up to the previous two proposals. I agree
wholeheartedly with the underlying goal, and I love the new Motivation
section. I'm OK with the idea of making "open" imply "public" by
default, which seems to be the gist of the new design.

However, reading the actual details in this third revision made me
uncomfortable. I expected the specification to be bulletproof by now;
but the actual text of the "Proposed design" section seems to be even
less coherent than the previous two.

I understand there's a (self-imposed) deadline, but this one could've
really used a little more time in the oven.

While I am still strongly in favor of the direction and high-level
design of the proposal, I cannot in good conscience argue in favor
of such ill-defined details in a _third_review_. So I'm giving
this specific revision -1 in protest. :cry:

Here are some of my problems with the text:

1. The proposal offers two separate design approaches for `open` classes, listing
   arguments for both. Please excuse my bluntness, but this is now the third attempt
   to push this through: With all due respect, would you please make up your mind?

2. The option to get rid of sealed classes is brand new with SE-0117rev3.
   Did someone argue for it in review #2?

   (I'm all for adding better support for sealed class hierarchies, so I prefer plan A.)

   It's nice that we're given the option of reducing language complexity, but then
   it's bizarre that the idea of eliminating sealed _members_ isn't mentioned at all.

   Although, a literal reading of "plan B" does in fact seem to imply that any member
   that is not explicitly marked open would not even be _internally_ overridable:

      "The second design says that there is no such thing as an open class because
      all classes are subclassable unless made final. Note that its direct methods
      would still not be overridable unless individually made open, although its
      inherited open methods could still be overridden."

   There is no mention of "public", and no mention of module boundaries.
   Thus, plan B basically proposes to make *all members* in *all classes* `final` by default.
   This seems inconsistent with the rest of the proposal, so I'll assume this is a
   mistake in wording, not the actual design intent. (Or is it?)

   Let's assume the intent was to keep members internally overridable by default.
   But then why not go the full way? We can achieve the proposal's goals by just tweaking
   existing defaults -- there's no need to introduce complicated new overridability levels:

   "This proposal does not change the rules of class subclassibility. However, it introduces the
   (previously implicit) "open" qualifier to explicitly declare an overridable member,
   and changes the rules of default overridability as follows:
     - Members of public classes are now final by default.
     - Members of internal and private classes remain overridable ("open") by default."

   I prefer plan A, but I'd also be OK with this bare-bones proposal.

   I'm strongly opposed to plan B (either as stated in the proposal or what (I assume
   above) is the intent behind it.)

3. The code examples are needlessly complicated by trying to describe both sub-proposals.
   I find this is confusing and it obscures the actual effect of both variants.

   The examples are also inconsistent with the text: e.g., AFAICT, property `size` below
   is still intended to be overridable inside the module that defines
   `SubclassableParentClass`.

       open class SubclassableParentClass {
         // This property is not overridable.
         public var size : Int
         ...
       }

4. The previous revisions ignored dynamic members, which wasn't great. The current
   document acknowleges that "dynamic" members exists, but then *prohibits*
   them from being overridden across a module boundary!

      "`open` is not permitted on declarations that are explicitly `final` or `dynamic`."
      "A class member that is not `open` cannot be overridden outside of the current module."

   If this was intentional, I'd love to see the reasoning behind this decision.
   Otherwise the wording should be fixed:

      "A class member that is not `open` or `dynamic` cannot be overridden outside
      of the current module."

5. It seems strangely inconsistent that `open` now implies `public` by default,
   but `final` doesn't. The proposal fails to explain this inconsistency.
   Was this point considered and dismissed, or was it not considered at all?

   Changing `final` to imply `public` later would be a source-breaking change.
   Given that SE-0117 is one of the last proposals where maintaining source
   compatibility isn't a major consideration, it's unclear if we'll ever have an
   opportunity to fix such minor inconsistencies.

   The same argument also applies to `dynamic`. If it's OK for `open func` to
   imply public visibility, wouldn't it follow from same logic that we should
   treat `final func` and `dynamic func` the same way?

6. The proposal does not suggest an explicit spelling for the new default level
   of internal-only overridability for public classes and members.

   We can add an optional contextual keyword later, in an additive proposal.
   However, failing to mention this point makes me question if it was intentionally
   omitted to prevent bikeshedding, or just forgotten.

7. "`open` is permitted only on class declarations and overridable class members
   (i.e. var, func, and subscript)."

   Presumably this list is to be taken to include `class func`, but not `static func`.
   Or would `class func` remain open by default?

8. The opening clause in the "open class members" section makes no sense to me.

      "A class member that overrides an open class member must be explicitly declared open
      unless it is explicitly final or it is a member of a final or non-public class.
      In any case, it is considered open."

   So, AFAICU, when a public member of a public open class overrides a superclass member,
   we will have to write either "open override" or "final override". Temporary restriction, fine.
   (Shouldn't "dynamic override" be a thing, though?)

   But if these are "considered open in any case", why force us to add misleading boilerplate?
   What does it mean for a member of a final class to be "considered open"?

9. I have trouble interpreting the parenthesized statement in the following clause:

      "`open` is not permitted on declarations that are explicitly `final` or `dynamic`.
      (Note that it's okay if one or both of the modifiers are implicitly inferred.)"

   Can "dynamic" ever be implicitly inferred? Can both "dynamic" and "final" ever apply at
   once? I assume "it's okay" here means `open` is allowed but ignored, in either case.
   Is that right?

Ayayay, so much drama!

  * Is the problem being addressed significant enough to warrant a change to Swift?

Yes.

  * Does this proposal fit well with the feel and direction of Swift?

Yes.

  * If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

See previous reviews.

  * How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Probably more than it deserves? :wink:

···

On 2016-07-21 15:33:37 +0000, Chris Lattner via swift-evolution said:

--
Károly
@lorentey


(James Froggatt) #16

   * What is your evaluation of the proposal?

I'm not entirely decided, but I think Option 2 is marginally better:
• As long as every member is closed by default, there is benefit to other modules being able to add storage through subclassing, which may never be matched by extensions.
• Knowing every subclass at compile time is possible no matter the default, although I understand the default could influence the direction of the language.

I would guess that most users of ‘final’ use it to ensure the behaviour of every instance of a class is consistent (rather than for the compile-time knowledge that there are no subclasses, or for the small performance gain). This only requires making every member final, something currently not possible to enforce automatically, but would be much easier to enforce for new classes with this proposal.

Option 1 finalises all members by default, but conflates it with the ability to subclass in general, preventing additive subclasses. Option 2 finalises all members by default, but makes an exception for inherited members.

Both options make preserving the openness of inherited members the default for open classes, which in my opinion is dangerous, since subclasses may make assumptions about their behaviour that the superclass did not. If preserving openness of inherited members is to be standard, we at least need a way to opt-out, something like ‘final(members)’, to still allow for additive subclassing. I'd prefer an opt-in solution.

In terms of practical benefit, Option 1 offers very little over Option 2 by making classes closed by default, since overrideability of members is opt-in. Most libraries I've tried make their own class heirachies from scratch.

   * Is the problem being addressed significant enough to warrant a change to Swift?

I've never had any problem with the current system, and this does risk adding a lot of complexity for only a minor gain in safety, but I support the idea in principle.

   * Does this proposal fit well with the feel and direction of Swift?

Having open be an alternative to public seems better than the previous revisions. Beyond that, it's hard to say.

   * If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

Most languages I've tried use an equivalent of final. This seems complicated in comparison, even with the revisions.

   * How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Quick reading through the proposal.


(Scott James Remnant) #17

-1

This proposal is starting to go around in circles, and now re-includes various parts of the first version of this proposal which I objected to in that review thread on grounds of lack of clarity.

I don’t think this is making the language better at this point, it feels like this is being rushed to get “something in for Swift 3 at all costs.”

I have no objection to the fundamental concept of the proposal, but this should not come at the cost of the language, just on the grounds of the timeline. I think it would be better for this to be re-proposed without the rush for future versions of Swift - especially since it’s “additive.”

Scott


(Daniel Duan) #18

* What is your evaluation of the proposal?
+1 for option 1.

* Is the problem being addressed significant enough to warrant a change to Swift?
Yes, accidental or thoughtless exposure of non-final classes from 3rd party libraries are something worth addressing.

* Does this proposal fit well with the feel and direction of Swift?
Yes. Having the privilege of working on a mature, pure Swift app, I can say that both code reuse and polymorphism can and should be better achieved without class inheritance (or classes at all, most of the time).

As alluded in the motivation section of the proposal, a third party Swift library down the line would mostly provide struct/enum types for users. Classes may be included to provide objects with reference semantics, these would be marked final by a responsible author today. And then, we have rare cases where inheritance is intended to provide customization and code reuse. The change in this proposal is sensible both for responsible library authors (who would stop worrying about missing `final`s), and for sloppy authors (who now are forced think about the interface of the library more). Ultimately, we'll get better libraries (to an extent) and happier users.

That's a direction I'd like Swift to go.

* If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
No.

* How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
Read all 3 versions and all discussions on this list. Also followed a lot of related threads on Twitter. Had quite a few discussions of it with ppl smarter than me IRL.

···

Sent from my iPad

On Jul 21, 2016, at 8:33 AM, Chris Lattner <clattner@apple.com> wrote:

   * What is your evaluation of the proposal?
   * Is the problem being addressed significant enough to warrant a change to Swift?
   * Does this proposal fit well with the feel and direction of Swift?
   * If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
   * How much effort did you put into your review? A glance, a quick reading, or an in-depth study?


(Brent Royal-Gordon) #19

  * What is your evaluation of the proposal?

Of the designs offered, I prefer #1, because:

1. I'm not convinced that there's actually any sense in subclassing a class with no open members; it seems to me that any class intended to be used like this ought to be redesigned to use composition instead of subclassing.

2. I also think that you're still effectively depending on implementation details about the instance's lifecycle. Suppose I write a database library which has a Record class. It supports uniquing—that is, a single database record is never represented by two different Record instances. If the last external reference to a Record disappears, does the library hold onto that Record and return it again in response to another query, or does it deallocate it and create a new one? That barely matters *unless* you've subclassed Record.

3. Even if there are no overrides, the class and the library around it probably still need to be designed for subclassing. Take the Record example from the previous point: the library will never use your subclass unless there's some hook for telling it which subclass to use. Subclassing isn't going to actually work right if the library doesn't expect any subclasses.

4. Even leaving that aside, you may still foisting surprising memory characteristics on the library. For instance, you might hang a large object graph off an instance that's intended to be lightweight, or create a retain cycle the library author took great pains to avoid.

5. Finally, this doesn't help with the stated goal of allowing you to make a class `final` in a later version of the library.

However, I'm actually strongly in favor of Garth Snyder's call for `open` to explicitly become an access level. In design #1, it almost is already; making it official would simplify many aspects of this design.

(If we do take that road, I would further suggest requiring protocols to be marked `open` instead of `public`. I can easily imagine having closed `public` protocols which are visible but can't be conformed to; this would help with certain typing problems, such as `CKRecordValue`. On the other hand, if `public` were conformable from outside, I can't think of a use for marking a protocol `open`.

Protocol extension members should continue to be marked `public`. I could imagine `open` being added later, meaning that the member should be added as a protocol requirement so a specialized implementation can be provided; this would avoid the current boilerplate for defaulted protocol members.)

  * Is the problem being addressed significant enough to warrant a change to Swift?

Yes.

  * Does this proposal fit well with the feel and direction of Swift?

Yes.

  * If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

N/A.

  * How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

I put a little bit less effort into this review than I did into the previous two reviews, the many discussion threads on this topic, or several months of thinking this over and changing my mind about the idea as the plan evolved.

···

On Jul 21, 2016, at 8:33 AM, Chris Lattner via swift-evolution <swift-evolution@swift.org> wrote:

--
Brent Royal-Gordon
Architechies


(David Rönnqvist) #20

  * What is your evaluation of the proposal?

+1
In its third revision I like this proposal more. I think “open” is a good keyword for both members and classes.
I’m in favor of the first design for open classes. That said, I also acknowledge that subclassing without overriding anything doesn’t suffer from the problems that overriding members do. Additionally, even for a sealed class, new methods could be added thought extensions. This means the the benefits of the first design over the second one are fairly small. However, in my experience (which might be wrong), designing a class for subclassing without any overridable methods is quite uncommon, and in the first design can easily be resolved by making it open. I see it as a benefit of the first design that it allows for this distinction (a class can be make public without being subclassable).

  * Is the problem being addressed significant enough to warrant a change to Swift?

Yes.

  * Does this proposal fit well with the feel and direction of Swift?

Yes.

  * If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

I’ve encountered virtual functions in C++ but only to a small extent.

I like this solution better because it makes the distinction between internally and externally overridable (and gets out of the way for internal overrides).

  * How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Read the proposal and much of the discussion (but not all, there’s been so much :wink:)

- David