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

Flag non-nullable functions in if statements as errors #32802

Closed
wants to merge 2 commits into from

Conversation

@jwbay
Copy link
Contributor

@jwbay jwbay commented Aug 10, 2019

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?

@RyanCavanaugh
Copy link
Member

@RyanCavanaugh RyanCavanaugh commented Aug 12, 2019

@typescript-bot test this
@typescript-bot user test this
@typescript-bot run dt

Loading

@typescript-bot
Copy link
Collaborator

@typescript-bot typescript-bot commented Aug 12, 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.

Loading

@typescript-bot
Copy link
Collaborator

@typescript-bot typescript-bot commented Aug 12, 2019

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.

Loading

@typescript-bot
Copy link
Collaborator

@typescript-bot typescript-bot commented Aug 12, 2019

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.

Loading

@RyanCavanaugh
Copy link
Member

@RyanCavanaugh RyanCavanaugh commented Aug 12, 2019

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

Loading

@typescript-bot
Copy link
Collaborator

@typescript-bot typescript-bot commented Aug 12, 2019

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

Loading

@falsandtru
Copy link
Contributor

@falsandtru falsandtru commented Aug 13, 2019

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.

Loading

@RyanCavanaugh
Copy link
Member

@RyanCavanaugh RyanCavanaugh commented Aug 16, 2019

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.

Loading

Copy link
Member

@RyanCavanaugh RyanCavanaugh left a comment

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

Loading

@RyanCavanaugh RyanCavanaugh self-assigned this Aug 16, 2019
@jwbay jwbay force-pushed the testNonNullableCallSignatures branch 2 times, most recently from 3906a1e to e81f5ec Aug 18, 2019
@jwbay
Copy link
Contributor Author

@jwbay jwbay commented Aug 18, 2019

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

Loading

@jwbay jwbay force-pushed the testNonNullableCallSignatures branch from e81f5ec to 327ff39 Aug 18, 2019
@RyanCavanaugh
Copy link
Member

@RyanCavanaugh RyanCavanaugh commented Aug 19, 2019

@typescript-bot test this
@typescript-bot user test this

Loading

@typescript-bot
Copy link
Collaborator

@typescript-bot typescript-bot commented Aug 19, 2019

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.

Loading

@typescript-bot
Copy link
Collaborator

@typescript-bot typescript-bot commented Aug 19, 2019

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.

Loading

@RyanCavanaugh
Copy link
Member

@RyanCavanaugh RyanCavanaugh commented Aug 19, 2019

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.

Loading

@RyanCavanaugh
Copy link
Member

@RyanCavanaugh RyanCavanaugh commented Aug 19, 2019

@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.

Loading

@falsandtru
Copy link
Contributor

@falsandtru falsandtru commented Sep 2, 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.

Loading

@orta
Copy link
Member

@orta orta commented Sep 25, 2019

Closing as #33178 is now merged

Loading

@luryson
Copy link

@luryson luryson commented Nov 17, 2021

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');
        }
    });

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants