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
_______________________________________________
swift-evolution mailing list
[email protected]
https://lists.swift.org/mailman/listinfo/swift-evolution
