Add config option for trailing closure rewriting #1789
Conversation
Default to the automatic expansion introduced in 90c124c but allow the user to disable it via a configuration file. This is expressed as an enumeration rather than a boolean flag because there is room for other levels of rewriting. E.g. for trailing closures, a "basic" level might be implemented as rewriting to a pair of brackets on the same line, with a single placeholder, rather than the "full" multi-line behavior.
There was a problem hiding this comment.
Thanks for the PR, @woolsweater. I agree that we are currently using trailing closure syntax for code completion fairly aggressively and that there is a problem to be solved here, but I don’t think that a global configuration option is the right solution here.
I think that whether you want to use a trailing closure depends less on personal preference and more on the context. For example, I think that in almost all cases a user is expecting Task.init to be followed by a trailing closure. For methods like filter it usually depends on whether the condition will be complex and span multiple lines or, as you stated in your issue, if you are intending to use a simple condition or key path.
Thus, I think the user should be given the choice of whether to use a trailing closure completion on every completion. The options I see right now are:
- Offer two different completion options for the trailing closure and the non-trailing closure case. But this might inflate the number of code completion items and make it harder to find the one you are looking for.
- Revert code completions to suggest snippets (like SourceKit-LSP did in 5.10 and earlier) and then offer a code action to expand snippets to trailing closure, similar to how Xcode expands placeholders to trailing closures when you press enter while they are selected. This may be tricky to implement because editors don’t represent snippets in a file’s source text, so SourceKit-LSP doesn’t know if the user is currently at a snippet that was inserted to represent a closure, unless SourceKit-LSP keeps track of that information itself, which I guess it could.
- Make code completion expand snippets to single-line closures but not trailing closures (e.g expand to
filter(where: { ${1:Int} in ${2:Bool} })instead offilter { ${1:Int} in \n${2:Bool}\n }). This would be a compromise from where you can go into either direction: (a) Deleting the closure to replace it with a function reference isn’t too much effort since it only spas a single line, (b) similarly using anonymous closure argument names ($0etc) is fairly straightforward, (c) to go to trailing closure syntax, we already have a code action for that, we would just need to make that refactoring more fault tolerant and be applicable in case there is a closure with snippets. - Continue expanding to trailing closure syntax like we do now and offer a code action to convert trailing closure syntax back to non-trailing closure syntax. The tricky part here is inferring the label for the function call, eg. the where to convert
firstIndex { … }tofirstIndex(where: { … })
While writing this, I think that I’m preferring option (2) or (3). What do you think, @woolsweater?
Thanks @ahoppen! Yes, absolutely; there are cases where it would be very unusual not to use a trailing closure. But I think there are more cases where the language server can't read the user's mind and can't know which it should choose. I agree that (2) and (3) are the best paths forward 🙌 It seems to me that they could mostly coexist, actually. Having a code action to expand a call to trailing closures would be a great thing to have regardless of how the snippet expansion works. I would think the server could get around the "was this a snippet?" problem by simply offering it on any call expression where there is a closure literal present? That would make it useful even when revisiting the code long after the current LSP session is closed. But perhaps that is unworkable for some reason? I saw this PR as a stepping stone to a state like your (3). I do genuinely think there's an element of user preference here: for example, I assume the author of #854 likes the current implementation, and I didn't want to break their experience. So they and anyone else who prefers it can use the full setting and get that behavior. The (new default) medium or basic level would be your (3).¹ And the curmudgeons can turn it off altogether with never. I think it's going to be impossible for the language server to please everyone in this regard. So having a good default with a little adjustability seems ideal. I see that reworking the expansion requires changes to swift-syntax, and I have not gotten a chance to dig into that, but I was planning to do so this coming weekend. I would be happy to try to implement your item (3) as a resolution of #1788 unless you would prefer to do it. ¹ And perhaps there's even a high setting between that and full...but then someone's go to figure out what that means and then implement it, so maybe not 😅 |
|
If you would be interested in implementing (3) to resolve #1788, that would be great. I’m happy to help with any concrete questions you might have. Regarding an option: I would prefer to try and avoid options as long as we don’t have two opinions that fundamentally oppose each other. Every option needs to be tested and with a growing number of configurations, the chance of missing some broken configuration rises. For now, I think that (2) or (3) offer a good middle ground and I would like to explore if they can please the majority before jumping to a configuration parameter. |
Yes, that's fair and understandable. Okay, I will put this idea aside and work on the swift-syntax changes. Is simply changing the behavior of |
SourceKit-LSP is the main client of |
|
Sorry for the delay, @ahoppen ; I did not get to this as soon as I'd hoped. I have the changes just about ready to go now. I will have to make a PR to swift-syntax, to change the expansion behavior, and then one to sourcekit-lsp to adopt the swift-syntax update. Is there a good way for me to make a draft PR to sourcekit-lsp, while the swift-syntax PR is pending, that includes my swift-syntax changes? Should I just change the pin in sourcekit-lsp's Package.resolved to point to my branch, and then update it after the swift-syntax merge? Or do you have another preferred way to handle this? |
Just open a PR to swift-syntax and reference it from this PR. We can do cross-PR testing in CI. |
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 introduce newlines to a closure when there is only a single statement in the body.
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.
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.
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.
|
Okay, I've opened swiftlang/swift-syntax#2897 and #1831 for the change discussed here. |
This would resolve #1787
The default remains the automatic expansion introduced in #1072, but the new option allows the user to disable it via a configuration file.
This is expressed as an enumeration rather than a boolean flag because there is room for other levels of rewriting. E.g. for trailing closures, a "basic" level might be implemented as rewriting to a pair of brackets on the same line, with a single placeholder, rather than the "full" multi-line behavior.
A configuration file like the following will disable the expansion feature:
{ "codeCompletion" : { "rewriteTrailingClosures" : "never" } }