Bind handler incorrectly invalidates other variables.

[discapes]

Describe the bug

REPL

Expected behaviour: program never changes so it is never invalidated.
Actual behaviour: program is invalidated inside adder_x_binding, so every time the counter is increased.

Expected compiler output:
function adder_x_binding(value) { counter = value; $$invalidate(1, counter); }
Actual compiler output:
function adder_x_binding(value) { counter = value; ($$invalidate(1, counter), $$invalidate(0, program)); }

Severity marked as "blocking an upgrade" as the REPL works in 3.0.1

Reproduction

REPL

Logs

No response

System Info

Example REPL tested in Firefox 104 on Windows 10.

Severity

blocking an upgrade

[discapes]

I looked at the source code, and it seems like when generating adder_x_binding, the following line is added (in render_dom/wrappers/Element/index.ts:679):
${this.renderer.invalidate(dep)}; - this line is supposed to invalidate counter.

When looking at renderer_invalidate in render_dom/invalidate.ts:132, I found that 'program' gets added to the deps set when calling the function with name: 'counter'. The deps, ie. program and counter are then invalidated.

I found out that the bug is the result of a bugfix of #2444. It is the reason that deps exist at all. Check out the REPL in the issue for insight. The REPL's behaviour was fixed in 3.2.1, and after that, if you have a reactive statement like $: x = y, then changing x.foo invalidates y too. This fixed the REPL in the issue, where x is a filtered and displayed version of the todos array (y). So the event handler for the checkbox that changes filtered[/*eachBlockIndex*/].done also invalidates todos.

This behaviour happens in renderer_invalidate, where when generating an invalidate for x, it checks all reactive statements to see if x is ever set by assignment to another variable, and then invalidates that too:

    // I added some comments to tie the above example of x and y to this piece of code
const deps = new Set([name/*x*/]);
deps.forEach(name/*x*/ => {
	const reactive_declarations = renderer.component.reactive_declarations.filter(z =>
            /* get all reactive declarations where name is assigned to */
		z.assignees.has(name)
	);
	reactive_declarations.forEach(declaration => {
              /* invalidate all dependencies of the reactive declaration */
		declaration.dependencies.forEach(name => deps.add(name));
	});)
});

It fixes the specific REPL, but introduces problems which are the source of many other issues (I would consider looking into at least #4933, #4448, #7416 and #7574)

• For example the case is where the "x" is reassigned entirely (x=foo). Only changing a property of x should result in y getting invalidated.
• Also, if the "y" in the reactive statement is a primitive, it's copied by value, so changing "x" (that was copied by value) shouldn't invalidate the primitive it was copied from.
• Third, the assignment might never happen, the reactive declaration could even be $: if (false) x = y

In my example, all of these are fulfilled. To fix the issues entirely, we would need to add checks for all of the above cases before invalidating other variables.

However, since the behaviour that #2444 introduced now works as it did before (change the todo REPL version to 3.49, it doesn't work again), I propose to just remove the code snippet entirely. Also, I think the behaviour that changing filtered should invalidate todos is not necessarily wanted, since the docs say nothing about it, and instead beginners are taught that variables are invalidated only by direct assignment or binding. In the other case, you would also need to consider assignments like x = y that happen outside of reactive declarations, which is a far bigger task.

[dummdidumm]

This will be fixed in Svelte 5, the dependencies are tracked at runtime now, resulting in the correct behavior. Furthermore, with the new runes API, such cases will be far easier to reason about, as you can clearly separate derivations and side effects through $derived and $effect.