Revamp user preferences serialization/deserialization #2272

jakebailey · 2025-12-08T17:56:31Z

In working on improving the fourslash testing, I wanted to make user preferences serializable, such that we can directly go from JSON to prefs and back, and therefore also test out the "actual" JSON encoding.

Making that work sort of snowballed into a total refactor, which eliminates the error-prone switch case for decoding, replacing it with struct tags and reflect.

Prefs now look like this:

type CodeLensUserPreferences struct {
	ReferencesCodeLensEnabled                     bool `raw:"referencesCodeLensEnabled" config:"referencesCodeLens.enabled"`
	ImplementationsCodeLensEnabled                bool `raw:"implementationsCodeLensEnabled" config:"implementationsCodeLens.enabled"`
	ReferencesCodeLensShowOnAllFunctions          bool `raw:"referencesCodeLensShowOnAllFunctions" config:"referencesCodeLens.showOnAllFunctions"`
	ImplementationsCodeLensShowOnInterfaceMethods bool `raw:"implementationsCodeLensShowOnInterfaceMethods" config:"implementationsCodeLens.showOnInterfaceMethods"`
	ImplementationsCodeLensShowOnAllClassMethods  bool `raw:"implementationsCodeLensShowOnAllClassMethods" config:"implementationsCodeLens.showOnAllClassMethods"`
}

A struct tag gives the name for the JSON propery (used by Strada), and also the name over LSP. https://github.com/microsoft/vscode/blob/main/extensions/typescript-language-features/package.json.

All prefs can still be set by the unstable object (which happens first, based on the raw name).

Structs can be nested, e.g.:

type UserPreferences struct {
	// ...
	ModuleSpecifier ModuleSpecifierUserPreferences
	// ...
}

type ModuleSpecifierUserPreferences struct {
	ImportModuleSpecifierPreference modulespecifiers.ImportModuleSpecifierPreference `raw:"importModuleSpecifierPreference" config:"preferences.importModuleSpecifier"` // !!!
	// Determines whether we import `foo/index.ts` as "foo", "foo/index", or "foo/index.js"
	ImportModuleSpecifierEnding       modulespecifiers.ImportModuleSpecifierEndingPreference `raw:"importModuleSpecifierEnding" config:"preferences.importModuleSpecifierEnding"`             // !!!
	AutoImportSpecifierExcludeRegexes []string                                               `raw:"autoImportSpecifierExcludeRegexes" config:"preferences.autoImportSpecifierExcludeRegexes"` // !!!
}

And, for prefs that are actually defined elsewhere (like the above), conversion works so long as the structure is the same, making this less error prone:

func (p *UserPreferences) ModuleSpecifierPreferences() modulespecifiers.UserPreferences {
	return modulespecifiers.UserPreferences(p.ModuleSpecifier)
}

UserPreferences are also now considered to be immutable. They should never be mutated for any reason. Copy() can be used if you need to make a deep copy. Copying is used for a test, but also for cloning the defaults to use during unmarshalling. Nothing else needs to mutate any preferences.

Copilot

Pull request overview

This PR refactors the user preferences system to use reflection-based serialization/deserialization with struct tags, replacing the error-prone switch-case approach. The changes make preferences immutable, support direct JSON marshaling/unmarshaling, and simplify configuration by mapping TypeScript config paths to struct fields via pref tags.

Key changes:

Implements reflection-based parsing using pref struct tags to map VS Code config paths to fields
Adds custom type parsers and serializers for enum-like types (Tristate, OrganizeImportsCollation, etc.)
Makes preferences immutable by removing mutation operations and returning the same pointer where possible
Adds generic deepCopy implementation for creating copies when needed

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
`internal/ls/lsutil/userpreferences.go`	Core refactor: adds struct tags, reflection-based parsing/serialization, type parsers/serializers, and immutable Copy() implementation
`internal/ls/lsutil/userpreferences_test.go`	New roundtrip test validating JSON marshaling/unmarshaling and parseWorker
`internal/project/snapshot.go`	Removes unnecessary Copy() call when assigning immutable preferences
`internal/project/session.go`	Removes Copy() calls and returns direct pointer to immutable preferences
`internal/lsp/server.go`	Updates to use ParseUserPreferences() function and DefaultUserPreferences; fixes typo
`internal/ls/autoimports.go`	Updates field access to use nested ModuleSpecifier struct
`internal/fourslash/fourslash.go`	Updates to use DefaultUserPreferences directly instead of calling constructor

Copilot · 2025-12-08T18:02:01Z

internal/ls/lsutil/userpreferences.go

+	case reflect.Int:
+		return int(field.Int())
+	case reflect.String:
+		return field.String()


The serializeField function will serialize empty string values for string-based enums (like QuotePreference, JsxAttributeCompletionStyle, and IncludePackageJsonAutoImports) when they have their "Unknown" zero value. This causes these fields to be included in the serialized JSON as empty strings rather than being omitted.

Consider either:

Adding a check to return nil for empty string values in the reflect.String case

Adding custom serializers for these types (similar to IncludeInlayParameterNameHints which handles empty strings)

This would prevent empty/zero values from polluting the serialized output.

Suggested change

return field.String()

s := field.String()

if s == "" {

return nil

}

return s

Do we need some sort of omitzero?

Eh, not sure. The roundtripping is not perfect because these types were switched away from things like *string for convenience.

internal/ls/lsutil/userpreferences.go

internal/ls/lsutil/userpreferences_test.go

internal/ls/lsutil/userpreferences.go

DanielRosenwasser · 2025-12-09T01:12:14Z

internal/ls/lsutil/userpreferences.go

-	PreferTypeOnlyAutoImports         bool                                                   // !!!
+	ModuleSpecifier ModuleSpecifierUserPreferences
+
+	IncludePackageJsonAutoImports IncludePackageJsonAutoImports `pref:"preferences.includePackageJsonAutoImports"` // !!!


Do you think it would be better to just omit these until the code is used?

IIRC this is upcoming, so it would just be churn to remove I think...

internal/ls/lsutil/userpreferences.go

DanielRosenwasser · 2025-12-09T01:27:39Z

internal/ls/lsutil/userpreferences.go

+		if name == "unstable" {
+			continue
+		}
+		// Skip known VS Code config sections that are handled by path-based parsing
 		switch name {
-		case "autoImports":
-			p.set("includeCompletionsForModuleExports", value)
-		case "objectLiteralMethodSnippets":
-			if v, ok := value.(map[string]any); ok {
-				p.set("includeCompletionsWithObjectLiteralMethodSnippets", parseEnabledBool(v))
-			}
-		case "classMemberSnippets":
-			if v, ok := value.(map[string]any); ok {
-				p.set("includeCompletionsWithClassMemberSnippets", parseEnabledBool(v))
+		case "preferences", "suggest", "inlayHints", "referencesCodeLens",
+			"implementationsCodeLens", "workspaceSymbols", "format", "tsserver", "tsc", "experimental":
+			continue


What's an example of a path that doesn't need to be skipped?

Yeah, I think this is actually dead code.

Ah, so this was here to roundtrip unstable settings, but it isn't doing that right.

I'm going to rethink unstable settings. Probably just make them explicit.

Okay, so the sort of problem is that we used to allow user prefs from like, anywhere, so you could have a object with say, { "quotePreference": ... } and that would be read in, which is a valid UserPreferences object. But, now over LSP, we are expecting things over typescript. So it's maybe a question of what non-vscode people do.

And, VS Code calls it quoteStyle, while preferences called it quotePreference, so that's uh, great

I split things apart so that we can both have the raw JS name we used to have, but also the LSP-based preference name.

DanielRosenwasser · 2025-12-09T01:33:19Z

internal/ls/lsutil/userpreferences.go

+	case reflect.Int:
+		return int(field.Int())
+	case reflect.String:
+		return field.String()


Do we need some sort of omitzero?

internal/ls/lsutil/userpreferences_test.go

…eference, etc

DanielRosenwasser · 2025-12-09T22:06:00Z

internal/ls/lsutil/userpreferences.go

-	prefCopy.AutoImportFileExcludePatterns = slices.Clone(p.AutoImportFileExcludePatterns)
-	return &prefCopy
+type fieldInfo struct {
+	rawName      string // raw name for unstable section lookup (e.g., "quotePreference")


So is the idea that to get to this, a user can only write the explicit path or typescript.unstable.quotePreference?

Yeah. Either, you are using unstable and you were working by spread directly into UserPreferences in Strada, or you were using the "real" name VS Code had, which takes precedence via the spread.

Arguably, this means we should entirely delete any unstable thing with a real name, though, because VS Code actually entirely overwrote these (by nature of config.get having defaults): https://github.com/microsoft/vscode/blob/f9e3a710d84879d41431e7c17d8a8fe8612dfb26/extensions/typescript-language-features/src/languageFeatures/fileConfigurationManager.ts#L186

So maybe I went overkill and we shouldn't allow both names.

Basically, it's hard to balance "LSP VS Code style settings" with "the old tsserver UserPreferences", defaults, spread, and then maybe even other editors...

So, I don't know what is exactly right.

I think in this case, needing to support all these unstable settings years later kind of defeats the purpose - but we'll probably need to support new unstable properties again anyway, right? So I'm not against keeping support around for it as a concept, and it is virtually no work for us to keep the current ones going if we want 6.0/7.0 to be smoother.

(So your call but I think this is fine)

Probably I'll just keep this.

jakebailey · 2025-12-09T22:47:03Z

I think I'm going to wait for the other two user pref related PRs to finish and just fix this PR instead of forcing that on everyone.

jakebailey added 12 commits December 5, 2025 11:01

Make user prefs serializable

c4286f5

Very redo

bca6d8f

Deep copy

1deb813

Another struct

1763af6

Simpler

86b1d94

Simpler

bf21cb2

Remove copying

88a928f

more unused copies

67db3e9

Restore comments

18be102

Merge branch 'main' into jabaile/user-prefs-json

820c516

Default just pointer

7ff185e

Remove

0c95a82

jakebailey requested review from DanielRosenwasser, andrewbranch, Copilot and iisaduan December 8, 2025 17:56

Copilot started reviewing on behalf of jakebailey December 8, 2025 17:57 View session

Copilot AI reviewed Dec 8, 2025

View reviewed changes

Cleanup

8b08250

DanielRosenwasser reviewed Dec 9, 2025

View reviewed changes

jakebailey added 8 commits December 8, 2025 19:54

remove old dead test code

46cf5f1

Just return

711f302

Delete alias support

6c703eb

Explict unstable

4728fba

Merge branch 'main' into jabaile/user-prefs-json

9c38c4e

test, what

8ae6651

Move stuff around but I don't know how to improve quoteStyle, quotePr…

74c815b

…eference, etc

Try and deal with old vs new

2b4b1f6

jakebailey mentioned this pull request Dec 9, 2025

Fix getReferencedSymbolsForSymbol, implement getReferencesAtExportSpecifier #2304

Merged

DanielRosenwasser reviewed Dec 9, 2025

View reviewed changes

DanielRosenwasser approved these changes Dec 9, 2025

View reviewed changes

-		return field.String()
+		s := field.String()
+		if s == "" {
+			return nil
+		}
+		return s

Revamp user preferences serialization/deserialization #2272

Are you sure you want to change the base?

Revamp user preferences serialization/deserialization #2272

Uh oh!

Conversation

jakebailey commented Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakebailey commented Dec 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jakebailey commented Dec 8, 2025 •

edited

Loading