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

Fri Jul 22 12:09:14 CDT 2016

Thank you very much for clarifying these! My position is now of course back in favor of the proposal. +1

(Sorry for being a stickler; I know what deadlines are like. I hope the team gets a bit of time to rest.)

<3,
-- 
Károly
@lorentey

> On 2016-07-22, at 18:36, John McCall <rjmccall at apple.com> wrote:
> 
>> 
>> 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