[no-unnecessary-condition] Wrong report "Unnecessary conditional"

[juergenzimmermann]

• [x ] I have tried restarting my IDE and the issue persists.
• [ x] I have updated to the latest version of the packages.
• [x ] I have read the FAQ and my problem is not listed.

Repro
The following code wrongly reports Unnecessary conditional, value is always falsy. It's a simplification of several cases where I have if-statements with negations for a function's boolean return value.

const isEven = (val: number) => val % 2 === 0;
if (!isEven(1)) {
    console.log('Odd value');
}

Rule definition in .eslintrc.yml:

rules:
  # ...
  "@typescript-eslint/no-unnecessary-condition": error

tsconfig.json:

{
  "compilerOptions": {
    "target": "ES2019",
    "module": "commonjs",
    "alwaysStrict": true,
    "esModuleInterop": true,
    "experimentalDecorators": true,
    "forceConsistentCasingInFileNames": true,
    "importHelpers": true,
    "inlineSourceMap": true,
    "inlineSources": true,
    "noErrorTruncation": true,
    "noFallthroughCasesInSwitch": true,
    "noImplicitAny": true,
    "noImplicitReturns": true,
    "noUnusedLocals": true,
    "noUnusedParameters": true,
    "preserveWatchOutput": true,
    "removeComments": false,
    "skipLibCheck": true,
    "strict": true,
    "strictFunctionTypes": true,
    "strictNullChecks": true,
    "strictPropertyInitialization": true,
    "lib": [
      "ES5",
      "ES2015",
      "ES2016",
      "ES2017",
      "ES2018",
      "ES2019",
      "ES2020",
      "dom"
    ],
    "outDir": "./dist"
  },
  "exclude": ["config", "dist", "test", "node_modules", "scripts", "temp"]
}

Expected Result

No message

Actual Result

Message Unnecessary conditional, value is always falsy.

Versions

package	version
@typescript-eslint/eslint-plugin	3.9.2-alpha.10
@typescript-eslint/parser	3.9.2-alpha.10
TypeScript	4.0.2
ESLint	7.7.0
node	14.8.0

[ehahn9]

Seems unary negation is very broken moving from 3.9.1 -> 3.10.0:

export function foo(value: unknown) {
  if (!value) throw new Error('bad value'); // always fails with @typescript-eslint/no-unnecessary-condition
}

[bradzacher]

probably due to #2382?

[mdziekon]

@bradzacher yes, this regression is caused by the mentioned Pull Request. In your code review, you were originally right when you mentioned that the unary expression checker should simply call checkNode on the arguments (https://github.com/typescript-eslint/typescript-eslint/pull/2382/files#r468215593, I believe you have to expand it yourself unfortunately). The problem was not with the logic of checkNode itself, but rather the error message, which to be correct, should be somehow flipped.

---

The problem with checkIfUnaryNegationExpressionIsNecessaryConditional is that it incorrectly assumes that, for example, when the expression is possibly truthy, it therefore must always be truthy (hence it reports always falsy, because the condition is negated), which is not true.

Let's consider the following code:

function runTest() {
    const element = document.querySelector('.classSelector');

    if (!element) {
        throw new Error('Element not found');
    }

    return element;
}

runTest();

element is possibly falsy (null, when no matching element was found), but the rule incorrectly reports:

Unnecessary conditional, value is always falsy

This is because it incorrectly thinks that since element is potentially truthy, it must always be truthy (therefore, reporting always falsy result of the entire expression).

---

Not only that, this function also does not check for other edge cases that checkNode already handles. For example, let's consider the following code:

declare function doSomething(): void;
declare function getBoolean(): boolean;
declare function getUnknown(): unknown;

function runTest(): void {
    const booleanTyped = getBoolean();
    const unknownTyped = getUnknown();

    if (!(booleanTyped || unknownTyped)) {
        doSomething();
    }
}

runTest();

The condition is necessary, because ultimately the result of the unary's arguments might be of type unknown. In this scenario, the newly introduced code reports a false positive:

error Unnecessary conditional, value is always falsy

---

In other words - checkNode(node.argument) is probably the best choice since it already handles different edge cases, but it would need to take an additional parameter to allow final message to be flipped (IIUC, this could also be possible by traversing the node's hierarchy backwards, but I wonder if detecting nested negations would be as easy as with boolean flag flipping, and if "stop condition" could be easily determined).

If my analysis is correct, I think #2382 should be reverted and reimplemented in a different way (a couple more test cases to check for this regression wouldn't hurt as well). Fixing and extending checkIfUnaryNegationExpressionIsNecessaryConditional would introduce unnecessary code duplication.

[bradzacher]

A canary version should be published within the next 15 mins. Give it a go so I can verify the fix worked, and I'll see if we can get an out-of-band hotfix published.

[juergenzimmermann]

@bradzacher Confirmed: the issue is gone when I use the canary release. Thank your for the immediate fix!

[Jessidhia]

Another confirmation here; issue encountered on 3.10.0 and fixed in 3.10.1-alpha.1.

[Retsam]

Reading through the original issue for #2255, I'm not sure I understand what the motivating use cases for that change were. I do agree that if(arr[0]) probably should get the same array workaround if it doesn't already have it, but I didn't find stuff like if(+a) or if(-a) to be worth adding significant complexity to cover.