Skip to content

Commit 327ff39

Browse files
jwbayjwbay
committed
Only error when testing functions that return booleans
1 parent 130615a commit 327ff39

6 files changed

+379
-93
lines changed

‎src/compiler/checker.ts

+54-18
Original file line numberDiff line numberDiff line change
@@ -27875,24 +27875,7 @@ namespace ts {
2787527875
checkGrammarStatementInAmbientContext(node);
2787627876

2787727877
const type = checkTruthinessExpression(node.expression);
27878-
27879-
if (strictNullChecks &&
27880-
(node.expression.kind === SyntaxKind.Identifier ||
27881-
node.expression.kind === SyntaxKind.PropertyAccessExpression ||
27882-
node.expression.kind === SyntaxKind.ElementAccessExpression)) {
27883-
const possiblyFalsy = !!getFalsyFlags(type);
27884-
if (!possiblyFalsy) {
27885-
// While it technically should be invalid for any known-truthy value
27886-
// to be tested, we de-scope to functions as a heuristic to identify
27887-
// the most common bugs. There are too many false positives for values
27888-
// sourced from type definitions without strictNullChecks otherwise.
27889-
const callSignatures = getSignaturesOfType(type, SignatureKind.Call);
27890-
if (callSignatures.length > 0) {
27891-
error(node.expression, Diagnostics.This_condition_will_always_return_true_since_the_function_is_always_defined_Did_you_mean_to_call_it_instead);
27892-
}
27893-
}
27894-
}
27895-
27878+
checkTestingKnownTruthyCallableType(node.expression, type);
2789627879
checkSourceElement(node.thenStatement);
2789727880

2789827881
if (node.thenStatement.kind === SyntaxKind.EmptyStatement) {
@@ -27902,6 +27885,59 @@ namespace ts {
2790227885
checkSourceElement(node.elseStatement);
2790327886
}
2790427887

27888+
function checkTestingKnownTruthyCallableType(testedNode: Expression, type: Type) {
27889+
if (!strictNullChecks) {
27890+
return;
27891+
}
27892+
27893+
if (testedNode.kind === SyntaxKind.Identifier ||
27894+
testedNode.kind === SyntaxKind.PropertyAccessExpression ||
27895+
testedNode.kind === SyntaxKind.ElementAccessExpression) {
27896+
const possiblyFalsy = getFalsyFlags(type);
27897+
if (possiblyFalsy) {
27898+
return;
27899+
}
27900+
27901+
// While it technically should be invalid for any known-truthy value
27902+
// to be tested, we de-scope to functions that return a boolean as a
27903+
// heuristic to identify the most common bugs. There are too many
27904+
// false positives for values sourced from type definitions without
27905+
// strictNullChecks otherwise.
27906+
const callSignatures = getSignaturesOfType(type, SignatureKind.Call);
27907+
if (callSignatures.length === 0) {
27908+
return;
27909+
}
27910+
27911+
const alwaysReturnsBoolean = every(callSignatures, signature => {
27912+
const returnType = getReturnTypeOfSignature(signature);
27913+
const exactlyBoolean = TypeFlags.Boolean | TypeFlags.Union;
27914+
if (returnType.flags === exactlyBoolean) {
27915+
return true;
27916+
}
27917+
27918+
if (returnType.flags & TypeFlags.Union) {
27919+
const allowedInUnion = TypeFlags.BooleanLike | TypeFlags.Nullable;
27920+
let unionContainsBoolean = false;
27921+
const unionContainsOnlyAllowedTypes = every((returnType as UnionType).types, innerType => {
27922+
if (innerType.flags & TypeFlags.BooleanLike) {
27923+
unionContainsBoolean = true;
27924+
}
27925+
27926+
return (innerType.flags | allowedInUnion) === allowedInUnion;
27927+
});
27928+
27929+
return unionContainsBoolean && unionContainsOnlyAllowedTypes;
27930+
}
27931+
27932+
return false;
27933+
});
27934+
27935+
if (alwaysReturnsBoolean) {
27936+
error(testedNode, Diagnostics.This_condition_will_always_return_true_since_the_function_is_always_defined_Did_you_mean_to_call_it_instead);
27937+
}
27938+
}
27939+
}
27940+
2790527941
function checkDoStatement(node: DoStatement) {
2790627942
// Grammar checking
2790727943
checkGrammarStatementInAmbientContext(node);

‎tests/baselines/reference/truthinessCallExpressionCoercion.errors.txt

+51-8
Original file line numberDiff line numberDiff line change
@@ -1,43 +1,86 @@
11
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?
22
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?
3-
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?
4-
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?
5-
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?
3+
tests/cases/compiler/truthinessCallExpressionCoercion.ts(29,9): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
4+
tests/cases/compiler/truthinessCallExpressionCoercion.ts(32,9): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
5+
tests/cases/compiler/truthinessCallExpressionCoercion.ts(35,9): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
6+
tests/cases/compiler/truthinessCallExpressionCoercion.ts(58,9): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
7+
tests/cases/compiler/truthinessCallExpressionCoercion.ts(61,9): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
8+
tests/cases/compiler/truthinessCallExpressionCoercion.ts(78,13): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
69

710

8-
==== tests/cases/compiler/truthinessCallExpressionCoercion.ts (5 errors) ====
11+
==== tests/cases/compiler/truthinessCallExpressionCoercion.ts (8 errors) ====
912
function func() { return Math.random() > 0.5; }
1013

1114
if (func) { // error
1215
~~~~
1316
!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
1417
}
1518

16-
function onlyErrorsWhenNonNullable(required: () => boolean, optional?: () => boolean) {
19+
function onlyErrorsWhenTestingNonNullableFunctionType(required: () => boolean, optional?: () => boolean) {
1720
if (required) { // error
1821
~~~~~~~~
1922
!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
2023
}
2124

25+
if (optional) { // ok
26+
}
27+
28+
if (!!required) { // ok
29+
}
30+
2231
if (required()) { // ok
2332
}
33+
}
2434

25-
if (optional) { // ok
35+
function onlyErrorsWhenReturnsBoolean(
36+
bool: () => boolean,
37+
nullableBool: () => boolean | undefined,
38+
nullableTrue: () => true | undefined,
39+
nullable: () => undefined | null,
40+
notABool: () => string,
41+
unionWithBool: () => boolean | string,
42+
nullableString: () => string | undefined
43+
) {
44+
if (bool) { // error
45+
~~~~
46+
!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
47+
}
48+
49+
if (nullableBool) { // error
50+
~~~~~~~~~~~~
51+
!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
52+
}
53+
54+
if (nullableTrue) { // error
55+
~~~~~~~~~~~~
56+
!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
57+
}
58+
59+
if (nullable) { // ok
60+
}
61+
62+
if (notABool) { // ok
63+
}
64+
65+
if (unionWithBool) { // ok
66+
}
67+
68+
if (nullableString) { // ok
2669
}
2770
}
2871

2972
function checksPropertyAndElementAccess() {
3073
const x = {
3174
foo: {
32-
bar() { }
75+
bar() { return true; }
3376
}
3477
}
3578

3679
if (x.foo.bar) { // error
3780
~~~~~~~~~
3881
!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
3982
}
40-
83+
4184
if (x.foo['bar']) { // error
4285
~~~~~~~~~~~~
4386
!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?

‎tests/baselines/reference/truthinessCallExpressionCoercion.js

+59-7
Original file line numberDiff line numberDiff line change
@@ -4,27 +4,61 @@ function func() { return Math.random() > 0.5; }
44
if (func) { // error
55
}
66

7-
function onlyErrorsWhenNonNullable(required: () => boolean, optional?: () => boolean) {
7+
function onlyErrorsWhenTestingNonNullableFunctionType(required: () => boolean, optional?: () => boolean) {
88
if (required) { // error
99
}
1010

11+
if (optional) { // ok
12+
}
13+
14+
if (!!required) { // ok
15+
}
16+
1117
if (required()) { // ok
1218
}
19+
}
1320

14-
if (optional) { // ok
21+
function onlyErrorsWhenReturnsBoolean(
22+
bool: () => boolean,
23+
nullableBool: () => boolean | undefined,
24+
nullableTrue: () => true | undefined,
25+
nullable: () => undefined | null,
26+
notABool: () => string,
27+
unionWithBool: () => boolean | string,
28+
nullableString: () => string | undefined
29+
) {
30+
if (bool) { // error
31+
}
32+
33+
if (nullableBool) { // error
34+
}
35+
36+
if (nullableTrue) { // error
37+
}
38+
39+
if (nullable) { // ok
40+
}
41+
42+
if (notABool) { // ok
43+
}
44+
45+
if (unionWithBool) { // ok
46+
}
47+
48+
if (nullableString) { // ok
1549
}
1650
}
1751

1852
function checksPropertyAndElementAccess() {
1953
const x = {
2054
foo: {
21-
bar() { }
55+
bar() { return true; }
2256
}
2357
}
2458

2559
if (x.foo.bar) { // error
2660
}
27-
61+
2862
if (x.foo['bar']) { // error
2963
}
3064
}
@@ -55,18 +89,36 @@ class Foo {
5589
function func() { return Math.random() > 0.5; }
5690
if (func) { // error
5791
}
58-
function onlyErrorsWhenNonNullable(required, optional) {
92+
function onlyErrorsWhenTestingNonNullableFunctionType(required, optional) {
5993
if (required) { // error
6094
}
95+
if (optional) { // ok
96+
}
97+
if (!!required) { // ok
98+
}
6199
if (required()) { // ok
62100
}
63-
if (optional) { // ok
101+
}
102+
function onlyErrorsWhenReturnsBoolean(bool, nullableBool, nullableTrue, nullable, notABool, unionWithBool, nullableString) {
103+
if (bool) { // error
104+
}
105+
if (nullableBool) { // error
106+
}
107+
if (nullableTrue) { // error
108+
}
109+
if (nullable) { // ok
110+
}
111+
if (notABool) { // ok
112+
}
113+
if (unionWithBool) { // ok
114+
}
115+
if (nullableString) { // ok
64116
}
65117
}
66118
function checksPropertyAndElementAccess() {
67119
var x = {
68120
foo: {
69-
bar: function () { }
121+
bar: function () { return true; }
70122
}
71123
};
72124
if (x.foo.bar) { // error

0 commit comments

Comments
 (0)