Bug: require-await has a bad report loc for arrow functions.

[kirkwaiblinger]

Environment

Node version:
npm version:
Local ESLint version:
Global ESLint version:
Operating System:

What parser are you using?

Default (Espree)

What did you do?

async () => { return; }

What did you expect to happen?

async () => { return; }
^^^^^

What actually happened?

async () => { return; }
         ^^

Link to Minimal Reproducible Example

https://eslint.org/play/#eyJ0ZXh0IjoiYXN5bmMgKCkgPT4geyByZXR1cm47IH1cbiIsIm9wdGlvbnMiOnsicnVsZXMiOnsicmVxdWlyZS1hd2FpdCI6WyJlcnJvciJdfSwibGFuZ3VhZ2VPcHRpb25zIjp7ImVjbWFWZXJzaW9uIjoibGF0ZXN0Iiwic291cmNlVHlwZSI6Im1vZHVsZSIsInBhcnNlck9wdGlvbnMiOnsiZWNtYUZlYXR1cmVzIjp7fX19fX0=

Participation

• I am willing to submit a pull request for this issue.

Additional comments

Note that the named function case is much more sensible

// current - good
async function f() { return; }
^^^^^^^^^^^^^^^^

// though kirk's preference might be
async function f() { return; }
^^^^^

but the non-arrow anonymous function expressions could use a little cleanup as well:

// current, sloppy.
const a = async function () { return; };
          ^^^^^^^^^^^^^^^
// should be
const a = async function () { return; };
          ^^^^^^^^^^^^^^
// Or, kirk's preference:
const a = async function () { return; };
          ^^^^^

[nzakas]

Thanks for the report. The location is being returned from getFunctionHeadLoc(), which is a utility function that multiple rules use. It looks as if this behavior is intentional, so before making a change, we'd need to come to some consensus over how we'd like to highlight the location of problems that affect an entire function.

@eslint/eslint-team thoughts?

[mdjermanovic]

I think getFunctionHeadLoc works well for cases where we want to mark the function as a whole as problematic (e.g, when a function that is expected to return something doesn't return in all code paths).

My understanding of this issue is that, in this particular rule, the question is whether we should mark the function as a whole, or specifically async as the concrete cause of the problem because the purpose of this rule is to suggest removing that async as unnecessary.

[nzakas]

My understanding of this issue is that, in this particular rule, the question is whether we should mark the function as a whole, or specifically async as the concrete cause of the problem because the purpose of this rule is to suggest removing that async as unnecessary.

That makes sense, although I disagree that the point of the rule is to suggest removing async. The problem reported is that the function as a whole does not have an await inside, and to address that, there are two different options:

1. Remove async
2. Add an await

To that end, it seems correct to flag the entire function as the problem rather than just async.

However, I don't feel very strongly, so happy to go along with the consensus of the team here.

[kirkwaiblinger]

IMO underlining the async keyword is analogous to underlining the name of an unused variable. Sure, if you removed it, it would silence the lint rule, but that doesn't imply it's the sole cause of the report, just that it's the trigger. Before you remove it, you'll first check your code for where you thought you were using it, and you'll find your misspelled variable if present (or, following the analogy, your missing await statement).

[kirkwaiblinger]

In other words, I personally read the linter underline as "Why is this here?" not "You should remove this.".

[nzakas]

In other words, I personally read the linter underline as "Why is this here?" not "You should remove this.".

Yeah, not everyone reads the red squigglies the same way. Our intent is to say "this is the location of the problem", but not everyone reads it that way.

[kirkwaiblinger]

In other words, I personally read the linter underline as "Why is this here?" not "You should remove this.".

Yeah, not everyone reads the red squigglies the same way. Our intent is to say "this is the location of the problem", but not everyone reads it that way.

Well, right, and fair enough. => is not the location of any problem here.

[nzakas]

Well, right, and fair enough. => is not the location of any problem here.

True enough, but we're in a no-win situation with arrow functions because they don't have any formal declaration syntax to hook onto except the arrow. So if we're going to highlight "this function has a problem" highlighting the arrow represents the least confusion vs. highlights the (potentially non-existent) arguments or the entire body. The arrow is, in fact, the only syntax for arrow functions that is guaranteed to be there 100% of the time.

@mdjermanovic @fasttime still waiting for your feedback on this issue.

[kirkwaiblinger]

we're in a no-win situation with arrow functions

Completely agree! 🙂

The arrow is, in fact, the only syntax for arrow functions that is guaranteed to be there 100% of the time.

In general, yes. But, not in this rule, since we'll always have the async keyword, too (for all syntax being reported, not even just arrow functions). That's really the crux of why I, personally, think that this is a worthwhile improvement within what's still very much a no-win-scenario. But, I can't claim to argue that it's unambiguously better 🙃

[mdjermanovic]

I still think the official point of this rule is to suggest removing async, as that's what the documentation implies:

eslint/docs/src/rules/require-await.md

Line 25 in eeec413

Asynchronous functions that don't use `await` might not need to be asynchronous functions and could be the unintentional result of refactoring.

That said, I completely agree that this rule can catch missing await as well, so highlighting "this function has a problem" as it currently does, and using the same location we use in other rules that highlight "this function has a problem" seems like the best solution.

The fact that the function is async is included in the error message:

Async arrow function has no 'await' expression. 

So I also don't feel strongly about making any changes here. If no one from the team has a strong opinion that the location should be changed, it is probably best to leave it as it is now.

[nzakas]

All right, it seems that we don't have anyone on the team that feels strongly to make this change, so closing. We appreciate the thoughtful discussion @kirkwaiblinger!

[kirkwaiblinger]

All right, it seems that we don't have anyone on the team that feels strongly to make this change, so closing. We appreciate the thoughtful discussion @kirkwaiblinger!

Makes sense! Likewise 🙂