fix: rework bindable types strategy (#2361)

Instead of using types that declare whether or not a type is bindable directly as part of the property, we're introducing a new for-types-only field to `SvelteComponent`: `$$bindings`, which is typed as the keys of the properties that are bindable (string by default, i.e. everything's bindable; for backwards compat). language-tools can then produce code that assigns to this property which results in an error we can display if the binding is invalid.
This means we can revert a lot of the changes we made to make the previous version of bindable types work

Author: dummdidumm

packages/language-server/src/plugins/typescript/features/DiagnosticsProvider.ts

@@ -145,28 +145,77 @@ export class DiagnosticsProviderImpl implements DiagnosticsProvider {
 
         diagnostics = resolveNoopsInReactiveStatements(lang, diagnostics);
 
-        return diagnostics
-            .map<Diagnostic>((diagnostic) => ({
-                range: convertRange(tsDoc, diagnostic),
-                severity: mapSeverity(diagnostic.category),
+        const mapRange = rangeMapper(tsDoc, document, lang);
+        const noFalsePositive = isNoFalsePositive(document, tsDoc);
+        const converted: Diagnostic[] = [];
+
+        for (const tsDiag of diagnostics) {
+            let diagnostic: Diagnostic = {
+                range: convertRange(tsDoc, tsDiag),
+                severity: mapSeverity(tsDiag.category),
                 source: isTypescript ? 'ts' : 'js',
-                message: ts.flattenDiagnosticMessageText(diagnostic.messageText, '\n'),
-                code: diagnostic.code,
-                tags: getDiagnosticTag(diagnostic)
-            }))
-            .map(mapRange(tsDoc, document, lang))
-            .filter(hasNoNegativeLines)
-            .filter(isNoFalsePositive(document, tsDoc))
-            .map(adjustIfNecessary)
-            .map(swapDiagRangeStartEndIfNecessary);
+                message: ts.flattenDiagnosticMessageText(tsDiag.messageText, '\n'),
+                code: tsDiag.code,
+                tags: getDiagnosticTag(tsDiag)
+            };
+            diagnostic = mapRange(diagnostic);
+
+            moveBindingErrorMessage(tsDiag, tsDoc, diagnostic, document);
+
+            if (!hasNoNegativeLines(diagnostic) || !noFalsePositive(diagnostic)) {
+                continue;
+            }
+
+            diagnostic = adjustIfNecessary(diagnostic);
+            diagnostic = swapDiagRangeStartEndIfNecessary(diagnostic);
+            converted.push(diagnostic);
+        }
+
+        return converted;
     }
 
     private async getLSAndTSDoc(document: Document) {
         return this.lsAndTsDocResolver.getLSAndTSDoc(document);
     }
 }
 
-function mapRange(
+function moveBindingErrorMessage(
+    tsDiag: ts.Diagnostic,
+    tsDoc: SvelteDocumentSnapshot,
+    diagnostic: Diagnostic,
+    document: Document
+) {
+    if (
+        tsDiag.code === DiagnosticCode.TYPE_X_NOT_ASSIGNABLE_TO_TYPE_Y &&
+        tsDiag.start &&
+        tsDoc.getText(tsDiag.start, tsDiag.start + tsDiag.length!).endsWith('.$$bindings')
+    ) {
+        let node = tsDoc.svelteNodeAt(diagnostic.range.start);
+        while (node && node.type !== 'InlineComponent') {
+            node = node.parent!;
+        }
+        if (node) {
+            let name = tsDoc.getText(
+                tsDiag.start + tsDiag.length!,
+                tsDiag.start + tsDiag.length! + 100
+            );
+            const quoteIdx = name.indexOf("'");
+            name = name.substring(quoteIdx + 1, name.indexOf("'", quoteIdx + 1));
+            const binding: any = node.attributes.find(
+                (attr: any) => attr.type === 'Binding' && attr.name === name
+            );
+            if (binding) {
+                diagnostic.message = 'Cannot bind: to this property\n\n' + diagnostic.message;
+                diagnostic.range = {
+                    start: document.positionAt(binding.start),
+                    end: document.positionAt(binding.end)
+                };
+            }
+        }
+    }
+}
+
+function rangeMapper(
     snapshot: SvelteDocumentSnapshot,
     document: Document,
     lang: ts.LanguageService
@@ -194,7 +243,7 @@ function mapRange(
                 diagnostic.code as number
             ) ||
                 (DiagnosticCode.TYPE_X_NOT_ASSIGNABLE_TO_TYPE_Y &&
-                    diagnostic.message.includes("'PropsWithChildren<"))) &&
+                    diagnostic.message.includes("'Properties<"))) &&
             !hasNonZeroRange({ range })
         ) {
             const node = getNodeIfIsInStartTag(document.html, document.offsetAt(range.start));
@@ -327,37 +376,6 @@ function adjustIfNecessary(diagnostic: Diagnostic): Diagnostic {
         };
     }
 
-    if (
-        (diagnostic.code === DiagnosticCode.TYPE_X_NOT_ASSIGNABLE_TO_TYPE_Y ||
-            diagnostic.code === DiagnosticCode.TYPE_X_NOT_ASSIGNABLE_TO_TYPE_Y_DID_YOU_MEAN) &&
-        diagnostic.message.includes("'Bindable<")
-    ) {
-        const countBindable = (diagnostic.message.match(/'Bindable\</g) || []).length;
-        const countBinding = (diagnostic.message.match(/'Binding\</g) || []).length;
-        if (countBindable === 1 && countBinding === 0) {
-            // Remove distracting Bindable<...> from diagnostic message
-            const start = diagnostic.message.indexOf("'Bindable<");
-            const startType = start + "'Bindable".length;
-            const end = traverseTypeString(diagnostic.message, startType, '>');
-            diagnostic.message =
-                diagnostic.message.substring(0, start + 1) +
-                diagnostic.message.substring(startType + 1, end) +
-                diagnostic.message.substring(end + 1);
-        } else if (countBinding === 3 && countBindable === 1) {
-            // Only keep Type '...' is not assignable to type '...' in
-            // Type Bindings<...> is not assignable to type Bindable<...>, Type Binding<...> is not assignable to type Bindable<...>, Type '...' is not assignable to type '...'
-            const lines = diagnostic.message.split('\n');
-            if (lines.length === 3) {
-                diagnostic.message = lines[2].trimStart();
-            }
-        }
-
-        return {
-            ...diagnostic,
-            message: diagnostic.message
-        };
-    }
-
     return diagnostic;
 }
 

packages/language-server/src/plugins/typescript/features/RenameProvider.ts

@@ -31,14 +31,14 @@ import {
 } from './utils';
 import { LSConfigManager } from '../../../ls-config';
 import { isAttributeName, isEventHandler } from '../svelte-ast-utils';
+import { Identifier } from 'estree';
 
 interface TsRenameLocation extends ts.RenameLocation {
     range: Range;
     newName?: string;
 }
 
 const bind = 'bind:';
-const bindShortHandGeneratedLength = ':__sveltets_2_binding('.length;
 
 export class RenameProviderImpl implements RenameProvider {
     constructor(
@@ -76,7 +76,7 @@ export class RenameProviderImpl implements RenameProvider {
 
         const renameLocations = lang.findRenameLocations(
             tsDoc.filePath,
-            offset + (renameInfo.bindShorthand || 0),
+            offset,
             false,
             false,
             true
@@ -101,7 +101,8 @@ export class RenameProviderImpl implements RenameProvider {
         convertedRenameLocations = this.checkShortHandBindingOrSlotLetLocation(
             lang,
             convertedRenameLocations,
-            docs
+            docs,
+            newName
         );
 
         const additionalRenameForPropRenameInsideComponentWithProp =
@@ -160,7 +161,6 @@ export class RenameProviderImpl implements RenameProvider {
     ):
         | (ts.RenameInfoSuccess & {
               isStore?: boolean;
-              bindShorthand?: number;
           })
         | null {
         // Don't allow renames in error-state, because then there is no generated svelte2tsx-code
@@ -170,14 +170,6 @@ export class RenameProviderImpl implements RenameProvider {
         }
 
         const svelteNode = tsDoc.svelteNodeAt(originalPosition);
-
-        let bindOffset = 0;
-        const bindingShorthand = this.getBindingShorthand(tsDoc, originalPosition, svelteNode);
-        if (bindingShorthand) {
-            bindOffset = bindingShorthand.end - bindingShorthand.start;
-            generatedOffset += bindShortHandGeneratedLength + bindOffset;
-        }
-
         const renameInfo = lang.getRenameInfo(tsDoc.filePath, generatedOffset, {
             allowRenameOfImportPath: false
         });
@@ -211,11 +203,6 @@ export class RenameProviderImpl implements RenameProvider {
             }
         }
 
-        if (bindOffset) {
-            renameInfo.triggerSpan.start -= bindShortHandGeneratedLength + bindOffset;
-            (renameInfo as any).bindShorthand = bindShortHandGeneratedLength + bindOffset;
-        }
-
         return renameInfo;
     }
 
@@ -366,48 +353,8 @@ export class RenameProviderImpl implements RenameProvider {
                 return location;
             }
 
-            const { parent } = snapshot;
-
-            const bindingShorthand = this.getBindingShorthand(snapshot, location.range.start);
-            if (bindingShorthand) {
-                // bind:|foo| -> bind:|newName|={foo}
-                const name = parent
-                    .getText()
-                    .substring(bindingShorthand.start, bindingShorthand.end);
-                return {
-                    ...location,
-                    prefixText: '',
-                    suffixText: `={${name}}`
-                };
-            }
-
-            let rangeStart = parent.offsetAt(location.range.start);
-
-            // suffix is of the form `: oldVarName` -> hints at a shorthand
-            if (
-                !location.suffixText?.trimStart()?.startsWith(':') ||
-                !getNodeIfIsInStartTag(parent.html, rangeStart)
-            ) {
-                return location;
-            }
-
-            if (snapshot.getOriginalText().charAt(rangeStart - 1) === '{') {
-                // {|foo|} -> |{foo|}
-                rangeStart--;
-                return {
-                    ...location,
-                    range: {
-                        start: parent.positionAt(rangeStart),
-                        end: location.range.end
-                    },
-                    // |{foo|} -> newName=|{foo|}
-                    newName: parent.getText(location.range),
-                    prefixText: `${newName}={`,
-                    suffixText: ''
-                };
-            }
-
-            return location;
+            const shorthandLocation = this.transformShorthand(snapshot, location, newName);
+            return shorthandLocation || location;
         });
     }
 
@@ -595,7 +542,8 @@ export class RenameProviderImpl implements RenameProvider {
     private checkShortHandBindingOrSlotLetLocation(
         lang: ts.LanguageService,
         renameLocations: TsRenameLocation[],
-        snapshots: SnapshotMap
+        snapshots: SnapshotMap,
+        newName?: string
     ): TsRenameLocation[] {
         return renameLocations.map((location) => {
             const sourceFile = lang.getProgram()?.getSourceFile(location.fileName);
@@ -616,6 +564,16 @@ export class RenameProviderImpl implements RenameProvider {
 
             const { parent } = snapshot;
 
+            if (snapshot.isSvelte5Plus && newName && location.suffixText) {
+                // Svelte 5 runes mode, thanks to $props(), is much easier to handle rename-wise.
+                // Notably, it doesn't need the "additional props rename locations" logic, because
+                // these renames already appear here.
+                const shorthandLocation = this.transformShorthand(snapshot, location, newName);
+                if (shorthandLocation) {
+                    return shorthandLocation;
+                }
+            }
+
             let rangeStart = parent.offsetAt(location.range.start);
             let prefixText = location.prefixText?.trimRight();
 
@@ -638,38 +596,6 @@ export class RenameProviderImpl implements RenameProvider {
                         suffixText: '}'
                     };
                 }
-
-                if (snapshot.isSvelte5Plus) {
-                    const bindingShorthand = this.getBindingShorthand(
-                        snapshot,
-                        location.range.start
-                    );
-                    if (bindingShorthand) {
-                        const name = parent
-                            .getText()
-                            .substring(bindingShorthand.start, bindingShorthand.end);
-                        const start = {
-                            line: location.range.start.line,
-                            character: location.range.start.character - name.length
-                        };
-                        // If binding is followed by the closing tag, start is one character too soon,
-                        // else binding is ending one character too far
-                        if (parent.getText().charAt(parent.offsetAt(start)) === ':') {
-                            start.character++;
-                        } else {
-                            location.range.end.character--;
-                        }
-                        return {
-                            ...location,
-                            range: {
-                                start: start,
-                                end: location.range.end
-                            },
-                            prefixText: name + '={',
-                            suffixText: '}'
-                        };
-                    }
-                }
             }
 
             if (!prefixText || prefixText.slice(-1) !== ':') {
@@ -714,16 +640,54 @@ export class RenameProviderImpl implements RenameProvider {
         });
     }
 
-    private getBindingShorthand(
+    private transformShorthand(
+        snapshot: SvelteDocumentSnapshot,
+        location: TsRenameLocation,
+        newName: string
+    ): TsRenameLocation | undefined {
+        const shorthand = this.getBindingOrAttrShorthand(snapshot, location.range.start);
+        if (shorthand) {
+            if (shorthand.isBinding) {
+                // bind:|foo| -> bind:|newName|={foo}
+                return {
+                    ...location,
+                    prefixText: '',
+                    suffixText: `={${shorthand.id.name}}`
+                };
+            } else {
+                return {
+                    ...location,
+                    range: {
+                        // {|foo|} -> |{foo|}
+                        start: {
+                            line: location.range.start.line,
+                            character: location.range.start.character - 1
+                        },
+                        end: location.range.end
+                    },
+                    // |{foo|} -> newName=|{foo|}
+                    newName: shorthand.id.name,
+                    prefixText: `${newName}={`,
+                    suffixText: ''
+                };
+            }
+        }
+    }
+
+    private getBindingOrAttrShorthand(
         snapshot: SvelteDocumentSnapshot,
         position: Position,
         svelteNode = snapshot.svelteNodeAt(position)
-    ) {
+    ): { id: Identifier; isBinding: boolean } | undefined {
         if (
-            svelteNode?.parent?.type === 'Binding' &&
+            (svelteNode?.parent?.type === 'Binding' ||
+                svelteNode?.parent?.type === 'AttributeShorthand') &&
             svelteNode.parent.expression.end === svelteNode.parent.end
         ) {
-            return svelteNode.parent.expression;
+            return {
+                id: svelteNode as any as Identifier,
+                isBinding: svelteNode.parent.type === 'Binding'
+            };
         }
     }
 }