Skip to content

Commit 06fb724

Browse files
sandersnsandersn
authored
Improve uncalled function checks (#41599)
Fixes #41586 Fixes #41588 1. For binary expressions, if the immediate parent is an IfStatement, then check the body of the if statement. I didn't walk upward to find an IfStatement because in my experimentation I found that binary expression uncalled-function errors are only issued when the expression is on the left of the top-most binary expression. 2. For property accesses with interspersed calls, I added a CallExpression case. In fact, any expression could appear here, but I only want to fix calls for now since that's all we've observed in Definitely Typed, and we didn't see anything else in the user tests or RWC tests. I also didn't examine parameters of the intermediate call expressions, but I don't think it's needed since the intent is to avoid false positives.
1 parent 23b3eb6 commit 06fb724

11 files changed

+338
-125
lines changed

‎src/compiler/checker.ts

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -30185,7 +30185,7 @@ namespace ts {
3018530185
const operator = node.operatorToken.kind;
3018630186
if (operator === SyntaxKind.AmpersandAmpersandToken || operator === SyntaxKind.BarBarToken || operator === SyntaxKind.QuestionQuestionToken) {
3018730187
if (operator === SyntaxKind.AmpersandAmpersandToken) {
30188-
checkTestingKnownTruthyCallableType(node.left, leftType);
30188+
checkTestingKnownTruthyCallableType(node.left, leftType, isIfStatement(node.parent) ? node.parent.thenStatement : undefined);
3018930189
}
3019030190
checkTruthinessOfType(leftType, node.left);
3019130191
}
@@ -33972,7 +33972,7 @@ namespace ts {
3397233972
checkSourceElement(node.elseStatement);
3397333973
}
3397433974

33975-
function checkTestingKnownTruthyCallableType(condExpr: Expression, type: Type, body?: Statement | Expression) {
33975+
function checkTestingKnownTruthyCallableType(condExpr: Expression, type: Type, body: Statement | Expression | undefined) {
3397633976
if (!strictNullChecks) {
3397733977
return;
3397833978
}
@@ -34007,9 +34007,8 @@ namespace ts {
3400734007
return;
3400834008
}
3400934009

34010-
const isUsed = isBinaryExpression(condExpr.parent) ? isFunctionUsedInBinaryExpressionChain(condExpr.parent, testedSymbol)
34011-
: body ? isFunctionUsedInConditionBody(condExpr, body, testedNode, testedSymbol)
34012-
: false;
34010+
const isUsed = isBinaryExpression(condExpr.parent) && isFunctionUsedInBinaryExpressionChain(condExpr.parent, testedSymbol)
34011+
|| body && isFunctionUsedInConditionBody(condExpr, body, testedNode, testedSymbol);
3401334012
if (!isUsed) {
3401434013
error(location, Diagnostics.This_condition_will_always_return_true_since_the_function_is_always_defined_Did_you_mean_to_call_it_instead);
3401534014
}
@@ -34032,14 +34031,17 @@ namespace ts {
3403234031
testedExpression.kind === SyntaxKind.ThisKeyword && childExpression.kind === SyntaxKind.ThisKeyword) {
3403334032
return getSymbolAtLocation(testedExpression) === getSymbolAtLocation(childExpression);
3403434033
}
34035-
34036-
if (isPropertyAccessExpression(testedExpression) && isPropertyAccessExpression(childExpression)) {
34034+
else if (isPropertyAccessExpression(testedExpression) && isPropertyAccessExpression(childExpression)) {
3403734035
if (getSymbolAtLocation(testedExpression.name) !== getSymbolAtLocation(childExpression.name)) {
3403834036
return false;
3403934037
}
3404034038
childExpression = childExpression.expression;
3404134039
testedExpression = testedExpression.expression;
3404234040
}
34041+
else if (isCallExpression(testedExpression) && isCallExpression(childExpression)) {
34042+
childExpression = childExpression.expression;
34043+
testedExpression = testedExpression.expression;
34044+
}
3404334045
else {
3404434046
return false;
3404534047
}

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

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ tests/cases/compiler/truthinessCallExpressionCoercion1.ts(3,5): error TS2774: Th
22
tests/cases/compiler/truthinessCallExpressionCoercion1.ts(19,5): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
33
tests/cases/compiler/truthinessCallExpressionCoercion1.ts(33,5): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
44
tests/cases/compiler/truthinessCallExpressionCoercion1.ts(46,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/truthinessCallExpressionCoercion1.ts(61,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/truthinessCallExpressionCoercion1.ts(76,9): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
66

77

88
==== tests/cases/compiler/truthinessCallExpressionCoercion1.ts (5 errors) ====
@@ -63,6 +63,21 @@ tests/cases/compiler/truthinessCallExpressionCoercion1.ts(61,9): error TS2774: T
6363

6464
// ok
6565
x.foo.bar ? x.foo.bar : undefined;
66+
67+
var chrome = {
68+
platformKeys: {
69+
subtleCrypto() {
70+
return {
71+
sign() {},
72+
exportKey() { return true }
73+
}
74+
}
75+
}
76+
}
77+
// ok
78+
if (chrome.platformKeys.subtleCrypto().exportKey) {
79+
chrome.platformKeys.subtleCrypto().exportKey
80+
}
6681
}
6782

6883
class Foo {

‎tests/baselines/reference/truthinessCallExpressionCoercion1.js

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,21 @@ function checksPropertyAccess() {
4848

4949
// ok
5050
x.foo.bar ? x.foo.bar : undefined;
51+
52+
var chrome = {
53+
platformKeys: {
54+
subtleCrypto() {
55+
return {
56+
sign() {},
57+
exportKey() { return true }
58+
}
59+
}
60+
}
61+
}
62+
// ok
63+
if (chrome.platformKeys.subtleCrypto().exportKey) {
64+
chrome.platformKeys.subtleCrypto().exportKey
65+
}
5166
}
5267

5368
class Foo {
@@ -110,6 +125,20 @@ function checksPropertyAccess() {
110125
x.foo.bar ? console.log('x.foo.bar') : undefined;
111126
// ok
112127
x.foo.bar ? x.foo.bar : undefined;
128+
var chrome = {
129+
platformKeys: {
130+
subtleCrypto: function () {
131+
return {
132+
sign: function () { },
133+
exportKey: function () { return true; }
134+
};
135+
}
136+
}
137+
};
138+
// ok
139+
if (chrome.platformKeys.subtleCrypto().exportKey) {
140+
chrome.platformKeys.subtleCrypto().exportKey;
141+
}
113142
}
114143
var Foo = /** @class */ (function () {
115144
function Foo() {

‎tests/baselines/reference/truthinessCallExpressionCoercion1.symbols

Lines changed: 55 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -134,53 +134,92 @@ function checksPropertyAccess() {
134134
>foo : Symbol(foo, Decl(truthinessCallExpressionCoercion1.ts, 38, 15))
135135
>bar : Symbol(bar, Decl(truthinessCallExpressionCoercion1.ts, 39, 14))
136136
>undefined : Symbol(undefined)
137+
138+
var chrome = {
139+
>chrome : Symbol(chrome, Decl(truthinessCallExpressionCoercion1.ts, 50, 7))
140+
141+
platformKeys: {
142+
>platformKeys : Symbol(platformKeys, Decl(truthinessCallExpressionCoercion1.ts, 50, 18))
143+
144+
subtleCrypto() {
145+
>subtleCrypto : Symbol(subtleCrypto, Decl(truthinessCallExpressionCoercion1.ts, 51, 23))
146+
147+
return {
148+
sign() {},
149+
>sign : Symbol(sign, Decl(truthinessCallExpressionCoercion1.ts, 53, 24))
150+
151+
exportKey() { return true }
152+
>exportKey : Symbol(exportKey, Decl(truthinessCallExpressionCoercion1.ts, 54, 30))
153+
}
154+
}
155+
}
156+
}
157+
// ok
158+
if (chrome.platformKeys.subtleCrypto().exportKey) {
159+
>chrome.platformKeys.subtleCrypto().exportKey : Symbol(exportKey, Decl(truthinessCallExpressionCoercion1.ts, 54, 30))
160+
>chrome.platformKeys.subtleCrypto : Symbol(subtleCrypto, Decl(truthinessCallExpressionCoercion1.ts, 51, 23))
161+
>chrome.platformKeys : Symbol(platformKeys, Decl(truthinessCallExpressionCoercion1.ts, 50, 18))
162+
>chrome : Symbol(chrome, Decl(truthinessCallExpressionCoercion1.ts, 50, 7))
163+
>platformKeys : Symbol(platformKeys, Decl(truthinessCallExpressionCoercion1.ts, 50, 18))
164+
>subtleCrypto : Symbol(subtleCrypto, Decl(truthinessCallExpressionCoercion1.ts, 51, 23))
165+
>exportKey : Symbol(exportKey, Decl(truthinessCallExpressionCoercion1.ts, 54, 30))
166+
167+
chrome.platformKeys.subtleCrypto().exportKey
168+
>chrome.platformKeys.subtleCrypto().exportKey : Symbol(exportKey, Decl(truthinessCallExpressionCoercion1.ts, 54, 30))
169+
>chrome.platformKeys.subtleCrypto : Symbol(subtleCrypto, Decl(truthinessCallExpressionCoercion1.ts, 51, 23))
170+
>chrome.platformKeys : Symbol(platformKeys, Decl(truthinessCallExpressionCoercion1.ts, 50, 18))
171+
>chrome : Symbol(chrome, Decl(truthinessCallExpressionCoercion1.ts, 50, 7))
172+
>platformKeys : Symbol(platformKeys, Decl(truthinessCallExpressionCoercion1.ts, 50, 18))
173+
>subtleCrypto : Symbol(subtleCrypto, Decl(truthinessCallExpressionCoercion1.ts, 51, 23))
174+
>exportKey : Symbol(exportKey, Decl(truthinessCallExpressionCoercion1.ts, 54, 30))
175+
}
137176
}
138177

139178
class Foo {
140-
>Foo : Symbol(Foo, Decl(truthinessCallExpressionCoercion1.ts, 49, 1))
179+
>Foo : Symbol(Foo, Decl(truthinessCallExpressionCoercion1.ts, 64, 1))
141180

142181
maybeIsUser?: () => boolean;
143-
>maybeIsUser : Symbol(Foo.maybeIsUser, Decl(truthinessCallExpressionCoercion1.ts, 51, 11))
182+
>maybeIsUser : Symbol(Foo.maybeIsUser, Decl(truthinessCallExpressionCoercion1.ts, 66, 11))
144183

145184
isUser() {
146-
>isUser : Symbol(Foo.isUser, Decl(truthinessCallExpressionCoercion1.ts, 52, 32))
185+
>isUser : Symbol(Foo.isUser, Decl(truthinessCallExpressionCoercion1.ts, 67, 32))
147186

148187
return true;
149188
}
150189

151190
test() {
152-
>test : Symbol(Foo.test, Decl(truthinessCallExpressionCoercion1.ts, 56, 5))
191+
>test : Symbol(Foo.test, Decl(truthinessCallExpressionCoercion1.ts, 71, 5))
153192

154193
// error
155194
this.isUser ? console.log('this.isUser') : undefined;
156-
>this.isUser : Symbol(Foo.isUser, Decl(truthinessCallExpressionCoercion1.ts, 52, 32))
157-
>this : Symbol(Foo, Decl(truthinessCallExpressionCoercion1.ts, 49, 1))
158-
>isUser : Symbol(Foo.isUser, Decl(truthinessCallExpressionCoercion1.ts, 52, 32))
195+
>this.isUser : Symbol(Foo.isUser, Decl(truthinessCallExpressionCoercion1.ts, 67, 32))
196+
>this : Symbol(Foo, Decl(truthinessCallExpressionCoercion1.ts, 64, 1))
197+
>isUser : Symbol(Foo.isUser, Decl(truthinessCallExpressionCoercion1.ts, 67, 32))
159198
>console.log : Symbol(Console.log, Decl(lib.dom.d.ts, --, --))
160199
>console : Symbol(console, Decl(lib.dom.d.ts, --, --))
161200
>log : Symbol(Console.log, Decl(lib.dom.d.ts, --, --))
162201
>undefined : Symbol(undefined)
163202

164203
// ok
165204
this.maybeIsUser ? console.log('this.maybeIsUser') : undefined;
166-
>this.maybeIsUser : Symbol(Foo.maybeIsUser, Decl(truthinessCallExpressionCoercion1.ts, 51, 11))
167-
>this : Symbol(Foo, Decl(truthinessCallExpressionCoercion1.ts, 49, 1))
168-
>maybeIsUser : Symbol(Foo.maybeIsUser, Decl(truthinessCallExpressionCoercion1.ts, 51, 11))
205+
>this.maybeIsUser : Symbol(Foo.maybeIsUser, Decl(truthinessCallExpressionCoercion1.ts, 66, 11))
206+
>this : Symbol(Foo, Decl(truthinessCallExpressionCoercion1.ts, 64, 1))
207+
>maybeIsUser : Symbol(Foo.maybeIsUser, Decl(truthinessCallExpressionCoercion1.ts, 66, 11))
169208
>console.log : Symbol(Console.log, Decl(lib.dom.d.ts, --, --))
170209
>console : Symbol(console, Decl(lib.dom.d.ts, --, --))
171210
>log : Symbol(Console.log, Decl(lib.dom.d.ts, --, --))
172211
>undefined : Symbol(undefined)
173212

174213
// ok
175214
if (this.isUser) {
176-
>this.isUser : Symbol(Foo.isUser, Decl(truthinessCallExpressionCoercion1.ts, 52, 32))
177-
>this : Symbol(Foo, Decl(truthinessCallExpressionCoercion1.ts, 49, 1))
178-
>isUser : Symbol(Foo.isUser, Decl(truthinessCallExpressionCoercion1.ts, 52, 32))
215+
>this.isUser : Symbol(Foo.isUser, Decl(truthinessCallExpressionCoercion1.ts, 67, 32))
216+
>this : Symbol(Foo, Decl(truthinessCallExpressionCoercion1.ts, 64, 1))
217+
>isUser : Symbol(Foo.isUser, Decl(truthinessCallExpressionCoercion1.ts, 67, 32))
179218

180219
this.isUser();
181-
>this.isUser : Symbol(Foo.isUser, Decl(truthinessCallExpressionCoercion1.ts, 52, 32))
182-
>this : Symbol(Foo, Decl(truthinessCallExpressionCoercion1.ts, 49, 1))
183-
>isUser : Symbol(Foo.isUser, Decl(truthinessCallExpressionCoercion1.ts, 52, 32))
220+
>this.isUser : Symbol(Foo.isUser, Decl(truthinessCallExpressionCoercion1.ts, 67, 32))
221+
>this : Symbol(Foo, Decl(truthinessCallExpressionCoercion1.ts, 64, 1))
222+
>isUser : Symbol(Foo.isUser, Decl(truthinessCallExpressionCoercion1.ts, 67, 32))
184223
}
185224
}
186225
}

‎tests/baselines/reference/truthinessCallExpressionCoercion1.types

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -180,6 +180,52 @@ function checksPropertyAccess() {
180180
>foo : { bar(): boolean; }
181181
>bar : () => boolean
182182
>undefined : undefined
183+
184+
var chrome = {
185+
>chrome : { platformKeys: { subtleCrypto(): { sign(): void; exportKey(): boolean; }; }; }
186+
>{ platformKeys: { subtleCrypto() { return { sign() {}, exportKey() { return true } } } } } : { platformKeys: { subtleCrypto(): { sign(): void; exportKey(): boolean; }; }; }
187+
188+
platformKeys: {
189+
>platformKeys : { subtleCrypto(): { sign(): void; exportKey(): boolean; }; }
190+
>{ subtleCrypto() { return { sign() {}, exportKey() { return true } } } } : { subtleCrypto(): { sign(): void; exportKey(): boolean; }; }
191+
192+
subtleCrypto() {
193+
>subtleCrypto : () => { sign(): void; exportKey(): boolean; }
194+
195+
return {
196+
>{ sign() {}, exportKey() { return true } } : { sign(): void; exportKey(): boolean; }
197+
198+
sign() {},
199+
>sign : () => void
200+
201+
exportKey() { return true }
202+
>exportKey : () => boolean
203+
>true : true
204+
}
205+
}
206+
}
207+
}
208+
// ok
209+
if (chrome.platformKeys.subtleCrypto().exportKey) {
210+
>chrome.platformKeys.subtleCrypto().exportKey : () => boolean
211+
>chrome.platformKeys.subtleCrypto() : { sign(): void; exportKey(): boolean; }
212+
>chrome.platformKeys.subtleCrypto : () => { sign(): void; exportKey(): boolean; }
213+
>chrome.platformKeys : { subtleCrypto(): { sign(): void; exportKey(): boolean; }; }
214+
>chrome : { platformKeys: { subtleCrypto(): { sign(): void; exportKey(): boolean; }; }; }
215+
>platformKeys : { subtleCrypto(): { sign(): void; exportKey(): boolean; }; }
216+
>subtleCrypto : () => { sign(): void; exportKey(): boolean; }
217+
>exportKey : () => boolean
218+
219+
chrome.platformKeys.subtleCrypto().exportKey
220+
>chrome.platformKeys.subtleCrypto().exportKey : () => boolean
221+
>chrome.platformKeys.subtleCrypto() : { sign(): void; exportKey(): boolean; }
222+
>chrome.platformKeys.subtleCrypto : () => { sign(): void; exportKey(): boolean; }
223+
>chrome.platformKeys : { subtleCrypto(): { sign(): void; exportKey(): boolean; }; }
224+
>chrome : { platformKeys: { subtleCrypto(): { sign(): void; exportKey(): boolean; }; }; }
225+
>platformKeys : { subtleCrypto(): { sign(): void; exportKey(): boolean; }; }
226+
>subtleCrypto : () => { sign(): void; exportKey(): boolean; }
227+
>exportKey : () => boolean
228+
}
183229
}
184230

185231
class Foo {

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

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,16 @@
11
tests/cases/compiler/truthinessCallExpressionCoercion2.ts(11,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/truthinessCallExpressionCoercion2.ts(14,10): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
33
tests/cases/compiler/truthinessCallExpressionCoercion2.ts(41,18): 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/truthinessCallExpressionCoercion2.ts(47,46): 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/truthinessCallExpressionCoercion2.ts(58,5): 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/truthinessCallExpressionCoercion2.ts(61,10): 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/truthinessCallExpressionCoercion2.ts(81,5): 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/truthinessCallExpressionCoercion2.ts(91,9): 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/truthinessCallExpressionCoercion2.ts(94,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/truthinessCallExpressionCoercion2.ts(44,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/truthinessCallExpressionCoercion2.ts(55,46): 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/truthinessCallExpressionCoercion2.ts(66,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/truthinessCallExpressionCoercion2.ts(69,10): 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/truthinessCallExpressionCoercion2.ts(89,5): 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/truthinessCallExpressionCoercion2.ts(99,9): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
10+
tests/cases/compiler/truthinessCallExpressionCoercion2.ts(102,14): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
1011

1112

12-
==== tests/cases/compiler/truthinessCallExpressionCoercion2.ts (9 errors) ====
13+
==== tests/cases/compiler/truthinessCallExpressionCoercion2.ts (10 errors) ====
1314
declare class A {
1415
static from(): string;
1516
}
@@ -18,7 +19,7 @@ tests/cases/compiler/truthinessCallExpressionCoercion2.ts(94,14): error TS2774:
1819
static from(): string;
1920
}
2021

21-
function test(required1: () => boolean, required2: () => boolean, optional?: () => boolean) {
22+
function test(required1: () => boolean, required2: () => boolean, b: boolean, optional?: () => boolean) {
2223
// error
2324
required1 && console.log('required');
2425
~~~~~~~~~
@@ -57,6 +58,16 @@ tests/cases/compiler/truthinessCallExpressionCoercion2.ts(94,14): error TS2774:
5758
required1 && required2 && required1() && console.log('foo');
5859
~~~~~~~~~
5960
!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
61+
62+
// error
63+
if (required1 && b) {
64+
~~~~~~~~~
65+
!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
66+
}
67+
// ok
68+
if (required1 && b) {
69+
required1()
70+
}
6071
}
6172

6273
function checksConsole() {

0 commit comments

Comments
 (0)