Only error when testing functions that are not used in the following …

…block
microsoft · Aug 31, 2019 · 3c9e338 · 3c9e338
1 parent 006a327
commit 3c9e338
Show file tree

Hide file tree

Showing 8 changed files with 436 additions and 190 deletions.
diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
@@ -27931,24 +27931,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, type);
             checkSourceElement(node.thenStatement);
 
             if (node.thenStatement.kind === SyntaxKind.EmptyStatement) {
@@ -27958,6 +27941,57 @@ namespace ts {
             checkSourceElement(node.elseStatement);
         }
 
+        function checkTestingKnownTruthyCallableType(ifStatement: IfStatement, type: Type) {
+            if (!strictNullChecks) {
+                return;
+            }
+
+            const testedNode = isIdentifier(ifStatement.expression)
+                ? ifStatement.expression
+                : isPropertyAccessExpression(ifStatement.expression)
+                    ? ifStatement.expression.name
+                    : undefined;
+
+            if (!testedNode) {
+                return;
+            }
+
+            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 unrefenced in the block 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 testedFunctionSymbol = getSymbolAtLocation(testedNode);
+            if (!testedFunctionSymbol) {
+                return;
+            }
+
+            const functionIsUsedInBody = forEachChild(ifStatement.thenStatement, function check(childNode): boolean | undefined {
+                if (isIdentifier(childNode)) {
+                    const childSymbol = getSymbolAtLocation(childNode);
+                    if (childSymbol && childSymbol.id === testedFunctionSymbol.id) {
+                        return true;
+                    }
+                }
+
+                return forEachChild(childNode, check);
+            });
+
+            if (!functionIsUsedInBody) {
+                error(ifStatement.expression, 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/src/compiler/core.ts b/src/compiler/core.ts
@@ -1653,7 +1653,7 @@ namespace ts {
      */
     export function compose<T>(...args: ((t: T) => T)[]): (t: T) => T;
     export function compose<T>(a: (t: T) => T, b: (t: T) => T, c: (t: T) => T, d: (t: T) => T, e: (t: T) => T): (t: T) => T {
-        if (e) {
+        if (!!e) {
             const args: ((t: T) => T)[] = [];
             for (let i = 0; i < arguments.length; i++) {
                 args[i] = arguments[i];

diff --git a/src/compiler/program.ts b/src/compiler/program.ts
@@ -1362,18 +1362,16 @@ namespace ts {
             // try to verify results of module resolution
             for (const { oldFile: oldSourceFile, newFile: newSourceFile } of modifiedSourceFiles) {
                 const newSourceFilePath = getNormalizedAbsolutePath(newSourceFile.originalFileName, currentDirectory);
-                if (resolveModuleNamesWorker) {
-                    const moduleNames = getModuleNames(newSourceFile);
-                    const resolutions = resolveModuleNamesReusingOldState(moduleNames, newSourceFilePath, newSourceFile);
-                    // ensure that module resolution results are still correct
-                    const resolutionsChanged = hasChangesInResolutions(moduleNames, resolutions, oldSourceFile.resolvedModules, moduleResolutionIsEqualTo);
-                    if (resolutionsChanged) {
-                        oldProgram.structureIsReused = StructureIsReused.SafeModules;
-                        newSourceFile.resolvedModules = zipToMap(moduleNames, resolutions);
-                    }
-                    else {
-                        newSourceFile.resolvedModules = oldSourceFile.resolvedModules;
-                    }
+                const moduleNames = getModuleNames(newSourceFile);
+                const resolutions = resolveModuleNamesReusingOldState(moduleNames, newSourceFilePath, newSourceFile);
+                // ensure that module resolution results are still correct
+                const resolutionsChanged = hasChangesInResolutions(moduleNames, resolutions, oldSourceFile.resolvedModules, moduleResolutionIsEqualTo);
+                if (resolutionsChanged) {
+                    oldProgram.structureIsReused = StructureIsReused.SafeModules;
+                    newSourceFile.resolvedModules = zipToMap(moduleNames, resolutions);
+                }
+                else {
+                    newSourceFile.resolvedModules = oldSourceFile.resolvedModules;
                 }
                 if (resolveTypeReferenceDirectiveNamesWorker) {
                     // We lower-case all type references because npm automatically lowercases all packages. See GH#9824.

diff --git a/tests/baselines/reference/truthinessCallExpressionCoercion.errors.txt b/tests/baselines/reference/truthinessCallExpressionCoercion.errors.txt
@@ -1,51 +1,73 @@
-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(2,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(18,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(36,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(66,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) ====
-    function func() { return Math.random() > 0.5; }
-
-    if (func) { // error
-        ~~~~
-!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
-    }
-
-    function onlyErrorsWhenNonNullable(required: () => boolean, optional?: () => boolean) {
+    function onlyErrorsWhenTestingNonNullableFunctionType(required: () => boolean, optional?: () => boolean) {
         if (required) { // error
             ~~~~~~~~
 !!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
         }
 
+        if (optional) { // ok
+        }
+
+        if (!!required) { // ok
+        }
+
         if (required()) { // ok
         }
+    }
 
-        if (optional) { // ok
+    function onlyErrorsWhenUnusedInBody() {
+        function test() { return Math.random() > 0.5; }
+
+        if (test) { // error
+            ~~~~
+!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
+            console.log('test');
+        }
+
+        if (test) { // ok
+            console.log(test);
+        }
+
+        if (test) { // ok
+            test();
+        }
+
+        if (test) { // ok
+            [() => null].forEach(() => {
+                test();
+            });
+        }
+
+        if (test) { // error
+            ~~~~
+!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
+            [() => null].forEach(test => {
+                test();
+            });
         }
     }
 
-    function checksPropertyAndElementAccess() {
+    function checksPropertyAccess() {
         const x = {
             foo: {
-                bar() { }
+                bar() { return true; }
             }
         }
 
         if (x.foo.bar) { // error
             ~~~~~~~~~
 !!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
         }
-
-        if (x.foo['bar']) { // error
-            ~~~~~~~~~~~~
-!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
-        }
-    }
 
-    function maybeBoolean(param: boolean | (() => boolean)) {
-        if (param) { // ok
+        if (x.foo.bar) { // ok
+            x.foo.bar;
         }
     }
 

diff --git a/tests/baselines/reference/truthinessCallExpressionCoercion.js b/tests/baselines/reference/truthinessCallExpressionCoercion.js
@@ -1,36 +1,58 @@
 //// [truthinessCallExpressionCoercion.ts]
-function func() { return Math.random() > 0.5; }
+function onlyErrorsWhenTestingNonNullableFunctionType(required: () => boolean, optional?: () => boolean) {
+    if (required) { // error
+    }
 
-if (func) { // error
-}
+    if (optional) { // ok
+    }
 
-function onlyErrorsWhenNonNullable(required: () => boolean, optional?: () => boolean) {
-    if (required) { // error
+    if (!!required) { // ok
     }
 
     if (required()) { // ok
     }
+}
 
-    if (optional) { // ok
+function onlyErrorsWhenUnusedInBody() {
+    function test() { return Math.random() > 0.5; }
+
+    if (test) { // error
+        console.log('test');
+    }
+
+    if (test) { // ok
+        console.log(test);
+    }
+
+    if (test) { // ok
+        test();
+    }
+
+    if (test) { // ok
+        [() => null].forEach(() => {
+            test();
+        });
+    }
+
+    if (test) { // error
+        [() => null].forEach(test => {
+            test();
+        });
     }
 }
 
-function checksPropertyAndElementAccess() {
+function checksPropertyAccess() {
     const x = {
         foo: {
-            bar() { }
+            bar() { return true; }
         }
     }
 
     if (x.foo.bar) { // error
     }
-
-    if (x.foo['bar']) { // error
-    }
-}
 
-function maybeBoolean(param: boolean | (() => boolean)) {
-    if (param) { // ok
+    if (x.foo.bar) { // ok
+        x.foo.bar;
     }
 }
 
@@ -52,30 +74,48 @@ class Foo {
 
 
 //// [truthinessCallExpressionCoercion.js]
-function func() { return Math.random() > 0.5; }
-if (func) { // error
-}
-function onlyErrorsWhenNonNullable(required, optional) {
+function onlyErrorsWhenTestingNonNullableFunctionType(required, optional) {
     if (required) { // error
     }
+    if (optional) { // ok
+    }
+    if (!!required) { // ok
+    }
     if (required()) { // ok
     }
-    if (optional) { // ok
+}
+function onlyErrorsWhenUnusedInBody() {
+    function test() { return Math.random() > 0.5; }
+    if (test) { // error
+        console.log('test');
+    }
+    if (test) { // ok
+        console.log(test);
+    }
+    if (test) { // ok
+        test();
+    }
+    if (test) { // ok
+        [function () { return null; }].forEach(function () {
+            test();
+        });
+    }
+    if (test) { // error
+        [function () { return null; }].forEach(function (test) {
+            test();
+        });
     }
 }
-function checksPropertyAndElementAccess() {
+function checksPropertyAccess() {
     var x = {
         foo: {
-            bar: function () { }
+            bar: function () { return true; }
         }
     };
     if (x.foo.bar) { // error
     }
-    if (x.foo['bar']) { // error
-    }
-}
-function maybeBoolean(param) {
-    if (param) { // ok
+    if (x.foo.bar) { // ok
+        x.foo.bar;
     }
 }
 var Foo = /** @class */ (function () {