Fixed #18763 Compiler crash on bad code in the IDE #20903

brianhartung · 2017-07-15T01:50:17Z

(First time contributor)

Fixes #18763

Customer scenario
Visual Studio crashes when hovering over invalid code in IDE.

Bugs this fixes:
#18763

Workarounds, if any
None

Risk
Low

Performance impact
Low (additional null check, no new allocations)

Is this a regression from a previous update?
No

Root cause analysis:
In CSharpSemanticModel method AddUnwrappingErrorTypes an existing null check failed to consider a null value on a child member access inside the body of the protected block.

How was the bug found?
Customer Reported

@jaredpar

…wrappingErrorTypes to fix crashing behavior in Issue #18763

agocke · 2017-07-16T18:15:23Z

I'd expect a compiler unit test to be added to verify this change.

agocke · 2017-07-16T18:18:38Z

Specifically, I'd expect an additional test in src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelGetSemanticInfoTests.cs

cston · 2017-07-16T22:33:55Z

src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs

@@ -1852,7 +1852,7 @@ private bool IsInTypeofExpression(int position)
        private static void AddUnwrappingErrorTypes(ArrayBuilder<Symbol> builder, Symbol s)
        {
            var originalErrorSymbol = s.OriginalDefinition as ErrorTypeSymbol;
-            if ((object)originalErrorSymbol != null)
+            if ((object)originalErrorSymbol != null && originalErrorSymbol.CandidateSymbols != null)


ErrorTypeSymbol.CandidateSymbols should return ImmutableArray<Symbol>.Empty rather than default(ImmutableArray<Symbol>).

@cston are you making a comment about what this PR should do or that possibly ErrorTypeSymbol is the source of the bug? Can't tell from the comment.

If ErrorTypeSymbol.CandidateSymbols is returning default(ImmutableArray<Symbol>), it seems like the bug is in ErrorTypeSymbol.

…mmutableArray<Symbol> leading to a VS crash

brianhartung · 2017-07-19T01:20:20Z

Ok, I think we might have narrowed in on the issue here. I haven't added any unit tests yet (not 100% sure what those would look like) but I was hoping to have someone eyeball this fix. I can get VS (15.3 Preview) to reliably crash without it. Looks like the crux of the issue is that GetSemanticSymbols expects to return an ImmutableArray<Symbol> but in this case because the "this" reference it's considering is not really valid, it ends up adding a null item in the returned array which causes an NRE later, hence the crash.

cston · 2017-07-19T05:30:09Z

A test would probably look something like the following. Please ensure the test fails without your fix.

        [Fact]
        public void AttributeArgumentLambdaThis()
        {
            string source =
@"class C
{
    [X(() => this._Y)]
    public void Z()
    {
    }
}";
            var compilation = CreateStandardCompilation(source);
            var tree = compilation.SyntaxTrees[0];
            var model = compilation.GetSemanticModel(tree);
            var syntax = tree.GetCompilationUnitRoot().DescendantNodes().Single(n => n.Kind() == SyntaxKind.ThisExpression);
            var info = model.GetSemanticInfoSummary(syntax);
            Assert.Equal("...", info.Type.ToTestDisplayString());
        }

cston · 2017-07-19T17:49:25Z

src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelGetSemanticInfoTests.cs

@@ -15213,5 +15213,24 @@ class K

            Assert.False(semanticInfo.IsCompileTimeConstant);
        }
+
+        [Fact]


Please add [WorkItem(18763, "https://github.com/dotnet/roslyn/issues/18763")]. (I should have included that originally.)

No problem...I'll do that now. Btw, as requested I did verify that with my change all tests pass locally (including my new one) but if I revert the fix, I get the original NRE in this new test case (in the same spot that caused the crash). Being new to this, I'm not sure what the best way is to provide evidence, but attached is a screenshot of the expected failure case

.

@brianhartung No problem. For future reference, if you say the test failed without your change we usually take your word on it :)

cston · 2017-07-19T17:57:00Z

LGTM

dpoeschl · 2017-07-19T23:12:44Z

retest windows_debug_vs-integration_prtest please
Reason: AppDomainUnloadedException

…sion-expression-rewrite * dotnet/features/ioperation: (229 commits) Adding Ioperation tests for WhileUntilLoopStatement (dotnet#21047) marked assert that needs to be re-enabled addressing more PR feedbacks PR feedback Remove InvocationReasons enum boxing PR feedbacks Expose if a Binary/Unary operator was 'Lifted' at the IExpression level. (dotnet#14779) addressing PR feedback added comments Update VS Integration machines to 15.3 Preview 6 (dotnet#21240) fixed typo Fixed dotnet#18763 Compiler crash on bad code in the IDE (dotnet#20903) Fix typo in ERR_RefReturnParameter2 (dotnet#21235) Fix unbound recursion with const var field in script (dotnet#21223) Typo fix (dotnet#20513) PR feedbacks and added some more tests When producing character literals, surrogate characters should be escaped. (dotnet#20720) Fix build correctness issues Fix possible null reference warnings Adding ioperation tests for ForEachStatement (dotnet#21048) ...

BRH - Added additional null check in CSharpSemanticModel method AddUn…

0ac5cab

…wrappingErrorTypes to fix crashing behavior in Issue #18763

dnfclas added the cla-already-signed label Jul 15, 2017

Pilchie added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Jul 16, 2017

Pilchie requested a review from a team July 16, 2017 18:12

Merge branch 'master' of https://github.com/dotnet/roslyn

6b8702f

cston reviewed Jul 16, 2017

View reviewed changes

brianhartung added 5 commits July 17, 2017 13:01

Merge branch 'master' of https://github.com/dotnet/roslyn

3224adb

Merge branch 'master' of https://github.com/dotnet/roslyn

7a0381e

Merge branch 'master' of https://github.com/dotnet/roslyn

140683b

Merge branch 'master' of https://github.com/dotnet/roslyn

cd88859

Fixed issue with GetSemanticSymbols returning a null item inside an I…

60e2cfe

…mmutableArray<Symbol> leading to a VS crash

brianhartung added 2 commits July 19, 2017 09:43

Merge branch 'master' of https://github.com/dotnet/roslyn

c07f150

Added AttributeArgumentLambdaThis test to validate fix for Issue #18763

9f44d0b

cston reviewed Jul 19, 2017

View reviewed changes

Added WorkItem attribute to new test case

c93b33a

agocke approved these changes Jul 19, 2017

View reviewed changes

Merge branch 'master' of https://github.com/dotnet/roslyn

c5bf0a4

jcouv merged commit fc73d57 into dotnet:master Aug 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed #18763 Compiler crash on bad code in the IDE #20903

Fixed #18763 Compiler crash on bad code in the IDE #20903

brianhartung commented Jul 15, 2017

agocke commented Jul 16, 2017

agocke commented Jul 16, 2017

cston Jul 16, 2017

jaredpar Jul 17, 2017

cston Jul 17, 2017 •

edited

Loading

brianhartung commented Jul 19, 2017 •

edited

Loading

cston commented Jul 19, 2017

cston Jul 19, 2017

brianhartung Jul 19, 2017

agocke Jul 19, 2017

cston commented Jul 19, 2017

dpoeschl commented Jul 19, 2017 •

edited

Loading

Fixed #18763 Compiler crash on bad code in the IDE #20903

Fixed #18763 Compiler crash on bad code in the IDE #20903

Conversation

brianhartung commented Jul 15, 2017

agocke commented Jul 16, 2017

agocke commented Jul 16, 2017

cston Jul 16, 2017

Choose a reason for hiding this comment

jaredpar Jul 17, 2017

Choose a reason for hiding this comment

cston Jul 17, 2017 • edited Loading

Choose a reason for hiding this comment

brianhartung commented Jul 19, 2017 • edited Loading

cston commented Jul 19, 2017

cston Jul 19, 2017

Choose a reason for hiding this comment

brianhartung Jul 19, 2017

Choose a reason for hiding this comment

agocke Jul 19, 2017

Choose a reason for hiding this comment

cston commented Jul 19, 2017

dpoeschl commented Jul 19, 2017 • edited Loading

cston Jul 17, 2017 •

edited

Loading

brianhartung commented Jul 19, 2017 •

edited

Loading

dpoeschl commented Jul 19, 2017 •

edited

Loading