[no-unnecessary-condition] Missing string compared to undefined as unnecessary

[pgsandstrom]

Repro

{
  "rules": {
    "@typescript-eslint/no-unnecessary-condition": ["error"],
  }
}

function test(s: string) {
  if (s === undefined) {
    console.log('this will never happen')
  }
}

Expected Result

It should warn about the unnecessary condition

Actual Result

It does not warn about it

Versions

package	version
@typescript-eslint/eslint-plugin	2.19.0
@typescript-eslint/parser	2.19.0
TypeScript	3.7.5
ESLint	6.7.2
node	13.8.0
npm	6.13.7

[pgsandstrom]

Im trying to understand the source code to see if there is an easy fix. I can understand the code enough to just add a fix to check if a non-optional string is compared to undefined and send out a 'alwaysFalsy' warning. But I'm unsure if there is a smarter more general solution that is available with a better understanding of the language intricacies.

I'd be happy to put together a PR in a week or two if this seems sensible.

[bradzacher]

I can't remember the reasoning for not checking both sides of a binary expression. cc @Retsam, who implemented the rule in #699, and is fairly active here.

IIRC it was two-fold:

• People often do "sanity check" guards against null/undefined, esp when converting from JSON/user input.
• TS itself errors against many comparisons.

For the former, an option means people can turn the check off if they purposely do these sanity checks.
For the latter I think the only cases TS doesn't cover is explicit comparisons with null/undefined.

I'd be happy to add a new feature behind a default "off" flag (so it's backwards compatible). Something like checkNullishComparisons?: boolean = false.
Then it can simply check the following cases:

<falsey type> === undefined             // error ❌
<falsey type> | null === undefined      // error ❌
<falsey type> | undefined === undefined // okay  ✅

<falsey type> === null                  // error ❌
<falsey type> | null === null           // okay  ✅
<falsey type> | undefined === null      // error ❌

<falsey type> == undefined              // error ❌
<falsey type> | null == undefined       // okay  ✅
<falsey type> | undefined == undefined  // okay  ✅

<falsey type> == null                   // error ❌
<falsey type> | null == null            // okay  ✅
<falsey type> | undefined == null       // okay  ✅

[Retsam]

Yeah, for me the the reasoning was entirely the "TS itself errors against many comparisons." reason - I thought TS would cover this case, and didn't realize that null and undefined are exceptions to the "This condition will always return 'false' since the types [...] have no overlap" rule.

I'm digging through the Typescript compiler issues to see if they have a specific reason for why it's an exception. I haven't found an issue for it yet, (my guess is that the answer is going to be strictNullChecks), if I don't find one I may file one, on the off-chance that this can be fixed in the TS compiler.

But I can probably get a PR that adds a special-case for it here.

My preference is just to make it a breaking-change PR against 3.0, rather than add it behind a flag and then turn around and remove the flag at 3.0. I don't see that needs to be configurable behavior - yeah, "sanity checks" against JS code will raise false positives, but I'd rather see sanity checks explicitly marked as such with an override comment than have the linter rule configured to ignore all === undefined checks.