1381030 - [meta] Turn on require-await ESLint rule for the whole tree (require await within async functions)

Status: RESOLVED WONTFIX

(Developer Infrastructure :: Lint and Formatting, enhancement)

Description

[Mark Banner (:standard8)]

We should work towards turning on require-await throughout the tree - as the ESLint page says "Async functions which have no await expression may be the unintentional result of refactoring.", though they may also cause extra async functionality that isn't needed.

Comment 1

[:Paolo Amadini]

I hope this rule won't discourage developers from designing async APIs even if the current implementation is synchronous. It's often much more difficult to convert code that relied on synchronous behavior to be asynchronous than the other way around. I'm saying this because I've done history and bookmarks API refactorings too! The "syncrhonicity" tends to creep through several layers and becomes difficult to remove later.

A current example of this can be found in the DownloadList object:

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadList.jsm#65-67

Very little of it is really asynchronous, but this API makes it possible to add improvements in the future like lazy intialization. Slight performance hit now from using a Promise, but higher potential win in the future. And the future is now, because in these days I'm working on a DownloadList backed by an asynchronous Places history API.

Comment 2

[Dave Townsend [:mossop]]

I think if you're going to explicitly make an API async before it is async then you can just eslint-disable the rule for the line until it becomes async.

Comment 3

[Mark Banner (:standard8)]

Yeah, if we need to we can disable the line.

In the cases of DownloadList.jsm they aren't "async" functions as such - there's no await in them. As they generally return a promise, callers are still able to `await getAll()` for instance.

If you later change the function to await on something inside it, then you can change the signature to flag it as async, without having to update the caller.

Comment 4

[:Paolo Amadini]

Actually, what I'm concerned about here is that this rule encourages developers to "justify" the choice of designing an async interface, despite that being the right choice. It even discourages declaring async callback functions as async, which would make the code more correct and readable.

In other words, it muddies design and implementation.

This is exactly what happened in bug 1381030 where the vast majority of instances resulted in a comment and exclusion being added to the code. Another majority resulted in no actual performance win, in cases like...

functionWithCallback(async () => { doSomething(); });

...becoming...

functionWithCallback(() => { doSomething(); });

...because functionWithCallback already uses "await" on the parameter, implicitly creating a Promise if the callback function didn't return one already. (As an aside, this could break instances where functionWithCallback does "callback().then()".)

Another instance is...

async update(info) { return asyncFunction(); }

...becoming...

update(info) { return asyncFunction(); }

...for no reason, when the function clearly has an async behavior. The correct way to ensure this case is optimized is the "no-return-await" rule, which is already respected here.

The actual issue detected in bug 1381049 comment 2 was fixed in the wrong way when just applying this rule. The correct rule that would have caught that bug should have said "force await if the called function is declared as async", which is clearly voided if the function is not declared as async to begin with.

I don't think this is a good linter rule to enable in the tree, for the reasons above. CC'ing Marco who took a look at the Places changes and may have an opinion, though I believe that between Downloads and Places we already have a good number of counter-examples.

Comment 5

[Marco Bonardo [:mak]]

I agree it has pros and cons.
You already marked out the negative sides. I think it mostly boils down to the verbosity of "// eslint-disable-line require-await" and the fact it may discourage newcomers to use fake-async behavior (which should be mitigated by the review process).
On the positive side, it may have a responsiveness benefit, since it wouldn't run delayed and won't add the await overhead. And it allows to see more easily when one had to await or when a chain of functions are no more async after a refactoring.

I have mixed feelings about this and so far I don't see one of the sides having more weight.
I guess we should evaluate it better before moving on with it globally.

The big "downside" we are seeing currently is that, additionally to requiring manual fixing since it can't be --fixed, the changes are likely to uncover a number of intermittent failures. And this could be enough to discourage us to move on with this.

(In reply to :Paolo Amadini from comment #4)
> The actual issue detected in bug 1381049 comment 2 was fixed in the wrong
> way when just applying this rule. The correct rule that would have caught
> that bug should have said "force await if the called function is declared as
> async", which is clearly voided if the function is not declared as async to
> begin with.

that's true, but the function was declared async by chance, it could have been declared non-async and we'd have not noticed the problem, with or without this rule. I don't think it's an evidence pro or against the change, since there's nothing forcing to pass an async function as callback. Force-async-callback + require-await is the only rule that would have avoided the bug, but I doubt the former is feasible.

Comment 6

[:Paolo Amadini]

(In reply to Marco Bonardo [::mak] from comment #5)
> On the positive side, it may have a responsiveness benefit, since it
> wouldn't run delayed and won't add the await overhead

There is a possible misunderstanding here about how async functions work. Async functions aren't delayed to the next microtask - this was discussed at the standard level and decided against, so they are required to execute immediately.

An async function that does not contain an await statement runs exactly as a normal function, except that if it does not return a Promise already, the return value is converted to a resolved Promise. When a Promise is returned anyways, or when  the caller converts to Promise anyways, there should be no detectable difference in performance.

Maybe this ESLint rule was created when async functions were cross-compiled and implemented by libraries, instead of being implemented natively? In that context, the rule would make more sense from a performance perspective.

> And it allows to see
> more easily when one had to await or when a chain of functions are no more
> async after a refactoring.

I think it's just the latter, but at the expense of the former - it makes it more difficult to see when one has to await.

> (In reply to :Paolo Amadini from comment #4)
> > The actual issue detected in bug 1381049 comment 2 was fixed in the wrong
> > way when just applying this rule. The correct rule that would have caught
> > that bug should have said "force await if the called function is declared as
> > async", which is clearly voided if the function is not declared as async to
> > begin with.
> 
> that's true, but the function was declared async by chance

What I'm saying is that it should have been declared async by design, not by chance. This rule discourages doing that based on implementation details, rather than developer design choice.

Comment 7

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Attachment: markh-example.patch

I've been thinking/playing with this via bug 1381315 (ie, doing this for services/), and while I think eslint offers great value ...

(In reply to :Paolo Amadini from comment #6)
> What I'm saying is that it should have been declared async by design, not by
> chance. This rule discourages doing that based on implementation details,
> rather than developer design choice.

I agree 100% with this. Further, I think the |async| keyword is a valuable signal and clarity is just as important as performance in most cases (and I'm heartened to hear Paulo suggest performance isn't an actual problem at all)

So really, I believe this is a style issue - and one that we've been thinking about in Sync, as we are in the process of moving towards being fully async. Some guidelines for working with async functions would be helpful, so with this in mind, I'm attaching an example of what Sync is facing.

This attachment is roughly a summary of the patterns Sync finds itself implementing. I believe it offers good readability and is of a style I'm leaning towards. As attached, it already issues one warning:

> $ ./mach eslint markh-example/
> o:\src\moz\gecko\markh-example\example.js
>   38:12  error  Redundant use of `await` on a return value.  no-return-await (eslint)

and with the eslint rule uncommented, it offers:

>  19:3   error  Async method 'shouldDoSomething' has no 'await' expression.  require-await (eslint)
>  26:3   error  Async method 'doSomethingElse' has no 'await' expression.    require-await (eslint)
>  38:12  error  Redundant use of `await` on a return value.                  no-return-await (eslint)
>  49:3   error  Async method 'doSomethingElse' has no 'await' expression.    require-await (eslint)
>  62:3   error  Async method 'shouldDoSomething' has no 'await' expression.  require-await (eslint)

Fixing these warning seems worse than the disease (I don't want to remove any |await|s, and I don't want //eslint comments added *everywhere*) - and IMO still doesn't catch the biggest foot-gun - forgetting to await on a promise in a large complex function.

So I find myself agreeing with Paulo that this doesn't seem worthwhile and may actually be a step backwards. I believe we should agree on style guidelines for async functions before attempting to use eslint to enforce them.

Comment 8

[Marco Bonardo [:mak]]

(In reply to Mark Hammond [:markh] from comment #7)
> I believe we should agree on style
> guidelines for async functions before attempting to use eslint to enforce
> them.

Absolutely, and that's what is happening here in the end.
Places was an experiment to see the problems.
Actually I must admit the change allowed to find a bug in PlacesDBUtils and the fact we were not properly disabling Places Maintenance during mochitest browser tests. So it can actually help finding bugs, maybe by chance or by mistake, but it does.
I'm also not very happy with all the annotations to keep things async.
And we have already multiple examples where this just doesn't work well with our requirements.

I'd probably suggest to give up on having this rule enabled, but imo it's a good rule to run periodically on a module, it CAN help finding bugs, it just has an high false positives ratio.

Comment 9

[Mark Banner (:standard8)]

I think the case where this was useful is bug 1381027 - the functions had been unnecessarily declared as async, and this was slowing down the UI and causing issues with intermittents as an additional result.

I can however, understand the reasoning behind wanting to declare functions that return promises as async. I'm not entirely convinced by the pattern of declaring a function as async even when it doesn't await or contain any promises (this is slightly different to the needing an overridable function which is expected to be async).

Given the high false positive case, maybe it doesn't make sense to enable require-await across the tree - maybe Mossop has some thoughts on this as well?


On the no-return-await front, pretty much everything I read when I was researching it said it was an unnecessary performance impact to include the await in the statement. If our js folks can tell us there's definitely no impact, then maybe we can allow it, but I think that's a separate conversation at the moment.

Comment 10

[Dave Townsend [:mossop]]

(In reply to Mark Banner (:standard8) (afk 21 - 30 July) from comment #9)
> I think the case where this was useful is bug 1381027 - the functions had
> been unnecessarily declared as async, and this was slowing down the UI and
> causing issues with intermittents as an additional result.
> 
> I can however, understand the reasoning behind wanting to declare functions
> that return promises as async. I'm not entirely convinced by the pattern of
> declaring a function as async even when it doesn't await or contain any
> promises (this is slightly different to the needing an overridable function
> which is expected to be async).
> 
> Given the high false positive case, maybe it doesn't make sense to enable
> require-await across the tree - maybe Mossop has some thoughts on this as
> well?

It sounds like this might be frustrating to folks and it isn't really going to catch us many problems so I suggest we drop trying to implement this rule.

Comment 11

[Mark Banner (:standard8)]

Ok, agreed lets wontfix this.