Potential false positive for ownership_invalid_binding warning

[reinhard-sanz]

Describe the bug

I've found a potential false positive for ownership_invalid_binding warning

I have a class instance in another file that is exported. I hand over said instance to a component.
When i now change an instance prop (which is a state underneath) i get this error.

If it is not a false positive, a quick explanation why is much appreciated.

See REPL for clarity

Reproduction

https://svelte.dev/playground/cb79cd8b0fcf4811a545b187df94e6e6?version=5.25.10

Logs

System Info

System:
    OS: Linux 5.15 Ubuntu 22.04.4 LTS 22.04.4 LTS (Jammy Jellyfish)
    CPU: (8) x64 Intel(R) Core(TM) i7-10510U CPU @ 1.80GHz
    Memory: 8.00 GB / 15.50 GB
    Container: Yes
    Shell: 5.1.16 - /bin/bash
  Binaries:
    Node: 22.10.0 - ~/.nvm/versions/node/v22.10.0/bin/node
    Yarn: 1.22.22 - /mnt/c/Users/Reinhard/AppData/Roaming/npm/yarn
    npm: 11.1.0 - ~/.nvm/versions/node/v22.10.0/bin/npm
    pnpm: 10.3.0 - ~/.local/share/pnpm/pnpm
    bun: 1.1.13 - /mnt/c/Users/Reinhard/AppData/Roaming/npm/bun
  npmPackages:
    rollup: ^4.24.0 => 4.34.8

Severity

annoyance

[reinhard-sanz]

Sorry just saw there is a merged update for the ownership_invalid_binding that is waiting to be released.
Will reopen if this fix does not solve the problem!

[brunnerh]

This is likely not a false positive and the recent fix is unrelated (it pertains to binding fallbacks).

The logic for how this warning is triggered and its wording have been changed in #15678.
The intention of the warning is that it should be clear in the code where changes happen. In your example you have this code:

<Sidebar sidebar={sidebarController.leftSidebar} />

This looks like the component is just reading the passed in value, but it is being mutated instead, which could have unintended side effects elsewhere in the application.

If these side effects are intended, you are encouraged to make this clear by using a binding instead:

<Sidebar bind:sidebar={sidebarController.leftSidebar} />

[dummdidumm]

This is a false positive regardless and should be fixed, after https://github.com/sveltejs/svelte/blob/main/packages/svelte/src/internal/client/dev/ownership.js#L34 we're missing a check whether or not the given value is a state proxy (if not we gotta assume it's fine, just like we do in the loop that follows). Reopening

[brunnerh]

we're missing a check whether or not the given value is a state proxy (if not we gotta assume it's fine [...]

Can you elaborate on why that is? This seems a bit confusing.

[reinhard-sanz]

@brunnerh Even if i do that:

<Sidebar bind:sidebar={sidebarController.leftSidebar} />

I just get another error (although not a svelte error) because under the hood the leftSidebar is actually just exposed via a getter 😅

See here:
https://svelte.dev/playground/d8f57aac03a14c65b959586d3653d4b0?version=5.25.10

Edit:
Locally i do get a svelte warning, but also just another one because the getter :/
[svelte] binding_property_non_reactivebind:sidebar={uiState.rightSidebar} (src/App.svelte:139:14) is binding to a non-reactive property

[brunnerh]

Things like this are why I am not a huge fan of this warning in general 😅

You can also run into this issue e.g. with the #each item, if you want to modify the object, because it cannot be bound either.

{#each items as item}
  <Editor bind:item /> <!-- errors -->
{/each}

[laszlokorte]

I suddenly get very many of these warnings. My use case is the following:

https://svelte.dev/playground/c8bdc97915a84239b3de8481ede9076b?version=5.25.10

<script>
	const atom = new customStore() // custom class that uses $state() internally, has setter and getter for `value`
	const name = view('name', atom, 'unknown') // custom proxy that reads/writes the "name" field from inside the store
       // the proxy itself is constant, but it exposes setter/getter named `value`
</script>

<p>Clicking the button causes a warning</p>

<h1>Hello {name.value}!</h1> <-- Outputs the name value, no warning

<input type="text" bind:value={name.value} /> <-- binds the name value, no warning

<Component name={name} />
can not use bind:name because name is const, but the Component shall be able to set name.value=...
but as soon as the setter name.value = is called, svelte reports a warning

[Thiagolino8]

@laszlokorte, In your case the warning is correct, your options are:
use let
use function bindings

[laszlokorte]

Using let is not a solution because the name variable is not meant to be reassigned it is explicitly meant to be const.

The function binding syntax is really verbose.
Whats the point of the warning if it just pushes the use of such verbose syntax as workaround?
I mean semantically my original code is working and has been working all the time.

If there would be unexpected behavior caused by my code then the warning should explain that.
But I really do not see the point.

[Thiagolino8]

Whats the point of the warning if it just pushes the use of such verbose syntax as workaround?

It's not a workaround, it's a statement of intent
Data should flow downwards by default, if you think this case is an exception then you should make that clear

[laszlokorte]

Sorry but I disagree.
const in javascript marks only the variable as not-reassignable, but not the full object as deeply immutable/frozen.
So whenever I pass a object as a prop into a component there a million ways to modify something inside the object.

As can be observed by the correct behavior of my example code, this internal modification of the object is handled correctly by svelte and the $state signal --- as one would expect, since this is the whole point of signals.

This counters the claim that data should always only flow downward.
I updated my example to should that it is trivially possible to modify the object by calling a method instead of a setter without triggering the warning:
https://svelte.dev/playground/c8bdc97915a84239b3de8481ede9076b?version=5.25.10

<button onclick={() => {name.value = "name1"}}>button1 causes warning</button>
<button onclick={() => {name.modify("name2")}}>button2 no warning</button>

maybe the warning should only be applied to objects/properties that do not define get(){}/set(){}?
I accept that there might be important cases in which the warning is appropriate but certainly not for preventing a.b= c, while allowing a.b(c)

Update: maybe I am simply misunderstanding the warning and the interplay between $bindable and deep mutability.

Update 2: I now got my usecase working by using writable instead of $state. A store can be passed as prop into a component without using ´bind´ and be modified inside the child component using store.set(foo) or $store = foo.
But now I am wondering if this is "allowed" or if in the future this will cause a warning as well:

https://svelte.dev/playground/c9a78e6425584356b797a2a63e92890a?version=5.25.10

[paoloricciuti]

@laszlokorte the warning is trying to prevent you from mutating state that you don't own...in this case the state is created in the parent component and it's better to modify it there be it through a method (like your modify or by passing a getter. You could technically bind to name.value directly if you want.

[laszlokorte]

Ok then my point is: why should a setter defined as set foo(newVal) { } not be handled the same as a method like modify(v) { ... }?
Either both should emit a warning or neither should or the warning should make the difference between the two cases more clear and explain whats the technical issue with the setter, except for some philosophical point of data flowing downwards.

Will the following example using writable also emit a warning in the future because the writable object is passed into the Sub component without using bind:?
https://svelte.dev/playground/ea7ca2df4a0f438c89fc9e81d13e872a?version=5.26.2