Handle new swift-syntax closure expansion behavior #1831
Conversation
| @testable import SourceKitLSP | ||
| import XCTest | ||
|
|
||
| final class RewriteSourceKitPlaceholdersTests : XCTestCase { |
There was a problem hiding this comment.
ℹ️ These overlap somewhat with the SwiftCompletionTests but I thought including tests specifically directed at nesting would be worthwhile.
woolsweater
force-pushed
the
the-trail-is-closed
branch
2 times, most recently
from
319f8bf
to
31fa3e9
Compare
There was a problem hiding this comment.
Thank you @woolsweater, great changes!
I only have a few comments, otherwise looks great.
| @@ -0,0 +1,62 @@ | |||
| import SKTestSupport | |||
There was a problem hiding this comment.
Could you add the copyright header to this file?
| case let .text(text) where placeholders.isEmpty: | ||
| result += text | ||
|
|
||
| case let .text(text): | ||
| placeholders.latest.contents += text |
There was a problem hiding this comment.
Instead of having two cases that just different in a where clause, I would have one case with a nested if. I think that highlights the difference between whether we’re currently inside a placeholder better.
| case let .text(text) where placeholders.isEmpty: | |
| result += text | |
| case let .text(text): | |
| placeholders.latest.contents += text | |
| case let .text(text): | |
| if placeholders.isEmpty { | |
| result += text | |
| } else { | |
| placeholders.latest.contents += text | |
| } |
| /// A syntactical element inside a SourceKit snippet. | ||
| private enum SnippetToken { | ||
| /// A placeholder delimiter. | ||
| case open, close |
There was a problem hiding this comment.
I would name them placeholderOpen and placeholderClose. open and close sound pretty ambiguous.
| let input = "foo(bar: \(placeholder: "{ \(placeholder: "T##Int##Int") }"))" | ||
| let rewritten = rewriteSourceKitPlaceholders(in: input, clientSupportsSnippets: true) | ||
|
|
||
| XCTAssertEqual(rewritten, "foo(bar: ${1:\\{ ${2:Int} \\}})") |
There was a problem hiding this comment.
You can use raw string literals so you don’t need to write double backslashes here. I think that would improve readability.
| XCTAssertEqual(rewritten, "foo(bar: ${1:\\{ ${2:Int} \\}})") | |
| XCTAssertEqual(rewritten, #"foo(bar: ${1:\{ ${2:Int} \}})"#) |
| private extension DefaultStringInterpolation { | ||
| mutating func appendInterpolation(placeholder string: String) { | ||
| self.appendLiteral(placeholderStart + string + placeholderEnd) | ||
| } | ||
| } |
There was a problem hiding this comment.
I don’t think I’m a fan of extending string interpolation in this way. I would prefer to spell out the placeholders in the tests as <#…#>. I know that it’s a little annoying when editing the file in Xcode but I think overall I’ve found that it’s easiest to reason about these sorts of tests if you can view the input text as it will returned from sourcekitd, even if that means opening the file in a non-Xcode editor at times.
There was a problem hiding this comment.
No problem; I don't use Xcode much for editing myself, but I figured it would bother other people 🙂 Removed.
| myMap { ${1:Int} in | ||
| ${2:Bool} | ||
| } | ||
| myMap(${1:\\{ ${2:Int} in ${3:Bool} \\}}) |
There was a problem hiding this comment.
Similar to my comment in the other test file, i would use raw string literals here so you don’t need to write \\.
This resolves <swiftlang#1788>, following the discussion of alternatives on <https://github.com/swiftlang/sourcekit-lsp/pulls/1789>. The bulk of the change updates the translation from SourceKit placeholders to LSP placeholders to handle nesting. The `CodeCompletionSession` also passes a new custom formatter to the swift-syntax expansion routine, which disables the transformation to trailing closures.
woolsweater
force-pushed
the
the-trail-is-closed
branch
from
31fa3e9
to
0eb410b
Compare
| /// its body. | ||
| private func isSmallClosureDelimiter( | ||
| _ token: TokenSyntax, | ||
| kind: KeyPath<ClosureExprSyntax, TokenSyntax> |
There was a problem hiding this comment.
Moving swiftlang/swift-syntax#2897 (comment) here
Do we need to pass in the key path here? It doesn’t really look very dynamic, so I’d just check for
\.leftBraceand\.rightBracein here.
It is defensive, but I think it's worthwhile. We don't want to override the BasicFormat for the token before a { or after a }. Passing the keypath guards against those.
| file: StaticString = #filePath, | ||
| line: UInt = #line | ||
| ) { | ||
| XCTAssertEqual(tree.formatted(using: format).description, expected, file: file, line: line) |
There was a problem hiding this comment.
ℹ️ Unfortunately it doesn't seem like _SwiftSyntaxTestSupport can be added to the dependencies, so I had to use plain XCTAssertEqual instead of the nice assertStringsEqualWithDiff.
| /// This is more conservative about newline insertion: unless the closure has | ||
| /// multiple statements in its body it will not be reformatted to multiple | ||
| /// lines. | ||
| class ClosureCompletionFormat: BasicFormat { |
There was a problem hiding this comment.
ℹ️ Moved into SourceKit from the prior revision of my swift-syntax PR, per swiftlang/swift-syntax#2897 (comment)
This resolves #1788, following the discussion of alternatives on https://github.com/swiftlang/sourcekit-lsp/pulls/1789. The bulk of the change updates the translation from SourceKit placeholders to LSP placeholders to handle nesting, which is implemented in swiftlang/swift-syntax#2897
closure-expansion.mov