Allow customization of expansion for function-typed placeholders #2897

woolsweater · 2024-11-17T01:01:03Z

This addresses swiftlang/sourcekit-lsp#1788, refining code completion for closure placeholders.

By default function-typed placeholders will continue to expand to multi-line trailing form. A caller, such as sourcekit-lsp, may now customize the behavior by passing its own formatter. It may additionally request that the closure itself be marked as a placeholder, with the argument and return type placeholders nested inside.

woolsweater · 2024-11-17T01:01:51Z

Sources/SwiftBasicFormat/ClosureLiteralFormat.swift

+///
+/// A closure that seems to be a simple predicate or transform — based on the
+/// number of enclosed statements — will not be reformatted to multiple lines.
+open class ClosureLiteralFormat: BasicFormat {


ℹ️ I wasn't confident that tweaking BasicFormat directly wouldn't break something else, so I opted to introduce a new formatter. It's easy enough to roll this into BasicFormat if preferred.

I think this is exactly the right choice. That’s why BasicFormat is an open class.

woolsweater · 2024-11-17T01:04:14Z

Sources/SwiftRefactor/ExpandEditorPlaceholder.swift

      // Strip the initial indentation from the placeholder itself. We only introduced the initial indentation to
      // format consecutive lines. We don't want it at the front of the initial line because it replaces an expression
      // that might be in the middle of a line.
      if formattedExpansion.hasPrefix(context.initialIndentation.description) {
        formattedExpansion = String(formattedExpansion.dropFirst(context.initialIndentation.description.count))
      }
-      expanded = formattedExpansion
+      expanded = wrapInPlaceholder(formattedExpansion)


ℹ️ @ahoppen This is an extension of your original suggestion. I think wrapping the whole closure in a placeholder, with inner placeholders for the args and body, provides an optimal UX: you can immediately delete/replace the entire thing or you can tab to the constituent parts. This does necessitate a change in sourcekit-lsp's placeholder translation routine to handle the nesting; this is done in swiftlang/sourcekit-lsp#1831, which also has a screen recording of the new behavior.

Unfortunately, we can’t do that. The Swift compiler and Xcode don’t support nested editor placeholders using the <# … #> syntax. But I do agree that wrapping the closure in a placeholder again makes sense in the context of automatic SourceKit-LSP expansions. Do you think we could do the re-wrapping on the SourceKit-LSP side?

Darn, I was afraid that might be the case. Okay, following this and your other comments, I'll figure out how to move the bulk of the behavior change into sourcekit-lsp.

I don't see a (good) way to move the nested wrapping into SourceKit-LSP; we'd essentially have to re-do most of this work: re-parsing the expansion and finding the closure body to wrap. I have added a flag to the Context to control this. It is off by default and CodeCompletionSession will set it for its own expansion requests.

Sources/SwiftSyntax/SyntaxProtocol.swift

woolsweater · 2024-11-17T01:08:41Z

Tests/SwiftRefactorTest/ExpandEditorPlaceholderTests.swift

@@ -18,6 +18,12 @@ import SwiftSyntaxBuilder
 import XCTest
 import _SwiftSyntaxTestSupport

+fileprivate extension DefaultStringInterpolation {
+  mutating func appendInterpolation(placeholder string: String) {
+    self.appendLiteral(wrapInPlaceholder(string))


ℹ️ Helper to keep literal <# and #> delimiters out of the test assertions.

@ahoppen I assume you would prefer I remove this, per your comment on the same method in the sourcekit-lsp PR?

Yes, correct.

ahoppen

Thanks for the changes @woolsweater. I don’t want to change the general behavior of the refactorings because there are applications where the current behavior makes sense, so we’d need a way to only get the modified behavior for SourceKit-LSP.

ahoppen · 2024-11-18T23:58:10Z

Sources/SwiftBasicFormat/ClosureLiteralFormat.swift

+///
+/// A closure that seems to be a simple predicate or transform — based on the
+/// number of enclosed statements — will not be reformatted to multiple lines.
+open class ClosureLiteralFormat: BasicFormat {


I think this is exactly the right choice. That’s why BasicFormat is an open class.

Sources/SwiftBasicFormat/ClosureLiteralFormat.swift

ahoppen · 2024-11-19T00:01:54Z

Sources/SwiftBasicFormat/ClosureLiteralFormat.swift

+  /// Returns `true` if `token` is the last token in the signature of a closure,
+  /// and that closure has no more than one statement in its body.
+  private func isEndOfSmallClosureSignature(_ token: TokenSyntax) -> Bool {
+    guard let signature = token.ancestorOrSelf(mapping: { $0.as(ClosureSignatureSyntax.self) }),


Do we need to generically look for the last token in the closure signature here or could we just look for the in keyword that terminates the signature?

That seems fine; done! (This code has moved to sourcekit-lsp.)

ahoppen · 2024-11-19T00:05:24Z

Sources/SwiftRefactor/ExpandEditorPlaceholder.swift

-/// { someInt in
-///   <#T##String#>
-/// }
+/// <#{ <#someInt#> in <#T##String#> }#>


If we expand the editor placeholder, we shouldn’t end up with an editor placeholder again.

Oops, reverted!

ahoppen · 2024-11-19T00:08:30Z

Sources/SwiftRefactor/ExpandEditorPlaceholder.swift

      // Strip the initial indentation from the placeholder itself. We only introduced the initial indentation to
      // format consecutive lines. We don't want it at the front of the initial line because it replaces an expression
      // that might be in the middle of a line.
      if formattedExpansion.hasPrefix(context.initialIndentation.description) {
        formattedExpansion = String(formattedExpansion.dropFirst(context.initialIndentation.description.count))
      }
-      expanded = formattedExpansion
+      expanded = wrapInPlaceholder(formattedExpansion)


Unfortunately, we can’t do that. The Swift compiler and Xcode don’t support nested editor placeholders using the <# … #> syntax. But I do agree that wrapping the closure in a placeholder again makes sense in the context of automatic SourceKit-LSP expansions. Do you think we could do the re-wrapping on the SourceKit-LSP side?

ahoppen · 2024-11-19T00:09:35Z

Sources/SwiftRefactor/ExpandEditorPlaceholder.swift

@@ -94,18 +92,18 @@ struct ExpandSingleEditorPlaceholder: EditRefactoringProvider {

    let expanded: String
    if let functionType = placeholder.typeForExpansion?.as(FunctionTypeSyntax.self) {
-      let basicFormat = BasicFormat(
+      let format = ClosureLiteralFormat(


Could we make the format that’s used to expand the editor placeholder an option that still defaults to BasicFormat? For example, when you explicitly run the refactoring to expand a placeholder to a trailing closure, I think it makes sense to have the multi-line behavior. Then we could just override it with ClosureLiteralFormat for SourceKit-LSP code completion expansion. That way ClosureLiteralFormat could also be specified in SourceKit-LSP and wouldn’t become public API of swift-syntax.

ahoppen · 2024-11-19T00:11:11Z

Sources/SwiftRefactor/ExpandEditorPlaceholder.swift

+///   closure2: { ... },
+///   closure3: { <#someInt#> in <#T##String#> },
+///   closure4: { <#someInt#> in <#T##String#> }
+/// )


Similar here, I think that the general ExpandSingleEditorPlaceholder refactoring should produce trailing closures, we just want to tweak the behavior from SourceKit-LSP. Maybe we need an option for that.

woolsweater

Thank you for the prompt reviews here and in sourcekit-lsp, @ahoppen! I will work on the revisions you've proposed.

woolsweater · 2024-11-19T02:03:21Z

Tests/SwiftRefactorTest/ExpandEditorPlaceholderTests.swift

@@ -18,6 +18,12 @@ import SwiftSyntaxBuilder
 import XCTest
 import _SwiftSyntaxTestSupport

+fileprivate extension DefaultStringInterpolation {
+  mutating func appendInterpolation(placeholder string: String) {
+    self.appendLiteral(wrapInPlaceholder(string))


@ahoppen I assume you would prefer I remove this, per your comment on the same method in the sourcekit-lsp PR?

woolsweater · 2024-11-19T02:13:57Z

Sources/SwiftRefactor/ExpandEditorPlaceholder.swift

      // Strip the initial indentation from the placeholder itself. We only introduced the initial indentation to
      // format consecutive lines. We don't want it at the front of the initial line because it replaces an expression
      // that might be in the middle of a line.
      if formattedExpansion.hasPrefix(context.initialIndentation.description) {
        formattedExpansion = String(formattedExpansion.dropFirst(context.initialIndentation.description.count))
      }
-      expanded = formattedExpansion
+      expanded = wrapInPlaceholder(formattedExpansion)


Darn, I was afraid that might be the case. Okay, following this and your other comments, I'll figure out how to move the bulk of the behavior change into sourcekit-lsp.

ahoppen

Nice. Thank you, @woolsweater. Just a couple minor comments.

ahoppen · 2024-12-03T17:27:45Z

Sources/SwiftRefactor/ExpandEditorPlaceholder.swift

-      self.indentationWidth = indentationWidth
-      self.initialIndentation = initialIndentation
+    let closureLiteralFormat: BasicFormat
+    let allowNestedPlaceholders: Bool


Could you add a short doc comment what allowNestedPlaceholders does, ideally with a short example. I think that would be useful in the future.

Absolutely; done!

ahoppen · 2024-12-03T17:29:51Z

Sources/SwiftRefactor/ExpandEditorPlaceholder.swift

-public struct ExpandEditorPlaceholdersToTrailingClosures: SyntaxRefactoringProvider {
+public struct ExpandEditorPlaceholdersToLiteralClosures: SyntaxRefactoringProvider {


Because this is a change to a public API, we should have a release note entry for it. Could you create a Release Notes/602.md file and add the change to it?

ahoppen

Thanks for addressing my last comments. Two more quick follow-ups. We’re really close.

ahoppen · 2024-12-04T04:19:42Z

Release Notes/602.md

Sorry, I should have maybe been a little more specific. Could you also include the other headings and the template? Ie. this file should look as follows before adding your entry:

# Swift Syntax 601 Release Notes ## New APIs ## API Behavior Changes ## Deprecations ## Template - *Affected API or two word description* - Description: *A 1-2 sentence description of the new/modified API* - Issue: *If an issue exists for this change, a link to the issue* - Pull Request: *Link to the pull request(s) that introduces this change* - Migration steps: Steps that adopters of swift-syntax should take to move to the new API (required for deprecations and API-incompatible changes). - Notes: *In case of deprecations or API-incompatible changes, the reason why this change was made and the suggested alternative* *Insert entries in chronological order, with newer entries at the bottom*

No problem; done!

ahoppen · 2024-12-04T04:21:22Z

Sources/SwiftRefactor/ExpandEditorPlaceholder.swift

+    /// ## With nested placeholders allowed
+    /// ### Before
+    /// ```swift
+    /// <#T##(Int) -> String##(Int) -> String##(_ someInt: Int) -> String#>
+    /// ```
+    ///
+    /// ### After
+    /// ```swift
+    /// <#{ someInt in <#String#> }#>
+    /// ```


I’m not sure Before and After are the correct headings here. I would suggest

Suggested change

/// ## With nested placeholders allowed

/// ### Before

/// ```swift

/// <#T##(Int) -> String##(Int) -> String##(_ someInt: Int) -> String#>

/// ```

///

/// ### After

/// ```swift

/// <#{ someInt in <#String#> }#>

/// ```

/// With `allowNestedPlaceholders = false`

/// ```swift

/// <#T##(Int) -> String##(Int) -> String##(_ someInt: Int) -> String#>

/// ```

///

/// With `allowNestedPlaceholders = true`

/// ```swift

/// <#{ someInt in <#String#> }#>

/// ```

Sure, was following the wording from the doc comment above this; changed!

ahoppen · 2024-12-04T14:54:45Z

swiftlang/sourcekit-lsp#1831

@swift-ci Please test

ahoppen · 2024-12-04T17:40:01Z

swiftlang/sourcekit-lsp#1831

@swift-ci Please test

ahoppen · 2024-12-04T21:43:00Z

Looks like CI is failing for two reasons:

Could you format your changes using swift-format
Compilation of SourceKit-LSP is failing with (+ maybe other similar issues, I didn’t scan the entire log)

/home/build-user/sourcekit-lsp/Tests/SourceKitLSPTests/ClosureCompletionFormatTests.swift:120:62: error: initializer 'init(integerLiteral:)' is not available due to missing import of defining module 'SwiftSyntaxBuilder'
 17 | @_spi(Testing) import SourceKitLSP
 18 | import XCTest
 19 | 
    | `- note: add import of module 'SwiftSyntaxBuilder'
 20 | fileprivate func assertFormatted<T: SyntaxProtocol>(
 21 |   tree: T,
    :
118 |       tree: ClosureExprSyntax(
119 |         statements: CodeBlockItemListSyntax([
120 |           CodeBlockItemSyntax(item: CodeBlockItemSyntax.Item(IntegerLiteralExprSyntax(integerLiteral: 2)))
    |                                                              `- error: initializer 'init(integerLiteral:)' is not available due to missing import of defining module 'SwiftSyntaxBuilder'
121 |         ])
122 |       ),

This addresses <swiftlang/sourcekit-lsp#1788>, refining code completion for closure placeholders. By default function-typed placeholders will continue to expand to multi-line trailing form. A caller, such as sourcekit-lsp, may now customize the behavior by passing its own formatter. It may additionally request that the closure itself be marked as a placeholder, with the argument and return type placeholders nested inside.

woolsweater · 2024-12-05T00:36:47Z

Could you format your changes using swift-format

Oops, had a stray empty line, sorry! Fixed.

Compilation of SourceKit-LSP is failing with (+ maybe other similar issues, I didn’t scan the entire log)
/home/build-user/sourcekit-lsp/Tests/SourceKitLSPTests/ClosureCompletionFormatTests.swift:120:62: error: initializer 'init(integerLiteral:)' is not available due to missing import of defining module 'SwiftSyntaxBuilder'

Huh, is import behavior different on Linux? I only have a Mac to test on. Well, I have added that import and that's the only error I see in the logs.

ahoppen · 2024-12-05T00:39:56Z

Huh, is import behavior different on Linux? I only have a Mac to test on. Well, I have added that import and that's the only error I see in the logs.

I think it’s because the diagnostic is created by the MemberImportVisibility upcoming feature and your local compiler probably doesn’t have it (I can’t remember whether it was introduced by 6.0 or 6.1).

ahoppen · 2024-12-05T00:40:42Z

swiftlang/sourcekit-lsp#1831

@swift-ci Please test

woolsweater · 2024-12-05T00:59:40Z

Huh, is import behavior different on Linux? I only have a Mac to test on. Well, I have added that import and that's the only error I see in the logs.

I think it’s because the diagnostic is created by the MemberImportVisibility upcoming feature and your local compiler probably doesn’t have it (I can’t remember whether it was introduced by 6.0 or 6.1).

Ah, I see, thanks. I am an Xcode point release or two behind the latest 🙂

ahoppen · 2024-12-05T01:02:06Z

swiftlang/sourcekit-lsp#1831

@swift-ci Please test Windows

ahoppen · 2024-12-05T20:43:02Z

swiftlang/sourcekit-lsp#1831

@swift-ci Please test Windows

woolsweater requested review from ahoppen and bnbarham as code owners November 17, 2024 01:01

woolsweater mentioned this pull request Nov 17, 2024

Handle new swift-syntax closure expansion behavior swiftlang/sourcekit-lsp#1831

Merged

woolsweater commented Nov 17, 2024

View reviewed changes

woolsweater mentioned this pull request Nov 17, 2024

Add config option for trailing closure rewriting swiftlang/sourcekit-lsp#1789

Closed

ahoppen reviewed Nov 19, 2024

View reviewed changes

woolsweater commented Nov 19, 2024

View reviewed changes

woolsweater force-pushed the the-trail-is-closed branch from d079e11 to 130a224 Compare November 25, 2024 19:24

woolsweater changed the title ~~Refine expansion of function-typed placeholders~~ Allow customization of expansion for function-typed placeholders Nov 25, 2024

ahoppen reviewed Dec 3, 2024

View reviewed changes

woolsweater force-pushed the the-trail-is-closed branch from 130a224 to 8c947dc Compare December 4, 2024 02:45

ahoppen reviewed Dec 4, 2024

View reviewed changes

woolsweater force-pushed the the-trail-is-closed branch from 8c947dc to 184115f Compare December 4, 2024 05:41

ahoppen approved these changes Dec 4, 2024

View reviewed changes

woolsweater force-pushed the the-trail-is-closed branch from 184115f to 2c96735 Compare December 5, 2024 00:33

ahoppen merged commit 507f323 into swiftlang:main Dec 6, 2024
3 checks passed

		public struct ExpandEditorPlaceholdersToTrailingClosures: SyntaxRefactoringProvider {
		public struct ExpandEditorPlaceholdersToLiteralClosures: SyntaxRefactoringProvider {

Allow customization of expansion for function-typed placeholders #2897

Allow customization of expansion for function-typed placeholders #2897

Conversation

woolsweater commented Nov 17, 2024 • edited Loading

woolsweater Nov 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woolsweater Nov 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahoppen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woolsweater Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woolsweater left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahoppen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahoppen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahoppen commented Dec 4, 2024

ahoppen commented Dec 4, 2024

ahoppen commented Dec 4, 2024

woolsweater commented Dec 5, 2024

ahoppen commented Dec 5, 2024

ahoppen commented Dec 5, 2024

woolsweater commented Dec 5, 2024

ahoppen commented Dec 5, 2024

ahoppen commented Dec 5, 2024

woolsweater commented Nov 17, 2024 •

edited

Loading

woolsweater Nov 17, 2024 •

edited

Loading

woolsweater Nov 17, 2024 •

edited

Loading

woolsweater Nov 25, 2024 •

edited

Loading