Checking for void in logical operations

[tomitrescak]

In many cases it is not desirable to allow comparison with undefined. Is it somehow enforceable that this will be detected during compilation?

function a() { }

if (a()) { } // this should throw error

I guess this is by design not available.

[RyanCavanaugh]

It's certainly very suspicious. We can look at how much code this might break.

[DanielRosenwasser]

Disallowing void from the LHS of a ||, &&, and conditionals, we catch this somewhat inappropriate code in the compiler.

12983                 checkFunctionOrMethodDeclaration(node) || checkGrammarForGenerator(node);
                      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

src/compiler/checker.ts(12983,17): error TS5063: The left-hand side of a '||' expression cannot have type 'void'.

Running tests now.

[tomitrescak]

Oh wow, that would save me from the world of problems! Please let me know whether this will make it into "next" version. I guess this can be configurable with a parameter in tsconfig.

[zpdDG4gta8XKpMCd]

can we instead add a flag that prohibits non-boolean expressions where a boolean is expected? #7306

[tomitrescak]

@DanielRosenwasser would you be able to share with us, what is the result? Will this (flag) make it to the compiler? Thanks!

[adidahiya]

This would be pretty great to have in the compiler. Linking to our TSLint issue tracking this: palantir/tslint#253

[DanielRosenwasser]

@tomitrescak sorry, I've been sidetracked on other things. The team will need to review whether we're certain that it's a net positive as well. In the mean time, my pull request can be found at #7261. I don't think there's a ton of controversial stuff in it. Feel free to try it out, review it, and get back to me.

[RyanCavanaugh]

👍 great idea

[zpdDG4gta8XKpMCd]

sometimes I really hard to follow your reasoning

void method/functions return undefined, returning undefined is a pretty normal thing in idiomatic JavaScript because it is one of the few types coerces to a false value naturally, so it seems an overkill to ban it on a this can never happen premise

a more vicious problem is when you had a boolean value in a condition expression, but after refactoring it turned, say, into a lazy boolean () => boolean

can we flag it?

oh sure, tslint (well actually something more capable that that) to the rescue, I know, I know

[RyanCavanaugh]

it is one of the few types coerces to a false value naturally, so it seems an overkill to ban it on a this can never happen premise

I don't understand this logic. The point is that a conditional on a known-falsy value is almost certainly a bug because it creates either unreachable code, or code that appears conditionally-executed when it is in fact unconditionally executed.

On the flipside, the problem is that there's lots of idiomatic code that does stuff like this:

// x.filterFunc: () => boolean
if (x.filterFunc) {
  arr = arr.filter(x.filterFunc);
}

Certainly one can have the opinion that it would be more correct to write if (x != filterFunc) { but that's ultimately just a matter of style.

[zpdDG4gta8XKpMCd]

what about void returning overloads mixed with non voids ones?

speaking of style, it's not sbout style at all, the problem is (seen it a
few times myself) that the code with a boolean expression in the condition
of the if statement during refactoring accidentally turns into something
else (a non boolean expression), and after it still type checks, because
non boolean expressions are totally welcome there, thus a bug is planted
that can't be found by the compiler

before

var isFirst:boolean;
if (isFirst) {
// reachable
}

after refactoring

var isFirst : () => boolean;
if (!isFirst) {
// unreachable
}

On Mar 14, 2016 10:59 PM, "Ryan Cavanaugh" [email protected] wrote:

it is one of the few types coerces to a false value naturally, so it seems
an overkill to ban it on a this can never happen premise

I don't understand this logic. The point is that a conditional on a
known-false value is almost certainly a bug because it creates either
unreachable code, or code that appears conditionally-executed when it is in
fact unconditionally executed.

On the flipside, the problem is that there's lots of idiomatic code that
does stuff like this:

// x.filterFunc: () => booleanif (x.filterFunc) {
arr = arr.filter(x.filterFunc);
}

Certainly one can have the opinion that it would be more correct to
write if (x != filterFunc) { but that's ultimately just a matter of style.

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
#7256 (comment)

[RyanCavanaugh]

I totally understand how you could find yourself in that state. I've been there myself.

At the same time, there is a lot of code that is in that state on purpose that does not have any bugs.

[dead-claudia]

I'll note that it's also very frequent that people (like me) use numbers in conditionals. It's not that uncommon for me to do something like this:

if (list.length) {
    // list isn't empty, let's do something with it.
}

So I personally agree that it's best left to a linter.

[RyanCavanaugh]

Done. Tracking break at #26262