Handle new swift-syntax closure expansion behavior #1831

woolsweater · 2024-11-17T01:18:32Z

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

Tests/SourceKitLSPTests/RewriteSourceKitPlaceholdersTests.swift

ahoppen

Thank you @woolsweater, great changes!

I only have a few comments, otherwise looks great.

Tests/SourceKitLSPTests/RewriteSourceKitPlaceholdersTests.swift

Sources/SourceKitLSP/Swift/RewriteSourceKitPlaceholders.swift

Tests/SourceKitLSPTests/RewriteSourceKitPlaceholdersTests.swift

Tests/SourceKitLSPTests/SwiftCompletionTests.swift

Sources/SourceKitLSP/Swift/ClosureCompletionFormat.swift

Tests/SourceKitLSPTests/ClosureCompletionFormatTests.swift

Sources/SourceKitLSP/Swift/ClosureCompletionFormat.swift

ahoppen

Thanks. I just have two minor comments, otherwise this looks great.

ahoppen · 2024-12-03T17:12:59Z

Sources/SourceKitLSP/Swift/ClosureCompletionFormat.swift

+      return false
+    }
+
+    return token.keyPathInParent == \ClosureSignatureSyntax.inKeyword


I would move the check for token.keyPathInParent == \ClosureSignatureSyntax.inKeyword to the top of the function because it’s an O(1) check vs the ancestorOrSelf check, which needs to traverse the tree up to the root to determine that a token is not in a ClosureExprSyntax.

Good point; done!

ahoppen · 2024-12-03T17:19:20Z

Tests/SourceKitLSPTests/ClosureCompletionFormatTests.swift

+import SwiftParser
+import SwiftSyntax
+import Swift
+@testable import SourceKitLSP


We build SourceKit-LSP with testability disabled in Swift CI because otherwise we would need to build everything twice: Once with testability enabled for testing and then with testability disabled to produce the binary we include in the toolchain. Could you change this to not use @testable? We use @_spi(Testing) to a similar effect where needed.

Got it, updated. 🤔 I am not very familiar with how @_spi works, but it seems to require everything including the imports to become public. Let me know if there is another way I should have done this.

ahoppen

Thanks. A small comment that you couldn’t have been aware of regarding the imports.

ahoppen · 2024-12-04T04:24:01Z

Sources/SourceKitLSP/Swift/ClosureCompletionFormat.swift

+public import SwiftSyntax
+public import SwiftBasicFormat


We need to support building with a Swift 5.10 compiler as well, which doesn’t support the access level imports.

Suggested change

public import SwiftSyntax

public import SwiftBasicFormat

#if compiler(>=6.0)

public import SwiftSyntax

public import SwiftBasicFormat

#else

import SwiftSyntax

import SwiftBasicFormat

#endif

ahoppen · 2024-12-04T04:24:59Z

Sources/SourceKitLSP/Swift/ClosureCompletionFormat.swift

+/// lines.
+@_spi(Testing)
+public class ClosureCompletionFormat: BasicFormat {
+  public override func requiresNewline(between first: TokenSyntax?, and second: TokenSyntax?) -> Bool {


I would add @_spi(Testing) here as well, just to be clear that it’s not a properly public declaration.

Suggested change

public override func requiresNewline(between first: TokenSyntax?, and second: TokenSyntax?) -> Bool {

@_spi(Testing) public override func requiresNewline(between first: TokenSyntax?, and second: TokenSyntax?) -> Bool {

Makes sense; done!

ahoppen

Looking good. Thanks for implementing this and following up on my review comments.

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

swiftlang/swift-syntax#2897

@swift-ci Please test

woolsweater · 2024-12-04T16:52:55Z

Tests/SourceKitLSPTests/RewriteSourceKitPlaceholdersTests.swift

+//===----------------------------------------------------------------------===//
+
+import SKTestSupport
+@testable import SourceKitLSP


Shoot, I missed this other @testable import last night. Does this need to be changed as well @ahoppen ?

Yes, we can’t have @testable imports.

ahoppen · 2024-12-04T17:39:52Z

swiftlang/swift-syntax#2897

@swift-ci Please test

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

swiftlang/swift-syntax#2897

@swift-ci Please test

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

swiftlang/swift-syntax#2897

@swift-ci Please test Windows

woolsweater · 2024-12-05T16:46:31Z

Updated just now to appease the linter.

ahoppen · 2024-12-05T16:53:02Z

swiftlang/swift-syntax#2897

@swift-ci Please test

ahoppen · 2024-12-05T16:54:20Z

Sources/SourceKitLSP/Swift/ClosureCompletionFormat.swift

Could you add this file to CMakeLists.txt. We use CMake to build SourceKit-LSP on Windows.

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.

ahoppen · 2024-12-05T18:44:09Z

swiftlang/swift-syntax#2897

@swift-ci Please test

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

swiftlang/swift-syntax#2897

@swift-ci Please test Windows

woolsweater · 2024-12-06T01:08:19Z

Thank you for shepherding these changes, @ahoppen ! 🙌

…is-closed" This reverts commit 21dfaf0, reversing changes made to f900b4e.

woolsweater requested a review from ahoppen as a code owner November 17, 2024 01:18

woolsweater mentioned this pull request Nov 17, 2024

Allow customization of expansion for function-typed placeholders swiftlang/swift-syntax#2897

Merged

woolsweater commented Nov 17, 2024

View reviewed changes

Tests/SourceKitLSPTests/RewriteSourceKitPlaceholdersTests.swift Outdated Show resolved Hide resolved

woolsweater force-pushed the the-trail-is-closed branch 2 times, most recently from 319f8bf to 31fa3e9 Compare November 17, 2024 01:30

woolsweater mentioned this pull request Nov 17, 2024

Add config option for trailing closure rewriting #1789

Closed

ahoppen reviewed Nov 18, 2024

View reviewed changes

woolsweater force-pushed the the-trail-is-closed branch from 31fa3e9 to 0eb410b Compare November 25, 2024 19:25

woolsweater commented Nov 25, 2024

View reviewed changes

Sources/SourceKitLSP/Swift/ClosureCompletionFormat.swift Show resolved Hide resolved

Tests/SourceKitLSPTests/ClosureCompletionFormatTests.swift Show resolved Hide resolved

Sources/SourceKitLSP/Swift/ClosureCompletionFormat.swift Outdated Show resolved Hide resolved

ahoppen reviewed Dec 3, 2024

View reviewed changes

woolsweater force-pushed the the-trail-is-closed branch from 0eb410b to f81206e Compare December 4, 2024 02:43

ahoppen reviewed Dec 4, 2024

View reviewed changes

woolsweater force-pushed the the-trail-is-closed branch from f81206e to 92a674e Compare December 4, 2024 05:25

ahoppen reviewed Dec 4, 2024

View reviewed changes

woolsweater commented Dec 4, 2024

View reviewed changes

woolsweater force-pushed the the-trail-is-closed branch from 92a674e to be3eeda Compare December 4, 2024 17:23

woolsweater force-pushed the the-trail-is-closed branch from be3eeda to 58ecdcc Compare December 5, 2024 00:33

woolsweater force-pushed the the-trail-is-closed branch from 58ecdcc to a714d41 Compare December 5, 2024 16:46

ahoppen reviewed Dec 5, 2024

View reviewed changes

woolsweater force-pushed the the-trail-is-closed branch from a714d41 to 60beed8 Compare December 5, 2024 16:57

ahoppen merged commit 21dfaf0 into swiftlang:main Dec 6, 2024
3 checks passed

ahoppen mentioned this pull request Dec 6, 2024

Don’t escape { inside placeholder snippets #1870

Merged

ahoppen added a commit to ahoppen/sourcekit-lsp that referenced this pull request Dec 10, 2024

Revert "Merge pull request swiftlang#1831 from woolsweater/the-trail-…

fd79c3d

…is-closed" This reverts commit 21dfaf0, reversing changes made to f900b4e.

ahoppen mentioned this pull request Dec 10, 2024

Merge main into release/6.1 #1879

Merged

ahoppen mentioned this pull request Jan 12, 2025

Configuration option for auto-rewrite of trailing closures #1787

Closed

	public override func requiresNewline(between first: TokenSyntax?, and second: TokenSyntax?) -> Bool {
	@_spi(Testing) public override func requiresNewline(between first: TokenSyntax?, and second: TokenSyntax?) -> Bool {

Handle new swift-syntax closure expansion behavior #1831

Handle new swift-syntax closure expansion behavior #1831

Uh oh!

Conversation

woolsweater commented Nov 17, 2024

Uh oh!

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Dec 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Dec 4, 2024

Uh oh!

ahoppen commented Dec 5, 2024

Uh oh!

ahoppen commented Dec 5, 2024

Uh oh!

woolsweater commented Dec 5, 2024

Uh oh!

ahoppen commented Dec 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Dec 5, 2024

Uh oh!

ahoppen commented Dec 5, 2024

Uh oh!

Uh oh!

woolsweater commented Dec 6, 2024

Uh oh!

Uh oh!