creating state inside derived expressions should be forbidden

[Rich-Harris]

Describe the problem

Per https://github.com/sveltejs/svelte/pull/10240#issuecomment-1912206863 — if you have a case like this...

function createCounter() {
  let count = $state(0);
  return {
    get count() { return count },
    increment: () => count += 1
  }
}

let { count, increment } = $derived(createCounter());

...you're creating state inside the derived expression and then depending on that state. When the state changes, the derived expression is re-evaluated, recreating the original state.

Unsurprisingly, this does not do anything useful.

Describe the proposed solution

Creating state inside a derived expression should be an error.

Importance

nice to have

[trueadm]

This is complicated. This is only true of primitive state, but for deeply nested state, it's the only way some patterns can work unless you invoke effects to sync state (see my shopping cart REPL example).

[Rich-Harris]

I'm not convinced that's true — my fork of that example works without creating state inside deriveds

[Rich-Harris]

Simpler example — we decided that we want to prevent reads of state/deriveds created inside the derived, but creating them is okay (they might be class fields etc)

[Rich-Harris]

Looked into this a bit just more, and I think we should close this issue. This is the change I made:

diff --git a/packages/svelte/src/internal/client/reactivity/deriveds.js b/packages/svelte/src/internal/client/reactivity/deriveds.js
index 6b5fc8dae..a221e894e 100644
--- a/packages/svelte/src/internal/client/reactivity/deriveds.js
+++ b/packages/svelte/src/internal/client/reactivity/deriveds.js
@@ -27,6 +27,7 @@ export function derived(fn) {
    /** @type {Derived<V>} */
    const signal = {
        deps: null,
+       sources: null,
        deriveds: null,
        equals,
        f: flags,
@@ -111,6 +112,17 @@ export function update_derived(derived) {
        value = update_reaction(derived);
    }

+   var sources = derived.sources;
+   var deps = derived.deps;
+
+   if (sources !== null && deps !== null) {
+       for (const dep of deps) {
+           if (sources.includes(dep)) {
+               throw new Error('eek');
+           }
+       }
+   }
+
    var status =
        (current_skip_reaction || (derived.f & UNOWNED) !== 0) && derived.deps !== null
            ? MAYBE_DIRTY
diff --git a/packages/svelte/src/internal/client/reactivity/sources.js b/packages/svelte/src/internal/client/reactivity/sources.js
index 86427a750..0dd137484 100644
--- a/packages/svelte/src/internal/client/reactivity/sources.js
+++ b/packages/svelte/src/internal/client/reactivity/sources.js
@@ -36,13 +36,20 @@ let inspect_effects = new Set();
  */
 /*#__NO_SIDE_EFFECTS__*/
 export function source(v) {
-   return {
+   var source = {
        f: 0, // TODO ideally we could skip this altogether, but it causes type errors
        v,
        reactions: null,
        equals,
        version: 0
    };
+
+   if (current_reaction && (current_reaction.f & DERIVED) !== 0) {
+       // prettier-ignore
+       (/** @type {Derived} */ (current_reaction).sources ??= []).push(source);
+   }
+
+   return source;
 }

 /**
diff --git a/packages/svelte/src/internal/client/reactivity/types.d.ts b/packages/svelte/src/internal/client/reactivity/types.d.ts
index 0e5437709..54634c32f 100644
--- a/packages/svelte/src/internal/client/reactivity/types.d.ts
+++ b/packages/svelte/src/internal/client/reactivity/types.d.ts
@@ -30,6 +30,8 @@ export interface Reaction extends Signal {
 export interface Derived<V = unknown> extends Value<V>, Reaction {
    /** The derived function */
    fn: () => V;
+   /** Sources created inside this signal */
+   sources: null | Value[];
    /** Deriveds created inside this signal */
    deriveds: null | Derived[];
 }

The overhead is probably quite modest, but this is buggy — if we read properties of a state proxy inside the derived, that results in sources being created that 'belong' to the derived, even if the initial $state(...) declaration was outside it. The book-keeping required to avoid treating those lazily-created sources as local declarations would be very onerous, I think, and this issue hasn't really been a widespread problem.