Reduce code size of lsproto package, use JSON roundtripping in fourslash (#4471)

Author: jakebailey

internal/fourslash/fourslash.go

@@ -242,8 +242,8 @@ func (f *FourslashTest) handleServerRequest(_ context.Context, req *lsproto.Requ
 		// Return current user preferences for each requested section.
 		// The server requests multiple sections (js/ts, typescript, javascript, editor);
 		// we return user preferences for "js/ts" and nil for others.
-		params, ok := req.Params.(*lsproto.ConfigurationParams)
-		if !ok || params == nil || params.Items == nil {
+		params, err := lsproto.UnmarshalParams[*lsproto.ConfigurationParams](req)
+		if err != nil || params == nil || params.Items == nil {
 			return &lsproto.ResponseMessage{
 				ID:      req.ID,
 				JSONRPC: req.JSONRPC,
@@ -1098,7 +1098,10 @@ func (f *FourslashTest) VerifyCompletions(t *testing.T, markerInput MarkerInput,
 			if item == nil {
 				t.Fatalf("Code action '%s' from source '%s' not found in completions.", expectedAction.Name, expectedAction.Source)
 			}
-			assert.Check(t, strings.Contains(*item.Detail, expectedAction.Description), "Completion item detail does not contain expected description.")
+			// Detail and AdditionalTextEdits for auto-import items are populated by
+			// completionItem/resolve, not in the initial completion list.
+			item = f.resolveCompletionItem(t, item)
+			assert.Check(t, item.Detail != nil && strings.Contains(*item.Detail, expectedAction.Description), "Completion item detail does not contain expected description.")
 			f.applyTextEdits(t, *item.AdditionalTextEdits)
 			assert.Equal(t, f.getScriptInfo(f.activeFilename).content, expectedAction.NewFileContent, fmt.Sprintf("File content after applying code action '%s' did not match expected content.", expectedAction.Name))
 		},
@@ -2532,7 +2535,7 @@ func (f *FourslashTest) verifyBaselineDefinitions(
 		} else if result.DefinitionLinks != nil {
 			var originRange *lsproto.Range
 			resultAsSpans = core.Map(*result.DefinitionLinks, func(link *lsproto.LocationLink) documentSpan {
-				if originRange != nil && originRange != link.OriginSelectionRange {
+				if originRange != nil && (link.OriginSelectionRange == nil || *originRange != *link.OriginSelectionRange) {
 					panic("multiple different origin ranges in definition links")
 				}
 				originRange = link.OriginSelectionRange
@@ -5487,18 +5490,22 @@ func (f *FourslashTest) VerifyBaselineDocumentSymbol(t *testing.T) {
 	}
 	result := sendRequest(t, f, lsproto.TextDocumentDocumentSymbolInfo, params)
 	uri := lsconv.FileNameToDocumentURI(f.activeFilename)
-	spansToSymbol := make(map[documentSpan]*lsproto.DocumentSymbol)
+	symbolBySpan := make(map[documentSpanKey]*lsproto.DocumentSymbol)
 	if result.DocumentSymbols != nil {
 		for _, symbol := range *result.DocumentSymbols {
-			collectDocumentSymbolSpans(uri, symbol, spansToSymbol)
+			collectDocumentSymbolSpans(uri, symbol, symbolBySpan)
 		}
 	}
+	spans := make([]documentSpan, 0, len(symbolBySpan))
+	for key, symbol := range symbolBySpan {
+		spans = append(spans, documentSpan{uri: key.uri, textSpan: key.textSpan, contextSpan: &symbol.Range})
+	}
 	f.addResultToBaseline(
 		t,
 		documentSymbolsCmd,
-		f.getBaselineForSpansWithFileContents(slices.Collect(maps.Keys(spansToSymbol)), baselineFourslashLocationsOptions{
+		f.getBaselineForSpansWithFileContents(spans, baselineFourslashLocationsOptions{
 			getLocationData: func(span documentSpan) string {
-				symbol := spansToSymbol[span]
+				symbol := symbolBySpan[documentSpanKey{uri: span.uri, textSpan: span.textSpan, contextSpan: *span.contextSpan}]
 				return fmt.Sprintf("{| name: %s, kind: %s |}", symbol.Name, symbol.Kind.String())
 			},
 		}),
@@ -5523,21 +5530,32 @@ func writeDocumentSymbolDetails(symbols []*lsproto.DocumentSymbol, indent int, b
 func collectDocumentSymbolSpans(
 	uri lsproto.DocumentUri,
 	symbol *lsproto.DocumentSymbol,
-	spansToSymbol map[documentSpan]*lsproto.DocumentSymbol,
+	symbolBySpan map[documentSpanKey]*lsproto.DocumentSymbol,
 ) {
-	span := documentSpan{
-		uri:         uri,
-		textSpan:    symbol.SelectionRange,
-		contextSpan: &symbol.Range,
+	// Deduplicate by value rather than by the documentSpan key, which holds a pointer to
+	// the symbol's Range. The same logical symbol can be reached more than once
+	// (e.g. a merged declaration), and depending on transport those occurrences may
+	// be the same object (shared *Range) or independent copies (distinct *Range
+	// after a JSON round-trip). A value-based key collapses them consistently.
+	key := documentSpanKey{uri: uri, textSpan: symbol.SelectionRange, contextSpan: symbol.Range}
+	if _, ok := symbolBySpan[key]; !ok {
+		symbolBySpan[key] = symbol
 	}
-	spansToSymbol[span] = symbol
 	if symbol.Children != nil {
 		for _, child := range *symbol.Children {
-			collectDocumentSymbolSpans(uri, child, spansToSymbol)
+			collectDocumentSymbolSpans(uri, child, symbolBySpan)
 		}
 	}
 }
 
+// documentSpanKey is a value-comparable variant of documentSpan used to deduplicate
+// document symbols regardless of pointer identity.
+type documentSpanKey struct {
+	uri         lsproto.DocumentUri
+	textSpan    lsproto.Range
+	contextSpan lsproto.Range
+}
+
 // VerifyNumberOfErrorsInCurrentFile verifies that the current file has the expected number of errors.
 func (f *FourslashTest) VerifyNumberOfErrorsInCurrentFile(t *testing.T, expectedCount int) {
 	diagnostics := f.getDiagnostics(t, f.activeFilename)

internal/ls/lsutil/userpreferences.go

@@ -455,6 +455,48 @@ var typeSerializers = map[reflect.Type]func(any) any{
 			return "auto"
 		}
 	},
+	// These enums distinguish an unset zero value (e.g. "") from their effective
+	// default (e.g. "auto"): the parser promotes unset/unknown input to the
+	// non-zero default. Plain string serialization would therefore write "" for
+	// an unset field and the parser would read it back as the non-zero default,
+	// breaking round-tripping. Mirror the core.Tristate serializer above and omit
+	// the unset value (return nil) so it decodes back to the zero value. (Enums
+	// whose default already is their zero value, like the OrganizeImports* ones,
+	// round-trip without this.)
+	//
+	// TODO: These three are the only parsers whose fallback is a non-zero value;
+	// every other parser returns its zero value as the fallback. They should be
+	// made consistent: change the parser fallback to return the zero value and
+	// remove this serializer (relying on the default string serialization, which
+	// already omits ""). The consumer must then treat the zero value as the
+	// effective default. The two module-specifier enums are safe to convert (all
+	// read sites already treat the "" zero identically to the promoted default).
+	reflect.TypeFor[JsxAttributeCompletionStyle](): func(val any) any {
+		// TODO: make consistent with other enums (see note above). Unlike the
+		// module-specifier enums, the consumer in completions.go distinguishes
+		// JsxAttributeCompletionStyleUnknown from ...Auto, so converting this one
+		// requires updating that consumer to treat the zero value as "auto".
+		if v := val.(JsxAttributeCompletionStyle); v != JsxAttributeCompletionStyleUnknown {
+			return string(v)
+		}
+		return nil
+	},
+	reflect.TypeFor[modulespecifiers.ImportModuleSpecifierPreference](): func(val any) any {
+		// TODO: make consistent with other enums (see note above): have the parser
+		// return the zero value (None) as its fallback and drop this serializer.
+		if v := val.(modulespecifiers.ImportModuleSpecifierPreference); v != "" {
+			return string(v)
+		}
+		return nil
+	},
+	reflect.TypeFor[modulespecifiers.ImportModuleSpecifierEndingPreference](): func(val any) any {
+		// TODO: make consistent with other enums (see note above): have the parser
+		// return the zero value (None) as its fallback and drop this serializer.
+		if v := val.(modulespecifiers.ImportModuleSpecifierEndingPreference); v != "" {
+			return string(v)
+		}
+		return nil
+	},
 }
 
 // configPathParsers provides field-specific config value parsers that override the default
@@ -741,9 +783,21 @@ func serializeField(field reflect.Value) any {
 	case reflect.Bool:
 		return field.Bool()
 	case reflect.Int:
-		return int(field.Int())
+		// Zero means "unset" for these preference fields. Omit it so a partial
+		// config does not clobber defaults with zeros when round-tripped through
+		// withConfig.
+		i := field.Int()
+		if i == 0 {
+			return nil
+		}
+		return int(i)
 	case reflect.String:
-		return field.String()
+		// Zero ("") means "unset"; omit it for the same reason as int above.
+		s := field.String()
+		if s == "" {
+			return nil
+		}
+		return s
 	case reflect.Slice:
 		if field.IsNil() {
 			return nil
@@ -770,28 +824,6 @@ func (p *UserPreferences) UnmarshalJSONFrom(dec *json.Decoder) error {
 
 // --- Helper methods ---
 
-// WithOverrides returns a copy of p with non-zero fields from overrides applied on top.
-// This is safe because all preference fields use types where zero = "not set":
-// Tristate (TSUnknown=0), int (0), string (""), slice (nil).
-func (p UserPreferences) WithOverrides(overrides UserPreferences) UserPreferences {
-	mergeNonZeroFields(reflect.ValueOf(&p).Elem(), reflect.ValueOf(&overrides).Elem())
-	return p
-}
-
-func mergeNonZeroFields(dst, src reflect.Value) {
-	for i := range dst.NumField() {
-		srcField := src.Field(i)
-		dstField := dst.Field(i)
-		if srcField.Kind() == reflect.Struct {
-			mergeNonZeroFields(dstField, srcField)
-			continue
-		}
-		if !srcField.IsZero() {
-			dstField.Set(srcField)
-		}
-	}
-}
-
 func (p UserPreferences) ModuleSpecifierPreferences() modulespecifiers.UserPreferences {
 	return modulespecifiers.UserPreferences{
 		ImportModuleSpecifierPreference:   p.ImportModuleSpecifierPreference,
@@ -821,11 +853,8 @@ func ParseUserPreferences(items map[string]any) UserPreferences {
 	// Apply javascript, then typescript, then js/ts (highest precedence).
 	for _, section := range []string{"javascript", "typescript", "js/ts"} {
 		if item, ok := items[section]; ok && item != nil {
-			switch settings := item.(type) {
-			case map[string]any:
+			if settings, ok := item.(map[string]any); ok {
 				prefs = prefs.withConfig(settings)
-			case UserPreferences:
-				prefs = prefs.WithOverrides(settings)
 			}
 		}
 	}