Add config option for trailing closure rewriting

[woolsweater]

This would resolve https://github.com/swiftlang/sourcekit-lsp/issues/1787

The default remains the automatic expansion introduced in https://github.com/swiftlang/sourcekit-lsp/pull/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"
  }
}

[woolsweater]

depends […] 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.

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 https://github.com/swiftlang/sourcekit-lsp/issues/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 https://github.com/swiftlang/sourcekit-lsp/issues/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 😅

[ahoppen]

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.

[woolsweater]

Every option needs to be tested and with a growing number of configurations…

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 ExpandEditorPlaceholdersToTrailingClosures the right approach? Are there any other clients of it besides sourcekit-lsp?

[ahoppen]

Yes, that's fair and understandable. Okay, I will put this idea aside and work on the swift-syntax changes. Thank you!

Is simply changing the behavior of ExpandEditorPlaceholdersToTrailingClosures the right approach? Are there any other clients of it besides sourcekit-lsp?

SourceKit-LSP is the main client of ExpandEditorPlaceholdersToTrailingClosures and I assume that any changes to it will also benefit any other clients that might exist.

[woolsweater]

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?

[ahoppen]

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.

[woolsweater]

Okay, I've opened https://github.com/swiftlang/swift-syntax/pull/2897 and https://github.com/swiftlang/sourcekit-lsp/pull/1831 for the change discussed here.