SE-0228: Fix Expressible By String Interpolation


(Michael Ilseman) #21

We should be careful to not design around implementation details that could be easily changed. I'm not sure what behavior you're observing, but the contract of reserveCapacity is flexible.

Currently the canonical empty string is a small-string with size 0 and any attempt to reserve capacity less than what a small-string can store (<=15), or less than what a stored string's existing capacity is, is a nop. However, these early checks happen to not be @inlinable, so it won't get constant folded. We can totally split the function into @inlinable early checks followed by a call to a @usableFromInline function, at the (perhaps inconsequential) cost of incorporating that particular pattern into the binary compatibility story.

edit: Omitted a word


(Brent Royal-Gordon) #22

There's some low-hanging fruit: About a third of the difference is because DefaultStringInterpolation.description was not marked @inlinable. That's easy to fix.

The rest seems to be what @Michael_Ilseman said—the relevant fast paths in reserveCapacity(_:) and append(_:) are not currently inlined, and this benchmark is so fast that merely making these calls is enough to substantially slow it down. I did a quick-and-dirty fix just to test this, and got it down to 193. That's not quite as good as the proposal's 167, but it's still pretty close; we might be able to do better.

(Inlining those checks also improved the other string interpolation benchmarks—ManySmallSegments, the fastest one, went from 2.18x to 2.55x—so it might be worth exploring either way.)

I guess the conclusion is, an implementation where init(stringLiteral:) calls through to init(stringInterpolation:) is natively slower, but good performance work can most likely recover the lost speed. I can't quite bring myself to endorse it, but it's a reasonable alternative.


(Michael Ilseman) #23

Could you elaborate on your hesitation? It seems like we know how to make the provided one nearly-optimal. If a conforming type also has their own conformance to ExpressibleByStringLiteral that's more efficient, that one will be chosen instead of the provided one. This seems like a nice usability win.


(Jordan Rose) #24

To be clear, I don't think we need the full-on reimplementation you tried. We just need those extension methods I provided originally (but with different constraints):

extension ExpressibleByStringInterpolation {
  init(stringLiteral: StringLiteralType) {
    var singleInterpolation = StringInterpolation(literalCapacity: 0, interpolationCount: 0)
    singleInterpolation.appendLiteral(stringLiteral)
    self.init(stringInterpolation: stringInterpolation)
  }
}

extension ExpressibleByStringInterpolation
  where StringLiteralType == String, StringInterpolation == String.StringInterpolation {
  init(stringInterpolation: StringInterpolation) {
    self.init(stringLiteral: stringInterpolation.make())
  }

  @available(*, deprecated, message="When using String.StringInterpolation as your interpolation type, you need to implement init(stringLiteral:) rather than init(stringInterpolation:)"
  init(stringLiteral: StringLiteralType) {
    fatalError("When using String.StringInterpolation as your interpolation type, you need to implement init(stringLiteral:) rather than init(stringInterpolation:)")
  }
}

(Michael Ilseman) #25

Brent has effectively that change, using unavailable here. What would be a reason for preferring a warning instead?


(Brent Royal-Gordon) #26

Yeah, I guess you're right. We can get very close to optimal, and you can always provide both if we don't get there for you. And if we run into problems making this fast enough (there are small regressions in other benchmarks from the quick-and-dirty version of the inlining I tested), we can always amend the proposal before we ship.

So, corrections to make to SE-0228:

  1. DefaultStringInterpolation.description should be inlinable.
  2. We should have a default StringInterpolationProtocol.init(stringLiteral:) which calls through to init(stringInterpolation:).
  3. We should keep the current constrained StringInterpolationProtocol.init(stringInterpolation: DefaultStringLiteral), but add an unavailable (right?) init(stringLiteral:) alongside it to keep us from using the other one.

Agreed?


(Brent Royal-Gordon) #27

Followup for @Karl:

I just found the language we cut from the proposal lurking in an editor window. Keep in mind that this was only ever a draft and I haven't updated it at all; even so, it might help you understand where we're coming from.

Have a formal appendInterpolation requirement, but still use overloads

We considered requiring an implementation of appendInterpolation(_:) with a number of different parameter types. A formal requirement would make string interpolation more useful in a generic context, but each option interfered with a plausible use case:

  • Unconstrained generic parameter: Would keep conforming types from restricting the types which can be interpolated into them.

  • The type being initialized: Would keep one type from using another type's StringInterpolation implementation; for instance, if StringProtocol refined ExpressibleByStringInterpolation, Substring could not use String.StringInterpolation.

  • A parameter of type String: Would keep conforming types from requiring all strings to be labeled; for instance, an OSLogString might want to require all Strings to be interpolated with either \(public:) or \(private:).

  • An associatedtype of any type, as long as you designate something which can be interpolated: Would keep conforming types from requiring that all interpolations include an extra parameter or label or from having only generic appendInterpolation methods (as String.StringInterpolation does!). Also, conforming types with several different concrete implementations would have to arbitrarily choose one to expose in a generic context, and would have to designate it explicitly instead of relying on associated type inference.


(Jordan Rose) #28

I'm concerned that we'll fix/change the behavior of requirement checking to match overload resolution to ignore unavailable candidates. I guess we can just have a test for that, though, and switch to deprecated if/when we do such a thing.


(Brent Royal-Gordon) #29

That PR includes a test which should fail if the compiler ever starts to accept conforming types which implement neither init(stringLiteral:) nor init(stringInterpolation:).

(Someday, I think we should add an attribute or something which says "use this default implementation only if a this other member has a non-default implementation"; that would permit, for instance, Hashable to provide both a hash(into:) default calling hashValue, and a hashValue default calling hash(into:). But that is definitely a separate proposal.)


(Dave Abrahams) #30

If it was omitted, someone should file a bug against the guidelines. I'm pretty sure we put it in.


(Jordan Rose) #31

It looks like we still have the Swift 2 version of the convention:

If an associated type is so tightly bound to its protocol constraint that the protocol name is the role, avoid collision by appending Type to the associated type name:

protocol Sequence {
  associatedtype IteratorType : Iterator
}

(Brent Royal-Gordon) #32

Yup, I was about to say. Pull request here.