swiftlang / sourcekit-lsp

Language Server Protocol implementation for Swift and C-based languages
Apache License 2.0
3.31k stars 276 forks source link

Refine automatic rewriting of trailing closures #1788

Open woolsweater opened 3 hours ago

woolsweater commented 3 hours ago

Description

Context

Following from the rationale given in https://github.com/swiftlang/sourcekit-lsp/issues/1787, whether to expand a trailing closure is not a syntactical decision; it depends on the function's semantics. As discussed, predicate-based functions (map, filter) are one case where an expansion is often inappropriate. Since the introduction of async functions we can also see callback-passing style become less common/prominent altogether, meaning that the relative proportion of predicate-like calls is increasing.

In the case of multiple closures, it is again a stylistic choice whether to expand all of them, or only some. Sometimes only the last should be expanded; e.g., this is a perfectly common style in SwiftUI code:

Button(action: { self.viewModel.makeItDo() }) {
  Text("Tap me!")
    .other(.modifiers)
    .etc(.etc)
}

Proposed Change

Since the expansion logic has limited or no knowledge of a function's semantics, I believe that it should be made much less aggressive. By default, it should always produce a closure on the same line as the call, with a single placeholder inside the closure: i.e. completion for a call to func foo(bar: Bar, baz: @escaping () -> Void) should expand to:

foo(baz: <#Baz#>) { <##> }

This is a much better experience for the user in the case where they did not want a multi-line expansion, while preserving most of the benefits of the current behavior from https://github.com/swiftlang/sourcekit-lsp/pull/1072. A "full expansion" configuration setting could opt the user in to current behavior.

A further refinement might be to use an allowlist for known "callback-like" methods, and produce the multi-line expansion only for them. But since users can always define their own methods with trailing closures, and the LSP server has no way to distinguish the semantics of those cases, the default should remain the single-line form.

ahoppen commented 3 hours ago

Synced to Apple’s issue tracker as rdar://138732066