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

Formatter situation? #1190

Open
MahdiBM opened this issue Nov 1, 2024 · 23 comments
Open

Formatter situation? #1190

MahdiBM opened this issue Nov 1, 2024 · 23 comments
Labels
enhancement New feature or request sourcekit-lsp SourceKit-LSP issue

Comments

@MahdiBM
Copy link
Contributor

MahdiBM commented Nov 1, 2024

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor Author

MahdiBM commented Nov 1, 2024

@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
Copy link
Contributor Author

MahdiBM commented Nov 1, 2024

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

@adam-fowler
Copy link
Contributor

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

@MahdiBM
Copy link
Contributor Author

MahdiBM commented Nov 1, 2024

@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
Copy link
Contributor Author

MahdiBM commented Nov 3, 2024

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
Copy link
Contributor Author

MahdiBM commented Nov 3, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

MahdiBM commented Nov 4, 2024

@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
Copy link
Contributor

@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
Copy link
Contributor Author

MahdiBM commented Nov 4, 2024

@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
Copy link
Contributor Author

MahdiBM commented Nov 4, 2024

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
Copy link
Contributor

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
Copy link
Member

ahoppen commented Nov 4, 2024

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
Copy link
Contributor Author

MahdiBM commented Nov 4, 2024

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

@ahoppen
Copy link
Member

ahoppen commented Nov 4, 2024

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

@MahdiBM
Copy link
Contributor Author

MahdiBM commented Nov 5, 2024

@ahoppen issue filed: swiftlang/sourcekit-lsp#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.

@award999 award999 added the sourcekit-lsp SourceKit-LSP issue label Nov 6, 2024
@MahdiBM
Copy link
Contributor Author

MahdiBM commented Nov 22, 2024

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
Copy link
Contributor Author

MahdiBM commented Nov 22, 2024

@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
Copy link
Member

ahoppen commented Nov 22, 2024

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.

@plemarquand
Copy link
Contributor

@MahdiBM regarding enabling background indexing, I've opened a PR that adds an "auto" mode for this setting and uses it as the default.

@plemarquand
Copy link
Contributor

Regarding range formatting, now that sourcekit-lsp supports it VS Code gets it pretty much for free. You can try it with a recent 6.1 toolchain:

Nov-28-2024 13-45-34

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request sourcekit-lsp SourceKit-LSP issue
Development

Successfully merging a pull request may close this issue.

6 participants