Omnibus fixes for telemetry-sourced crashes #20545

RyanCavanaugh · 2017-12-07T20:45:31Z

Fixes #20523 - occurs when the number of references found didn't match the function arity
Fixes #20520 - "impossible" case as described by asserts actually does occur, but can be handled safely
Fixes #20527, #20474, #20472 - containing function may be undefined in the presence of syntax errors
Fixes #20542 - symbol writer may sometimes encounter index signature with no types
Fixes #20475 - symbol writer may sometimes encounter types with no symbol

Recommend reviewing commit-by-commit. Some of these crashes are from telemetry and we've been unable to reconstruct a repro for them.

RyanCavanaugh · 2017-12-07T20:46:37Z

... we should turn on strict null checks 😠

billti · 2017-12-07T21:26:07Z

src/services/codefixes/inferFromUsage.ts

@@ -191,8 +200,9 @@ namespace ts.codefix {
            sourceFile,
            token.getStart(sourceFile));

-        Debug.assert(!!references, "Found no references!");
-        Debug.assert(references.length === 1, "Found more references than expected");
+        if (!references || references.length !== 1) {


Is the references.length !== 1 required? Seems weird to expect an array of exactly 1 object. If there are 2, is it better to return an empty array than ignore the 2nd unexpected element? Is it guaranteed/expected to always return 1 element in this context?

This variable is probably badly named. references is not an array of references to a symbol, it's (effectively) an array of arrays; each element of the outer array refers to a different symbol (as could occur if you had a type and a value with the same name used in a position where either meaning might be valid, such as an import statement), and each subelement is the references to that symbol. In this case it would be very unusual (IOW we have no idea what's going on or what the right thing to do is) if we found more than one symbol matching a certain property.

Is it better to remove the asserts or leave them? There is minimal user impact either way (though throwing errors probably isn't good for perf), since we merely end up swallowing the error and writing it as part of the failure response.

billti

Nice! Worth porting to 2.6? Were most of the reports coming back from VS usage?

RyanCavanaugh · 2017-12-07T21:34:31Z

Looking at CI failures.

Some of the fixes were for crashes I found when trying to reconstruct repros for those stacks. I believe VS is the only place we get these stack traces from - @aozgaa ?

aozgaa · 2017-12-07T21:50:25Z

The stack traces are from vscode only.

aozgaa

Good fixes, just want to clarify the desired failure scenarios.

aozgaa · 2017-12-07T21:55:09Z

src/services/codefixes/inferFromUsage.ts

@@ -71,6 +71,10 @@ namespace ts.codefix {
        }

        const containingFunction = getContainingFunction(token);
+        if (containingFunction === undefined) {
+            // Possible in certain syntax error cases


Is a syntax error synonymous with a parse error?

aozgaa · 2017-12-07T21:56:12Z

src/compiler/checker.ts

@@ -3951,7 +3951,12 @@ namespace ts {
                    writePunctuation(writer, SyntaxKind.CloseBracketToken);
                    writePunctuation(writer, SyntaxKind.ColonToken);
                    writeSpace(writer);
-                    buildTypeDisplay(info.type, writer, enclosingDeclaration, globalFlags, symbolStack);
+                    if (info.type) {
+                        buildTypeDisplay(info.type, writer, enclosingDeclaration, globalFlags, symbolStack);


FYI, I believe this will be deprecated by #18860. Good catch!

aozgaa · 2017-12-07T23:08:51Z

src/services/codefixes/inferFromUsage.ts

+        if (containingFunction.parameters.length !== types.length) {
+            return undefined;
+        }
+
        const textChanges = arrayFrom(mapDefinedIterator(zipToIterator(containingFunction.parameters, types), ([parameter, type]) =>


Now that we are doing the length check above, can we unroll this to an index-based for-loop? the intermediate iterator and maps are unnecessary

aozgaa · 2017-12-07T23:15:34Z

src/services/codefixes/inferFromUsage.ts

@@ -191,8 +200,9 @@ namespace ts.codefix {
            sourceFile,
            token.getStart(sourceFile));

-        Debug.assert(!!references, "Found no references!");
-        Debug.assert(references.length === 1, "Found more references than expected");
+        if (!references || references.length !== 1) {


Is it better to remove the asserts or leave them? There is minimal user impact either way (though throwing errors probably isn't good for perf), since we merely end up swallowing the error and writing it as part of the failure response.

aozgaa · 2017-12-07T23:20:27Z

src/services/symbolDisplay.ts

@@ -496,7 +496,7 @@ namespace ts.SymbolDisplay {
            addNewLineIfDisplayPartsExist();
            if (symbolKind) {
                pushTypePart(symbolKind);
-                if (!some(symbol.declarations, d => isArrowFunction(d) || (isFunctionExpression(d) || isClassExpression(d)) && !d.name)) {
+                if (symbol && !some(symbol.declarations, d => isArrowFunction(d) || (isFunctionExpression(d) || isClassExpression(d)) && !d.name)) {


So if symbol === undefined we will just leave the name blank? Is that desirable behavior? Should we perhaps instead fail to build the display completely?

This code path builds either anonymous or non-anonymous declarations (e.g. function (x) or function y(x)), so it's already regularly producing non-named symbol displays. In the case where the originating type doesn't have a symbol, it's sort of de facto anonymous anyway so it seems like this won't ever produce any particularly weird output.

RyanCavanaugh · 2017-12-07T23:27:38Z

Is it better to remove the asserts or leave them?

I see this as a correctness thing - in an ideal world, a failing assert in TS Server would pop an assert dialog in VS in debug builds. Since we now know that these conditions (e.g. references being length non-1) under non-impossible situations, we should write the code accordingly

Omnibus fixes for telemetry-sourced crashes

RyanCavanaugh added 5 commits December 7, 2017 12:38

Fix microsoft#20523

6e74f7e

Fix microsoft#20520

2cf7295

Fixes microsoft#20527

b7b43fe

Fixes microsoft#20542

5c99c67

Fixes microsoft#20475 (no repro found yet)

fa988ea

RyanCavanaugh requested review from billti and aozgaa December 7, 2017 20:45

billti reviewed Dec 7, 2017

View reviewed changes

billti approved these changes Dec 7, 2017

View reviewed changes

Bail at the correct point when containingFunction is undefined

92c3b23

aozgaa reviewed Dec 7, 2017

View reviewed changes

RyanCavanaugh added a commit that referenced this pull request Dec 13, 2017

Merge pull request #20545 from RyanCavanaugh/codefixOmnibus

f6603cd

Omnibus fixes for telemetry-sourced crashes

RyanCavanaugh merged commit f6603cd into microsoft:master Dec 13, 2017

RyanCavanaugh merged commit 92c3b23 into microsoft:master Dec 13, 2017

mhegazy mentioned this pull request Feb 7, 2018

Debug Failure. False expression: Found more references than expected in getReferences during getCodeActionsForParameters #20790

Closed

microsoft locked and limited conversation to collaborators Jun 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Omnibus fixes for telemetry-sourced crashes #20545

Omnibus fixes for telemetry-sourced crashes #20545

Uh oh!

RyanCavanaugh commented Dec 7, 2017

Uh oh!

RyanCavanaugh commented Dec 7, 2017

Uh oh!

billti Dec 7, 2017

Uh oh!

RyanCavanaugh Dec 7, 2017

Uh oh!

aozgaa Dec 7, 2017

Uh oh!

billti left a comment

Uh oh!

RyanCavanaugh commented Dec 7, 2017

Uh oh!

aozgaa commented Dec 7, 2017 •

edited

Loading

Uh oh!

aozgaa left a comment

Uh oh!

aozgaa Dec 7, 2017

Uh oh!

aozgaa Dec 7, 2017

Uh oh!

aozgaa Dec 7, 2017

Uh oh!

aozgaa Dec 7, 2017

Uh oh!

aozgaa Dec 7, 2017

Uh oh!

RyanCavanaugh Dec 7, 2017

Uh oh!

RyanCavanaugh commented Dec 7, 2017

Uh oh!

Uh oh!

Omnibus fixes for telemetry-sourced crashes #20545

Omnibus fixes for telemetry-sourced crashes #20545

Uh oh!

Conversation

RyanCavanaugh commented Dec 7, 2017

Uh oh!

RyanCavanaugh commented Dec 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

billti left a comment

Choose a reason for hiding this comment

Uh oh!

RyanCavanaugh commented Dec 7, 2017

Uh oh!

aozgaa commented Dec 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aozgaa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RyanCavanaugh commented Dec 7, 2017

Uh oh!

Uh oh!

aozgaa commented Dec 7, 2017 •

edited

Loading