From 8c080cf36286984439b468e089e6e4eaacd4230e Mon Sep 17 00:00:00 2001 From: Simon H <5968653+dummdidumm@users.noreply.github.com> Date: Wed, 28 Aug 2024 17:21:32 +0200 Subject: [PATCH] feat: better type checking for bindings in Svelte 5 (#2477) #1392 This adds enhanced type checking for bindings in Svelte 5: We're not only passing the variable in, we're also assigning the value of the component property back to the variable. That way, we can catch type errors like the child binding having a wider type as the input we give it. It's done for Svelte 5 only because it's a) easier there b) doesn't break as much code (people who upgrade can then also upgrade the types, or use type assertions in the template, which is only possible in Svelte 5). There's one limitation: Because of how the transformation works, we cannot infer generics. In other words, we will not catch type errors for bindings that rely on a generic type. The combination of generics + bindings is probably rare enough that this is okay, and we can revisit this later to try to find a way to make it work, if it comes up. Also note that this does not affect DOM element bindings like , this is only about component bindings. --- .../typescript/features/RenameProvider.ts | 10 +++++- .../bindings-two-way-check/Legacy.svelte | 7 ++++ .../bindings-two-way-check/Runes.svelte | 6 ++++ .../RunesGeneric.svelte | 6 ++++ .../expected_svelte_5.json | 36 +++++++++++++++++++ .../bindings-two-way-check/input.svelte | 19 ++++++++++ packages/svelte2tsx/repl/index.svelte | 12 +++---- .../src/htmlxtojsx_v2/nodes/Binding.ts | 6 ++++ .../htmlxtojsx_v2/nodes/InlineComponent.ts | 1 + packages/svelte2tsx/svelte-shims-v4.d.ts | 13 +++++++ .../samples/binding-bare/expected-svelte5.js | 4 +-- .../samples/binding/expected-svelte5.js | 8 ++--- .../editing-binding/expected-svelte5.js | 2 +- 13 files changed, 116 insertions(+), 14 deletions(-) create mode 100644 packages/language-server/test/plugins/typescript/features/diagnostics/fixtures/bindings-two-way-check/Legacy.svelte create mode 100644 packages/language-server/test/plugins/typescript/features/diagnostics/fixtures/bindings-two-way-check/Runes.svelte create mode 100644 packages/language-server/test/plugins/typescript/features/diagnostics/fixtures/bindings-two-way-check/RunesGeneric.svelte create mode 100644 packages/language-server/test/plugins/typescript/features/diagnostics/fixtures/bindings-two-way-check/expected_svelte_5.json create mode 100644 packages/language-server/test/plugins/typescript/features/diagnostics/fixtures/bindings-two-way-check/input.svelte diff --git a/packages/language-server/src/plugins/typescript/features/RenameProvider.ts b/packages/language-server/src/plugins/typescript/features/RenameProvider.ts index 3b9caf86e..8340478c3 100644 --- a/packages/language-server/src/plugins/typescript/features/RenameProvider.ts +++ b/packages/language-server/src/plugins/typescript/features/RenameProvider.ts @@ -463,8 +463,16 @@ export class RenameProviderImpl implements RenameProvider { const mappedLocations = await Promise.all( renameLocations.map(async (loc) => { const snapshot = await snapshots.retrieve(loc.fileName); + const text = snapshot.getFullText(); + const end = loc.textSpan.start + loc.textSpan.length; - if (!isTextSpanInGeneratedCode(snapshot.getFullText(), loc.textSpan)) { + if ( + !(snapshot instanceof SvelteDocumentSnapshot) || + (!isTextSpanInGeneratedCode(text, loc.textSpan) && + // prevent generated code for bindings from being renamed + // (it's not inside a generate comment because diagnostics should show up) + text.slice(end + 3, end + 27) !== '__sveltets_binding_value') + ) { return { ...loc, range: this.mapRangeToOriginal(snapshot, loc.textSpan), diff --git a/packages/language-server/test/plugins/typescript/features/diagnostics/fixtures/bindings-two-way-check/Legacy.svelte b/packages/language-server/test/plugins/typescript/features/diagnostics/fixtures/bindings-two-way-check/Legacy.svelte new file mode 100644 index 000000000..18db75ea2 --- /dev/null +++ b/packages/language-server/test/plugins/typescript/features/diagnostics/fixtures/bindings-two-way-check/Legacy.svelte @@ -0,0 +1,7 @@ + + +{legacy1} +{legacy2} \ No newline at end of file diff --git a/packages/language-server/test/plugins/typescript/features/diagnostics/fixtures/bindings-two-way-check/Runes.svelte b/packages/language-server/test/plugins/typescript/features/diagnostics/fixtures/bindings-two-way-check/Runes.svelte new file mode 100644 index 000000000..299be4d66 --- /dev/null +++ b/packages/language-server/test/plugins/typescript/features/diagnostics/fixtures/bindings-two-way-check/Runes.svelte @@ -0,0 +1,6 @@ + + +{runes1} +{runes2} \ No newline at end of file diff --git a/packages/language-server/test/plugins/typescript/features/diagnostics/fixtures/bindings-two-way-check/RunesGeneric.svelte b/packages/language-server/test/plugins/typescript/features/diagnostics/fixtures/bindings-two-way-check/RunesGeneric.svelte new file mode 100644 index 000000000..6accfc4b3 --- /dev/null +++ b/packages/language-server/test/plugins/typescript/features/diagnostics/fixtures/bindings-two-way-check/RunesGeneric.svelte @@ -0,0 +1,6 @@ + + +{foo} +{bar} \ No newline at end of file diff --git a/packages/language-server/test/plugins/typescript/features/diagnostics/fixtures/bindings-two-way-check/expected_svelte_5.json b/packages/language-server/test/plugins/typescript/features/diagnostics/fixtures/bindings-two-way-check/expected_svelte_5.json new file mode 100644 index 000000000..6b1d57c83 --- /dev/null +++ b/packages/language-server/test/plugins/typescript/features/diagnostics/fixtures/bindings-two-way-check/expected_svelte_5.json @@ -0,0 +1,36 @@ +[ + { + "code": 2322, + "message": "Type 'string | number' is not assignable to type 'string'.\n Type 'number' is not assignable to type 'string'.", + "range": { + "end": { + "character": 28, + "line": 17 + }, + "start": { + "character": 28, + "line": 17 + } + }, + "severity": 1, + "source": "ts", + "tags": [] + }, + { + "code": 2322, + "message": "Type 'string | number' is not assignable to type 'string'.\n Type 'number' is not assignable to type 'string'.", + "range": { + "end": { + "character": 45, + "line": 18 + }, + "start": { + "character": 45, + "line": 18 + } + }, + "severity": 1, + "source": "ts", + "tags": [] + } +] diff --git a/packages/language-server/test/plugins/typescript/features/diagnostics/fixtures/bindings-two-way-check/input.svelte b/packages/language-server/test/plugins/typescript/features/diagnostics/fixtures/bindings-two-way-check/input.svelte new file mode 100644 index 000000000..49020f784 --- /dev/null +++ b/packages/language-server/test/plugins/typescript/features/diagnostics/fixtures/bindings-two-way-check/input.svelte @@ -0,0 +1,19 @@ + + + + + + + + + + + diff --git a/packages/svelte2tsx/repl/index.svelte b/packages/svelte2tsx/repl/index.svelte index 49517cc72..bf8061fb0 100644 --- a/packages/svelte2tsx/repl/index.svelte +++ b/packages/svelte2tsx/repl/index.svelte @@ -1,7 +1,7 @@ - - -{#if value} - -{/if} + + \ No newline at end of file diff --git a/packages/svelte2tsx/src/htmlxtojsx_v2/nodes/Binding.ts b/packages/svelte2tsx/src/htmlxtojsx_v2/nodes/Binding.ts index 5bb17cca7..555269b61 100644 --- a/packages/svelte2tsx/src/htmlxtojsx_v2/nodes/Binding.ts +++ b/packages/svelte2tsx/src/htmlxtojsx_v2/nodes/Binding.ts @@ -131,6 +131,12 @@ export function handleBinding( if (isSvelte5Plus && element instanceof InlineComponent) { // To check if property is actually bindable element.appendToStartEnd([`${element.name}.$$bindings = '${attr.name}';`]); + // To check if the binding is also assigned to the variable (only works when there's no assertion, we can't transform that) + if (!isTypescriptNode(attr.expression)) { + element.appendToStartEnd([ + `${expressionStr} = __sveltets_binding_value(${element.originalName}, '${attr.name}');` + ]); + } } if (element instanceof Element) { diff --git a/packages/svelte2tsx/src/htmlxtojsx_v2/nodes/InlineComponent.ts b/packages/svelte2tsx/src/htmlxtojsx_v2/nodes/InlineComponent.ts index b1bab058d..7459f227e 100644 --- a/packages/svelte2tsx/src/htmlxtojsx_v2/nodes/InlineComponent.ts +++ b/packages/svelte2tsx/src/htmlxtojsx_v2/nodes/InlineComponent.ts @@ -39,6 +39,7 @@ export class InlineComponent { private startTagEnd: number; private isSelfclosing: boolean; public child?: any; + public originalName = this.node.name; // Add const $$xxx = ... only if the variable name is actually used // in order to prevent "$$xxx is defined but never used" TS hints diff --git a/packages/svelte2tsx/svelte-shims-v4.d.ts b/packages/svelte2tsx/svelte-shims-v4.d.ts index 87e365798..5227cb041 100644 --- a/packages/svelte2tsx/svelte-shims-v4.d.ts +++ b/packages/svelte2tsx/svelte-shims-v4.d.ts @@ -254,3 +254,16 @@ declare function __sveltets_2_isomorphic_component< declare function __sveltets_2_isomorphic_component_slots< Props extends Record, Events extends Record, Slots extends Record, Exports extends Record, Bindings extends string >(klass: {props: Props, events: Events, slots: Slots, exports?: Exports, bindings?: Bindings }): __sveltets_2_IsomorphicComponent<__sveltets_2_PropsWithChildren, Events, Slots, Exports, Bindings>; + +type __sveltets_NonUndefined = T extends undefined ? never : T; + +declare function __sveltets_binding_value< + // @ts-ignore this is only used for Svelte 5, which knows about the Component type + Comp extends typeof import('svelte').Component, + Key extends string +>(comp: Comp, key: Key): Key extends keyof import('svelte').ComponentProps ? + // bail on unknown because it hints at a generic type which we can't properly resolve here + // remove undefined because optional properties have it, and would result in false positives + unknown extends import('svelte').ComponentProps[Key] ? any : __sveltets_NonUndefined[Key]> : any; +// Overload to ensure typings that only use old SvelteComponent class or something invalid are gracefully handled +declare function __sveltets_binding_value(comp: any, key: string): any diff --git a/packages/svelte2tsx/test/htmlx2jsx/samples/binding-bare/expected-svelte5.js b/packages/svelte2tsx/test/htmlx2jsx/samples/binding-bare/expected-svelte5.js index bb934f658..43291dbae 100644 --- a/packages/svelte2tsx/test/htmlx2jsx/samples/binding-bare/expected-svelte5.js +++ b/packages/svelte2tsx/test/htmlx2jsx/samples/binding-bare/expected-svelte5.js @@ -1,5 +1,5 @@ { svelteHTML.createElement("input", { "type":`text`,"bind:value":value,});/*Ωignore_startΩ*/() => value = __sveltets_2_any(null);/*Ωignore_endΩ*/} { svelteHTML.createElement("input", { "type":`checkbox`,"bind:checked":checked,});/*Ωignore_startΩ*/() => checked = __sveltets_2_any(null);/*Ωignore_endΩ*/} - { const $$_tupnI0C = __sveltets_2_ensureComponent(Input); const $$_tupnI0 = new $$_tupnI0C({ target: __sveltets_2_any(), props: { "type":`text`,value,}});/*Ωignore_startΩ*/() => value = __sveltets_2_any(null);/*Ωignore_endΩ*/$$_tupnI0.$$bindings = 'value';} - { const $$_tupnI0C = __sveltets_2_ensureComponent(Input); const $$_tupnI0 = new $$_tupnI0C({ target: __sveltets_2_any(), props: { "type":`checkbox`,checked,}});/*Ωignore_startΩ*/() => checked = __sveltets_2_any(null);/*Ωignore_endΩ*/$$_tupnI0.$$bindings = 'checked';} \ No newline at end of file + { const $$_tupnI0C = __sveltets_2_ensureComponent(Input); const $$_tupnI0 = new $$_tupnI0C({ target: __sveltets_2_any(), props: { "type":`text`,value,}});/*Ωignore_startΩ*/() => value = __sveltets_2_any(null);/*Ωignore_endΩ*/$$_tupnI0.$$bindings = 'value';value = __sveltets_binding_value(Input, 'value');} + { const $$_tupnI0C = __sveltets_2_ensureComponent(Input); const $$_tupnI0 = new $$_tupnI0C({ target: __sveltets_2_any(), props: { "type":`checkbox`,checked,}});/*Ωignore_startΩ*/() => checked = __sveltets_2_any(null);/*Ωignore_endΩ*/$$_tupnI0.$$bindings = 'checked';checked = __sveltets_binding_value(Input, 'checked');} \ No newline at end of file diff --git a/packages/svelte2tsx/test/htmlx2jsx/samples/binding/expected-svelte5.js b/packages/svelte2tsx/test/htmlx2jsx/samples/binding/expected-svelte5.js index 6c881fec9..355227003 100644 --- a/packages/svelte2tsx/test/htmlx2jsx/samples/binding/expected-svelte5.js +++ b/packages/svelte2tsx/test/htmlx2jsx/samples/binding/expected-svelte5.js @@ -2,7 +2,7 @@ { svelteHTML.createElement("input", { "type":`text`,"bind:value":test,});/*Ωignore_startΩ*/() => test = __sveltets_2_any(null);/*Ωignore_endΩ*/} { svelteHTML.createElement("input", { "type":`text`,"bind:value":test,});/*Ωignore_startΩ*/() => test = __sveltets_2_any(null);/*Ωignore_endΩ*/} - { const $$_tupnI0C = __sveltets_2_ensureComponent(Input); const $$_tupnI0 = new $$_tupnI0C({ target: __sveltets_2_any(), props: { "type":`text`,value:test,}});/*Ωignore_startΩ*/() => test = __sveltets_2_any(null);/*Ωignore_endΩ*/$$_tupnI0.$$bindings = 'value';} - { const $$_tupnI0C = __sveltets_2_ensureComponent(Input); const $$_tupnI0 = new $$_tupnI0C({ target: __sveltets_2_any(), props: { "type":`text`,value:test,}});/*Ωignore_startΩ*/() => test = __sveltets_2_any(null);/*Ωignore_endΩ*/$$_tupnI0.$$bindings = 'value';} - { const $$_tupnI0C = __sveltets_2_ensureComponent(Input); const $$_tupnI0 = new $$_tupnI0C({ target: __sveltets_2_any(), props: { "type":`text`,value:test,}});/*Ωignore_startΩ*/() => test = __sveltets_2_any(null);/*Ωignore_endΩ*/$$_tupnI0.$$bindings = 'value';} - { const $$_tupnI0C = __sveltets_2_ensureComponent(Input); const $$_tupnI0 = new $$_tupnI0C({ target: __sveltets_2_any(), props: { "type":`text`,value:test,}});/*Ωignore_startΩ*/() => test = __sveltets_2_any(null);/*Ωignore_endΩ*/$$_tupnI0.$$bindings = 'value'; Input} \ No newline at end of file + { const $$_tupnI0C = __sveltets_2_ensureComponent(Input); const $$_tupnI0 = new $$_tupnI0C({ target: __sveltets_2_any(), props: { "type":`text`,value:test,}});/*Ωignore_startΩ*/() => test = __sveltets_2_any(null);/*Ωignore_endΩ*/$$_tupnI0.$$bindings = 'value';test = __sveltets_binding_value(Input, 'value');} + { const $$_tupnI0C = __sveltets_2_ensureComponent(Input); const $$_tupnI0 = new $$_tupnI0C({ target: __sveltets_2_any(), props: { "type":`text`,value:test,}});/*Ωignore_startΩ*/() => test = __sveltets_2_any(null);/*Ωignore_endΩ*/$$_tupnI0.$$bindings = 'value';test = __sveltets_binding_value(Input, 'value');} + { const $$_tupnI0C = __sveltets_2_ensureComponent(Input); const $$_tupnI0 = new $$_tupnI0C({ target: __sveltets_2_any(), props: { "type":`text`,value:test,}});/*Ωignore_startΩ*/() => test = __sveltets_2_any(null);/*Ωignore_endΩ*/$$_tupnI0.$$bindings = 'value';test = __sveltets_binding_value(Input, 'value');} + { const $$_tupnI0C = __sveltets_2_ensureComponent(Input); const $$_tupnI0 = new $$_tupnI0C({ target: __sveltets_2_any(), props: { "type":`text`,value:test,}});/*Ωignore_startΩ*/() => test = __sveltets_2_any(null);/*Ωignore_endΩ*/$$_tupnI0.$$bindings = 'value';test = __sveltets_binding_value(Input, 'value'); Input} \ No newline at end of file diff --git a/packages/svelte2tsx/test/htmlx2jsx/samples/editing-binding/expected-svelte5.js b/packages/svelte2tsx/test/htmlx2jsx/samples/editing-binding/expected-svelte5.js index 7bb207fea..31e8b2747 100644 --- a/packages/svelte2tsx/test/htmlx2jsx/samples/editing-binding/expected-svelte5.js +++ b/packages/svelte2tsx/test/htmlx2jsx/samples/editing-binding/expected-svelte5.js @@ -1,3 +1,3 @@ { svelteHTML.createElement("input", { });obj = __sveltets_2_any(null);} { svelteHTML.createElement("input", { "bind:value":obj.,});/*Ωignore_startΩ*/() => obj = __sveltets_2_any(null);/*Ωignore_endΩ*/} - { const $$_tupnI0C = __sveltets_2_ensureComponent(Input); const $$_tupnI0 = new $$_tupnI0C({ target: __sveltets_2_any(), props: { value:obj.,}});/*Ωignore_startΩ*/() => obj = __sveltets_2_any(null);/*Ωignore_endΩ*/$$_tupnI0.$$bindings = 'value';} \ No newline at end of file + { const $$_tupnI0C = __sveltets_2_ensureComponent(Input); const $$_tupnI0 = new $$_tupnI0C({ target: __sveltets_2_any(), props: { value:obj.,}});/*Ωignore_startΩ*/() => obj = __sveltets_2_any(null);/*Ωignore_endΩ*/$$_tupnI0.$$bindings = 'value';obj = __sveltets_binding_value(Input, 'value');} \ No newline at end of file