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

Heya@orta, I've started to run the extended test suite on this PR at3c9e338. You can monitor the buildhere. It should now contribute to this PR's status checks.

Heya@orta, I've started to run the parallelized community code test suite on this PR at3c9e338. You can monitor the buildhere. It should now contribute to this PR's status checks.

Heya@orta, I've started to run the parallelized Definitely Typed test suite on this PR at3c9e338. You can monitor the buildhere. It should now contribute to this PR's status checks.

The user suite test run you requested has finished andfailed. I've opened aPR with the baseline diff from master.

The RWC diff is unrelated.

The User Test Suite diff is a false positive, but not a bad one:https://github.com/npm/cli/blob/latest/test/tap/check-permissions.js#L27

@typescript-bot perf test this

Heya@RyanCavanaugh, I've started to run the perf test suite on this PR at3c9e338. You can monitor the buildhere. It should now contribute to this PR's status checks.

Update:The results are in!

@RyanCavanaugh
The results of the perf run you requested are in!

Hrm, none of these builds have accessible logs.

I bet it's likely that community projects will need changes like:

-        if (e) {+       if (!!e) {

Will re-run and take a look.

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

Heya@orta, I've started to run the extended test suite on this PR at3c9e338. You can monitor the buildhere. It should now contribute to this PR's status checks.

Heya@orta, I've started to run the parallelized community code test suite on this PR at3c9e338. You can monitor the buildhere. It should now contribute to this PR's status checks.

The user suite test run you requested has finished andfailed. I've opened aPR with the baseline diff from master.

OK@jwbay - to get this mergeable you'll need to acceptjwbay#3

OK, perf looks acceptable to me.

The idea is a good one, and I feel good about the implementation - thanks@jwbay!

@jwbay
Is there any reason why it affects only functions and not arrays, records, etc?

function foo(array: string[]) {  if (array) {    return true  }}function bar(record: Record<string, string>) {  if (record) {    return true  }}

@user753 a few major problems appeared when we tried that approach.

Array access is optimistically assumed to be in-bounds, so code like this gets incorrectly flagged as an error. The same thing happens forMap.get and a few other APIs that are assumed to "always return non-undefined values when used correctly".

constarr=[rec,rec,rec];constel=arr[someIndex];if(el){// do something}

Separately, many definition files were written prior to the introduction ofstrictNullChecks, so correctnull/undefined checks on properties from those definitions would get incorrectly flagged as an error.

Ultimately the very narrow check implemented here was the only one that didn't generate a huge amount of false positives.

@RyanCavanaugh Thanks.
Do I understand correctly that It isn't a problem for functions because a function inside an array isn't a common use case?

const arr = [func, func, func];const el = arr[someIndex];if (el) {  // do something}

Normally if that does happen, a call toel() appears in the body of theif, which is the additional check that this PR implements. Ifel *isn't` called, that'll be a false positive.

@jwbay this might be my favorite check this release.https://github.com/microsoft/vscode/blob/ecba37c6ef3aab85b7a9b15afb3610bbe4eaac8c/extensions/css-language-features/server/src/cssServerMain.ts#L78

It would be great if this was extended to functions that return promises as well.

In the announcement for 3.7 (which I'mthrilled about!!!), you include this code example.

functiondoAdminThing(user:User){if(user.isAdministrator){//  ~~~~~~~~~~~~~~~~~~~~// error! This condition will always return true since the function is always defined.//        Did you mean to call it instead?t

It would be incredible if this was also extended to raise an error when the function returns a promise and it's not awaited before using it in a condition (I've personally been bitten by this exact bug before).

classUser{// ...asyncisAdministrator(){/* ... */}}functiondoAdminThing(user:User){if(user.isAdministrator()){//  ~~~~~~~~~~~~~~~~~~~~~~// error! This condition will always return true since a promise is always truthy.//        Did you mean to await it instead?

I tried searching for issues related to this but couldn't find any (it's possible that I just missed it because there are so many issues and I'm not sure what keywords to search).

I've definitely been bitten by a bug where I did something like this:

asyncfunctionpasswordMatches(email:string,password:string){/* return true if the password matches the result in the database */}if(passwordMatches("foo@bar.gov","passw0rd")){/* issue auth token */}

Regarding Uncalled Function Checks, I think it'd be pretty safe to also error on this:

functionprocess(){console.log('Processing...');}process;

BTW this is breaking all of my polyfills, like checks forif (String.prototype.trimLeft) {}. Any way to disable this?

The problem with this is, it's not allowing any of the valid ways to pass. As@nebrelbug pointed above.

if (String.prototype.trimLeft) {} //fails
if (typeof String.prototype.trimLeft === 'function') {} //fails
if ('trimLeft' in String.prototype) //fails

We had to adapt to this, which passes

// eslint-disable-next-line no-extra-boolean-cast
if (!!String.prototype.trimLeft)