Skip to content

Commit b08d886

Browse files
a-tarasyuka-tarasyuk
committed
feat(40197): handle uncalled function checks in binary expressions
1 parent bfa69b7 commit b08d886

15 files changed

+975
-24
lines changed

‎src/compiler/binder.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -660,7 +660,7 @@ namespace ts {
660660
}
661661
// We create a return control flow graph for IIFEs and constructors. For constructors
662662
// we use the return control flow graph in strict property initialization checks.
663-
currentReturnTarget = isIIFE || node.kind === SyntaxKind.Constructor || (isInJSFile && (node.kind === SyntaxKind.FunctionDeclaration || node.kind === SyntaxKind.FunctionExpression)) ? createBranchLabel() : undefined;
663+
currentReturnTarget = isIIFE || node.kind === SyntaxKind.Constructor || (isInJSFile(node) && (node.kind === SyntaxKind.FunctionDeclaration || node.kind === SyntaxKind.FunctionExpression)) ? createBranchLabel() : undefined;
664664
currentExceptionTarget = undefined;
665665
currentBreakTarget = undefined;
666666
currentContinueTarget = undefined;
@@ -681,7 +681,7 @@ namespace ts {
681681
if (currentReturnTarget) {
682682
addAntecedent(currentReturnTarget, currentFlow);
683683
currentFlow = finishFlowLabel(currentReturnTarget);
684-
if (node.kind === SyntaxKind.Constructor || (isInJSFile && (node.kind === SyntaxKind.FunctionDeclaration || node.kind === SyntaxKind.FunctionExpression))) {
684+
if (node.kind === SyntaxKind.Constructor || (isInJSFile(node) && (node.kind === SyntaxKind.FunctionDeclaration || node.kind === SyntaxKind.FunctionExpression))) {
685685
(<FunctionLikeDeclaration>node).returnFlowNode = currentFlow;
686686
}
687687
}

‎src/compiler/checker.ts

Lines changed: 32 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -29327,6 +29327,9 @@ namespace ts {
2932729327
workStacks.leftType[stackIndex] = leftType;
2932829328
const operator = node.operatorToken.kind;
2932929329
if (operator === SyntaxKind.AmpersandAmpersandToken || operator === SyntaxKind.BarBarToken || operator === SyntaxKind.QuestionQuestionToken) {
29330+
if (operator === SyntaxKind.AmpersandAmpersandToken) {
29331+
checkTestingKnownTruthyCallableType(node.left, leftType);
29332+
}
2933029333
checkTruthinessOfType(leftType, node.left);
2933129334
}
2933229335
advanceState(CheckBinaryExpressionState.FinishCheck);
@@ -29860,7 +29863,7 @@ namespace ts {
2986029863

2986129864
function checkConditionalExpression(node: ConditionalExpression, checkMode?: CheckMode): Type {
2986229865
const type = checkTruthinessExpression(node.condition);
29863-
checkTestingKnownTruthyCallableType(node.condition, node.whenTrue, type);
29866+
checkTestingKnownTruthyCallableType(node.condition, type, node.whenTrue);
2986429867
const type1 = checkExpression(node.whenTrue, checkMode);
2986529868
const type2 = checkExpression(node.whenFalse, checkMode);
2986629869
return getUnionType([type1, type2], UnionReduction.Subtype);
@@ -33069,7 +33072,7 @@ namespace ts {
3306933072
// Grammar checking
3307033073
checkGrammarStatementInAmbientContext(node);
3307133074
const type = checkTruthinessExpression(node.expression);
33072-
checkTestingKnownTruthyCallableType(node.expression, node.thenStatement, type);
33075+
checkTestingKnownTruthyCallableType(node.expression, type, node.thenStatement);
3307333076
checkSourceElement(node.thenStatement);
3307433077

3307533078
if (node.thenStatement.kind === SyntaxKind.EmptyStatement) {
@@ -33079,16 +33082,15 @@ namespace ts {
3307933082
checkSourceElement(node.elseStatement);
3308033083
}
3308133084

33082-
function checkTestingKnownTruthyCallableType(condExpr: Expression, body: Statement | Expression, type: Type) {
33085+
function checkTestingKnownTruthyCallableType(condExpr: Expression, type: Type, body?: Statement | Expression) {
3308333086
if (!strictNullChecks) {
3308433087
return;
3308533088
}
3308633089

33087-
const testedNode = isIdentifier(condExpr)
33088-
? condExpr
33089-
: isPropertyAccessExpression(condExpr)
33090-
? condExpr.name
33091-
: undefined;
33090+
const location = isBinaryExpression(condExpr) ? condExpr.right : condExpr;
33091+
const testedNode = isIdentifier(location) ? location :
33092+
isPropertyAccessExpression(location) ? location.name :
33093+
isBinaryExpression(location) && isIdentifier(location.right) ? location.right : undefined;
3309233094

3309333095
if (!testedNode) {
3309433096
return;
@@ -33109,27 +33111,33 @@ namespace ts {
3310933111
return;
3311033112
}
3311133113

33112-
const testedFunctionSymbol = getSymbolAtLocation(testedNode);
33113-
if (!testedFunctionSymbol) {
33114+
const testedSymbol = getSymbolAtLocation(testedNode);
33115+
if (!testedSymbol) {
3311433116
return;
3311533117
}
3311633118

33117-
const functionIsUsedInBody = forEachChild(body, function check(childNode): boolean | undefined {
33119+
const isUsed = isBinaryExpression(condExpr.parent) ? isFunctionUsedInBinaryExpressionChain(condExpr.parent, testedSymbol) :
33120+
body ? isFunctionUsedInConditionBody(condExpr, body, testedNode, testedSymbol) : false;
33121+
if (!isUsed) {
33122+
error(location, Diagnostics.This_condition_will_always_return_true_since_the_function_is_always_defined_Did_you_mean_to_call_it_instead);
33123+
}
33124+
}
33125+
33126+
function isFunctionUsedInConditionBody(expr: Expression, body: Statement | Expression, testedNode: Node, testedSymbol: Symbol): boolean {
33127+
return !!forEachChild(body, function check(childNode): boolean | undefined {
3311833128
if (isIdentifier(childNode)) {
3311933129
const childSymbol = getSymbolAtLocation(childNode);
33120-
if (childSymbol && childSymbol === testedFunctionSymbol) {
33130+
if (childSymbol && childSymbol === testedSymbol) {
3312133131
// If the test was a simple identifier, the above check is sufficient
33122-
if (isIdentifier(condExpr)) {
33132+
if (isIdentifier(expr)) {
3312333133
return true;
3312433134
}
3312533135
// Otherwise we need to ensure the symbol is called on the same target
3312633136
let testedExpression = testedNode.parent;
3312733137
let childExpression = childNode.parent;
3312833138
while (testedExpression && childExpression) {
33129-
3313033139
if (isIdentifier(testedExpression) && isIdentifier(childExpression) ||
33131-
testedExpression.kind === SyntaxKind.ThisKeyword && childExpression.kind === SyntaxKind.ThisKeyword
33132-
) {
33140+
testedExpression.kind === SyntaxKind.ThisKeyword && childExpression.kind === SyntaxKind.ThisKeyword) {
3313333141
return getSymbolAtLocation(testedExpression) === getSymbolAtLocation(childExpression);
3313433142
}
3313533143

@@ -33146,13 +33154,18 @@ namespace ts {
3314633154
}
3314733155
}
3314833156
}
33149-
3315033157
return forEachChild(childNode, check);
3315133158
});
33159+
}
3315233160

33153-
if (!functionIsUsedInBody) {
33154-
error(condExpr, Diagnostics.This_condition_will_always_return_true_since_the_function_is_always_defined_Did_you_mean_to_call_it_instead);
33161+
function isFunctionUsedInBinaryExpressionChain(node: Node, testedSymbol: Symbol): boolean {
33162+
while (isBinaryExpression(node) && node.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken) {
33163+
if (isCallExpression(node.right) && testedSymbol === getSymbolAtLocation(node.right.expression)) {
33164+
return true;
33165+
}
33166+
node = node.parent;
3315533167
}
33168+
return false;
3315633169
}
3315733170

3315833171
function checkDoStatement(node: DoStatement) {

‎src/compiler/emitter.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2779,7 +2779,7 @@ namespace ts {
27792779
if (isSimilarNode && currentSourceFile) {
27802780
pos = skipTrivia(currentSourceFile.text, pos);
27812781
}
2782-
if (emitLeadingCommentsOfPosition && isSimilarNode && contextNode.pos !== startPos) {
2782+
if (isSimilarNode && contextNode.pos !== startPos) {
27832783
const needsIndent = indentLeading && currentSourceFile && !positionsAreOnSameLine(startPos, pos, currentSourceFile);
27842784
if (needsIndent) {
27852785
increaseIndent();
@@ -2790,7 +2790,7 @@ namespace ts {
27902790
}
27912791
}
27922792
pos = writeTokenText(token, writer, pos);
2793-
if (emitTrailingCommentsOfPosition && isSimilarNode && contextNode.end !== pos) {
2793+
if (isSimilarNode && contextNode.end !== pos) {
27942794
emitTrailingCommentsOfPosition(pos, /*prefixSpace*/ true);
27952795
}
27962796
return pos;
@@ -3436,7 +3436,7 @@ namespace ts {
34363436
// "comment1" is not considered to be leading comment for node.initializer
34373437
// but rather a trailing comment on the previous node.
34383438
const initializer = node.initializer;
3439-
if (emitTrailingCommentsOfPosition && (getEmitFlags(initializer) & EmitFlags.NoLeadingComments) === 0) {
3439+
if ((getEmitFlags(initializer) & EmitFlags.NoLeadingComments) === 0) {
34403440
const commentRange = getCommentRange(initializer);
34413441
emitTrailingCommentsOfPosition(commentRange.pos);
34423442
}

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

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

‎tests/baselines/reference/truthinessCallExpressionCoercion2.js

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

0 commit comments

Comments
 (0)