Disallow expressions of type void to be used in truthiness checks

[RyanCavanaugh]

Fixes an offhand comment from #17892

This found a bug (!) in session.ts - reloadFromFile returned void so we were never hitting this line. I added return statements to reloadFromFile based on what I think it should do but it's not immediately apparent.

        private reload(args: protocol.ReloadRequestArgs, reqSeq: number) {
            const file = toNormalizedPath(args.file);
            const tempFileName = args.tmpfile === undefined ? undefined : toNormalizedPath(args.tmpfile);
            const info = this.projectService.getScriptInfoForNormalizedPath(file);
            if (info) {
                this.changeSeq++;
                // make sure no changes happen before this one is finished
                if (info.reloadFromFile(tempFileName)) {
                    this.doOutput(/*info*/ undefined, CommandNames.Reload, reqSeq, /*success*/ true);
/*---------         !!!!!!!!!!!!! unreachable! */
                }
            }
        }

[RyanCavanaugh]

@typescript-bot test this

[typescript-bot]

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

[RyanCavanaugh]

@weswigham is it possible to view the RWC diffs?

[weswigham]

Not easily (or without doubling the build time). I think I could probably surface them as an extra vsts pr without too much consternation - I'll look into it. Although we should also strive for usually having a clean RWC run, too. ❤️

[ajafff]

What about the condition in a ConditionalExpresson a ? b : c, the LHS of a && b and a || b and the operand of prefix unary negation !a?

[RyanCavanaugh]

New RWC failures:

https://github.com/firebase/firebase-js-sdk/blob/master/packages/database/src/core/util/Tree.ts#L156 - this function accepts an (arg: T) => void callback and tests its result - basically a type error but not a real bug per se in this file (may be an error in other places though)

VS Code has a similar issue: https://github.com/Microsoft/vscode/blob/master/src/vs/base/parts/tree/browser/treeDefaults.ts#L268

[weswigham]

@typescript-bot test this

[typescript-bot]

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

[ahejlsberg]

We should also include this check in conditional expressions.

[ahejlsberg]

A better way to write this is if (type.flags & TypeFlags.Void) as we may at some point introduce other type instances that represent a void type.

[weswigham]

@typescript-bot test this

[typescript-bot]

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

[RyanCavanaugh]

lodash RWC turned up one "not obviously broken" example (simplified):

const noop = () => { };
const blah = () => 5;
var fn = Math.random() > 0.5 ? noop : blah;
if (fn()) {

}

Due to subtype reduction, fn is () => void and we issue an error.

RWC also found some code in VS Code (since fixed) where they were writing

assert.ok(fs.exists("foo"));

where fs.exists is (filename: string, callback: (r: boolean) => void) => void, so very much a real bug.

[weswigham]

@typescript-bot test this

[typescript-bot]

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

[ahejlsberg]

@RyanCavanaugh I agree the lodash example isn't obviously broken, but it definitely is suspicious given that a function returning anything can be assigned to a function returning void. So, with that in mind, I think it is a reasonable catch.

[chharvey]

I don't think this should have been done. A lot of times people use void in a logical OR operator to simplify code:

before:

let timesTwo = (x: number) => {
  console.log(`original: ${x}`);
  return x * 2;
}

after:

let timesTwo = (x: number) => console.log(`original: ${x}`) || x * 2;

another example:

let timesTwo = (x: number) => assert(!Number.isNaN(x) && Number.isFinite(x), `Must provide a finite number.`) || x * 2

because console.log(), assert(), etc. are of type void, TypeScript v3.1.1 throws this error:

error TS1345: An expression of type 'void' cannot be tested for truthiness

[Jessidhia]

That looks obfuscated, not simplified... if you really want to do something like that that still clearly shows the side-effect nature of your call, you should use the comma operator.

let timesTwo = (x: number) => (console.log(`original: ${x}`), x * 2);

[RyanCavanaugh]

Let's take all discussion to #26262