Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refine automatic rewriting of trailing closures #1788

Open
woolsweater opened this issue Oct 27, 2024 · 1 comment · May be fixed by #1831
Open

Refine automatic rewriting of trailing closures #1788

woolsweater opened this issue Oct 27, 2024 · 1 comment · May be fixed by #1831
Labels
enhancement New feature or request

Comments

@woolsweater
Copy link

woolsweater commented Oct 27, 2024

Description

Context

Following from the rationale given in #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 #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
Copy link
Member

ahoppen commented Oct 27, 2024

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

woolsweater added a commit to woolsweater/sourcekit-lsp that referenced this issue Nov 17, 2024
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.
woolsweater added a commit to woolsweater/swift-syntax that referenced this issue Nov 17, 2024
This addresses <swiftlang/sourcekit-lsp#1788>.

Per <swiftlang/sourcekit-lsp#1789>, these
placeholders no longer expand to multi-line trailing closures. Instead
they become inline closures, with nested placeholders for the signature
and body.

This introduces a new formatter, `ClosureLiteralFormat`, so that other
uses of `BasicFormat` are not impacted. `ClosureLiteralFormat` does not
insert newlines into a closure when there is only a single statement in
the body.
woolsweater added a commit to woolsweater/swift-syntax that referenced this issue Nov 17, 2024
This addresses <swiftlang/sourcekit-lsp#1788>.

Per <swiftlang/sourcekit-lsp#1789>, these
placeholders no longer expand to multi-line trailing closures. Instead
they become inline closures, with nested placeholders for the signature
and body.

This introduces a new formatter, `ClosureLiteralFormat`, so that other
uses of `BasicFormat` are not impacted. `ClosureLiteralFormat` does not
insert newlines into a closure when there is only a single statement in
the body.
woolsweater added a commit to woolsweater/sourcekit-lsp that referenced this issue Nov 17, 2024
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.
woolsweater added a commit to woolsweater/sourcekit-lsp that referenced this issue Nov 17, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants