Omit effects-free conditional constructs from control flow graph #58013
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
typescript-bot
added
Author: Team
For Uncommitted Bug
|
@typescript-bot test it |
|
Hey @ahejlsberg, the results of running the DT tests are ready. Everything looks the same! |
|
@ahejlsberg Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@ahejlsberg Here are the results of running the user tests comparing Something interesting changed - please have a look. Details |
|
@typescript-bot test it |
|
Hey @ahejlsberg, the results of running the DT tests are ready. Everything looks the same! |
|
@ahejlsberg Here are the results of running the user tests comparing Something interesting changed - please have a look. Details |
|
@ahejlsberg Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@ahejlsberg Here are the results of running the top 400 repos comparing Everything looks good! |
1 similar comment
|
@ahejlsberg Here are the results of running the top 400 repos comparing Everything looks good! |
|
@typescript-bot user test this |
|
@ahejlsberg Here are the results of running the user tests comparing Something interesting changed - please have a look. Details |
|
@ahejlsberg Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Any idea why we're seeing the test failure here #58013 (comment)? I can't reproduce it locally. Meaning, when I run |
|
@jakebailey Ok, I wasn't running with |
|
I was out yesterday; will take a look at this shortly. |
|
Cloning webpack, doing $ git clone git@github.com:webpack/webpack.git
$ cd webpack
$ yarn
$ node ~/work/TypeScript/built/local-old/tsc.js -p tsconfig.types.json |& tee ~/out-main.txt
$ node ~/work/TypeScript/built/local/tsc.js -p tsconfig.types.json |& tee ~/out-pr.txt
$ wc -l ~/out-main.txt ~/out-pr.txt
3188 /home/jabaile/out-main.txt
3187 /home/jabaile/out-pr.txt
6375 total
$ gdiff ~/out-main.txt ~/out-pr.txtdiff --git a/home/jabaile/out-main.txt b/home/jabaile/out-pr.txt
index c825c4baf..0b70c6f9f 100644
--- a/home/jabaile/out-main.txt
+++ b/home/jabaile/out-pr.txt
@@ -2004,7 +2004,6 @@ lib/optimize/SplitChunksPlugin.js(1011,30): error TS7006: Parameter 'key' implic
lib/optimize/SplitChunksPlugin.js(1022,37): error TS7006: Parameter 'key' implicitly has an 'any' type.
lib/optimize/SplitChunksPlugin.js(1095,35): error TS18048: 'cacheGroup.minChunks' is possibly 'undefined'.
lib/optimize/SplitChunksPlugin.js(1099,9): error TS2722: Cannot invoke an object which is possibly 'undefined'.
-lib/optimize/SplitChunksPlugin.js(1099,9): error TS18048: 'cacheGroup.getName' is possibly 'undefined'.
lib/optimize/SplitChunksPlugin.js(1254,21): error TS18048: 'cacheGroup.minChunks' is possibly 'undefined'.
lib/optimize/SplitChunksPlugin.js(1687,37): error TS7006: Parameter 'value' implicitly has an 'any' type.
lib/optimize/SplitChunksPlugin.js(1687,44): error TS7006: Parameter 'key' implicitly has an 'any' type.This difference appears no matter what order I run things or the clean-ness of the repo. So, I think the bot is correct here. |
|
Also, I modified this PR to force Trial and error says that it's something to do with the |
|
@jakebailey Yeah, sorry, I see what's going on now. What confused me is that we used to issue two errors, and now we only issue one, but the errors have the exact same location. The missing error is an effect of optimizing away part of the control flow graph, causing us to not call |
|
Now that we've sorted out the missing error, I think this is ready for a review. |
jakebailey
approved these changes
Apr 3, 2024
weswigham
approved these changes
Apr 3, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With this PR we optimize control flow graph construction to omit conditional constructs that have no control flow effects. Consider this example:
This currently creates the following control flow graph for the reference to
xin thefoo(x)call:With this PR, the effects-free conditional operator expression is removed from the control flow graph for references that follow it in the graph:
Intuitively, this optimization is possible because the observed control flow types at the bottom BranchLabel node are the same as the observed control flow types at the Start node for any variable, parameter, or property reference.
The shallower control flow graphs reduce check time up to 2% in several projects.