search

swiftlang / vscode-swift

Visual Studio Code Extension for Swift
https://marketplace.visualstudio.com/items?itemName=sswg.swift-lang
Apache License 2.0
759 stars 54 forks source link

Formatter situation? #1190

Open MahdiBM opened 3 weeks ago

MahdiBM commented 3 weeks ago

What are the plans about providing a formatter in this repo now that swift-format is bundled with the toolchains?

Currently it this extension does show up as a formatter but apparently it just triggers prettier? EDIT: that's not true, see below.

This would look like a bit unfair for SwiftFormat (Nick Lockwood's) which is what we currently use, since swift-format can be hard to bear with at times ... but it still makes sense since swift-format is part of the toolchain.

matthewbastien commented 3 weeks ago

From a discussion in the Swift Open Source slack workspace: https://swift-open-source.slack.com/archives/C02PV8T5HQD/p1730480017329059

The Swift extension is getting formatting "for free" right now from SourceKit-LSP's implementation of the textDocument/formatting request which VS Code finds in the server capabilities. However, right now SourceKit-LSP only supports formatting a whole file and not a selection range. It could be implemented on the SourceKit-LSP side, but waiting for SourceKit-LSP to support this would mean that it'd only work in Swift >=6.1 (or whenever a new SourceKit-LSP happens to get released) despite the fact that range formatting is available with swift-format right now.

It might be worthwhile to have the extension call swift-format directly now that it's in the toolchain rather than call out to SourceKit-LSP for formatting. Some investigation is needed here.

plemarquand commented 3 weeks ago

I'd love to have range based formatting. We could use it on the built in snippets and comment blocks after they're inserted so they can be more seamlessly provided to match the existing formatting.

MahdiBM commented 3 weeks ago

@plemarquand I could work something out, formatting is one the weakest points of VSCode and kinds of pushes me away from fully moving to VSCode, so I'll be happy to contribute something to make the formatting lives of everyone easier.

MahdiBM commented 3 weeks ago

Let me know if there are any special concerns you'd have with that 🙂

adam-fowler commented 3 weeks ago

You sure it doesn't trigger swift-format. The formatter appears because sourcekit-lsp says it can format the code.

MahdiBM commented 3 weeks ago

@adam-fowler no I wasn't sure, I just briefly tried it and saw prettier triggered which could have been due to some mis-config on my part.

MahdiBM commented 3 weeks ago

Just put a PR together, though I have yet to locally test it so you shouldn't bother testing it until my next notice hopefully.

MahdiBM commented 3 weeks ago

Worth noting I took a look at how rust registers a formatting provider. Apparently this is how range formatting is implemented in rust-analyzer. It appeared that they are somehow using rust itself the implement the whole thing (?), so we can't go their way, at least not with how the project is currently set up.

adam-fowler commented 3 weeks ago

The formatter is available via SourceKit-LSP (select Swift when asked to select a formatter). I have verified this by adding a .swift-format file to a project and verified changes to it affect how the file is formatted.

Adding support via the extension is unnecessary

MahdiBM commented 3 weeks ago

@adam-fowler like we discussed, we'd want to have full support for formatting. Like format-on-type and selection-formatting.

Does the current thing support these? If not, how should we proceed then? Rely on sourcekit-lsp and add these supports to it, or just use the toolchain's swift-format like the PR I put together?

I think one concern was that it might take a bit for things to be merged into sourcekit-lsp then end up in the toolchains.

adam-fowler commented 3 weeks ago

@adam-fowler like we discussed, we'd want to have full support for formatting. Like format-on-type and selection-formatting.

swift-format doesn't support formatting on range, as it needs to load the full swift syntax tree. So sourcekit-lsp won't either. Having it called directly won't make it any quicker to implement

I don't know what you mean by selection-formatting

MahdiBM commented 3 weeks ago

@adam-fowler I noticed range-formatting / selection-formatting was implemented in swift-format a while ago. Might not be perfect but yeah.

Not sure about sourcekit-lsp and how it works though.

MahdiBM commented 3 weeks ago

I'm not looking to make things faster. I want to be able to use swift-format without needing to format whole files of a project which i don't maintain.

adam-fowler commented 3 weeks ago

Perhaps @ahoppen can enlighten us to what the current situation is with formatting of ranges and access to it via LSP command textDocument.rangeFormatting

ahoppen commented 3 weeks ago

Implementing textDocument/rangeFormatting shouldn’t be too hard now that swift-format supports range formatting. The only caveat is that swift-format is not able to format files with syntax errors, so as long as the file has a syntax error, range formatting won’t work either.

MahdiBM commented 3 weeks ago

@ahoppen should I create an issue in the sourcekit-lsp / swift repo? If yes, which one?

ahoppen commented 3 weeks ago

An issue in https://github.com/swiftlang/sourcekit-lsp would be great.

MahdiBM commented 2 weeks ago

@ahoppen issue filed: https://github.com/swiftlang/sourcekit-lsp/issues/1805

I agree that the proper place for this would be right in sourcekit-lsp, although it might take a bit to reach end users like me. I'll be closing the half-implemented PR I did.

MahdiBM commented 3 days ago

Thanks to @ahoppen we have 2 merged PRs for selection/range-formatting and on-type formatting.

Considering on-type formatting is currently hidden behind an experimental flag, perhaps we should add support for it to this VS Code extension.

Unrelated but i think swift.sourcekit-lsp.backgroundIndexing should also be marked as non-experimental for Swift 6.1 (somehow?) considering the recent changes in sourcekit-lsp: Enable background indexing by default.

MahdiBM commented 3 days ago

@ahoppen I notice sourcekit-lsp has branched for Swift 6.1 since a few days ago. Optimally we can have range and on-type formatting make it to the Swift 6.1 cut. What do you think?

ahoppen commented 3 days ago

Considering on-type formatting is currently hidden behind an experimental flag, perhaps we should add support for it to this VS Code extension.

I think it’s fine for now that users enable it through the SourceKit-LSP configuration file.

I notice sourcekit-lsp has branched for Swift 6.1 since a few days ago. Optimally we can have range and on-type formatting make it to the Swift 6.1 cut. What do you think?

I’m still thinking about how to handle cherry-picking this release but I think the change will make it into release/6.1.