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

[John McCall]

> On Jul 22, 2016, at 9:09 AM, Károly Lőrentey via swift-evolution <swift-evolution at swift.org> wrote:
> 
> On 2016-07-21 15:33:37 +0000, Chris Lattner via swift-evolution said:
>> 	* 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. 😢

I'm sorry if my drafting wasn't up to snuff.  In my defense, I needed to get it out quickly,
and I had a lot to rewrite.  I was pretty tired by the time I actually finished the motivation
section.

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

Most of the proposal has been "pushed through", frankly.  We are looking for feedback
on this specific question.  I agree that the proposal framework is an awkward fit for
this kind of discussion prompt.

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

Because it was settled in the core-team review.

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

It is a mistake in wording.

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

Thank you, that's what we're looking for.

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

I'm sorry, I didn't have much of a choice about this.  Perhaps I could have broken it out
into two completely different code examples.

>  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
>        ...
>      }

Yes, this should read "not overridable outside of the current module".

> 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."

Yes, sorry, this was a drafting error again.

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

I think the best model of thinking about "open" is that it is an access level above "public".
The idea of allowing "internal open" came out of discussion but in retrospect does not
seem to hold its own.

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

"class" members are still members.  The model for "static" on class members has always
been that they are implicitly final.

> 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"?

Today, you would have to write "public override" on such a member.  It is not added
boilerplate to say that you have to instead write "open override" unless your intent is to
close off overriding (in which case you have to write "public final override").

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

It's inherited.  I can't remember if we require it to be explicit on the override.

> Can both "dynamic" and "final" ever apply at once?

This is currently forbidden when explicit, but you can declare a dynamic member of a final class.

> I assume "it's okay" here means `open` is allowed but ignored, in either case.
>  Is that right?

Probably the right rule is that open can be subsumed by dynamic, so that you can override
an open member and make it dynamic.  I'm not sure whether you should be able to say just
"open" on an override of a dynamic member when it clearly remains dynamic.

Inherited open-ness will be ignored on a final class or member.

Honestly, this is why I don't like to include this level of detail in proposals.  People complain if
it's not there, but including it just invites a bunch of complaints about every detail and distracts
from the important parts of the discussion.

John.

> 
> 
> 
> 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? ;-)
> 
> -- 
> Károly
> @lorentey
> 
> 
> _______________________________________________
> swift-evolution mailing list
> swift-evolution at swift.org
> https://lists.swift.org/mailman/listinfo/swift-evolution