[swift-evolution] [Review] SE-0027 Expose code unit initializers on String

[Dave Abrahams]

on Mon Feb 15 2016, Zach Waldowski <swift-evolution at swift.org> wrote:

> See responses inline.
>
> Aside: omgomgomg I got Dave Abrahams to respond to a proposal of mine

Don't worry; it'll wear off soon enough...

> Excitedly,
>   Zachary Waldowski
>   zach at waldowski.me
>
> On Mon, Feb 15, 2016, at 11:48 AM, Dave Abrahams via swift-evolution
> wrote:
>> 
>> on Thu Feb 11 2016, Douglas Gregor <swift-evolution at swift.org> wrote:
>> 
>> > Hello Swift community,
>> >
>> > The review of SE-0027 "Expose code unit initializers on String" begins
>> > now and runs through February 16, 2016. The proposal is available
>> > here:
>> >
>> > https://github.com/apple/swift-evolution/blob/master/proposals/0027-string-from-code-units.md
>> > Reviews are an important part of the Swift evolution process. All
>> > reviews should be sent to the swift-evolution mailing list at
>> >
>> > https://lists.swift.org/mailman/listinfo/swift-evolution
>> > <https://lists.swift.org/mailman/listinfo/swift-evolution>
>> > or, if you would like to keep your feedback private, directly to the
>> > review manager. When replying, please try to keep the proposal link at
>> > the top of the message:
>> >
>> > Proposal link:
>> >
>> > https://github.com/apple/swift-evolution/blob/master/proposals/0027-string-from-code-units.md
>> > Reply text
>> >
>> > Other replies
>> >  <https://github.com/apple/swift-evolution#what-goes-into-a-review-1>What goes into a review?
>> >
>> > The goal of the review process is to improve the proposal under review
>> > through constructive criticism and, eventually, determine the
>> > direction of Swift. When writing your review, here are some questions
>> > you might want to answer in your review:
>> >
>> > What is your evaluation of the proposal?
>> 
>> I support the intent of the proposal but am -1 on the specific proposal
>> as offered.
>> 
>> First, in the presentation, there are several things that make it hard
>> to evaluate:
>> 
>> 1. The "decode" function signature is so wide that it can't be read
>>    without scrolling at
>>    https://github.com/apple/swift-evolution/blob/master/proposals/0027-string-from-code-units.md
>> 
>> 2. The proposal doesn't show any usage of the proposed APIs, so it
>>    is hard to understand what effect these APIs would have on real
>>    code.  There are some examples of uses in
>>    https://github.com/apple/swift/compare/master...zwaldowski:string-from-code-units
>>    but I have to dig for them.
>> 
>> 3. The description of the “Detailed Design” doesn't show any examples
>>    either, so I have to imagine what “backporting the Swift 3.0 versions
>>    of the CString constructors, then making them generic over their
>>    input and codec” means.
>
> The notes are appreciated. I'll take care to add examples and fix
> formatting. The proposal was always intended to be paired with the PR to
> the stdlib, and that lack of clarity did not come up during the proposal
> vetting stage.
>
>> Next, when I look at *uses* that I can find (in the tests), I don't find
>> them to be clear.
>> 
>>   String(validatingCodeUnits: result, as: UTF8.self)
>> 
>> What does “validatingCodeUnits” mean?  Clearly we're going to do some
>> checking.  Is there a repair?  Is the initializer failable?  
>
> I'm open to suggestions. The names as they are were lifted from the
> changes to the CString APIs made on swift-3-api-guidelines.

I suspected that they might be so I went and checked on GitHub... but
now I see that only searches the default branch :-P

    https://help.github.com/articles/searching-code/

But if the naming error was on our side, my apologies.  We should do
better, regardless.

> "repairingCodeUnits" implies to me that there is something already wrong
> with them. 

That doesn't bother me.  If you are requesting the repair, you are
effectively assuming that there is something already wrong with them.

> I'm also not sure as to the point of the initializer being failable;
> many domain initializers in the stdlib don't indicate that they can
> fail directly in the name.

The point I'm trying to make is that the implications of “validating”
aren't clear, not that I can't tell whether it's failable.

>> 
>> In this change, for example:
>> https://github.com/apple/swift/compare/master...zwaldowski:string-from-code-units#diff-d39ec2c4819c950aeef95f899f53b104R79
>> I find the old code much clearer than the new code (I'm not sure why
>> _decode starts with an underscore here; isn't it part of your proposed
>> public API?)
>
> The PR is ahead of the proposal here. It was pointed out in the review
> thread up to this point that that that "primitive" isn't truly needed;
> the initializers on String should be the API used by all.

Okay, well, proposals (including any diffs they reference) should remain
stable under review so we know what we're reviewing.  I really don't
know what's being proposed here.

> static func decode<
>   Encoding: UnicodeCodecType, 
>   Input: CollectionType where Input.Generator.Element == Encoding.CodeUnit
> >(
>    _: Input, as: Encoding.Type, repairingInvalidCodeUnits: Bool = default
> ) -> (result: String, repairsMade: Bool)?

It seems like you are withdrawing decode from the proposal?

> For convenience, the Bool flag here is also separated out to a more
> common-case pair of String initializers:

I don't understand what it means for a “bool flag to be separated out to
a... pair of ... initializers.”

> init<...>(codeUnits: Input, as: Encoding.Type)
> init?<...>(validatingCodeUnits: Input, as: Encoding.Type)

Are you saying that the first initializer corresponds to
`repairingInvalidCodeUnits: true` and the second to
`repairingInvalidCodeUnits: false`?

What about the user who wants repairs if necessary and wants to know
about it when repairs were made?

> Finally, for more direct compatibility with String.Type.fromCString(_:)
> and String.Type.fromCStringRepairingIllFormedUTF8(_:), these
> constructors are overloaded for pointer-based strings of unknown length:
> 
> init(cString: UnsafePointer<CChar>)
> init?(validatingCString: UnsafePointer<CChar>)

Why are we trying to “be compatible” with those static methods?  

> The ugliness in this instance is transitional; 

What ugliness?

> it stems from bridging the old-style constructor to the new-style
> one. 

I don't know what that means either.

> The old signature's return type is awkward in that it returns the
> extra hadError flag regardless of whether decoding failed, which is
> discarded almost universally by clients in the stdlib.
>
> I intend to revert the change to `fromCStringRepairingIllFormedUTF8`, or
> elide `_decode` into it instead.

I don't know what that means either.

>> Lastly, I am very concerned about the “Alternatives Considered” section,
>> where, of one alternative, it says:
>> 
>>       This might be the better long-term solution from the perspective
>>       of API maintenance, but in the meantime this proposal has a fairly
>>       low impact.
>> 
>> We can't accept changes into the standard library “in the meantime,”
>> with the expectation that something more comprehensive will make them
>> obsolete.  Even though we've had migration tools, we never operated that
>> way in the past, and as we head toward API and ABI stability it is even
>> more true today.
>
> I understand the sentiment here, but I don't think "wait for the next
> rewrite of String" is a good solution to a problem that the stdlib
> already resolves. 

I'm really confused.  If the stdlib already resolves the problem, why is
there a proposal?

> See below.
>
>> 
>> > 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?  
>> 
>> W.r.t. direction, the fact that we have a major String overhaul planned
>> means that tackling this one corner of the API is probably not entirely
>> appropriate.
>
> "Corner" is an unfair characterization; 

Not at all; this addresses an important use-case, but accounts for a
very small part of the API.

> users of Swift want to implement things to the level of safety and
> performance bar set by the stdlib, but we are currently at a
> disadvantage. No better example can be found but in
> corelibs-foundation:
> https://github.com/apple/swift-corelibs-foundation/blob/546dc8e16c3c34ca50f5752c6d0f39c3524f5f0a/Foundation/NSString.swift#L305.

Heh, well, that's ironic.  Before Swift was released we had all of this
functionality for unicode transcoding available in the String API, and
it was decided that to avoid treading into Foundation's territory, any
functionality already exposed by NSString should not be public API on
String without Foundation loaded.

I have ambitions that Swift strings will be first-class standalone types
without the need to rely on Foundation some day soon, but under the
circumstances we may need a more comprehensive change than merely adding
fast unicode transcoding to justify treading into this area.

> Is it not a deficiency of the stdlib when code has to resort to
> non-public methods? 

Yes.  There are lots of deficiencies in the stdlib.  I'm not saying this
shouldn't be addressed, but I'm concerned about addressing this in a
temporary way that we think may be sub-optimal.

> The stdlib (i.e., the parts touched by the PR) and corelibs-foundation
> would have to move in lockstep to adopt a replacement, so the existing
> underscored versions are as good as public API.
>
> I understand, and 100% encourage, the reticence around new API. However,
> we've created a worse problem by encouraging slow, buggy, custom
> versions of behavior that already exists in the stdlib, or implying that
> the underscored API should be used because we haven't managed something
> better yet. :/

I am not reticent to have new API.  I am reticent to accept partial or
known-suboptimal temporary solutions when we are developing a
comprehensive plan that ought to address the same problems (among many
others).

-- 
-Dave