Flag non-nullable functions in `if` statements as errors by jwbay · Pull Request #32802 · microsoft/TypeScript (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation18 Commits2 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Under --strictNullChecks, we now error on testing non-nullable function types in if statements if they're not called. This is a subset of #9041 as mentioned here: #9041 (comment).

Example:

function onlyErrorsWhenNonNullable(required: () => boolean, optional?: () => boolean) { if (required) { // now an error... }

if (required()) { // ...because you probably meant this, which is still ok
}

if (optional) { // still ok
}

}

Note the concern about third-party values coming in without a strictNullChecks context is still valid :

import { ThirdPartyType, create } from 'some-package'

const x: ThirdPartyType = create();

// this may now flag an error because the type definitions for some-package // don't mark someFunc as nullable when it should be if (x.someFunc) { }

Should we make a recommendation here in the error message like casting to a nullable type?

jwbay mentioned this pull request

Aug 10, 2019

Heya @RyanCavanaugh, I've started to run the parallelized community code test suite on this PR at 130615a. You can monitor the build here. It should now contribute to this PR's status checks.

Heya @RyanCavanaugh, I've started to run the parallelized Definitely Typed test suite on this PR at 130615a. You can monitor the build here. It should now contribute to this PR's status checks.

Heya @RyanCavanaugh, I've started to run the extended test suite on this PR at 130615a. You can monitor the build here. It should now contribute to this PR's status checks.

There should probably be a special case for if(!!expr) { so that there's an idiomatic non-casting workaround for checkJs scenarios

I don't think this is the scope of strictNullChecks. This change injures the certainty and reliance of strictNullChecks. This change should be enabled by a new flag.

This didn't go nearly as well as expected, unfortunately.

RWC turned up over a hundred examples of code of the form

if (someExpr) { someExpr(); }

where someExpr isn't really provably not-undefined because:

It was a class property not covered by strictPropertyInitialization
It was an array or map property lookup, this might be undefined even though we generally pretend this never happens
It's a DOM property that isn't present in all browsers, so it's valid feature-detection code
It comes from props that might have bad JS callers (this one is more questionable)

Some of this also probably happened as codebases started out without strictNullChecks on, wrote code correctly to defensively handle undefineds, then turned SNC on and it just so happened that there weren't identifiable undefineds manifest in those positions after all.

However, there were a few hits some code that didn't call someExpr where it did appear to be a bug:

            if (this.isComponentMounted) {
                ~~~~~~~~~~~~~~~~~~~~~~~

!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead? this.setState({ isUnblocking: false }); }

Further restricting the check to functions returning boolean might make the false positive : true positive ratio good enough to be acceptable.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try with the "functions returning boolean only" approach and hope for good results

Let's try with the "functions returning boolean only" approach and hope for good results

Added a commit for this 🤞

I imagine one way to achieve a far better signal-to-noise ratio would be checking syntax to see if and how the thing being tested is used inside the block body.

if (this.isComponentMounted) { // definitely suspicious because isComponentMounted isn't used in the block this.setState({ isUnblocking: false }); }

if (this.isComponentMounted) { // but probably ok if it's called in the block this.setState({ isUnblocking: this.isComponentMounted() });

// or is passed to something else
this.setState({ isUnblocking: someCheck(this.isComponentMounted) });

}

However... I'm guessing a brute-force syntax walk over arbitrarily deep children would be too expensive to perform here. Is there some way to attach information like this during an earlier syntax walk, like I imagine might be done for scope checking or CFA?

There should probably be a special case for if(!!expr) { so that there's an idiomatic non-casting workaround for checkJs scenarios

This happens to already work due to the way we're checking specific syntax kinds. Added a test for it

Heya @RyanCavanaugh, I've started to run the extended test suite on this PR at 327ff39. You can monitor the build here. It should now contribute to this PR's status checks.

Heya @RyanCavanaugh, I've started to run the parallelized community code test suite on this PR at 327ff39. You can monitor the build here. It should now contribute to this PR's status checks.

Latest commit has one false positive (the event handler happened to return boolean) and three (!) instances of

if (this.isComponentMounted) { ... }

which is pretty good IMO.

@jwbay re: tree walks!

We talked about this more and actually think a tree walk of the body of an if could be a plausibly-cheap thing to do in this particular case. Observations:

The check is extremely scoped, so wouldn't fire very often
98%+ of the time when an error wouldn't be issued, the subsequent call will be very early in a depth-first check, so the walk can bail before covering too much
The other 1% of the time when we do end up erroring, well, erroring compilations are allowed to be slow
The very small proportion of cases where we do walk the entire if body to find the call "late" in the tree is rare enough to not be a meaningful perf hit

This would allow TS to check this for any function, not just those returning boolean, and would eliminate one of the false positives we had.

So if it's OK with you, we'd like to compare-and-contrast the results from the boolean-test PR with maybe a fresh PR that does do the walk? Would you be game to implement both ways so we can see which has the best yield?

Thanks again for your work on this so far; I think this is going to be a high-value check either way.

jwbay mentioned this pull request

Aug 31, 2019

I agree with the new approach #33178. This approach should be adopted. However, I still this is not null checks. This check should be called like strictCallChecks.

Closing as #33178 is now merged

can't under stand why if (true) will report TS2774, but if (false) reports nothing.

interface A {
        a: () => boolean;
    }

    class AImpl implements A {
        public a(): boolean {
            return true;
        }
    }

    it('should fail', function () {
        const aImpl = new AImpl();
        if (aImpl.a) { // TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
            console.log("always true");
        }
    });

    it('why success', function () {
        const aImpl = new AImpl();
        if (!aImpl.a) { // nothing happens here
            console.log('always false');
        }
    });

Flag non-nullable functions in if statements as errors by jwbay · Pull Request #32802 · microsoft/TypeScript (original) (raw)

Conversation

Choose a reason for hiding this comment

Flag non-nullable functions in `if` statements as errors by jwbay · Pull Request #32802 · microsoft/TypeScript (original) (raw)