Port formatter, enable in LSP

[weswigham]

This isn't extensively tested yet, as the fourslash tests aughta do that once they're in, but I've got a test/bench running the formatter over checker.ts just to check that it doesn't crash, at least.

[Copilot]

Pull Request Overview

This PR ports the formatter functionality for use in the language server protocol (LSP) and refactors several internal formatting and utility methods.

• Integrates new LSP formatting handlers and updates formatting APIs.
• Refactors functions to use methods from the lsutil and ast packages, and removes legacy implementations.
• Introduces new formatting utilities, scanner changes, and updated tests for formatting operations.

Reviewed Changes

Copilot reviewed 30 out of 30 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
internal/ls/utilities.go	Removed legacy utility functions and replaced them with calls to the new lsutil functions.
internal/ls/types.go	Removed redundant local TextChange type in favor of core.TextChange.
internal/ls/format.go	Introduced new LSP formatting functions including document, selection, and on-type formatting.
internal/ls/converters.go	Updated the FromLSPTextChange signature to leverage core.TextChange.
internal/ls/completions.go	Updated completion logic to use lsutil functions for token analysis.
internal/format/*	Added and refactored formatting utilities, scanner, rules mapping, context, and tests.
internal/checker/*	Refactored decorator checks to use ast.HasDecorators instead of legacy methods.
internal/ast/*	Added helper functions like IsTrivia and HasDecorators, and updated token kind definitions.

Comments suppressed due to low confidence (2)

internal/ast/utilities.go:3483

• [nitpick] Add a doc comment for 'HasDecorators' explaining its purpose and how it replaces legacy decorator checks for clarity to future maintainers.

func HasDecorators(node *Node) bool {

internal/ls/completions.go:411

• Ensure that lsutil.GetLastChild(node, file) is not nil before accessing its 'Kind' property to prevent potential nil pointer dereferences.

if lsutil.GetLastChild(node, file).Kind != ast.KindCloseParenToken) {

[jakebailey]

(largely superficial comments in my initial pass)

[jakebailey]

Not in the slightest needed to be done in this PR, but I've been wanting to apply this to all "non critical" requests

[jakebailey]

Nit but I usually avoid the name "context" given its conflict with the package name, but it's probably fine.

[jakebailey]

Is this function intentionally like this? Everything falls through.

[weswigham]

I left it structurally the same as in Strada - where some blocks have comments and explicit fallthroughs, rather than just one big block. Don't really know why, other than to logically group the kinds.

[jakebailey]

I'm not sure whether to be happy or sad to see that this pattern still works even when ported 😄

[weswigham]

The only thing that needed any modification was the option-dependent context checks, which all had to be swapped from dynamic property lookup to hardcoded selectors.

[jakebailey]

One thing that I think we'll have to document is that formatting won't work by default unless users set something like:

"editor.defaultFormatter": "TypeScriptTeam.native-preview"

Formatting is a request that can only be handled by one provider at a time, so it has to know which one to choose, and by default that is surely the built-in TS extension, which will fail.

[jakebailey]

Though, maybe we can abuse:

        "configurationDefaults": {
            "[typescript]": {
                "editor.defaultFormatter": "TypeScriptTeam.native-preview"
            },
            "[typescriptreact]": {
                "editor.defaultFormatter": "TypeScriptTeam.native-preview"
            },
            "[javascript]": {
                "editor.defaultFormatter": "TypeScriptTeam.native-preview"
            },
            "[javascriptreact]": {
                "editor.defaultFormatter": "TypeScriptTeam.native-preview"
            }
        },

Though ideally this would only be the case if the LSP were running. Not sure if these can be conditioned like other package.json level things.

[weswigham]

One thing that I think we'll have to document is that formatting won't work by default unless users set something like:

Do they? With the native preview enabled, before I added the capability, I got a "no formatting provider" error, and then after I added the capability to the server, it "just worked". Dunno if that's an artifact of the extension dev mode, though.

[jakebailey]

Ah, you know, maybe having the tsgo option enabled means the builtin TS extension never registers the format provider, and therefore the format request comes to us naturally. That seems fine!

[weswigham]

@jakebailey With the bulk edit application in place (so go isn't making hundreds of thousands of multi-megabyte strings), 03015d6 removes another source of memory pressure - that change should be good even outside the formatter, too, no?

Even after 24bbe7d and 2d519b3, formatting checker.ts still takes around 2x longer than just pretty printing it via the printer (not that the printer is a complete replacement at present, since it discards a bunch of text and lacks configuration), but I expect that gap can be narrowed further - in theory the formatter really aughta be pretty close, since it's not building any strings itself (just a list of edits), and is a pretty similar AST visit. Unless I'm reading it wrong, though, the majority of the time is now spent in scanner util functions like utf8 rune decoding, counting, and position calculation (things the pretty printer generally doesn't need to bother with, since it's not rescanning and checking adjacent tokens for whitespace rules).

[jakebailey]

I assume the 2x is not "2x slower than Strada", but rather 2x slower than Corsa emit?

I haven't profiled it myself yet, but I think that as long as it works and doesn't crash, it's fine to leave like this for future PRs. This all gets to run asynchronously too.

[jakebailey]

IIRC formatting checker.ts in Strada took way more than a couple seconds, so 😄

[weswigham]

but rather 2x slower than Corsa emit?

Yeah.

IIRC formatting checker.ts in Strada took way more than a couple seconds, so 😄

Well, now it's 0.21 sec to format checker.ts (when it's already available parsed) on my machine (vs 0.1s to emit). It was only in the "seconds" order of magnitude with unoptimized edit application included (which there is now a helper to avoid, so).