[no-unnecessary-condition] Add support for non-strict mode

[glen-84]

Repro

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

class Thing {}

class Example {
    private test: Thing | null;

    public run(): void {
        if (this.test) { // Error here.
            console.log(this.test);
        }
    }
}

Expected Result

No error.

Actual Result

Error: Unnecessary conditional, value is always truthy.

Additional Info

n/a

Versions

package	version
@typescript-eslint/eslint-plugin	2.3.0
@typescript-eslint/parser	2.3.0
TypeScript	3.5.3
ESLint	6.1.0
node	10.16.3
npm	6.9.0

[bradzacher]

I'm unable to reproduce this against master.
Do you have strictNullChecks on?

If you don't - then the actual type of this.test degrades to Thing - hence the rule flags the condition as unnecessary.

[glen-84]

I'll try to test this again on Friday.

[glen-84]

strictNullChecks was enabled, but I think that I had forgotten to reload VS Code, so the ESLint extension may have been running with strictNullChecks disabled.

Having said that, with strictNullChecks disabled, shouldn't:

class Thing {}

class Example {
    private test: Thing;

    public run(): void {
        if (this.test) {
            // Error here.
            console.log(this.test);
        }
    }
}

... be fine, if null and undefined are in the domain of every type?

It's possible that I am misunderstanding something, but you'd think that code like the above would be common in a world without strictNullChecks.

[bradzacher]

You're correct in your understanding of what happens when it's turned off.

But incorrect in the understanding of how the typechecker reports types.
When strict mode is off, the types reported by the checker API aren't any different to when it's on.

So when we query if the type is nullable etc, the checker api just tells us that it's of type Thing - it's up to us to implement the non-strict logic.

We've done it for a few rules, so we need to add it where appropriate.

[glen-84]

Ya, I knew that it was being reported as Thing (by hovering over the symbol in VS Code), I just wasn't sure whether the APIs provided additional metadata, or whether it had to be handled manually.

[Stephen2]

Am I seeing the same thing here?

[Stephen2]

Seems inaccurate to me. Just started happening after an upgrade, I'm gonna rollback and see what happens

EDIT: Actually, there's something else going on. I'm narrowing it down, and will report back.

[Stephen2]

Confirmed, going from 2.1.0 -> 2.2.0 causes these false positives for me.

I even rolled back typescript version to 3.5.3 because I was getting the giant warning message about being outside estree supported version

But still no luck.

[Stephen2]

blerg... I tried hacking up node-modules to figure out what might be the cause, but I didn't understand it :(

Sorry... I tried to help

[glen-84]

@Stephen2 Are you using "ignoreRhs": true?

[Stephen2]

I'm not

[Stephen2]

Turning that on makes the list of 20 silly ones go down to 3 mostly reasonable ones 🍾

Am I misunderstanding how the rules works?

[glen-84]

@Stephen2 #1017.

[derekparsons718]

I would love to see this rule work better when strict null checks is disabled.

Until that happens, I would suggest updating the documentation to indicate this limitation. A little note like "this rule requires strict-null-checks" could save people like me from scratching their heads so much :)

Thanks for all of your hard work!

[bradzacher]

The rule now errors for non-strict files.

And is documented here:
https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md#allowruletorunwithoutstrictnullchecksiknowwhatiamdoing