"await" should not be used redundantly

In Javascript/Typescript RSPEC-4326 it is declared that returning an awaited promise is redundant code smell.
However in the case that the promise may throw and there is a surrounding try/catch expression, eliminating the await will also bypass the try-catch error handling leading to a bug.
It seems an issue to micro-optimize over proper maintainable code. it would be better to warn of methods that return promises that are not awaited which would be a more typical issue.
If the code is as simplistic as the example then it should probably not be async in the first place and just directly return the promise.

1 Like

hello @steam,

thanks for reporting. Could you please include short code snippet demonstrating the bug you are describing to be sure I understand what you mean?

I have to assume this has been addressed in some way that is not mentioned in the rule itself as testing the lint on example code did not report on the cases where the return was inside a try-catch block. The risk of maintaining the rule still exists if the method is refactored later without noticing the method is not awaiting the promise.

function testThrows() {
    return Promise.reject(new Error("test throw message"));
}

// Does not work as expected. No rules detect this issue.
async function test4() {
    try {
        return testThrows();
    } catch (error) {
        return "error: " + error;
    }
}

// Works as expected, Surprisingly not reported as S4326
async function test3() {
    try {
        return await testThrows();
    } catch (error) {
        return "error: " + error;
    }
}

// Triggers the rule S4326 in sonarlint as expected.
async function test1() {
    return await testThrows();
}

For the Typescript case there appears to be an improvement to the rules via S4822 which would at least note the issue, after refactoring is done later, that introduced try-catch without await. However the text and examples of the rule do not distinguish from a method that isn’t awaited and the return foo() case from S4326

function testThrows(): Promise<never> {
    return Promise.reject(new Error("test throw message"));
}

// Does not work as expected.
// Does report typescript:S4822 catching the coding issue.
export async function test4(): Promise<string|undefined> {
    try {
        return testThrows();
    } catch (error) {
        return "error: " + error;
    }
}

// Works as expected
// Surprisingly does not report typescript:S4326
export async function test3(): Promise<string|undefined> {
    try {
        return await testThrows();
    } catch (error) {
        return "error: " + error;
    }
}

// Triggers the rule in sonarlint.
export async function test1(): Promise<never> {
    return await testThrows();
}

Hi,

Better late than never, right?:slight_smile:
Since according to you the rule does not [anymore] report on the initial case you are pointing:

// Works as expected
// Surprisingly does not report typescript:S4326
export async function test3(): Promise<string|undefined> {
    try {
        return await testThrows();
    } catch (error) {
        return "error: " + error;
    }
}

could you tell me if you see other problems with the rule as of latest releases?

P.S. The rule is implemented by ESLint (no-return-await - Rules - ESLint - Pluggable JavaScript linter) so even if we consider some improvements, we will need to have them accepted there. Also we will probably remove rule from default profile (Remove S4326 from default profile and improve its description · Issue #2674 · SonarSource/SonarJS · GitHub).

Wow, it is an old thread! May I suggest removing RSPEC-4326 entirely or marking it as a deprecated rule? I think that the industry is slowly arriving at the conclusion that this rule is actually harmful.

The performance benefit claims are shown to be actually false, it is slower when you omit that await

None of the IDE-s (or the developers for that matter) are checking if there is a return asyncFn() when wrapping the code in try-catch-finally - so the code is going to be broken

The documentation does not mention that a direct return breaks “finally” and “catch” blocks.

It is true that the warning is not raised in this case, but many people only remember to avoid return await and do not even know about the try-catch try-finally exceptions.

And last, I had to explain it many times to colleagues, and some of them were referring to this sonar rule as the source of their confusion.

Hi @Tamas_Hegedus1,

thanks for your detailed answer. We disabled this rule last month and updated the documentation accordingly. SonarJS 9.9 was already released with these changes.

Thanks!

We have upgraded to * Community Edition

  • Version 9.9.1 (build 69595) but still reporting “await” error!

Hi @Rashmi_Murthy ,

when I said ‘rule disabled’ I meant to say that the rule is not part of the ‘Sonar way’ quality profile. If you still see this issue I guess it is because you are using your own custom quality profile that still includes S4326. You will need to disable the rule in your quality profile.

Cheers,
Victor

async function foo(): Promise<string> {
  return testThrows();
}

is not the same as

async function bar(): Promise<string> {
  try {
    return testThrows();
  } catch (error) {
    // ...
  }
}

Inside an async function, returning a Promise retrieved from another (unawaited) async function called from within that function, is an optimization. You do not need to unwrap the value and then have JS rewrap that value in another (completed) promise by the fact that you are returning from an async function. That is an unnecessary operation.

On the other hand, await a Promise inside a try catch block is the only way to make sure the catch block runs, because you need to await (pause) block execution in that scope before returning from that scope, and effectively destroying it, thus the catch cannot run anymore.

If you return without awaiting, from inside a try catch block, the Promise is returned immediately, and when let’s say a network error occurs later inside the Promise, the promise is no longer inside the try-catch block to be caught, because it’s already been returned from the function scope to the calling scope.

1 Like