track assignment locations for infinite loop reporting

[Rich-Harris]

An idea to close #13256. I haven't completely worked out if I like it or not.

Basically, it's tricky to add location information to effects, because sometimes template effects are grouped together (and the assignment that causes the infinite loop could be in a <span>{n++}</span> or whatever). But we can add location information for the assignments that cause the infinite loops, which gives us more granular information:

Caveats:

• it can't track assignments that happen outside .svelte/.svelte.js files (though it fails gracefully in these cases)
• it doesn't (yet?) track array mutations or e.g. Object.assign(...) or set.add(blah)
• right now it just reports the most recent assignment, maybe that's insufficient

Notwithstanding those caveats, I kinda prefer this to what we have currently.

Before submitting the PR, please make sure you do the following

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• Prefix your PR title with feat:, fix:, chore:, or docs:.
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests and linting

• Run the tests with pnpm test and lint the project with pnpm lint

[changeset-bot]

⚠️ No Changeset found

Latest commit: f5b525e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[trueadm]

I'm also not really sure if I like it too. It seems useful, but if it is fragile then will it just cause more frustrations in the long term?

[dummdidumm]

Knowing which assignment it was exactly last is useful, but for more involved loops I also want to know the stack. What about doing this as an additional thing rather than replacing what was there before? i.e. you get the "last ten effects were ..." (maybe with a note that you'll see the transformed contents and can look up the source by exanding it) + what we have here.

[trueadm]

@dummdidumm Having used the last 10 effects logic since it landed, I've actually not found it terribly useful at all. Firstly, 10 is far too many and many of them are internal effects which are just confusing to present to a user – then then effects I look at don't really tell me anything. Like, you really need to know the effect and the dependency, without the dependency it's lost.

[Rich-Harris]

if it is fragile

All that happens is you don't get the location — for example if you do array.push(...) inside an effect, because we don't currently mark those call expressions (unclear whether we should)

[dummdidumm]

"all that happens" sounds like it's no big deal, but it's basically impossible to know what caused this then just by looking at the error message.

Like, you really need to know the effect and the dependency, without the dependency it's lost

what do you mean by "you need to know the dependency"? How could we present that?

many of them are internal effects which are just confusing to present to a user

we can filter them out. I didn't do that originally because I feared that it could make loops harder to debug where a render effect plays a role (AFAIK it could happen if you have a binding that is repeatedly set from above and the below again, but maybe that's not true / not worth for the common case where it's noisy)

[Rich-Harris]

One thing we could do is annotate user effects, so that if an update happens that isn't tracked (because it's an array.push or a map.set or whatever), we still track the effect in which it happened. So if the offending effect does x++ then you'll get

effect_update_depth_exceeded Maximum update depth exceeded after assignment at App.svelte:9:2

but if it does array.push(blah) then you'll get

effect_update_depth_exceeded Maximum update depth exceeded during effect at App.svelte:9:2

We'd miss cases like <span>{array.push(blah)}</span> but those are imaginary. If we needed to add additional logic for bindings then we could do that too

[trueadm]

One thing we could do is annotate user effects, so that if an update happens that isn't tracked (because it's an array.push or a map.set or whatever), we still track the effect in which it happened. So if the offending effect does x++ then you'll get

effect_update_depth_exceeded Maximum update depth exceeded after assignment at App.svelte:9:2

but if it does array.push(blah) then you'll get

effect_update_depth_exceeded Maximum update depth exceeded during effect at App.svelte:9:2

We'd miss cases like <span>{array.push(blah)}</span> but those are imaginary. If we needed to add additional logic for bindings then we could do that too

That sounds like a good idea.

[dummdidumm]

I'd still like to see a list of effects leading up to this. I'm confident we can get that on the effect object to then print it out.