Skip to content

Commit 91be368

Browse files
author
Orta
authored
Merge pull request #33178 from jwbay/nonNullableCallSignaturesTreeWalk
Flag non-nullable functions in `if` statements as errors (tree walk version)
2 parents 75117ac + 4c9986f commit 91be368

10 files changed

+701
-15
lines changed

‎src/compiler/checker.ts

+53-1
Original file line numberDiff line numberDiff line change
@@ -28306,7 +28306,8 @@ namespace ts {
2830628306
// Grammar checking
2830728307
checkGrammarStatementInAmbientContext(node);
2830828308

28309-
checkTruthinessExpression(node.expression);
28309+
const type = checkTruthinessExpression(node.expression);
28310+
checkTestingKnownTruthyCallableType(node, type);
2831028311
checkSourceElement(node.thenStatement);
2831128312

2831228313
if (node.thenStatement.kind === SyntaxKind.EmptyStatement) {
@@ -28316,6 +28317,57 @@ namespace ts {
2831628317
checkSourceElement(node.elseStatement);
2831728318
}
2831828319

28320+
function checkTestingKnownTruthyCallableType(ifStatement: IfStatement, type: Type) {
28321+
if (!strictNullChecks) {
28322+
return;
28323+
}
28324+
28325+
const testedNode = isIdentifier(ifStatement.expression)
28326+
? ifStatement.expression
28327+
: isPropertyAccessExpression(ifStatement.expression)
28328+
? ifStatement.expression.name
28329+
: undefined;
28330+
28331+
if (!testedNode) {
28332+
return;
28333+
}
28334+
28335+
const possiblyFalsy = getFalsyFlags(type);
28336+
if (possiblyFalsy) {
28337+
return;
28338+
}
28339+
28340+
// While it technically should be invalid for any known-truthy value
28341+
// to be tested, we de-scope to functions unrefenced in the block as a
28342+
// heuristic to identify the most common bugs. There are too many
28343+
// false positives for values sourced from type definitions without
28344+
// strictNullChecks otherwise.
28345+
const callSignatures = getSignaturesOfType(type, SignatureKind.Call);
28346+
if (callSignatures.length === 0) {
28347+
return;
28348+
}
28349+
28350+
const testedFunctionSymbol = getSymbolAtLocation(testedNode);
28351+
if (!testedFunctionSymbol) {
28352+
return;
28353+
}
28354+
28355+
const functionIsUsedInBody = forEachChild(ifStatement.thenStatement, function check(childNode): boolean | undefined {
28356+
if (isIdentifier(childNode)) {
28357+
const childSymbol = getSymbolAtLocation(childNode);
28358+
if (childSymbol && childSymbol.id === testedFunctionSymbol.id) {
28359+
return true;
28360+
}
28361+
}
28362+
28363+
return forEachChild(childNode, check);
28364+
});
28365+
28366+
if (!functionIsUsedInBody) {
28367+
error(ifStatement.expression, Diagnostics.This_condition_will_always_return_true_since_the_function_is_always_defined_Did_you_mean_to_call_it_instead);
28368+
}
28369+
}
28370+
2831928371
function checkDoStatement(node: DoStatement) {
2832028372
// Grammar checking
2832128373
checkGrammarStatementInAmbientContext(node);

‎src/compiler/core.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -1654,7 +1654,7 @@ namespace ts {
16541654
*/
16551655
export function compose<T>(...args: ((t: T) => T)[]): (t: T) => T;
16561656
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 {
1657-
if (e) {
1657+
if (!!e) {
16581658
const args: ((t: T) => T)[] = [];
16591659
for (let i = 0; i < arguments.length; i++) {
16601660
args[i] = arguments[i];

‎src/compiler/diagnosticMessages.json

+4-1
Original file line numberDiff line numberDiff line change
@@ -2722,7 +2722,10 @@
27222722
"category": "Error",
27232723
"code": 2773
27242724
},
2725-
2725+
"This condition will always return true since the function is always defined. Did you mean to call it instead?": {
2726+
"category": "Error",
2727+
"code": 2774
2728+
},
27262729
"Import declaration '{0}' is using private name '{1}'.": {
27272730
"category": "Error",
27282731
"code": 4000

‎src/compiler/program.ts

+10-12
Original file line numberDiff line numberDiff line change
@@ -1389,18 +1389,16 @@ namespace ts {
13891389
// try to verify results of module resolution
13901390
for (const { oldFile: oldSourceFile, newFile: newSourceFile } of modifiedSourceFiles) {
13911391
const newSourceFilePath = getNormalizedAbsolutePath(newSourceFile.originalFileName, currentDirectory);
1392-
if (resolveModuleNamesWorker) {
1393-
const moduleNames = getModuleNames(newSourceFile);
1394-
const resolutions = resolveModuleNamesReusingOldState(moduleNames, newSourceFilePath, newSourceFile);
1395-
// ensure that module resolution results are still correct
1396-
const resolutionsChanged = hasChangesInResolutions(moduleNames, resolutions, oldSourceFile.resolvedModules, moduleResolutionIsEqualTo);
1397-
if (resolutionsChanged) {
1398-
oldProgram.structureIsReused = StructureIsReused.SafeModules;
1399-
newSourceFile.resolvedModules = zipToMap(moduleNames, resolutions);
1400-
}
1401-
else {
1402-
newSourceFile.resolvedModules = oldSourceFile.resolvedModules;
1403-
}
1392+
const moduleNames = getModuleNames(newSourceFile);
1393+
const resolutions = resolveModuleNamesReusingOldState(moduleNames, newSourceFilePath, newSourceFile);
1394+
// ensure that module resolution results are still correct
1395+
const resolutionsChanged = hasChangesInResolutions(moduleNames, resolutions, oldSourceFile.resolvedModules, moduleResolutionIsEqualTo);
1396+
if (resolutionsChanged) {
1397+
oldProgram.structureIsReused = StructureIsReused.SafeModules;
1398+
newSourceFile.resolvedModules = zipToMap(moduleNames, resolutions);
1399+
}
1400+
else {
1401+
newSourceFile.resolvedModules = oldSourceFile.resolvedModules;
14041402
}
14051403
if (resolveTypeReferenceDirectiveNamesWorker) {
14061404
// We lower-case all type references because npm automatically lowercases all packages. See GH#9824.

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

+91
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
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?
2+
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?
3+
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?
4+
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?
5+
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?
6+
7+
8+
==== tests/cases/compiler/truthinessCallExpressionCoercion.ts (5 errors) ====
9+
function onlyErrorsWhenTestingNonNullableFunctionType(required: () => boolean, optional?: () => boolean) {
10+
if (required) { // error
11+
~~~~~~~~
12+
!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
13+
}
14+
15+
if (optional) { // ok
16+
}
17+
18+
if (!!required) { // ok
19+
}
20+
21+
if (required()) { // ok
22+
}
23+
}
24+
25+
function onlyErrorsWhenUnusedInBody() {
26+
function test() { return Math.random() > 0.5; }
27+
28+
if (test) { // error
29+
~~~~
30+
!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
31+
console.log('test');
32+
}
33+
34+
if (test) { // ok
35+
console.log(test);
36+
}
37+
38+
if (test) { // ok
39+
test();
40+
}
41+
42+
if (test) { // ok
43+
[() => null].forEach(() => {
44+
test();
45+
});
46+
}
47+
48+
if (test) { // error
49+
~~~~
50+
!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
51+
[() => null].forEach(test => {
52+
test();
53+
});
54+
}
55+
}
56+
57+
function checksPropertyAccess() {
58+
const x = {
59+
foo: {
60+
bar() { return true; }
61+
}
62+
}
63+
64+
if (x.foo.bar) { // error
65+
~~~~~~~~~
66+
!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
67+
}
68+
69+
if (x.foo.bar) { // ok
70+
x.foo.bar;
71+
}
72+
}
73+
74+
class Foo {
75+
maybeIsUser?: () => boolean;
76+
77+
isUser() {
78+
return true;
79+
}
80+
81+
test() {
82+
if (this.isUser) { // error
83+
~~~~~~~~~~~
84+
!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
85+
}
86+
87+
if (this.maybeIsUser) { // ok
88+
}
89+
}
90+
}
91+

‎tests/baselines/reference/truthinessCallExpressionCoercion.js

+134
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,134 @@
1+
//// [truthinessCallExpressionCoercion.ts]
2+
function onlyErrorsWhenTestingNonNullableFunctionType(required: () => boolean, optional?: () => boolean) {
3+
if (required) { // error
4+
}
5+
6+
if (optional) { // ok
7+
}
8+
9+
if (!!required) { // ok
10+
}
11+
12+
if (required()) { // ok
13+
}
14+
}
15+
16+
function onlyErrorsWhenUnusedInBody() {
17+
function test() { return Math.random() > 0.5; }
18+
19+
if (test) { // error
20+
console.log('test');
21+
}
22+
23+
if (test) { // ok
24+
console.log(test);
25+
}
26+
27+
if (test) { // ok
28+
test();
29+
}
30+
31+
if (test) { // ok
32+
[() => null].forEach(() => {
33+
test();
34+
});
35+
}
36+
37+
if (test) { // error
38+
[() => null].forEach(test => {
39+
test();
40+
});
41+
}
42+
}
43+
44+
function checksPropertyAccess() {
45+
const x = {
46+
foo: {
47+
bar() { return true; }
48+
}
49+
}
50+
51+
if (x.foo.bar) { // error
52+
}
53+
54+
if (x.foo.bar) { // ok
55+
x.foo.bar;
56+
}
57+
}
58+
59+
class Foo {
60+
maybeIsUser?: () => boolean;
61+
62+
isUser() {
63+
return true;
64+
}
65+
66+
test() {
67+
if (this.isUser) { // error
68+
}
69+
70+
if (this.maybeIsUser) { // ok
71+
}
72+
}
73+
}
74+
75+
76+
//// [truthinessCallExpressionCoercion.js]
77+
function onlyErrorsWhenTestingNonNullableFunctionType(required, optional) {
78+
if (required) { // error
79+
}
80+
if (optional) { // ok
81+
}
82+
if (!!required) { // ok
83+
}
84+
if (required()) { // ok
85+
}
86+
}
87+
function onlyErrorsWhenUnusedInBody() {
88+
function test() { return Math.random() > 0.5; }
89+
if (test) { // error
90+
console.log('test');
91+
}
92+
if (test) { // ok
93+
console.log(test);
94+
}
95+
if (test) { // ok
96+
test();
97+
}
98+
if (test) { // ok
99+
[function () { return null; }].forEach(function () {
100+
test();
101+
});
102+
}
103+
if (test) { // error
104+
[function () { return null; }].forEach(function (test) {
105+
test();
106+
});
107+
}
108+
}
109+
function checksPropertyAccess() {
110+
var x = {
111+
foo: {
112+
bar: function () { return true; }
113+
}
114+
};
115+
if (x.foo.bar) { // error
116+
}
117+
if (x.foo.bar) { // ok
118+
x.foo.bar;
119+
}
120+
}
121+
var Foo = /** @class */ (function () {
122+
function Foo() {
123+
}
124+
Foo.prototype.isUser = function () {
125+
return true;
126+
};
127+
Foo.prototype.test = function () {
128+
if (this.isUser) { // error
129+
}
130+
if (this.maybeIsUser) { // ok
131+
}
132+
};
133+
return Foo;
134+
}());

0 commit comments

Comments
 (0)