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

[Károly Lőrentey]

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

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

-- 
Károly
@lorentey