Skip to content

Commit 8ca5cdf

Browse files
jonhuejonhue
committed
review
1 parent f1d4a81 commit 8ca5cdf

6 files changed

+353
-67
lines changed

‎src/compiler/checker.ts

Lines changed: 49 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -30731,7 +30731,7 @@ namespace ts {
3073130731
if (operator === SyntaxKind.AmpersandAmpersandToken || operator === SyntaxKind.BarBarToken || operator === SyntaxKind.QuestionQuestionToken) {
3073230732
if (operator === SyntaxKind.AmpersandAmpersandToken) {
3073330733
const parent = walkUpParenthesizedExpressions(node.parent);
30734-
checkTestingKnownTruthyCallableOrAwaitableType(node.left, leftType, isIfStatement(parent) ? parent.thenStatement : undefined);
30734+
checkTestingKnownTruthyCallableOrAwaitableType(node.left, isIfStatement(parent) ? parent.thenStatement : undefined);
3073530735
}
3073630736
checkTruthinessOfType(leftType, node.left);
3073730737
}
@@ -31274,8 +31274,7 @@ namespace ts {
3127431274
}
3127531275

3127631276
function checkConditionalExpression(node: ConditionalExpression, checkMode?: CheckMode): Type {
31277-
const type = checkTruthinessExpression(node.condition);
31278-
checkTestingKnownTruthyCallableOrAwaitableType(node.condition, type, node.whenTrue);
31277+
checkTestingKnownTruthyCallableOrAwaitableType(node.condition, node.whenTrue);
3127931278
const type1 = checkExpression(node.whenTrue, checkMode);
3128031279
const type2 = checkExpression(node.whenFalse, checkMode);
3128131280
return getUnionType([type1, type2], UnionReduction.Subtype);
@@ -34525,8 +34524,7 @@ namespace ts {
3452534524
function checkIfStatement(node: IfStatement) {
3452634525
// Grammar checking
3452734526
checkGrammarStatementInAmbientContext(node);
34528-
const type = checkTruthinessExpression(node.expression);
34529-
checkTestingKnownTruthyCallableOrAwaitableType(node.expression, type, node.thenStatement);
34527+
checkTestingKnownTruthyCallableOrAwaitableType(node.expression, node.thenStatement);
3453034528
checkSourceElement(node.thenStatement);
3453134529

3453234530
if (node.thenStatement.kind === SyntaxKind.EmptyStatement) {
@@ -34536,52 +34534,59 @@ namespace ts {
3453634534
checkSourceElement(node.elseStatement);
3453734535
}
3453834536

34539-
function checkTestingKnownTruthyCallableOrAwaitableType(condExpr: Expression, type: Type, body?: Statement | Expression) {
34537+
function checkTestingKnownTruthyCallableOrAwaitableType(condExpr: Expression, body?: Statement | Expression) {
3454034538
if (!strictNullChecks) return;
34541-
if (getFalsyFlags(type)) return;
3454234539

34543-
if (getAwaitedTypeOfPromise(type)) {
34544-
errorAndMaybeSuggestAwait(
34545-
condExpr,
34546-
/*maybeMissingAwait*/ true,
34547-
Diagnostics.This_condition_will_always_return_0_since_the_types_1_and_2_have_no_overlap,
34548-
"true", getTypeNameForErrorDisplay(type), "false");
34549-
return;
34540+
helper(condExpr, body)
34541+
while (isBinaryExpression(condExpr) && condExpr.operatorToken.kind === SyntaxKind.BarBarToken) {
34542+
condExpr = condExpr.left
34543+
helper(condExpr, body)
3455034544
}
3455134545

34552-
const location = isBinaryExpression(condExpr) ? condExpr.right : condExpr;
34553-
if (isBinaryExpression(condExpr) && condExpr.operatorToken.kind === SyntaxKind.BarBarToken) {
34554-
checkTestingKnownTruthyCallableOrAwaitableType(condExpr.left, type, body);
34555-
}
34556-
const testedNode = isIdentifier(location) ? location
34557-
: isPropertyAccessExpression(location) ? location.name
34558-
: isBinaryExpression(location) && isIdentifier(location.right) ? location.right
34559-
: undefined;
34560-
const isPropertyExpressionCast = isPropertyAccessExpression(location)
34561-
&& isAssertionExpression(skipParentheses(location.expression));
34562-
if (!testedNode || isPropertyExpressionCast) {
34563-
return;
34564-
}
34546+
function helper(condExpr: Expression, body: Expression | Statement | undefined) {
34547+
const location = isBinaryExpression(condExpr) ? condExpr.right : condExpr;
34548+
const type = checkTruthinessExpression(location);
34549+
if (getFalsyFlags(type)) return;
3456534550

34566-
// While it technically should be invalid for any known-truthy value
34567-
// to be tested, we de-scope to functions unrefenced in the block as a
34568-
// heuristic to identify the most common bugs. There are too many
34569-
// false positives for values sourced from type definitions without
34570-
// strictNullChecks otherwise.
34571-
const callSignatures = getSignaturesOfType(type, SignatureKind.Call);
34572-
if (callSignatures.length === 0) {
34573-
return;
34574-
}
34551+
if (getAwaitedTypeOfPromise(type)) {
34552+
errorAndMaybeSuggestAwait(
34553+
condExpr,
34554+
/*maybeMissingAwait*/ true,
34555+
Diagnostics.This_condition_will_always_return_0_since_the_types_1_and_2_have_no_overlap,
34556+
"true", getTypeNameForErrorDisplay(type), "false");
34557+
return;
34558+
}
3457534559

34576-
const testedSymbol = getSymbolAtLocation(testedNode);
34577-
if (!testedSymbol) {
34578-
return;
34579-
}
34560+
const testedNode = isIdentifier(location) ? location
34561+
: isPropertyAccessExpression(location) ? location.name
34562+
: isBinaryExpression(location) && isIdentifier(location.right) ? location.right
34563+
: undefined;
34564+
const isPropertyExpressionCast = isPropertyAccessExpression(location)
34565+
&& isAssertionExpression(skipParentheses(location.expression));
34566+
if (!testedNode || isPropertyExpressionCast) {
34567+
return;
34568+
}
34569+
34570+
// While it technically should be invalid for any known-truthy value
34571+
// to be tested, we de-scope to functions unrefenced in the block as a
34572+
// heuristic to identify the most common bugs. There are too many
34573+
// false positives for values sourced from type definitions without
34574+
// strictNullChecks otherwise.
34575+
const callSignatures = getSignaturesOfType(type, SignatureKind.Call);
34576+
if (callSignatures.length === 0) {
34577+
return;
34578+
}
34579+
34580+
const testedSymbol = getSymbolAtLocation(testedNode);
34581+
if (!testedSymbol) {
34582+
return;
34583+
}
3458034584

34581-
const isUsed = isBinaryExpression(condExpr.parent) && isFunctionUsedInBinaryExpressionChain(condExpr.parent, testedSymbol)
34582-
|| body && isFunctionUsedInConditionBody(condExpr, body, testedNode, testedSymbol);
34583-
if (!isUsed) {
34584-
error(location, Diagnostics.This_condition_will_always_return_true_since_the_function_is_always_defined_Did_you_mean_to_call_it_instead);
34585+
const isUsed = isBinaryExpression(condExpr.parent) && isFunctionUsedInBinaryExpressionChain(condExpr.parent, testedSymbol)
34586+
|| body && isFunctionUsedInConditionBody(condExpr, body, testedNode, testedSymbol);
34587+
if (!isUsed) {
34588+
error(location, Diagnostics.This_condition_will_always_return_true_since_the_function_is_always_defined_Did_you_mean_to_call_it_instead);
34589+
}
3458534590
}
3458634591
}
3458734592

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

Lines changed: 55 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,34 +1,81 @@
1-
tests/cases/compiler/uncalledFunctionChecksInConditional.ts(4,5): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
2-
tests/cases/compiler/uncalledFunctionChecksInConditional.ts(8,5): 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/uncalledFunctionChecksInConditional.ts(8,14): 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/uncalledFunctionChecksInConditional.ts(12,5): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
1+
tests/cases/compiler/uncalledFunctionChecksInConditional.ts(5,5): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
2+
tests/cases/compiler/uncalledFunctionChecksInConditional.ts(9,5): 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/uncalledFunctionChecksInConditional.ts(9,14): 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/uncalledFunctionChecksInConditional.ts(13,5): 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/uncalledFunctionChecksInConditional.ts(32,10): 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/uncalledFunctionChecksInConditional.ts(36,5): 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/uncalledFunctionChecksInConditional.ts(40,22): 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/uncalledFunctionChecksInConditional.ts(44,16): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
9+
tests/cases/compiler/uncalledFunctionChecksInConditional.ts(48,22): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
510

611

7-
==== tests/cases/compiler/uncalledFunctionChecksInConditional.ts (4 errors) ====
12+
==== tests/cases/compiler/uncalledFunctionChecksInConditional.ts (9 errors) ====
813
declare function isFoo(): boolean;
914
declare function isBar(): boolean;
15+
declare const isUndefinedFoo: (() => boolean) | undefined;
1016

1117
if (isFoo) {
1218
~~~~~
1319
!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
14-
// error
20+
// error on isFoo
1521
}
1622

1723
if (isFoo || isBar) {
1824
~~~~~
1925
!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
2026
~~~~~
2127
!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
22-
// error
28+
// error on isFoo, isBar
2329
}
2430

2531
if (isFoo || isFoo()) {
2632
~~~~~
2733
!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
28-
// error
34+
// error on isFoo
35+
}
36+
37+
if (isUndefinedFoo || isFoo()) {
38+
// no error
2939
}
3040

3141
if (isFoo && isFoo()) {
3242
// no error
3343
}
44+
45+
declare const x: boolean;
46+
declare const ux: boolean | undefined;
47+
declare const y: boolean;
48+
declare const uy: boolean | undefined;
49+
declare function z(): boolean;
50+
declare const uz: (() => boolean) | undefined;
51+
52+
if (x || isFoo) {
53+
~~~~~
54+
!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
55+
// error on isFoo
56+
}
57+
58+
if (isFoo || x) {
59+
~~~~~
60+
!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
61+
// error on isFoo
62+
}
63+
64+
if (x || y || z() || isFoo) {
65+
~~~~~
66+
!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
67+
// error on isFoo
68+
}
69+
70+
if (x || uy || z || isUndefinedFoo) {
71+
~
72+
!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
73+
// error on z
74+
}
75+
76+
if (ux || y || uz || isFoo) {
77+
~~~~~
78+
!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
79+
// error on isFoo
80+
}
3481

‎tests/baselines/reference/uncalledFunctionChecksInConditional.js

Lines changed: 56 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,34 +1,84 @@
11
//// [uncalledFunctionChecksInConditional.ts]
22
declare function isFoo(): boolean;
33
declare function isBar(): boolean;
4+
declare const isUndefinedFoo: (() => boolean) | undefined;
45

56
if (isFoo) {
6-
// error
7+
// error on isFoo
78
}
89

910
if (isFoo || isBar) {
10-
// error
11+
// error on isFoo, isBar
1112
}
1213

1314
if (isFoo || isFoo()) {
14-
// error
15+
// error on isFoo
16+
}
17+
18+
if (isUndefinedFoo || isFoo()) {
19+
// no error
1520
}
1621

1722
if (isFoo && isFoo()) {
1823
// no error
1924
}
25+
26+
declare const x: boolean;
27+
declare const ux: boolean | undefined;
28+
declare const y: boolean;
29+
declare const uy: boolean | undefined;
30+
declare function z(): boolean;
31+
declare const uz: (() => boolean) | undefined;
32+
33+
if (x || isFoo) {
34+
// error on isFoo
35+
}
36+
37+
if (isFoo || x) {
38+
// error on isFoo
39+
}
40+
41+
if (x || y || z() || isFoo) {
42+
// error on isFoo
43+
}
44+
45+
if (x || uy || z || isUndefinedFoo) {
46+
// error on z
47+
}
48+
49+
if (ux || y || uz || isFoo) {
50+
// error on isFoo
51+
}
2052

2153

2254
//// [uncalledFunctionChecksInConditional.js]
2355
if (isFoo) {
24-
// error
56+
// error on isFoo
2557
}
2658
if (isFoo || isBar) {
27-
// error
59+
// error on isFoo, isBar
2860
}
2961
if (isFoo || isFoo()) {
30-
// error
62+
// error on isFoo
63+
}
64+
if (isUndefinedFoo || isFoo()) {
65+
// no error
3166
}
3267
if (isFoo && isFoo()) {
3368
// no error
3469
}
70+
if (x || isFoo) {
71+
// error on isFoo
72+
}
73+
if (isFoo || x) {
74+
// error on isFoo
75+
}
76+
if (x || y || z() || isFoo) {
77+
// error on isFoo
78+
}
79+
if (x || uy || z || isUndefinedFoo) {
80+
// error on z
81+
}
82+
if (ux || y || uz || isFoo) {
83+
// error on isFoo
84+
}

0 commit comments

Comments
 (0)