Final: Implementation for SE-0228: Fix ExpressibleByStringInterpolation #20214
Conversation
It doesn’t actually need to mutate them.
This change allows ExprParentFinder to restrict certain searches for parents to just AST nodes within the nearest surrounding BraceStmt. In the string interpolation rework, BraceStmts can appear in new places in the AST; this keeps code completion from looking at irrelevant context. NFC in this commit, but keeps code completion from crashing once TapExpr is introduced.
Since soon enough, it won’t be anymore.
TapExpr allows a block of code to to be inserted between two expressions, accessing and potentially mutating the result of its subexpression before giving it to its parent expression. It’s roughly equivalent to this function:
func _tap<T>(_ value: T, do body: (inout T) throws -> Void) rethrows -> T {
var copy = value
try body(©)
return copy
}
Except that it doesn’t use a closure, so no variables are captured and no call frame is (even notionally) added.
This commit does not include tests because nothing in it actually uses TapExpr yet. It will be used by string interpolation.
This is the bulk of the implementation of the string interpolation rework. It includes a redesigned AST node, new parsing logic, new constraints and post-typechecking code generation, and new standard library types and members.
With new string interpolation in place, it is no longer used by anything in the compiler.
StringInterpolationProtocol informally requires conforming types to provide at least one method with the base name “appendInterpolation” with no (or a discardable) return value and visibility at least as broad as the conforming type’s. This change diagnoses an error when a conforming type does not have a method that meets those criteria.
Some users, including some in the source compatibility suite, accidentally used init(stringInterpolationSegment:) by writing code like `map(String.init)`. Now that these intializers have been removed, the remaining initializers often end up tying during overload resolution. This change adds several overloads of `String.init(describing:)` which will break these ties in cases where the compiler previously selected `String.init(stringInterpolationSegment:)`.
This change avoids constructing a String when interpolating a Float, Double, or Float80. Instead, we write the characters to a fixed-size buffer and then append them directly to the string’s storage. This seems to improve performance for all three types, but especially for Double and Float80, which cannot always fit into a small string when stringified.
In rare cases usually involving generated code, an overload added by an extension in the middle of a file would not be visible below it if the type had lazy members and the same base name had already been referenced above the extension. This change essentially dirties a type’s member lookup table whenever an extension is added to it, ensuring the entries in it will be updated. This change also includes some debugging improvements for NameLookup.
The DeadObjectRemoval pass in SILOptimizer does not currently remove reworked string interpolations as well as the old design because their effects cannot be described by @_effects(readonly). That causes a test failure on Linux. This change temporarily silences that test. The SILOptimizer issue has been filed as SR-9008.
Previously, the parser had an odd asymmetry which caused the same function to accept foo(), but reject “\()”. This change fixes the issue. Already tested by test/Parse/try.swift, which uses this construct in one of its throwing interpolation tests.
|
@swift-ci please test |
|
@swift-ci please test source compatibility |
|
@swift-ci please smoke test compiler performance |
|
@swift-ci please benchmark |
Build comment file:Performance: -O
Code size: -O
Performance: -Osize
Code size: -Osize
Performance: -Onone
Code size: Swift libraries
How to read the dataThe tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.If you see any unexpected regressions, you should consider fixing the regressions before you merge the PR. Noise: Sometimes the performance results (not code size!) contain false alarms. Unexpected regressions which are marked with '(?)' are probably noise. If you see regressions which you cannot explain you can try to run the benchmarks again. If regressions still show up, please consult with the performance team (@eeckstein). Hardware Overview |
|
Alamofire failed to compile Source/MultipartFormData.swift line 95. It's using a string interpolation in a weird way—in an initializer for a lazy property, interpolating the value of a non-lazy property—which is causing its temporary to become immutable for some reason. Unfortunately, I haven't been able to reproduce it yet, even by downloading and compiling the entire project locally. The console indicates that Alamofire did fail in CI release mode too, though. Perhaps my setup isn't a precise enough reproduction of the CI environment. I really wanted to get this in tonight, but I guess it'll have to wait for tomorrow. The other CI results don't concern me; the performance benchmarks are showing both more regressions and more improvements, so either the CI is being weird tonight or something recently landed (like BuiltinInt) that shifted performance around in complicated ways. Either way, it's not a good reason to not merge. (If someone with a little more seniority than me wants to merge this despite the source compatibility failure, go ahead. I may be too conservative about this.) |
|
@swift-ci please test source compatibility |
|
Those CI "failures" are actually because two projects unexpectedly passed. Should land tomorrow. |
The temporary variable used by string interpolation needs to be recontextualized when it’s inserted into a synthesized getter. Fixes a compilation failure in Alamofire. I’ll probably follow up on this bug a bit more after merging.
beccadax
force-pushed
the
interpolation-rework-final-2-fixed
branch
from
8766798 to
c479a0c
Compare
|
@swift-ci please test |
This comment has been minimized.
This comment has been minimized.
|
Ah, the famous "Please ignore this build failure" build failure... |
This comment has been minimized.
This comment has been minimized.
|
@swift-ci please test os x platform |
|
Previous incarnations of this PR have been reviewed by @xedin, @milseman, @slavapestov, and @benrimmington. Hold on to your butts... |
| } | ||
|
|
||
| extension TextOutputStream { | ||
| public mutating func _lock() {} | ||
| public mutating func _unlock() {} | ||
|
|
||
| @inlinable | ||
| public mutating func _writeASCII(_ buffer: UnsafeBufferPointer<UInt8>) { |
There was a problem hiding this comment.
Does this have to be public? Since _fromASCII is not inlinable, then this probably shouldn't be either.
There was a problem hiding this comment.
Err crap, is it because we can't have non-public requirements on public protocols?
BTW, I think that floating point types' TextOutputStreamable.write<Target> should have a @_specialize(where Target == String for the 99% case, which is also the perf-sensitive case. That should give us better perf, and since that's the only call site for this, we can form an immortal String there.
edit: Perhaps also DefaultStringInterpolation now...
Longer-term, we don't really care for a buffer-returning function for _float${Bits}ToString, we just want to write directly into the allocated extra capacity. Same for Int->String, etc. CC @stephentyrone who might be looking at that.
This is something we can fix up after I land my branch
There was a problem hiding this comment.
I agree that it could make sense to specialize write(to:) for String and possibly DefaultStringInterpolation.
The implementation and use of _float${Bits}ToString is entirely behind resilience barriers, so we can change it in the future. I suppose what we can't do in the future, though, is replace _writeASCII(_:) with whatever primitives we'd need to have a stream allocate some empty space, let us write into it, and then tell it how much of that space we used.
| /// - Parameter other: A string to append. | ||
| public mutating func write(_ other: String) { | ||
| self += other | ||
| } | ||
|
|
||
| public mutating func _writeASCII(_ buffer: UnsafeBufferPointer<UInt8>) { |
| @@ -836,6 +836,7 @@ extension String { | |||
| /// // Prints "Hello, friend" | |||
| /// | |||
| /// - Parameter other: Another string. | |||
| @inlinable | |||
There was a problem hiding this comment.
If _StringGuts.append isn't inlinable, then this shouldn't be either.
There was a problem hiding this comment.
You're right. I'll let you fix that up.
| self.init(_large: storage) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
How important is the @inlinable part? Is it because of forming a canonical empty string with the small check? If so, then I'll adapt my adaptation, but otherwise would like to keep the allocation out of inlinable code when possible. Helps code size and resilience.
There was a problem hiding this comment.
I want to inline if capacity <= _SmallUTF8String.capacity because, if we only use this in string interpolation, capacity will always be a constant and the optimizer can fold away one of the branches. I also want to inline self.init() because it should reduce down to the empty string singleton constant.
I don't really care about what's in the else branch, though (although ideally it would skip any small-string tests). If this implementation inlines more than you want to, you can choose a different one.
| return | ||
| } | ||
| _appendSlow(other) | ||
| } |
There was a problem hiding this comment.
Is this inlinable still necessary now that we have the _initialCapacity init? Could we drop it?
There was a problem hiding this comment.
Currently, DefaultStringInterpolation.write(_:) is calling _appendSlow(_:) directly to bypass append(_:)'s empty-string check, which is false for the majority of segments. This seemed to benchmark well, but it might be over-fitting the benchmarks and/or an unnecessary complication. Or...
| } | ||
| } | ||
|
|
||
| extension DefaultStringInterpolation: TextOutputStream { | ||
| @inlinable |
There was a problem hiding this comment.
Can drop inlinable if _appendSlow isn't
There was a problem hiding this comment.
...it might make sense to make this non-inlineable. If you'd like, I can make a throwaway branch to see how that benchmarks (against the current implementation).
| } | ||
|
|
||
| @inlinable | ||
| public mutating func _writeASCII(_ buffer: UnsafeBufferPointer<UInt8>) { |
There was a problem hiding this comment.
public?inlinable if append isn't
…ftlang#20214) * [CodeCompletion] Restrict ancestor search to brace This change allows ExprParentFinder to restrict certain searches for parents to just AST nodes within the nearest surrounding BraceStmt. In the string interpolation rework, BraceStmts can appear in new places in the AST; this keeps code completion from looking at irrelevant context. NFC in this commit, but keeps code completion from crashing once TapExpr is introduced. * Remove test relying on ExpressibleByStringInterpolation being deprecated Since soon enough, it won’t be anymore. * [AST] Introduce TapExpr TapExpr allows a block of code to to be inserted between two expressions, accessing and potentially mutating the result of its subexpression before giving it to its parent expression. It’s roughly equivalent to this function: func _tap<T>(_ value: T, do body: (inout T) throws -> Void) rethrows -> T { var copy = value try body(©) return copy } Except that it doesn’t use a closure, so no variables are captured and no call frame is (even notionally) added. This commit does not include tests because nothing in it actually uses TapExpr yet. It will be used by string interpolation. * SE-0228: Fix ExpressibleByStringInterpolation This is the bulk of the implementation of the string interpolation rework. It includes a redesigned AST node, new parsing logic, new constraints and post-typechecking code generation, and new standard library types and members. * [Sema] Rip out typeCheckExpressionShallow() With new string interpolation in place, it is no longer used by anything in the compiler. * [Sema] Diagnose invalid StringInterpolationProtocols StringInterpolationProtocol informally requires conforming types to provide at least one method with the base name “appendInterpolation” with no (or a discardable) return value and visibility at least as broad as the conforming type’s. This change diagnoses an error when a conforming type does not have a method that meets those criteria. * [Stdlib] Fix map(String.init) source break Some users, including some in the source compatibility suite, accidentally used init(stringInterpolationSegment:) by writing code like `map(String.init)`. Now that these intializers have been removed, the remaining initializers often end up tying during overload resolution. This change adds several overloads of `String.init(describing:)` which will break these ties in cases where the compiler previously selected `String.init(stringInterpolationSegment:)`. * [Sema] Make callWitness() take non-mutable arrays It doesn’t actually need to mutate them. * [Stdlib] Improve floating-point interpolation performance This change avoids constructing a String when interpolating a Float, Double, or Float80. Instead, we write the characters to a fixed-size buffer and then append them directly to the string’s storage. This seems to improve performance for all three types, but especially for Double and Float80, which cannot always fit into a small string when stringified. * [NameLookup] Improve MemberLookupTable invalidation In rare cases usually involving generated code, an overload added by an extension in the middle of a file would not be visible below it if the type had lazy members and the same base name had already been referenced above the extension. This change essentially dirties a type’s member lookup table whenever an extension is added to it, ensuring the entries in it will be updated. This change also includes some debugging improvements for NameLookup. * [SILOptimizer] XFAIL dead object removal failure The DeadObjectRemoval pass in SILOptimizer does not currently remove reworked string interpolations as well as the old design because their effects cannot be described by @_effects(readonly). That causes a test failure on Linux. This change temporarily silences that test. The SILOptimizer issue has been filed as SR-9008. * Confess string interpolation’s source stability sins * [Parser] Parse empty interpolations Previously, the parser had an odd asymmetry which caused the same function to accept foo(), but reject “\()”. This change fixes the issue. Already tested by test/Parse/try.swift, which uses this construct in one of its throwing interpolation tests. * [Sema] Fix batch-mode-only lazy var bug The temporary variable used by string interpolation needs to be recontextualized when it’s inserted into a synthesized getter. Fixes a compilation failure in Alamofire. I’ll probably follow up on this bug a bit more after merging.
|
@rintaro Interesting. That particular part of the PR is very old and, at the time, was necessary to resolve some failing (and I think crashing) tests. If those tests no longer fail without it, I agree that it's probably no longer needed. |
This PR implements a new string interpolation API with a different, finalized ABI from what we're currently shipping.
This is #19963 with a clean revision history and a final pass for style. The only functional changes are that
literalCapacityis now calculated as the UTF-8 length of the string and theCustomStringtype I accidentally left in the benchmarks has been removed. I'll be running the last set of CI checks here and then merging.I expect the final checks to still show some benchmark regressions. A few are known issues with non-ABI-affecting fixes planned. The others we think will be easier to fix on master, where we can coordinate changes more easily.
Implements SE-0228. Resolves SR-1260, SR-2303, and SR-3969. Resolves rdar://43621912.
cc @milseman @ravikandhadai