Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
juergenzimmermann opened this issue Aug 24, 2020 · 7 comments
Closed

Comments

@juergenzimmermann
Copy link

@juergenzimmermann juergenzimmermann commented Aug 24, 2020

  • [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
Copy link

@ehahn9 ehahn9 commented Aug 24, 2020

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
Copy link
Member

@bradzacher bradzacher commented Aug 24, 2020

probably due to #2382?

@bradzacher bradzacher added bug and removed triage labels Aug 24, 2020
@mdziekon
Copy link
Contributor

@mdziekon mdziekon commented Aug 25, 2020

@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 bradzacher self-assigned this Aug 25, 2020
@bradzacher bradzacher added the has pr label Aug 25, 2020
bradzacher added a commit that referenced this issue Aug 25, 2020
…h unary negations

Fixes #2421
bradzacher added a commit that referenced this issue Aug 25, 2020
…h unary negations (#2422)

Fixes #2421
@bradzacher
Copy link
Member

@bradzacher bradzacher commented Aug 25, 2020

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
Copy link
Author

@juergenzimmermann juergenzimmermann commented Aug 25, 2020

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

@Jessidhia
Copy link
Contributor

@Jessidhia Jessidhia commented Aug 25, 2020

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

@Retsam
Copy link
Contributor

@Retsam Retsam commented Aug 25, 2020

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

6 participants
You can’t perform that action at this time.