FP: Unsafe usage of ThrowStatement.sonarlint(typescript:S1143) even when not overriding jump

Consider the following code (Playground link here):

//Note: in tsconfig, target needs to be ES2018 or later
const demonstrateSonarIssueWithPromise = function() {
    const errorToReturn = new Error('Some error text better describing context.');
    return new Promise(function(resolve, reject) {
        doSomethingThatMightFail().then(function() {
            console.log('First thing that might fail actually succeeded.');
        }).catch(function(_errorFromFailure: any) {
            console.log('First thing that might fail actually failed, but choosing to bubble up a different error.');
        }).finally(function() {
            reject(errorToReturn); //no problem
        });
    });
}
const demonstrateSonarIssueAsync = async function() {
    const errorToReturn = new Error('Some error text better describing context.');
    try {
        await doSomethingThatMightFail();
    } catch (_errorFromFailure: any) {
        console.log('First thing that might fail actually failed, but choosing to bubble up a different error.');
    } finally {
        throw(errorToReturn); //CRITICAL BUG: Jump statements should not occur in "finally" blocks (typescript:S1143)
    }
}
const doSomethingThatMightFail = function() : Promise<void> { /*...*/ return Promise.resolve(); }

I might be wrong, but my understanding is that in JavaScript the second function is just syntactical sugar for the first one. (TypeScript does treat the two differently for control flow analysis and type narrowing, but that shouldn’t matter here.) Code in the try block should be executed until an error is reached, at which point execution jumps to the next catch or finally block (in this case, catch) and at the end of that (without additional exceptions) it should proceed into the next then or finally block. You put things in the finally block to avoid duplication between the then and catch blocks.

The rule description states:

Using return, break, throw, and continue from a finally block overwrites similar statements from the suspended try and catch blocks.
This rule raises an issue when a jump statement (break, continue, return and throw) would force control flow to leave a finally block.

The example shows an unexpected overwrite of a similar statement from the try/catch block.
However, in the example code, there aren’t jump statements from the try or catch block to be unexpectedly overwritten.

The link at the bottom is to CWE-584, which focuses on Java, which I think has a different behavior, and also focuses on a discarded error where in some cases (like the one motivating this report), that is intentional.

I think the test for typescript:S1143 (and any corresponding JavaScript rule) should only trigger if the try/catch block ALSO contain jump statements, in addition to the finally block.

I think the async and Promise structures should be treated the same as each other. Currently they are treated differently by Sonar; the CRITICAL BUG is only flagged when code is written in the async style, and rewriting in the Promise style is one workaround.

I also think the wording of the rule description probably should also be updated to recommend putting code that you want to execute regardless of try/catch success after the end of the try/catch/finally block.

Observed with SonarLint v3.5.4 in connected mode to SonarQube Community Edition Version 8.9.7 (build 52159).

Hello @unblocker

while it’s true that functionally both functions could be considered equal, the consequences of using one or another are indeed different in terms of throwing/returning in a finally block. The finally method of a promise will not suspend the execution of a previous then or catch, as previous thenables have been already resolved/rejected. And one cannot overwrite the result of a resolved promise with finally (on the other hand, throwing inside the finally will indeed overwrite the reason). source

However, in the try…catch…finally version, one can unexpectectly overwrite previous return/throw statements, as you correctly mentioned.

We have discussed internally how reasonable would be to consider the case where there are no return/throw statements in the try…catch block, but in this case, we would recommend the option to move the throw line outside of the whole, like so:

const demonstrateSonarIssueAsync = async function() {
    const errorToReturn = new Error('Some error text better describing context.');
    try {
        await doSomethingThatMightFail();
    } catch (_errorFromFailure: any) {
        console.log('First thing that might fail actually failed, but choosing to bubble up a different error.');
    }
    throw errorToReturn;
}

As returning/throwing inside a finally block is generally considered a bad practice, we would prefer to keep the rule as is.

Please let us know if you consider we are missing some valid use cases where our proposed workaround would not be applicable.

Cheers

The explanation of the rule states that throw inside the finally is a problem specifically because it overwrites jump statements in the try/catch block. When there are no such statements to override, the confusion issue isn’t coming from the code, but rather from Sonar. If you can state why returning/throwing in a finally block should be generally considered a bad practice, you may want to express that in the description of a split-out rule, but I don’t think this should stand as-is. Also, the “workaround” proposed as acceptable code is not readily discernible from the issue description; one should not conclude it’s obvious especially when the issue described is verifiably absent.

Also note that async code using this try-catch-finally may be code originally written in the Promise style and subsequently converted (e.g. to overcome Typescript’s issue with control flow analysis only narrowing with the async style).

Hi @unblocker
You are right that the wording in the rule description focuses on overwriting return, break, throw, and continue statements, but says explicitly:

This rule raises an issue when a jump statement (break, continue, return and throw) would force control flow to leave a finally block.

Without further conditions. Also, the link to CWE-584 mentions explicitly:

The finally block should have “cleanup” code

We’ll change the description of the rule to make it clear that existing return, break, throw, and continue statements are not a must for this rule to report. PR

Consider the following code:

function test() {
   let foo = {};
   try {
      await doSomethingThatMightFail();
   }
   catch {
      foo.bar();
   }
   finally {
      return 42;
   }
}

foo.bar() would throw a runtime TypeError exception, so checking the lack of throw or return statements does not guarantee that you are not overwriting suspended statements.