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 <[email protected]> wrote: > >> >> On Jul 22, 2016, at 9:09 AM, Károly Lőrentey via swift-evolution >> <[email protected]> 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 >> [email protected] >> https://lists.swift.org/mailman/listinfo/swift-evolution _______________________________________________ swift-evolution mailing list [email protected] https://lists.swift.org/mailman/listinfo/swift-evolution
