Skip to content

Ensure tests of non-returning functions don't cause unreachability errors#38596

Merged
ahejlsberg merged 1 commit intomasterfrom
fixNeverReturningFunctions
Sep 25, 2019
Merged

Ensure tests of non-returning functions don't cause unreachability errors#38596
ahejlsberg merged 1 commit intomasterfrom
fixNeverReturningFunctions

Conversation

@ahejlsberg
Copy link
Copy Markdown
Collaborator

Fixes issues resulting from unreachable code in node/v10 and vfile following merge of microsoft/TypeScript#32695.

@ahejlsberg ahejlsberg requested a review from sandersn September 25, 2019 16:18
@typescript-bot typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). Awaiting reviewer feedback labels Sep 25, 2019
@typescript-bot
Copy link
Copy Markdown
Contributor

typescript-bot commented Sep 25, 2019

@ahejlsberg Thank you for submitting this PR!

🔔 @microsoft @DefinitelyTyped @jkomyno @a-tarasyuk @alvis @r3nya @btoueg @BrunoScheufler @smac89 @tellnes @Touffy @DeividasBakanas @eyqs @Flarna @Hannes-Magnusson-CK @KSXGitHub @hoo29 @kjin @ajafff @islishude @mwiktorczyk @matthieusieben @mohsen1 @n-e @octo-sniffle @parambirs @eps1lon @SimonSchick @ThomasdenH @WilcoBakker @wwwy3y3 @ZaneHannanAU @samuela @kuehlein @j-oliveras @bhongy @chyzwar @trivikr @jeremiergz @bizen241 @Rokt33r - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

@ahejlsberg ahejlsberg merged commit 24cd81e into master Sep 25, 2019
@ahejlsberg ahejlsberg deleted the fixNeverReturningFunctions branch September 25, 2019 17:34
@typescript-bot
Copy link
Copy Markdown
Contributor

👋 Hi there! I’ve run some quick performance metrics against master and your PR. This is still an experiment, so don’t panic if I say something crazy! I’m still learning how to interpret these metrics.

Let’s review the numbers, shall we?

node/v10

Comparison details for node/v10 📊
master #38596 diff
Batch compilation
Memory usage (MiB) 110.8 116.7 +5.3%
Type count 17814 17814 0.0%
Assignability cache size 34036 34036 0.0%
Subtype cache size 2060 2060 0.0%
Identity cache size 3 3 0.0%
Language service
Samples taken 4476 4451 -0.6%
Identifiers in tests 9830 9830 0.0%
getCompletionsAtPosition
    Mean duration (ms) 816.4 818.7 +0.3%
    Median duration (ms) 798.3 797.7 -0.1%
    Mean CV 0.0% 0.0%
    Worst duration (ms) 1443.1 1451.9 +0.6%
    Worst identifier level text
getQuickInfoAtPosition
    Mean duration (ms) 780.3 788.5 +1.1%
    Median duration (ms) 750.3 758.6 +1.1%
    Mean CV 0.0% 0.0%
    Worst duration (ms) 1480.2 1319.8 -10.8%
    Worst identifier HTTP_STATUS_PARTIAL_CONTENT inspector

It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place.

vfile/v3

Comparison details for vfile/v3 📊
master #38596 diff
Batch compilation
Memory usage (MiB) 63.9 63.1 -1.2%
Type count 9354 9354 0.0%
Assignability cache size 3065 3065 0.0%
Subtype cache size 6 6 0.0%
Identity cache size 1 1 0.0%
Language service
Samples taken 119 119 0.0%
Identifiers in tests 119 119 0.0%
getCompletionsAtPosition
    Mean duration (ms) 352.6 363.9 +3.2%
    Median duration (ms) 350.4 364.4 +4.0%
    Mean CV 13.9% 12.9% -7.4%
    Worst duration (ms) 453.2 441.0 -2.7%
    Worst identifier extname vfileMessage
getQuickInfoAtPosition
    Mean duration (ms) 371.3 375.5 +1.1%
    Median duration (ms) 367.0 376.0 +2.5%
    Mean CV 16.4% 15.0% -9.0%
    Worst duration (ms) 472.3 445.4 -5.7%
    Worst identifier message Unist

It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place.


If you have any questions or comments about me, you can ping @andrewbranch. Have a nice day!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Popular package This PR affects a popular package (as counted by NPM download counts).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants