Skip to content

Commit 523e84c

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

15 files changed

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

‎tests/baselines/reference/truthinessCallExpressionCoercion2.js

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

0 commit comments

Comments
 (0)