sloops77 / eslint-plugin-better-mutation

Eslint rules to enforce function-scope mutation only
MIT License
20 stars 6 forks source link

change how end of block works for determing let scope #22

Open nweber-gh opened 3 years ago

nweber-gh commented 3 years ago

We have code like this, which I believe should be valid.

function fooValid1() {
  let a = 2;
  [1, 2].forEach((x) => {
    a = a + x;
  });
  [1, 2].forEach(function(x) {
    a = a + x;
  });
  return a;
}

Since the let is in the same "local scope" (so to speak) as the forEach callback I believe that mutation should be allowed. To contrast, this code should not be allowed.

function doMutation(a) {
  [1, 2].forEach((x) => {
    a = a + x;
  });
  [1, 2].forEach(function (x) {
    a = a + x;
  });
  return a;
}

function fooInvalid() {
  let a = 2;
  doMutation(a);
}

Doing some debugging, this appears to not work as I expect because the "upwards" recursion when looking for the let definition stops at the callback scope. By letting the recursion continue this works exactly like I expect it should.

A lot of tests started to fail if I simply omitted the function and arrow from the isEndOfBlock predicate though, so I have doubts that this is the fully correct solution. Perhaps it is appropriate for the let checks and not other spots that use the function.. But I don't have enough context about all the uses to understand.

For example, this test fails:

[1,2,3].reduce((acc, x) => {
  acc += x;
  return acc;
}, 0);

The failure indicates that acc is not allowed to be mutated. With that said, I'm actually a little confused that this test passes on master, since acc is a function argument, but I'm guessing that I'm missing something.

I could use some extra eyes on this to determine if this change is appropriate.