Only error when testing functions that return booleans

jwbay · jwbay · commit 3906a1e42773 · 2019-08-18T13:44:47.000-04:00
diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
@@ -27875,24 +27875,7 @@ namespace ts {
             checkGrammarStatementInAmbientContext(node);
 
             const type = checkTruthinessExpression(node.expression);
-
-            if (strictNullChecks &&
-                (node.expression.kind === SyntaxKind.Identifier ||
-                node.expression.kind === SyntaxKind.PropertyAccessExpression ||
-                node.expression.kind === SyntaxKind.ElementAccessExpression)) {
-                const possiblyFalsy = !!getFalsyFlags(type);
-                if (!possiblyFalsy) {
-                    // While it technically should be invalid for any known-truthy value
-                    // to be tested, we de-scope to functions as a heuristic to identify
-                    // the most common bugs. There are too many false positives for values
-                    // sourced from type definitions without strictNullChecks otherwise.
-                    const callSignatures = getSignaturesOfType(type, SignatureKind.Call);
-                    if (callSignatures.length > 0) {
-                        error(node.expression, Diagnostics.This_condition_will_always_return_true_since_the_function_is_always_defined_Did_you_mean_to_call_it_instead);
-                    }
-                }
-            }
-
+            checkTestingKnownTruthyCallableType(node.expression, type);
             checkSourceElement(node.thenStatement);
 
             if (node.thenStatement.kind === SyntaxKind.EmptyStatement) {
@@ -27902,6 +27885,52 @@ namespace ts {
             checkSourceElement(node.elseStatement);
         }
 
+        function checkTestingKnownTruthyCallableType(testedNode: Expression, type: Type) {
+            if (!strictNullChecks) {
+                return;
+            }
+
+            if (testedNode.kind === SyntaxKind.Identifier ||
+                testedNode.kind === SyntaxKind.PropertyAccessExpression ||
+                testedNode.kind === SyntaxKind.ElementAccessExpression) {
+                const possiblyFalsy = getFalsyFlags(type);
+                if (possiblyFalsy) {
+                    return;
+                }
+
+                // While it technically should be invalid for any known-truthy value
+                // to be tested, we de-scope to functions that return a boolean as a
+                // heuristic to identify the most common bugs. There are too many
+                // false positives for values sourced from type definitions without
+                // strictNullChecks otherwise.
+                const callSignatures = getSignaturesOfType(type, SignatureKind.Call);
+                if (callSignatures.length === 0) {
+                    return;
+                }
+
+                const alwaysReturnsBoolean = every(callSignatures, signature => {
+                    const returnType = getReturnTypeOfSignature(signature);
+                    const exactlyBoolean = TypeFlags.Boolean | TypeFlags.Union;
+                    if ((returnType.flags & exactlyBoolean) === exactlyBoolean) {
+                        return true;
+                    }
+
+                    if (returnType.flags & TypeFlags.Union) {
+                        const allowedInUnion = TypeFlags.Boolean | TypeFlags.BooleanLiteral | TypeFlags.Nullable;
+                        return every((returnType as UnionType).types, innerType => {
+                            return (innerType.flags | allowedInUnion) === allowedInUnion;
+                        });
+                    }
+
+                    return false;
+                });
+
+                if (alwaysReturnsBoolean) {
+                    error(testedNode, Diagnostics.This_condition_will_always_return_true_since_the_function_is_always_defined_Did_you_mean_to_call_it_instead);
+                }
+            }
+        }
+
         function checkDoStatement(node: DoStatement) {
             // Grammar checking
             checkGrammarStatementInAmbientContext(node);
diff --git a/tests/baselines/reference/truthinessCallExpressionCoercion.errors.txt b/tests/baselines/reference/truthinessCallExpressionCoercion.errors.txt
@@ -1,11 +1,13 @@
 tests/cases/compiler/truthinessCallExpressionCoercion.ts(3,5): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
 tests/cases/compiler/truthinessCallExpressionCoercion.ts(7,9): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
-tests/cases/compiler/truthinessCallExpressionCoercion.ts(24,9): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
 tests/cases/compiler/truthinessCallExpressionCoercion.ts(27,9): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
-tests/cases/compiler/truthinessCallExpressionCoercion.ts(44,13): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
+tests/cases/compiler/truthinessCallExpressionCoercion.ts(30,9): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
+tests/cases/compiler/truthinessCallExpressionCoercion.ts(50,9): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
+tests/cases/compiler/truthinessCallExpressionCoercion.ts(53,9): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
+tests/cases/compiler/truthinessCallExpressionCoercion.ts(70,13): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
 
 
-==== tests/cases/compiler/truthinessCallExpressionCoercion.ts (5 errors) ====
+==== tests/cases/compiler/truthinessCallExpressionCoercion.ts (7 errors) ====
     function func() { return Math.random() > 0.5; }
     
     if (func) { // error
@@ -19,17 +21,47 @@ tests/cases/compiler/truthinessCallExpressionCoercion.ts(44,13): error TS2774: T
 !!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
         }
     
+        if (!!required) { // ok
+        }
+    
         if (required()) { // ok
         }
     
         if (optional) { // ok
         }
     }
     
+    function onlyErrorsWhenReturnsBoolean(
+        bool: () => boolean,
+        nullableBool: () => boolean | undefined,
+        notABool: () => string,
+        unionWithBool: () => boolean | string,
+        nullableString: () => string | undefined
+    ) {
+        if (bool) { // error
+            ~~~~
+!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
+        }
+    
+        if (nullableBool) { // error
+            ~~~~~~~~~~~~
+!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
+        }
+    
+        if (notABool) { // ok
+        }
+    
+        if (unionWithBool) { // ok
+        }
+    
+        if (nullableString) { // ok
+        }
+    }
+    
     function checksPropertyAndElementAccess() {
         const x = {
             foo: {
-                bar() { }
+                bar() { return true; }
             }
         }
     
diff --git a/tests/baselines/reference/truthinessCallExpressionCoercion.js b/tests/baselines/reference/truthinessCallExpressionCoercion.js
@@ -8,17 +8,43 @@ function onlyErrorsWhenNonNullable(required: () => boolean, optional?: () => boo
     if (required) { // error
     }
 
+    if (!!required) { // ok
+    }
+
     if (required()) { // ok
     }
 
     if (optional) { // ok
     }
 }
 
+function onlyErrorsWhenReturnsBoolean(
+    bool: () => boolean,
+    nullableBool: () => boolean | undefined,
+    notABool: () => string,
+    unionWithBool: () => boolean | string,
+    nullableString: () => string | undefined
+) {
+    if (bool) { // error
+    }
+
+    if (nullableBool) { // error
+    }
+
+    if (notABool) { // ok
+    }
+
+    if (unionWithBool) { // ok
+    }
+
+    if (nullableString) { // ok
+    }
+}
+
 function checksPropertyAndElementAccess() {
     const x = {
         foo: {
-            bar() { }
+            bar() { return true; }
         }
     }
 
@@ -58,15 +84,29 @@ if (func) { // error
 function onlyErrorsWhenNonNullable(required, optional) {
     if (required) { // error
     }
+    if (!!required) { // ok
+    }
     if (required()) { // ok
     }
     if (optional) { // ok
     }
 }
+function onlyErrorsWhenReturnsBoolean(bool, nullableBool, notABool, unionWithBool, nullableString) {
+    if (bool) { // error
+    }
+    if (nullableBool) { // error
+    }
+    if (notABool) { // ok
+    }
+    if (unionWithBool) { // ok
+    }
+    if (nullableString) { // ok
+    }
+}
 function checksPropertyAndElementAccess() {
     var x = {
         foo: {
-            bar: function () { }
+            bar: function () { return true; }
         }
     };
     if (x.foo.bar) { // error
diff --git a/tests/baselines/reference/truthinessCallExpressionCoercion.symbols b/tests/baselines/reference/truthinessCallExpressionCoercion.symbols
@@ -18,6 +18,10 @@ function onlyErrorsWhenNonNullable(required: () => boolean, optional?: () => boo
 >required : Symbol(required, Decl(truthinessCallExpressionCoercion.ts, 5, 35))
     }
 
+    if (!!required) { // ok
+>required : Symbol(required, Decl(truthinessCallExpressionCoercion.ts, 5, 35))
+    }
+
     if (required()) { // ok
 >required : Symbol(required, Decl(truthinessCallExpressionCoercion.ts, 5, 35))
     }
@@ -27,70 +31,110 @@ function onlyErrorsWhenNonNullable(required: () => boolean, optional?: () => boo
     }
 }
 
+function onlyErrorsWhenReturnsBoolean(
+>onlyErrorsWhenReturnsBoolean : Symbol(onlyErrorsWhenReturnsBoolean, Decl(truthinessCallExpressionCoercion.ts, 17, 1))
+
+    bool: () => boolean,
+>bool : Symbol(bool, Decl(truthinessCallExpressionCoercion.ts, 19, 38))
+
+    nullableBool: () => boolean | undefined,
+>nullableBool : Symbol(nullableBool, Decl(truthinessCallExpressionCoercion.ts, 20, 24))
+
+    notABool: () => string,
+>notABool : Symbol(notABool, Decl(truthinessCallExpressionCoercion.ts, 21, 44))
+
+    unionWithBool: () => boolean | string,
+>unionWithBool : Symbol(unionWithBool, Decl(truthinessCallExpressionCoercion.ts, 22, 27))
+
+    nullableString: () => string | undefined
+>nullableString : Symbol(nullableString, Decl(truthinessCallExpressionCoercion.ts, 23, 42))
+
+) {
+    if (bool) { // error
+>bool : Symbol(bool, Decl(truthinessCallExpressionCoercion.ts, 19, 38))
+    }
+
+    if (nullableBool) { // error
+>nullableBool : Symbol(nullableBool, Decl(truthinessCallExpressionCoercion.ts, 20, 24))
+    }
+
+    if (notABool) { // ok
+>notABool : Symbol(notABool, Decl(truthinessCallExpressionCoercion.ts, 21, 44))
+    }
+
+    if (unionWithBool) { // ok
+>unionWithBool : Symbol(unionWithBool, Decl(truthinessCallExpressionCoercion.ts, 22, 27))
+    }
+
+    if (nullableString) { // ok
+>nullableString : Symbol(nullableString, Decl(truthinessCallExpressionCoercion.ts, 23, 42))
+    }
+}
+
 function checksPropertyAndElementAccess() {
->checksPropertyAndElementAccess : Symbol(checksPropertyAndElementAccess, Decl(truthinessCallExpressionCoercion.ts, 14, 1))
+>checksPropertyAndElementAccess : Symbol(checksPropertyAndElementAccess, Decl(truthinessCallExpressionCoercion.ts, 40, 1))
 
     const x = {
->x : Symbol(x, Decl(truthinessCallExpressionCoercion.ts, 17, 9))
+>x : Symbol(x, Decl(truthinessCallExpressionCoercion.ts, 43, 9))
 
         foo: {
->foo : Symbol(foo, Decl(truthinessCallExpressionCoercion.ts, 17, 15))
+>foo : Symbol(foo, Decl(truthinessCallExpressionCoercion.ts, 43, 15))
 
-            bar() { }
->bar : Symbol(bar, Decl(truthinessCallExpressionCoercion.ts, 18, 14))
+            bar() { return true; }
+>bar : Symbol(bar, Decl(truthinessCallExpressionCoercion.ts, 44, 14))
         }
     }
 
     if (x.foo.bar) { // error
->x.foo.bar : Symbol(bar, Decl(truthinessCallExpressionCoercion.ts, 18, 14))
->x.foo : Symbol(foo, Decl(truthinessCallExpressionCoercion.ts, 17, 15))
->x : Symbol(x, Decl(truthinessCallExpressionCoercion.ts, 17, 9))
->foo : Symbol(foo, Decl(truthinessCallExpressionCoercion.ts, 17, 15))
->bar : Symbol(bar, Decl(truthinessCallExpressionCoercion.ts, 18, 14))
+>x.foo.bar : Symbol(bar, Decl(truthinessCallExpressionCoercion.ts, 44, 14))
+>x.foo : Symbol(foo, Decl(truthinessCallExpressionCoercion.ts, 43, 15))
+>x : Symbol(x, Decl(truthinessCallExpressionCoercion.ts, 43, 9))
+>foo : Symbol(foo, Decl(truthinessCallExpressionCoercion.ts, 43, 15))
+>bar : Symbol(bar, Decl(truthinessCallExpressionCoercion.ts, 44, 14))
     }
     
     if (x.foo['bar']) { // error
->x.foo : Symbol(foo, Decl(truthinessCallExpressionCoercion.ts, 17, 15))
->x : Symbol(x, Decl(truthinessCallExpressionCoercion.ts, 17, 9))
->foo : Symbol(foo, Decl(truthinessCallExpressionCoercion.ts, 17, 15))
->'bar' : Symbol(bar, Decl(truthinessCallExpressionCoercion.ts, 18, 14))
+>x.foo : Symbol(foo, Decl(truthinessCallExpressionCoercion.ts, 43, 15))
+>x : Symbol(x, Decl(truthinessCallExpressionCoercion.ts, 43, 9))
+>foo : Symbol(foo, Decl(truthinessCallExpressionCoercion.ts, 43, 15))
+>'bar' : Symbol(bar, Decl(truthinessCallExpressionCoercion.ts, 44, 14))
     }
 }
 
 function maybeBoolean(param: boolean | (() => boolean)) {
->maybeBoolean : Symbol(maybeBoolean, Decl(truthinessCallExpressionCoercion.ts, 28, 1))
->param : Symbol(param, Decl(truthinessCallExpressionCoercion.ts, 30, 22))
+>maybeBoolean : Symbol(maybeBoolean, Decl(truthinessCallExpressionCoercion.ts, 54, 1))
+>param : Symbol(param, Decl(truthinessCallExpressionCoercion.ts, 56, 22))
 
     if (param) { // ok
->param : Symbol(param, Decl(truthinessCallExpressionCoercion.ts, 30, 22))
+>param : Symbol(param, Decl(truthinessCallExpressionCoercion.ts, 56, 22))
     }
 }
 
 class Foo {
->Foo : Symbol(Foo, Decl(truthinessCallExpressionCoercion.ts, 33, 1))
+>Foo : Symbol(Foo, Decl(truthinessCallExpressionCoercion.ts, 59, 1))
 
     maybeIsUser?: () => boolean;
->maybeIsUser : Symbol(Foo.maybeIsUser, Decl(truthinessCallExpressionCoercion.ts, 35, 11))
+>maybeIsUser : Symbol(Foo.maybeIsUser, Decl(truthinessCallExpressionCoercion.ts, 61, 11))
 
     isUser() {
->isUser : Symbol(Foo.isUser, Decl(truthinessCallExpressionCoercion.ts, 36, 32))
+>isUser : Symbol(Foo.isUser, Decl(truthinessCallExpressionCoercion.ts, 62, 32))
 
         return true;
     }
 
     test() {
->test : Symbol(Foo.test, Decl(truthinessCallExpressionCoercion.ts, 40, 5))
+>test : Symbol(Foo.test, Decl(truthinessCallExpressionCoercion.ts, 66, 5))
 
         if (this.isUser) { // error
->this.isUser : Symbol(Foo.isUser, Decl(truthinessCallExpressionCoercion.ts, 36, 32))
->this : Symbol(Foo, Decl(truthinessCallExpressionCoercion.ts, 33, 1))
->isUser : Symbol(Foo.isUser, Decl(truthinessCallExpressionCoercion.ts, 36, 32))
+>this.isUser : Symbol(Foo.isUser, Decl(truthinessCallExpressionCoercion.ts, 62, 32))
+>this : Symbol(Foo, Decl(truthinessCallExpressionCoercion.ts, 59, 1))
+>isUser : Symbol(Foo.isUser, Decl(truthinessCallExpressionCoercion.ts, 62, 32))
         }
 
         if (this.maybeIsUser) { // ok
->this.maybeIsUser : Symbol(Foo.maybeIsUser, Decl(truthinessCallExpressionCoercion.ts, 35, 11))
->this : Symbol(Foo, Decl(truthinessCallExpressionCoercion.ts, 33, 1))
->maybeIsUser : Symbol(Foo.maybeIsUser, Decl(truthinessCallExpressionCoercion.ts, 35, 11))
+>this.maybeIsUser : Symbol(Foo.maybeIsUser, Decl(truthinessCallExpressionCoercion.ts, 61, 11))
+>this : Symbol(Foo, Decl(truthinessCallExpressionCoercion.ts, 59, 1))
+>maybeIsUser : Symbol(Foo.maybeIsUser, Decl(truthinessCallExpressionCoercion.ts, 61, 11))
         }
     }
 }
diff --git a/tests/baselines/reference/truthinessCallExpressionCoercion.types b/tests/baselines/reference/truthinessCallExpressionCoercion.types
diff --git a/tests/cases/compiler/truthinessCallExpressionCoercion.ts b/tests/cases/compiler/truthinessCallExpressionCoercion.ts
