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
, andcontinue
from afinally
block overwrites similar statements from the suspendedtry
andcatch
blocks.
This rule raises an issue when a jump statement (break
,continue
,return
andthrow
) would force control flow to leave afinally
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).