Improve uncalled function check related to parenthesized and binary expressions #50756

Andarist · 2022-09-13T17:37:05Z

…xpressions

tests/baselines/reference/truthinessCallExpressionCoercion2.errors.txt

Andarist · 2022-09-14T08:18:26Z

src/compiler/checker.ts

+                        let parent = node.parent;
+                        while (parent.kind === SyntaxKind.ParenthesizedExpression
+                            || isBinaryExpression(parent) && (parent.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken || parent.operatorToken.kind === SyntaxKind.BarBarToken)) {
+                            parent = parent.parent;
+                        }
+                        if (operator === SyntaxKind.AmpersandAmpersandToken || isIfStatement(parent)) {


I've thought that perhaps with some of my recent changes this could be reverted but it seems that it can't. Without this change the error here goes away:

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

Lines 100 to 101 in 8f0a392

~~~~~~~~~

!!! error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?

It's quite interesting because a similar error here doesn't go away:

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

Lines 108 to 109 in 8f0a392

~~~~~~~~~

!!! error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?

So it seems that the logic is very sensitive when it comes to handling left/right side of a binary expression. Should I strive to revert this change here and find another solution? Or is this acceptable?

…g operator

Andarist · 2022-09-14T09:07:20Z

src/compiler/checker.ts

-            helper(condExpr, body);
-            while (isBinaryExpression(condExpr) && condExpr.operatorToken.kind === SyntaxKind.BarBarToken) {
-                condExpr = condExpr.left;
+            function bothHelper(condExpr: Expression, body: Expression | Statement | undefined) {


todo: this could use a better name, the helper itself probably could use a better name too. I can try to figure out something better later on

jakebailey · 2022-09-26T21:55:12Z

@typescript-bot test this
@typescript-bot user test this inline
@typescript-bot perf test this faster
@typescript-bot test top100

typescript-bot · 2022-09-26T21:55:16Z

Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at bc24f9b. You can monitor the build here.

Update: The results are in!

typescript-bot · 2022-09-26T21:55:16Z

Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at bc24f9b. You can monitor the build here.

Update: The results are in!

typescript-bot · 2022-09-26T21:55:16Z

Heya @jakebailey, I've started to run the extended test suite on this PR at bc24f9b. You can monitor the build here.

typescript-bot · 2022-09-26T21:55:16Z

Heya @jakebailey, I've started to run the abridged perf test suite on this PR at bc24f9b. You can monitor the build here.

Update: The results are in!

typescript-bot · 2022-09-26T22:10:04Z

@jakebailey Here are the results of running the user test suite comparing main and refs/pull/50756/merge:

Everything looks good!

typescript-bot · 2022-09-26T22:13:56Z

@jakebailey
The results of the perf run you requested are in!

Here they are:

Comparison Report - main..50756

_Metric	_main	₅₀₇₅₆	_Delta	_Best	_Worst
_{Angular - node (v14.15.1, x64)}
_{Memory used}	_{337,918k (± 0.08%)}	_{338,065k (± 0.01%)}	_{+146k (+ 0.04%)}	_338,016k	_338,170k
_{Parse Time}	_{2.06s (± 0.48%)}	_{2.06s (± 0.47%)}	_{+0.00s (+ 0.05%)}	_2.04s	_2.08s
_{Bind Time}	_{0.79s (± 0.94%)}	_{0.79s (± 0.75%)}	_{+0.00s (+ 0.13%)}	_0.78s	_0.81s
_{Check Time}	_{5.87s (± 0.89%)}	_{5.88s (± 0.50%)}	_{+0.01s (+ 0.14%)}	_5.83s	_5.97s
_{Emit Time}	_{6.24s (± 0.82%)}	_{6.26s (± 0.38%)}	_{+0.02s (+ 0.29%)}	_6.22s	_6.33s
_{Total Time}	_{14.96s (± 0.65%)}	_{14.99s (± 0.30%)}	_{+0.02s (+ 0.16%)}	_14.91s	_15.11s
_{Compiler-Unions - node (v14.15.1, x64)}
_{Memory used}	_{190,791k (± 0.67%)}	_{190,778k (± 0.68%)}	_{-13k (- 0.01%)}	_190,133k	_196,029k
_{Parse Time}	_{0.86s (± 0.61%)}	_{0.85s (± 0.72%)}	_{-0.01s (- 0.93%)}	_0.84s	_0.87s
_{Bind Time}	_{0.49s (± 1.06%)}	_{0.49s (± 0.75%)}	_{-0.00s (- 0.61%)}	_0.48s	_0.49s
_{Check Time}	_{6.76s (± 0.62%)}	_{6.70s (± 0.52%)}	_{-0.06s (- 0.84%)}	_6.65s	_6.80s
_{Emit Time}	_{2.40s (± 1.18%)}	_{2.41s (± 0.74%)}	_{+0.01s (+ 0.33%)}	_2.38s	_2.44s
_{Total Time}	_{10.50s (± 0.53%)}	_{10.45s (± 0.37%)}	_{-0.05s (- 0.52%)}	_10.39s	_10.58s
_{Monaco - node (v14.15.1, x64)}
_{Memory used}	_{326,572k (± 0.01%)}	_{326,560k (± 0.01%)}	_{-12k (- 0.00%)}	_326,472k	_326,616k
_{Parse Time}	_{1.58s (± 0.67%)}	_{1.59s (± 0.69%)}	_{+0.01s (+ 0.32%)}	_1.57s	_1.61s
_{Bind Time}	_{0.72s (± 0.80%)}	_{0.72s (± 0.83%)}	_{+0.00s (+ 0.28%)}	_0.71s	_0.74s
_{Check Time}	_{5.70s (± 0.49%)}	_{5.70s (± 0.43%)}	_{+0.01s (+ 0.12%)}	_5.62s	_5.75s
_{Emit Time}	_{3.36s (± 0.38%)}	_{3.37s (± 0.62%)}	_{+0.01s (+ 0.27%)}	_3.33s	_3.42s
_{Total Time}	_{11.37s (± 0.27%)}	_{11.39s (± 0.22%)}	_{+0.02s (+ 0.18%)}	_11.34s	_11.46s
_{TFS - node (v14.15.1, x64)}
_{Memory used}	_{289,687k (± 0.01%)}	_{289,678k (± 0.01%)}	_{-10k (- 0.00%)}	_289,619k	_289,709k
_{Parse Time}	_{1.30s (± 0.43%)}	_{1.31s (± 1.10%)}	_{+0.01s (+ 0.77%)}	_1.28s	_1.35s
_{Bind Time}	_{0.80s (± 0.75%)}	_{0.80s (± 0.86%)}	_{-0.00s (- 0.13%)}	_0.79s	_0.82s
_{Check Time}	_{5.37s (± 0.20%)}	_{5.37s (± 0.42%)}	_{+0.00s (+ 0.06%)}	_5.32s	_5.44s
_{Emit Time}	_{3.60s (± 0.53%)}	_{3.59s (± 0.64%)}	_{-0.01s (- 0.39%)}	_3.55s	_3.64s
_{Total Time}	_{11.06s (± 0.24%)}	_{11.05s (± 0.40%)}	_{-0.00s (- 0.05%)}	_10.97s	_11.20s
_{material-ui - node (v14.15.1, x64)}
_{Memory used}	_{435,593k (± 0.00%)}	_{435,488k (± 0.06%)}	_{-105k (- 0.02%)}	_434,440k	_435,664k
_{Parse Time}	_{1.86s (± 0.71%)}	_{1.87s (± 0.62%)}	_{+0.01s (+ 0.54%)}	_1.85s	_1.90s
_{Bind Time}	_{0.58s (± 0.58%)}	_{0.58s (± 0.77%)}	_{-0.00s (- 0.17%)}	_0.57s	_0.59s
_{Check Time}	_{12.89s (± 0.33%)}	_{12.86s (± 0.70%)}	_{-0.03s (- 0.20%)}	_12.71s	_13.09s
_{Emit Time}	_{0.00s (± 0.00%)}	_{0.00s (± 0.00%)}	_{0.00s ( NaN%)}	_0.00s	_0.00s
_{Total Time}	_{15.34s (± 0.35%)}	_{15.32s (± 0.65%)}	_{-0.02s (- 0.12%)}	_15.13s	_15.56s
_{xstate - node (v14.15.1, x64)}
_{Memory used}	_{544,017k (± 0.00%)}	_{544,041k (± 0.00%)}	_{+24k (+ 0.00%)}	_544,007k	_544,087k
_{Parse Time}	_{2.61s (± 0.34%)}	_{2.61s (± 0.60%)}	_{+0.00s (+ 0.04%)}	_2.57s	_2.64s
_{Bind Time}	_{0.99s (± 1.39%)}	_{0.98s (± 0.86%)}	_{-0.01s (- 0.61%)}	_0.97s	_1.00s
_{Check Time}	_{1.52s (± 0.50%)}	_{1.52s (± 0.51%)}	_{+0.00s (+ 0.20%)}	_1.51s	_1.54s
_{Emit Time}	_{0.07s (± 0.00%)}	_{0.07s (± 0.00%)}	_{0.00s ( 0.00%)}	_0.07s	_0.07s
_{Total Time}	_{5.19s (± 0.35%)}	_{5.18s (± 0.28%)}	_{-0.01s (- 0.15%)}	_5.14s	_5.20s

System

_{Machine Name}	_ts-ci-ubuntu
_Platform	_{linux 4.4.0-210-generic}
_Architecture	_x64
_{Available Memory}	_{16 GB}
_{Available Memory}	_{15 GB}
_CPUs	_{4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz}

Hosts

node (v14.15.1, x64)

Scenarios

Angular - node (v14.15.1, x64)
Compiler-Unions - node (v14.15.1, x64)
Monaco - node (v14.15.1, x64)
TFS - node (v14.15.1, x64)
material-ui - node (v14.15.1, x64)
xstate - node (v14.15.1, x64)

_Benchmark	_Name	_Iterations
_Current	₅₀₇₅₆	₁₀
_Baseline	_main	₁₀

Developer Information:

Download Benchmark

typescript-bot · 2022-09-26T23:07:49Z

@jakebailey Here are the results of running the top-repos suite comparing main and refs/pull/50756/merge:

Everything looks good!

…tion-check # Conflicts: # src/compiler/checker.ts # src/compiler/factory/utilities.ts

Andarist · 2022-12-09T23:17:14Z

@jakebailey you requested perf results for this some time ago. Any chance that you'd have some time to review this now when they are already here? :)

jakebailey

The code LGTM and the perf does appear to be fine. I think from the issue that this is a behavior we want, so, SGTM.

Andarist · 2023-01-17T20:09:39Z

@jakebailey I was wondering if there is a chance to land this before entering the 5.0 beta period (3 days left 😅 )

jakebailey · 2023-01-17T20:59:38Z

I was hoping for someone else to weigh in (I don't entirely trust myself to merge large-ish checker PRs), but it still seems fine to me given it's an "experience enhancement" aka "obviously an improvement" and top100 passed (so, probably low-risk).

Improve uncalled function check related to parenthesized and binary e…

43da47b

…xpressions

typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Sep 13, 2022

Andarist commented Sep 13, 2022

View reviewed changes

tests/baselines/reference/truthinessCallExpressionCoercion2.errors.txt Outdated Show resolved Hide resolved

Andarist added 2 commits September 14, 2022 09:35

Improve uncalled function check for both sides of binary expressions

8f0a392

Add tests for property access case and the uncalled function check

081cdf9

Andarist commented Sep 14, 2022

View reviewed changes

Andarist added 4 commits September 14, 2022 10:22

Remove redundant line of code

ba75dd8

Improve uncalled function check for cases involving nullish coalescin…

0d1dad9

…g operator

Improve yet another case related to the nullish coalescing operator

500bbf2

Add isValueCoalescingBinaryExpression utility

bc24f9b

Andarist commented Sep 14, 2022

View reviewed changes

sandersn requested review from jakebailey and navya9singh September 23, 2022 23:35

sandersn assigned navya9singh Sep 23, 2022

Merge remote-tracking branch 'origin/main' into improve-uncalled-func…

17e37ea

…tion-check # Conflicts: # src/compiler/checker.ts # src/compiler/factory/utilities.ts

jakebailey approved these changes Dec 9, 2022

View reviewed changes

jakebailey requested a review from weswigham December 15, 2022 21:06

Refactor the introduced helper and reuse it in more places

34c014a

jakebailey merged commit 16c695c into microsoft:main Jan 17, 2023

Andarist deleted the improve-uncalled-function-check branch January 17, 2023 23:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Improve uncalled function check related to parenthesized and binary expressions #50756

Improve uncalled function check related to parenthesized and binary expressions #50756

Uh oh!

Andarist commented Sep 13, 2022

Uh oh!

Uh oh!

Andarist Sep 14, 2022

Uh oh!

Andarist Sep 14, 2022

Uh oh!

jakebailey commented Sep 26, 2022

Uh oh!

typescript-bot commented Sep 26, 2022 •

edited

Loading

Uh oh!

typescript-bot commented Sep 26, 2022 •

edited

Loading

Uh oh!

typescript-bot commented Sep 26, 2022 •

edited

Loading

Uh oh!

typescript-bot commented Sep 26, 2022 •

edited

Loading

Uh oh!

typescript-bot commented Sep 26, 2022

Uh oh!

typescript-bot commented Sep 26, 2022

Uh oh!

typescript-bot commented Sep 26, 2022

Uh oh!

Andarist commented Dec 9, 2022

Uh oh!

jakebailey left a comment

Uh oh!

Andarist commented Jan 17, 2023

Uh oh!

jakebailey commented Jan 17, 2023

Uh oh!

Uh oh!

	~~~~~~~~~
	!!! error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?

Improve uncalled function check related to parenthesized and binary expressions #50756

Improve uncalled function check related to parenthesized and binary expressions #50756

Uh oh!

Conversation

Andarist commented Sep 13, 2022

Uh oh!

Uh oh!

Andarist Sep 14, 2022

Choose a reason for hiding this comment

Uh oh!

Andarist Sep 14, 2022

Choose a reason for hiding this comment

Uh oh!

jakebailey commented Sep 26, 2022

Uh oh!

typescript-bot commented Sep 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Sep 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Sep 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Sep 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Sep 26, 2022

Uh oh!

typescript-bot commented Sep 26, 2022

Uh oh!

typescript-bot commented Sep 26, 2022

Uh oh!

Andarist commented Dec 9, 2022

Uh oh!

jakebailey left a comment

Choose a reason for hiding this comment

Uh oh!

Andarist commented Jan 17, 2023

Uh oh!

jakebailey commented Jan 17, 2023

Uh oh!

Uh oh!

typescript-bot commented Sep 26, 2022 •

edited

Loading

typescript-bot commented Sep 26, 2022 •

edited

Loading

typescript-bot commented Sep 26, 2022 •

edited

Loading

typescript-bot commented Sep 26, 2022 •

edited

Loading