Skip to content

Commit 64be2a8

Browse files
sandersnsandersn
authored
Revert "Revert "feat(40197): handle uncalled function checks in binary expressions (#40260)"" (#41462)
This reverts commit cf3e28e.
1 parent 06a2210 commit 64be2a8

15 files changed

+1069
-24
lines changed

‎src/compiler/binder.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -670,7 +670,7 @@ namespace ts {
670670
}
671671
// We create a return control flow graph for IIFEs and constructors. For constructors
672672
// we use the return control flow graph in strict property initialization checks.
673-
currentReturnTarget = isIIFE || node.kind === SyntaxKind.Constructor || (isInJSFile && (node.kind === SyntaxKind.FunctionDeclaration || node.kind === SyntaxKind.FunctionExpression)) ? createBranchLabel() : undefined;
673+
currentReturnTarget = isIIFE || node.kind === SyntaxKind.Constructor || (isInJSFile(node) && (node.kind === SyntaxKind.FunctionDeclaration || node.kind === SyntaxKind.FunctionExpression)) ? createBranchLabel() : undefined;
674674
currentExceptionTarget = undefined;
675675
currentBreakTarget = undefined;
676676
currentContinueTarget = undefined;
@@ -691,7 +691,7 @@ namespace ts {
691691
if (currentReturnTarget) {
692692
addAntecedent(currentReturnTarget, currentFlow);
693693
currentFlow = finishFlowLabel(currentReturnTarget);
694-
if (node.kind === SyntaxKind.Constructor || (isInJSFile && (node.kind === SyntaxKind.FunctionDeclaration || node.kind === SyntaxKind.FunctionExpression))) {
694+
if (node.kind === SyntaxKind.Constructor || (isInJSFile(node) && (node.kind === SyntaxKind.FunctionDeclaration || node.kind === SyntaxKind.FunctionExpression))) {
695695
(<FunctionLikeDeclaration>node).returnFlowNode = currentFlow;
696696
}
697697
}

‎src/compiler/checker.ts

Lines changed: 34 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -30156,6 +30156,9 @@ namespace ts {
3015630156
workStacks.leftType[stackIndex] = leftType;
3015730157
const operator = node.operatorToken.kind;
3015830158
if (operator === SyntaxKind.AmpersandAmpersandToken || operator === SyntaxKind.BarBarToken || operator === SyntaxKind.QuestionQuestionToken) {
30159+
if (operator === SyntaxKind.AmpersandAmpersandToken) {
30160+
checkTestingKnownTruthyCallableType(node.left, leftType);
30161+
}
3015930162
checkTruthinessOfType(leftType, node.left);
3016030163
}
3016130164
advanceState(CheckBinaryExpressionState.FinishCheck);
@@ -30689,7 +30692,7 @@ namespace ts {
3068930692

3069030693
function checkConditionalExpression(node: ConditionalExpression, checkMode?: CheckMode): Type {
3069130694
const type = checkTruthinessExpression(node.condition);
30692-
checkTestingKnownTruthyCallableType(node.condition, node.whenTrue, type);
30695+
checkTestingKnownTruthyCallableType(node.condition, type, node.whenTrue);
3069330696
const type1 = checkExpression(node.whenTrue, checkMode);
3069430697
const type2 = checkExpression(node.whenFalse, checkMode);
3069530698
return getUnionType([type1, type2], UnionReduction.Subtype);
@@ -33926,7 +33929,7 @@ namespace ts {
3392633929
// Grammar checking
3392733930
checkGrammarStatementInAmbientContext(node);
3392833931
const type = checkTruthinessExpression(node.expression);
33929-
checkTestingKnownTruthyCallableType(node.expression, node.thenStatement, type);
33932+
checkTestingKnownTruthyCallableType(node.expression, type, node.thenStatement);
3393033933
checkSourceElement(node.thenStatement);
3393133934

3393233935
if (node.thenStatement.kind === SyntaxKind.EmptyStatement) {
@@ -33936,16 +33939,16 @@ namespace ts {
3393633939
checkSourceElement(node.elseStatement);
3393733940
}
3393833941

33939-
function checkTestingKnownTruthyCallableType(condExpr: Expression, body: Statement | Expression, type: Type) {
33942+
function checkTestingKnownTruthyCallableType(condExpr: Expression, type: Type, body?: Statement | Expression) {
3394033943
if (!strictNullChecks) {
3394133944
return;
3394233945
}
3394333946

33944-
const testedNode = isIdentifier(condExpr)
33945-
? condExpr
33946-
: isPropertyAccessExpression(condExpr)
33947-
? condExpr.name
33948-
: undefined;
33947+
const location = isBinaryExpression(condExpr) ? condExpr.right : condExpr;
33948+
const testedNode = isIdentifier(location) ? location
33949+
: isPropertyAccessExpression(location) ? location.name
33950+
: isBinaryExpression(location) && isIdentifier(location.right) ? location.right
33951+
: undefined;
3394933952

3395033953
if (!testedNode) {
3395133954
return;
@@ -33966,27 +33969,34 @@ namespace ts {
3396633969
return;
3396733970
}
3396833971

33969-
const testedFunctionSymbol = getSymbolAtLocation(testedNode);
33970-
if (!testedFunctionSymbol) {
33972+
const testedSymbol = getSymbolAtLocation(testedNode);
33973+
if (!testedSymbol) {
3397133974
return;
3397233975
}
3397333976

33974-
const functionIsUsedInBody = forEachChild(body, function check(childNode): boolean | undefined {
33977+
const isUsed = isBinaryExpression(condExpr.parent) ? isFunctionUsedInBinaryExpressionChain(condExpr.parent, testedSymbol)
33978+
: body ? isFunctionUsedInConditionBody(condExpr, body, testedNode, testedSymbol)
33979+
: false;
33980+
if (!isUsed) {
33981+
error(location, Diagnostics.This_condition_will_always_return_true_since_the_function_is_always_defined_Did_you_mean_to_call_it_instead);
33982+
}
33983+
}
33984+
33985+
function isFunctionUsedInConditionBody(expr: Expression, body: Statement | Expression, testedNode: Node, testedSymbol: Symbol): boolean {
33986+
return !!forEachChild(body, function check(childNode): boolean | undefined {
3397533987
if (isIdentifier(childNode)) {
3397633988
const childSymbol = getSymbolAtLocation(childNode);
33977-
if (childSymbol && childSymbol === testedFunctionSymbol) {
33989+
if (childSymbol && childSymbol === testedSymbol) {
3397833990
// If the test was a simple identifier, the above check is sufficient
33979-
if (isIdentifier(condExpr)) {
33991+
if (isIdentifier(expr)) {
3398033992
return true;
3398133993
}
3398233994
// Otherwise we need to ensure the symbol is called on the same target
3398333995
let testedExpression = testedNode.parent;
3398433996
let childExpression = childNode.parent;
3398533997
while (testedExpression && childExpression) {
33986-
3398733998
if (isIdentifier(testedExpression) && isIdentifier(childExpression) ||
33988-
testedExpression.kind === SyntaxKind.ThisKeyword && childExpression.kind === SyntaxKind.ThisKeyword
33989-
) {
33999+
testedExpression.kind === SyntaxKind.ThisKeyword && childExpression.kind === SyntaxKind.ThisKeyword) {
3399034000
return getSymbolAtLocation(testedExpression) === getSymbolAtLocation(childExpression);
3399134001
}
3399234002

@@ -34003,13 +34013,18 @@ namespace ts {
3400334013
}
3400434014
}
3400534015
}
34006-
3400734016
return forEachChild(childNode, check);
3400834017
});
34018+
}
3400934019

34010-
if (!functionIsUsedInBody) {
34011-
error(condExpr, Diagnostics.This_condition_will_always_return_true_since_the_function_is_always_defined_Did_you_mean_to_call_it_instead);
34020+
function isFunctionUsedInBinaryExpressionChain(node: Node, testedSymbol: Symbol): boolean {
34021+
while (isBinaryExpression(node) && node.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken) {
34022+
if (isCallExpression(node.right) && testedSymbol === getSymbolAtLocation(node.right.expression)) {
34023+
return true;
34024+
}
34025+
node = node.parent;
3401234026
}
34027+
return false;
3401334028
}
3401434029

3401534030
function checkDoStatement(node: DoStatement) {

‎src/compiler/emitter.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2812,7 +2812,7 @@ namespace ts {
28122812
if (isSimilarNode && currentSourceFile) {
28132813
pos = skipTrivia(currentSourceFile.text, pos);
28142814
}
2815-
if (emitLeadingCommentsOfPosition && isSimilarNode && contextNode.pos !== startPos) {
2815+
if (isSimilarNode && contextNode.pos !== startPos) {
28162816
const needsIndent = indentLeading && currentSourceFile && !positionsAreOnSameLine(startPos, pos, currentSourceFile);
28172817
if (needsIndent) {
28182818
increaseIndent();
@@ -2823,7 +2823,7 @@ namespace ts {
28232823
}
28242824
}
28252825
pos = writeTokenText(token, writer, pos);
2826-
if (emitTrailingCommentsOfPosition && isSimilarNode && contextNode.end !== pos) {
2826+
if (isSimilarNode && contextNode.end !== pos) {
28272827
emitTrailingCommentsOfPosition(pos, /*prefixSpace*/ true);
28282828
}
28292829
return pos;
@@ -3469,7 +3469,7 @@ namespace ts {
34693469
// "comment1" is not considered to be leading comment for node.initializer
34703470
// but rather a trailing comment on the previous node.
34713471
const initializer = node.initializer;
3472-
if (emitTrailingCommentsOfPosition && (getEmitFlags(initializer) & EmitFlags.NoLeadingComments) === 0) {
3472+
if ((getEmitFlags(initializer) & EmitFlags.NoLeadingComments) === 0) {
34733473
const commentRange = getCommentRange(initializer);
34743474
emitTrailingCommentsOfPosition(commentRange.pos);
34753475
}

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

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

0 commit comments

Comments
 (0)