fix: bail out if slot name changes and $$slots assigned to variable (#13678)

paoloricciuti · web-flow · commit 18c5a5ba2d06 · 2024-10-19T01:45:53.000+02:00
diff --git a/.changeset/early-rockets-join.md b/.changeset/early-rockets-join.md
@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: bail out if slot name changes and $$slots assigned to variable
diff --git a/packages/svelte/src/compiler/migrate/index.js b/packages/svelte/src/compiler/migrate/index.js
@@ -418,7 +418,7 @@ const instance_script = {
 			state.str.remove(/** @type {number} */ (node.start), /** @type {number} */ (node.end));
 		}
 	},
-	VariableDeclaration(node, { state, path, visit }) {
+	VariableDeclaration(node, { state, path, visit, next }) {
 		if (state.scope !== state.analysis.instance.scope) {
 			return;
 		}
@@ -439,12 +439,14 @@ const instance_script = {
 				bindings = state.scope.get_bindings(declarator);
 			} catch (e) {
 				// no bindings, so we can skip this
+				next();
 				continue;
 			}
 			const has_state = bindings.some((binding) => binding.kind === 'state');
 			const has_props = bindings.some((binding) => binding.kind === 'bindable_prop');
 
 			if (!has_state && !has_props) {
+				next();
 				continue;
 			}
 
@@ -491,25 +493,31 @@ const instance_script = {
 
 				const prop = state.props.find((prop) => prop.exported === (binding.prop_alias || name));
 				if (prop) {
+					next();
 					// $$Props type was used
 					prop.init = declarator.init
-						? state.str.original.substring(
-								/** @type {number} */ (declarator.init.start),
-								/** @type {number} */ (declarator.init.end)
-							)
+						? state.str
+								.snip(
+									/** @type {number} */ (declarator.init.start),
+									/** @type {number} */ (declarator.init.end)
+								)
+								.toString()
 						: '';
 					prop.bindable = binding.updated;
 					prop.exported = binding.prop_alias || name;
 					prop.type_only = false;
 				} else {
+					next();
 					state.props.push({
 						local: name,
 						exported: binding.prop_alias ? binding.prop_alias : name,
 						init: declarator.init
-							? state.str.original.substring(
-									/** @type {number} */ (declarator.init.start),
-									/** @type {number} */ (declarator.init.end)
-								)
+							? state.str
+									.snip(
+										/** @type {number} */ (declarator.init.start),
+										/** @type {number} */ (declarator.init.end)
+									)
+									.toString()
 							: '',
 						optional: !!declarator.init,
 						bindable: binding.updated,
@@ -1036,6 +1044,11 @@ const template = {
 			name = existing_prop.local;
 		} else if (slot_name !== 'default') {
 			name = state.scope.generate(slot_name);
+			if (name !== slot_name) {
+				throw new Error(
+					'This migration would change the name of a slot making the component unusable'
+				);
+			}
 		}
 
 		if (!existing_prop) {
@@ -1462,7 +1475,12 @@ function handle_identifier(node, state, path) {
 			if (existing_prop) {
 				name = existing_prop.local;
 			} else if (name !== 'default') {
-				name = state.scope.generate(name);
+				let new_name = state.scope.generate(name);
+				if (new_name !== name) {
+					throw new Error(
+						'This migration would change the name of a slot making the component unusable'
+					);
+				}
 			}
 
 			name = name === 'default' ? 'children' : name;
@@ -1479,7 +1497,7 @@ function handle_identifier(node, state, path) {
 					// we start with any and delegate to when the slot
 					// is actually rendered (it might not happen in that case)
 					// any is still a safe bet
-					type: `import('svelte').Snippet<[any]>}`,
+					type: `import('svelte').Snippet<[any]>`,
 					needs_refine_type: true
 				});
 			}
diff --git a/packages/svelte/tests/migrate/samples/$$slots-used-as-variable-$$props/input.svelte b/packages/svelte/tests/migrate/samples/$$slots-used-as-variable-$$props/input.svelte
@@ -0,0 +1,10 @@
+<script>
+	let showMessage = $$slots.message;
+	$: extraTitle = $$slots.extra;
+</script>
+
+{#if showMessage}
+	<slot name="message" />
+{/if}
+
+{$$props}
diff --git a/packages/svelte/tests/migrate/samples/$$slots-used-as-variable-$$props/output.svelte b/packages/svelte/tests/migrate/samples/$$slots-used-as-variable-$$props/output.svelte
@@ -0,0 +1,12 @@
+<script>
+	/** @type {{message?: import('svelte').Snippet, extra?: import('svelte').Snippet<[any]>, [key: string]: any}} */
+	let { ...props } = $props();
+	let showMessage = props.message;
+	let extraTitle = $derived(props.extra);
+</script>
+
+{#if showMessage}
+	{@render props.message?.()}
+{/if}
+
+{props}
diff --git a/packages/svelte/tests/migrate/samples/$$slots-used-as-variable/input.svelte b/packages/svelte/tests/migrate/samples/$$slots-used-as-variable/input.svelte
@@ -0,0 +1,11 @@
+<script>
+	export let showMessage = $$slots.message;
+
+	let showTitle = $$slots.title;
+
+	$: extraTitle = $$slots.extra;
+</script>
+
+{#if showMessage}
+	<slot name="message" />
+{/if}
diff --git a/packages/svelte/tests/migrate/samples/$$slots-used-as-variable/output.svelte b/packages/svelte/tests/migrate/samples/$$slots-used-as-variable/output.svelte
@@ -0,0 +1,17 @@
+<script>
+	/** @type {{message?: import('svelte').Snippet, showMessage?: any, title?: import('svelte').Snippet<[any]>, extra?: import('svelte').Snippet<[any]>}} */
+	let {
+		message,
+		showMessage = message,
+		title,
+		extra
+	} = $props();
+
+	let showTitle = title;
+
+	let extraTitle = $derived(extra);
+</script>
+
+{#if showMessage}
+	{@render message?.()}
+{/if}
diff --git a/packages/svelte/tests/migrate/samples/impossible-migrate-slot-change-name/_config.js b/packages/svelte/tests/migrate/samples/impossible-migrate-slot-change-name/_config.js
@@ -0,0 +1,7 @@
+import { test } from '../../test';
+
+export default test({
+	logs: [
+		'One or more `@migration-task` comments were added to `output.svelte`, please check them and complete the migration manually.'
+	]
+});
diff --git a/packages/svelte/tests/migrate/samples/impossible-migrate-slot-change-name/input.svelte b/packages/svelte/tests/migrate/samples/impossible-migrate-slot-change-name/input.svelte
@@ -0,0 +1,5 @@
+<script>
+	let body;
+</script>
+
+<slot name="body"></slot>
diff --git a/packages/svelte/tests/migrate/samples/impossible-migrate-slot-change-name/output.svelte b/packages/svelte/tests/migrate/samples/impossible-migrate-slot-change-name/output.svelte
@@ -0,0 +1,6 @@
+<!-- @migration-task Error while migrating Svelte code: This migration would change the name of a slot making the component unusable -->
+<script>
+	let body;
+</script>
+
+<slot name="body"></slot>
diff --git a/packages/svelte/tests/migrate/samples/impossible-migrate-slot-non-identifier/_config.js b/packages/svelte/tests/migrate/samples/impossible-migrate-slot-non-identifier/_config.js
@@ -0,0 +1,7 @@
+import { test } from '../../test';
+
+export default test({
+	logs: [
+		'One or more `@migration-task` comments were added to `output.svelte`, please check them and complete the migration manually.'
+	]
+});
diff --git a/packages/svelte/tests/migrate/samples/impossible-migrate-slot-non-identifier/input.svelte b/packages/svelte/tests/migrate/samples/impossible-migrate-slot-non-identifier/input.svelte
@@ -0,0 +1 @@
+<slot name="dashed-name"></slot>
diff --git a/packages/svelte/tests/migrate/samples/impossible-migrate-slot-non-identifier/output.svelte b/packages/svelte/tests/migrate/samples/impossible-migrate-slot-non-identifier/output.svelte
@@ -0,0 +1,2 @@
+<!-- @migration-task Error while migrating Svelte code: This migration would change the name of a slot making the component unusable -->
+<slot name="dashed-name"></slot>
diff --git a/packages/svelte/tests/migrate/samples/slots-multiple/input.svelte b/packages/svelte/tests/migrate/samples/slots-multiple/input.svelte
@@ -1,5 +1,2 @@
 <button><slot /></button>
-<button><slot /></button>
-
-<slot name="dashed-name" />
-<slot name="dashed-name" />
+<button><slot /></button>
diff --git a/packages/svelte/tests/migrate/samples/slots-multiple/output.svelte b/packages/svelte/tests/migrate/samples/slots-multiple/output.svelte
@@ -1,10 +1,7 @@
 <script>
-	/** @type {{children?: import('svelte').Snippet, dashed_name?: import('svelte').Snippet}} */
-	let { children, dashed_name } = $props();
+	/** @type {{children?: import('svelte').Snippet}} */
+	let { children } = $props();
 </script>
 
 <button>{@render children?.()}</button>
-<button>{@render children?.()}</button>
-
-{@render dashed_name?.()}
-{@render dashed_name?.()}
+<button>{@render children?.()}</button>
diff --git a/packages/svelte/tests/migrate/samples/slots-with-$$props/input.svelte b/packages/svelte/tests/migrate/samples/slots-with-$$props/input.svelte
@@ -1,7 +1,7 @@
 <button><slot /></button>
 
-{#if foo}
-	<slot name="foo" {foo} />
+{#if foos}
+	<slot name="foo" foo={foos} />
 {/if}
 
 {#if $$slots.bar}
@@ -13,8 +13,4 @@
 
 {#if $$slots['default']}foo{/if}
 
-{#if $$slots['dashed-name']}foo{/if}
-
-<slot name="dashed-name" />
-
 <slot header="something" title={$$props.cool} {id} />
diff --git a/packages/svelte/tests/migrate/samples/slots-with-$$props/output.svelte b/packages/svelte/tests/migrate/samples/slots-with-$$props/output.svelte
@@ -1,14 +1,12 @@
 <script>
-	/** @type {{children?: import('svelte').Snippet, foo_1?: import('svelte').Snippet<[any]>, bar?: import('svelte').Snippet, dashed_name?: import('svelte').Snippet, [key: string]: any}} */
-	let {
-		...props
-	} = $props();
+	/** @type {{children?: import('svelte').Snippet, foo?: import('svelte').Snippet<[any]>, bar?: import('svelte').Snippet, [key: string]: any}} */
+	let { ...props } = $props();
 </script>
 
 <button>{@render props.children?.()}</button>
 
-{#if foo}
-	{@render props.foo_1?.({ foo, })}
+{#if foos}
+	{@render props.foo?.({ foo: foos, })}
 {/if}
 
 {#if props.bar}
@@ -20,8 +18,4 @@
 
 {#if props.children}foo{/if}
 
-{#if props.dashed_name}foo{/if}
-
-{@render props.dashed_name?.()}
-
 {@render props.children?.({ header: "something", title: props.cool, id, })}
diff --git a/packages/svelte/tests/migrate/samples/slots/input.svelte b/packages/svelte/tests/migrate/samples/slots/input.svelte
@@ -1,7 +1,7 @@
 <button><slot /></button>
 
-{#if foo}
-	<slot name="foo" {foo} />
+{#if foos}
+	<slot name="foo" foo={foos} />
 {/if}
 
 {#if $$slots.bar}
@@ -13,8 +13,4 @@
 
 {#if $$slots['default']}foo{/if}
 
-{#if $$slots['dashed-name']}foo{/if}
-
-<slot name="dashed-name" />
-
 <slot header="something" title={my_title} {id} />
diff --git a/packages/svelte/tests/migrate/samples/slots/output.svelte b/packages/svelte/tests/migrate/samples/slots/output.svelte
@@ -1,17 +1,12 @@
 <script>
-	/** @type {{children?: import('svelte').Snippet, foo_1?: import('svelte').Snippet<[any]>, bar?: import('svelte').Snippet, dashed_name?: import('svelte').Snippet}} */
-	let {
-		children,
-		foo_1,
-		bar,
-		dashed_name
-	} = $props();
+	/** @type {{children?: import('svelte').Snippet, foo?: import('svelte').Snippet<[any]>, bar?: import('svelte').Snippet}} */
+	let { children, foo, bar } = $props();
 </script>
 
 <button>{@render children?.()}</button>
 
-{#if foo}
-	{@render foo_1?.({ foo, })}
+{#if foos}
+	{@render foo?.({ foo: foos, })}
 {/if}
 
 {#if bar}
@@ -23,8 +18,4 @@
 
 {#if children}foo{/if}
 
-{#if dashed_name}foo{/if}
-
-{@render dashed_name?.()}
-
 {@render children?.({ header: "something", title: my_title, id, })}

-Original file line number
+Diff line change
@@ @@ -0,0 +1,5 @@ @@
 +---
 +'svelte': patch
 +---
++
 +fix: bail out if slot name changes and $$slots assigned to variable