Skip to content

fix: detect mutations within assignments expressions (alternative approach) #12429

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Jul 15, 2024

Conversation

Rich-Harris
Copy link
Member

Alternative to #12411.

We use the extract_identifiers utility to find the identifiers in patterns, like these:

let a = ...; // ['a']
let { b, c: d } = ...; // ['b', 'd']

Patterns also occur in e.g. function declarations and assignment expressions. But there are two kinds of pattern — binding patterns and assignment patterns — and they're subtly different. Binding patterns can ultimately only contain identifiers (the things being declared), while assignment patterns can contain any lvalue (i.e. Identifier or MemberExpression in ESTree terminology).

The drawback of #12411 is that it lumps identifiers and member expressions together. This works, but it has a cost — it means that the bindings referenced in an assignment pattern are always marked as reassigned (rather than merely mutated) even if they're member expressions. This means that the compiler sees something like this...

function swap() {
  [array[1], array[0]] = [array[0], array[1]];
}

...and marks array as being reassigned, which means that we have to create a source for it:

-let array = $.proxy([1, 2]);
+let array = $.source($.proxy([1, 2]));

It's better if we can differentiate between these things. That's what this PR does: it expands the pattern unwrapping logic to include member expression nodes, and updates the AssignmentExpression logic to use it. The existing extract_identifiers function is kept with the same API, by calling unwrap_pattern under the hood but filtering out non-identifier nodes.

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

dummdidumm and others added 6 commits July 11, 2024 20:36
This enhances our "variable was mutated" detection to also recognize that `foo` in `[foo[1]] = [..]` was mutated. This allows us to be more granular about detecting mutations to each expressions in legacy mode, which fixes #12169
Copy link

changeset-bot bot commented Jul 12, 2024

🦋 Changeset detected

Latest commit: a80ea10

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte Patch

Not sure what this means? Click here to learn what changesets are.

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

@dummdidumm dummdidumm merged commit a8dc96e into main Jul 15, 2024
9 checks passed
@dummdidumm dummdidumm deleted the each-block-mutation-detection-alt branch July 15, 2024 07:08
trueadm pushed a commit that referenced this pull request Jul 16, 2024
…roach) (#12429)

This enhances our "variable was mutated" detection to also recognize that `foo` in `[foo[1]] = [..]` was mutated. This allows us to be more granular about detecting mutations to each expressions in legacy mode, which fixes #12169

---------

Co-authored-by: Simon Holthausen <simon.holthausen@vercel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants