Skip to content

Commit fb1066e

Browse files
jonhuesandersn
jonhue
and
sandersn
authored
Uncalled function checks only works with single conditional (#42835)
* Uncalled function checks only works with single conditional * fix type errors in compiler * remove uncalled function checks with negations * review * fix test * Cleanup after merge, accept baselines Co-authored-by: Nathan Shively-Sanders <[email protected]>
1 parent 53809d8 commit fb1066e

12 files changed

+500
-49
lines changed

‎src/compiler/checker.ts

Lines changed: 52 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -33203,7 +33203,7 @@ namespace ts {
3320333203
if (operator === SyntaxKind.AmpersandAmpersandToken || operator === SyntaxKind.BarBarToken || operator === SyntaxKind.QuestionQuestionToken) {
3320433204
if (operator === SyntaxKind.AmpersandAmpersandToken) {
3320533205
const parent = walkUpParenthesizedExpressions(node.parent);
33206-
checkTestingKnownTruthyCallableOrAwaitableType(node.left, leftType, isIfStatement(parent) ? parent.thenStatement : undefined);
33206+
checkTestingKnownTruthyCallableOrAwaitableType(node.left, isIfStatement(parent) ? parent.thenStatement : undefined);
3320733207
}
3320833208
checkTruthinessOfType(leftType, node.left);
3320933209
}
@@ -33776,8 +33776,8 @@ namespace ts {
3377633776
}
3377733777

3377833778
function checkConditionalExpression(node: ConditionalExpression, checkMode?: CheckMode): Type {
33779-
const type = checkTruthinessExpression(node.condition);
33780-
checkTestingKnownTruthyCallableOrAwaitableType(node.condition, type, node.whenTrue);
33779+
checkTruthinessExpression(node.condition);
33780+
checkTestingKnownTruthyCallableOrAwaitableType(node.condition, node.whenTrue);
3378133781
const type1 = checkExpression(node.whenTrue, checkMode);
3378233782
const type2 = checkExpression(node.whenFalse, checkMode);
3378333783
return getUnionType([type1, type2], UnionReduction.Subtype);
@@ -37274,8 +37274,8 @@ namespace ts {
3727437274
function checkIfStatement(node: IfStatement) {
3727537275
// Grammar checking
3727637276
checkGrammarStatementInAmbientContext(node);
37277-
const type = checkTruthinessExpression(node.expression);
37278-
checkTestingKnownTruthyCallableOrAwaitableType(node.expression, type, node.thenStatement);
37277+
checkTruthinessExpression(node.expression);
37278+
checkTestingKnownTruthyCallableOrAwaitableType(node.expression, node.thenStatement);
3727937279
checkSourceElement(node.thenStatement);
3728037280

3728137281
if (node.thenStatement.kind === SyntaxKind.EmptyStatement) {
@@ -37285,48 +37285,57 @@ namespace ts {
3728537285
checkSourceElement(node.elseStatement);
3728637286
}
3728737287

37288-
function checkTestingKnownTruthyCallableOrAwaitableType(condExpr: Expression, type: Type, body?: Statement | Expression) {
37288+
function checkTestingKnownTruthyCallableOrAwaitableType(condExpr: Expression, body?: Statement | Expression) {
3728937289
if (!strictNullChecks) return;
37290-
if (getFalsyFlags(type)) return;
37291-
37292-
const location = isBinaryExpression(condExpr) ? condExpr.right : condExpr;
37293-
if (isPropertyAccessExpression(location) && isTypeAssertion(location.expression)) {
37294-
return;
37295-
}
37296-
37297-
const testedNode = isIdentifier(location) ? location
37298-
: isPropertyAccessExpression(location) ? location.name
37299-
: isBinaryExpression(location) && isIdentifier(location.right) ? location.right
37300-
: undefined;
3730137290

37302-
// While it technically should be invalid for any known-truthy value
37303-
// to be tested, we de-scope to functions and Promises unreferenced in
37304-
// the block as a heuristic to identify the most common bugs. There
37305-
// are too many false positives for values sourced from type
37306-
// definitions without strictNullChecks otherwise.
37307-
const callSignatures = getSignaturesOfType(type, SignatureKind.Call);
37308-
const isPromise = !!getAwaitedTypeOfPromise(type);
37309-
if (callSignatures.length === 0 && !isPromise) {
37310-
return;
37311-
}
37312-
37313-
const testedSymbol = testedNode && getSymbolAtLocation(testedNode);
37314-
if (!testedSymbol && !isPromise) {
37315-
return;
37316-
}
37291+
helper(condExpr, body);
37292+
while (isBinaryExpression(condExpr) && condExpr.operatorToken.kind === SyntaxKind.BarBarToken) {
37293+
condExpr = condExpr.left;
37294+
helper(condExpr, body);
37295+
}
37296+
37297+
function helper(condExpr: Expression, body: Expression | Statement | undefined) {
37298+
const location = isBinaryExpression(condExpr) &&
37299+
(condExpr.operatorToken.kind === SyntaxKind.BarBarToken || condExpr.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken)
37300+
? condExpr.right
37301+
: condExpr;
37302+
const type = checkTruthinessExpression(location);
37303+
const isPropertyExpressionCast = isPropertyAccessExpression(location) && isTypeAssertion(location.expression);
37304+
if (getFalsyFlags(type) || isPropertyExpressionCast) return;
37305+
37306+
// While it technically should be invalid for any known-truthy value
37307+
// to be tested, we de-scope to functions and Promises unreferenced in
37308+
// the block as a heuristic to identify the most common bugs. There
37309+
// are too many false positives for values sourced from type
37310+
// definitions without strictNullChecks otherwise.
37311+
const callSignatures = getSignaturesOfType(type, SignatureKind.Call);
37312+
const isPromise = !!getAwaitedTypeOfPromise(type);
37313+
if (callSignatures.length === 0 && !isPromise) {
37314+
return;
37315+
}
3731737316

37318-
const isUsed = testedSymbol && isBinaryExpression(condExpr.parent) && isSymbolUsedInBinaryExpressionChain(condExpr.parent, testedSymbol)
37319-
|| testedSymbol && body && isSymbolUsedInConditionBody(condExpr, body, testedNode, testedSymbol);
37320-
if (!isUsed) {
37321-
if (isPromise) {
37322-
errorAndMaybeSuggestAwait(
37323-
location,
37324-
/*maybeMissingAwait*/ true,
37325-
Diagnostics.This_condition_will_always_return_true_since_this_0_is_always_defined,
37326-
getTypeNameForErrorDisplay(type));
37317+
const testedNode = isIdentifier(location) ? location
37318+
: isPropertyAccessExpression(location) ? location.name
37319+
: isBinaryExpression(location) && isIdentifier(location.right) ? location.right
37320+
: undefined;
37321+
const testedSymbol = testedNode && getSymbolAtLocation(testedNode);
37322+
if (!testedSymbol && !isPromise) {
37323+
return;
3732737324
}
37328-
else {
37329-
error(location, Diagnostics.This_condition_will_always_return_true_since_this_function_is_always_defined_Did_you_mean_to_call_it_instead);
37325+
37326+
const isUsed = testedSymbol && isBinaryExpression(condExpr.parent) && isSymbolUsedInBinaryExpressionChain(condExpr.parent, testedSymbol)
37327+
|| testedSymbol && body && isSymbolUsedInConditionBody(condExpr, body, testedNode, testedSymbol);
37328+
if (!isUsed) {
37329+
if (isPromise) {
37330+
errorAndMaybeSuggestAwait(
37331+
location,
37332+
/*maybeMissingAwait*/ true,
37333+
Diagnostics.This_condition_will_always_return_true_since_this_0_is_always_defined,
37334+
getTypeNameForErrorDisplay(type));
37335+
}
37336+
else {
37337+
error(location, Diagnostics.This_condition_will_always_return_true_since_this_function_is_always_defined_Did_you_mean_to_call_it_instead);
37338+
}
3733037339
}
3733137340
}
3733237341
}

‎src/compiler/debug.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,7 @@ namespace ts {
185185
}
186186

187187
export function assertNever(member: never, message = "Illegal value:", stackCrawlMark?: AnyFunction): never {
188-
const detail = typeof member === "object" && hasProperty(member, "kind") && hasProperty(member, "pos") && formatSyntaxKind ? "SyntaxKind: " + formatSyntaxKind((member as Node).kind) : JSON.stringify(member);
188+
const detail = typeof member === "object" && hasProperty(member, "kind") && hasProperty(member, "pos") ? "SyntaxKind: " + formatSyntaxKind((member as Node).kind) : JSON.stringify(member);
189189
return fail(`${message} ${detail}`, stackCrawlMark || assertNever);
190190
}
191191

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

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
tests/cases/compiler/uncalledFunctionChecksInConditional.ts(5,5): error TS2774: This condition will always return true since this 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 this 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 this 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 this 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 this 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 this 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 this 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 this 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 this function is always defined. Did you mean to call it instead?
10+
11+
12+
==== tests/cases/compiler/uncalledFunctionChecksInConditional.ts (9 errors) ====
13+
declare function isFoo(): boolean;
14+
declare function isBar(): boolean;
15+
declare const isUndefinedFoo: (() => boolean) | undefined;
16+
17+
if (isFoo) {
18+
~~~~~
19+
!!! error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
20+
// error on isFoo
21+
}
22+
23+
if (isFoo || isBar) {
24+
~~~~~
25+
!!! error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
26+
~~~~~
27+
!!! error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
28+
// error on isFoo, isBar
29+
}
30+
31+
if (isFoo || isFoo()) {
32+
~~~~~
33+
!!! error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
34+
// error on isFoo
35+
}
36+
37+
if (isUndefinedFoo || isFoo()) {
38+
// no error
39+
}
40+
41+
if (isFoo && isFoo()) {
42+
// no error
43+
}
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 this 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 this 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 this 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 this 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 this function is always defined. Did you mean to call it instead?
79+
// error on isFoo
80+
}
81+

‎tests/baselines/reference/uncalledFunctionChecksInConditional.js

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
//// [uncalledFunctionChecksInConditional.ts]
2+
declare function isFoo(): boolean;
3+
declare function isBar(): boolean;
4+
declare const isUndefinedFoo: (() => boolean) | undefined;
5+
6+
if (isFoo) {
7+
// error on isFoo
8+
}
9+
10+
if (isFoo || isBar) {
11+
// error on isFoo, isBar
12+
}
13+
14+
if (isFoo || isFoo()) {
15+
// error on isFoo
16+
}
17+
18+
if (isUndefinedFoo || isFoo()) {
19+
// no error
20+
}
21+
22+
if (isFoo && isFoo()) {
23+
// no error
24+
}
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+
}
52+
53+
54+
//// [uncalledFunctionChecksInConditional.js]
55+
if (isFoo) {
56+
// error on isFoo
57+
}
58+
if (isFoo || isBar) {
59+
// error on isFoo, isBar
60+
}
61+
if (isFoo || isFoo()) {
62+
// error on isFoo
63+
}
64+
if (isUndefinedFoo || isFoo()) {
65+
// no error
66+
}
67+
if (isFoo && isFoo()) {
68+
// no error
69+
}
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)