Rule proposal: no-await-with-operators

[esamattis]

What category of rule is this? (place an "X" next to just one item)

[ ] Enforces code style
[x] Warns about a potential error
[ ] Suggests an alternate way of doing something
[ ] Other (please specify:)

Not sure about the rule name but I think this would be a good thing to avoid in any circumstances as it is very surprising.

Jake Archibald explains it very well here https://twitter.com/jaffathecake/status/999610181452533760

So to sum it up this should warn:

let x =  0;

async function test() {
  x += await 2;
  console.log(x)
}

and this

let x =  0;

async function test() {
  x = x + await 2;
  console.log(x)
}

but this should be ok

let x =  0;

async function test() {
  let two = await 2;
  x = x + two;
  console.log(x)
}

Why should this rule be included in ESLint (instead of a plugin)?

It's not related to any library and can be a source for really confusing bugs. This is not a very well known Javascript feature and I think it should be considered to be a "bad part" which should be avoided.

[ljharb]

I'm not sure this warrants a rule; in the linked tweet, this only is "confusing" because the variable is being mutated both inside and outside the async function.

[esamattis]

Well, this is quite simple example. The file size reading example in the video is way more confusing since the variable is syntax wise mutated only in one place.

const wait = t => new Promise(r => setTimeout(r, t));

const files = ["file1", "file12", "file123", "file1234", "file12345"];

async function getFileSize(file) {
    console.log("reading", file);
    await wait(Math.round(Math.random() * 100)); // simulate disk io slowness
    return file.length;
}

let total = 0;

async function sumAll() {
    await Promise.all(
        files.map(async file => {
            total += await getFileSize(file);
        }),
    );

    console.log(total); // quite random output
}

sumAll();

[not-an-aardvark]

To me, it seems like the confusing part of that code is the use of operator assignment with an await expression in the right-hand side.

x += await foo;

If I'm understanding correctly, if two functions run that code at the same time, x will effectively only be incremented once because await foo will be added to the original value of x on both runs.

Maybe a rule could report an error whenever (a) a statement assigns to a variable, (b) the assigned expression contains an await or yield, and (c) the assigned expression references the variable being assigned to, outside of the await/yield.

[mysticatea]

Interesting. In my understanding,

// this is same as `x += await y`
x = x + await y

1. It evaluates the 2nd x and it stores the result into memory.
2. It evaluates the await y, (off course it's asynchronous and other codes can interrupt it), and it stores the result into memory.
3. It assigns the sum of 1. and 2. into the x.

So..., this is same as a popular problem in multi-threading software. Or we have often seen it in the explanation of the RDB transactions.
It causes unintentional behaviors if others modified the x between 2. and 3..

Clearer expression of what happens is:

let local1 = x
let local2 = await y
x = local1 + local2

---

I think that we can consider the following rule, "no-non-atomic-update" (I'm not sure what is the antonym of atomic):

var outer = 0
var outerObj = {}

var outerFunc = () => {}

async function f(param) {
    //× BAD
    outer += await x
    outer = outer + await x
    outerObj.a = outerObj.a + await x
    outerObj.a = outerFunc() + await x
    param.a = param.a + await x
    param.a = outerObj.a + await x
    param.a = outerFunc() + await x
    outer = g(outer, await x)

    //× BAD maybe; I'm not sure if we should warn this case.
    const local1 = outer
    outer = local1 + await x

    //✔ GOOD; this is atomic
    outer = (await x) + outer
    outer = g(await x, outer)
}

[ajafff]

@mysticatea

This one may not be a valid example:

outer = g(await x, outer)

What if g is reassigned while you await x? The call would still use the original value.

[ljharb]

I don't think this really needs a rule specific to await - the simple best-practices answer in JavaScript is don't mutate or reassign things, and if you do, be very very careful about it.

[not-an-aardvark]

I agree that using immutable objects/bindings is often a good idea, but sometimes developers still need to use mutable objects/bindings for a variety of reasons. I think having a rule to make that use case safer is better than not having such a rule.

[not-an-aardvark]

outer = g(await x, outer)

What if g is reassigned while you await x? The call would still use the original value.

That's correct, but it doesn't convince me that this should be invalid example. It seems like this rule is generally intended to avoid "race condition" situations where an variable is updated based on its original value, and the updates don't see each others' results. Since g isn't being reassigned in this statement, it doesn't seem like a problem that the old value of g would get used if g is modified elsewhere. (If the statement was g = g(await x, outer) then I would agree that it should be reported by the rule.)

[Andarist]

It seems to me that this should be a new option of operator-assignment . I agree with others that this is sufficiently confusing and would make a good candidate for a rule.

[jamiebuilds]

I'm not sure this warrants a rule; in the linked tweet, this only is "confusing" because the variable is being mutated both inside and outside the async function.

I can give a motivating example with totally reasonable JavaScript:

async function countFiles(dir) {
  let files = await readdir(dir);
  let count = 0;

  await Promise.all(files.map(file => {
    let filePath = path.join(dir, file);
    let fileStat = await stat(filePath);

    if (fileStat.isDirectory()) {
      count += await countFiles(file);
    } else {
      count += 1;
    }
  });

  return count;
}

I doubt that most (even experienced) developers would be able to spot the bug here.

This just seems like a footgun and that there's little benefit in allowing await to be used in these types of expressions.

[not-an-aardvark]

I'll champion this proposal based on the behavior proposed in #10405 (comment).

[aladdin-add]

now it is accepted.