Supporting complex types in ENC (WAS: ENC for tuples in foreach) #19283

ivanbasov · 2017-05-05T00:11:57Z

Customer scenario

On ENC, user changes code near a variable declaration of a complex type, e.g. tuple or array of tuples like

var a = (1, 2);
<breakpoint is here> Console.WriteLine(0);

to

var a = (1, 2);
Console.WriteLine(1);

Expected: the variable has a value defined just above.
Actual: the variable value is reseted. Internally, a new variable is declared within ENC without a match to the existing one.

Bugs this fixes:

Fixes #19303

Workarounds, if any

Risk

Low

Performance impact

Low

Is this a regression from a previous update?

Root cause analysis:

How did we miss it? What tests are we adding to guard against it in the future?

How was the bug found?

Planned

tmat · 2017-05-09T19:00:33Z

src/Compilers/CSharp/Test/Emit/Emit/EditAndContinue/EditAndContinueTests.cs

@@ -8077,5 +8077,403 @@ .maxstack  2
 }
 ");
        }
+
+        [Fact]
+        public void ForeachStatement_Reorder()


ForeachStatement_Reorder [](start = 20, length = 24)

You can remove this test. Instead expand ArrayOfTuples to test 2nd generation. #Resolved

Also, add ArrayOfTuple test to VB

In reply to: 115573899 [](ancestors = 115573899)

Actually... let's make the ArrayOfTuples test more interesting and make the type of the variable something like an array of tuples of anonymous types of a List. You can also add more variables of more complex types, including pointers, ref variables, etc.

And rename the test to ComplexTypes, or something like that.

In reply to: 115574092 [](ancestors = 115574092,115573899)

Thank you! Fixed

In reply to: 115576722 [](ancestors = 115576722,115574092,115573899)

tmat · 2017-05-11T20:25:48Z

src/Compilers/CSharp/Test/Emit/Emit/EditAndContinue/EditAndContinueTests.cs

+        var <N:0>a</N:0> = new { key = ""a"", value = new List<(int, int)>()};
+        var <N:1>b</N:1> = (number: 5, value: a);
+        var <N:2>c</N:2> = new[] { b };
+        IntPtr[] <N:3>d</N:3> = new IntPtr[1];


IntPtr [](start = 8, length = 6)

This is not really a pointer type. IntPtr is just ordinary value type.

You can use the following trick to cover quite a few interesting cases:

class C<T> { public enum E { A } } C<(int, dynamic)>.E***[,,] x;

#Resolved

Thank you! This is done

In reply to: 116093049 [](ancestors = 116093049)

tmat · 2017-05-11T20:27:02Z

tmat · 2017-05-11T20:28:13Z

src/Compilers/VisualBasic/Test/Emit/Emit/EditAndContinue/EditAndContinueTests.vb

+Class C
+    Sub G()
+        Dim <N:0>a</N:0> = New With {.Key = ""a"", .Value = New List(Of Tuple(Of Integer, Integer))()}
+        Dim <N:1>b</N:1> = Tuple.Create(5, a)


Tuple.Create(5, a) [](start = 27, length = 18)

This doesn't create a value tuple. I think VB has syntax for tuples as well. #Resolved

Thank you! Added a test and made similar changes to VB. An existing VB test fails.

In reply to: 116093622 [](ancestors = 116093622)

tmat · 2017-05-11T20:28:44Z

src/Compilers/VisualBasic/Test/Emit/Emit/EditAndContinue/EditAndContinueTests.vb

+        Dim <N:0>a</N:0> = New With {.Key = ""a"", .Value = New List(Of Tuple(Of Integer, Integer))()}
+        Dim <N:1>b</N:1> = Tuple.Create(5, a)
+        Dim <N:2>c</N:2> = {b}
+        Dim <N:3>d</N:3> = New IntPtr(1)


Dim <N:3>d</N:3> = New IntPtr(1) [](start = 8, length = 32)

VB doesn't have pointers. We can remove this case. #Resolved

OK. Thanks!

In reply to: 116093741 [](ancestors = 116093741)

ivanbasov · 2017-05-11T23:43:19Z

Thank you for the feedback, @tmat ! I made corresponding changes. As I said, an existing VB test failed after changes in VisualBasicSymbolMatcher.vb #Resolved

ivanbasov · 2017-05-11T23:51:40Z

E2E tests in VB with tuples, e.g. reordering foreach over tuples work fine #Resolved

tmat · 2017-05-12T00:20:36Z

src/Compilers/VisualBasic/Portable/Emit/EditAndContinue/VisualBasicSymbolMatcher.vb

@@ -692,6 +692,11 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Emit
            End Function

            Public Overrides Function VisitNamedType(type As NamedTypeSymbol) As Symbol
+                If (type.IsTupleType) Then


( [](start = 19, length = 1)

nit: No parentheses needed in VB #Resolved

thanks!

In reply to: 116131000 [](ancestors = 116131000)

jcouv · 2017-05-12T18:05:49Z

src/Compilers/CSharp/Portable/Emitter/EditAndContinue/CSharpSymbolMatcher.cs

@@ -498,7 +498,7 @@ public override Symbol VisitNamedType(NamedTypeSymbol sourceType)
                        return null;
                    }

-                    return TupleTypeSymbol.Create(otherDef, sourceType.TupleElementNames);
+                    return otherDef;


Consider adding a comment. #Pending

I could not find an idea for the comment. It seems like a simplification change. Do you have ideas for the comment?

In reply to: 116290923 [](ancestors = 116290923)

jcouv · 2017-05-12T18:07:11Z

src/Compilers/CSharp/Test/Emit/Emit/EditAndContinue/LocalSlotMappingTests.cs

+        var <N:2>c</N:2> = new[] { b };
+        int[] <N:3>array</N:3> = { 1, 2, 3 };
+        ref int <N:4>d</N:4> = ref array[0];
+        C1<(int, dynamic)>.E***[,,] <N:5>x</N:5> = null;


Whoa #Resolved

jcouv · 2017-05-12T18:09:51Z

The PR title mentions tuples in foreach, but I see no such test in the diff. I'm confused. #Resolved

jcouv · 2017-05-12T18:14:28Z

src/Compilers/CSharp/Test/Emit/Emit/EditAndContinue/LocalSlotMappingTests.cs

+            var diff1 = compilation1.EmitDifference(
+                generation0,
+                ImmutableArray.Create(
+                    new SemanticEdit(SemanticEditKind.Update, f0, f1, GetSyntaxMapFromMarkers(source0, source1), preserveLocalVariables: true)));


I don't understand what is the edit. Aren't f0 and f1 the same (both use the same source)? #Resolved

There is no edit. The compiler doesn't care about edits, only how nodes that declare variables map to previous syntax. We just want to verify that the local slots are assigned correctly. #Resolved

jcouv · 2017-05-12T23:12:45Z

src/Compilers/VisualBasic/Portable/Emit/EditAndContinue/VisualBasicSymbolMatcher.vb

@@ -692,6 +692,11 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Emit
            End Function

            Public Overrides Function VisitNamedType(type As NamedTypeSymbol) As Symbol
+                If (type.IsTupleType) Then


From our discussion, let's figure out why this didn't result in the extra locals (from previous PR and tests) to be optimized away. #Resolved

the original question was in #19114

re-reviewing it now

In reply to: 116339869 [](ancestors = 116339869)

I have reviewed scenarios mentioned in the discussion from the past code review #19114
There are EditAndContinueTests.cs with tuples in the foreach statement. We attempted to change their names or types.

We still do not have a match for them, i.e. extra locals appear.

We still have a rude edit warning for this situation. This prevents us from run-time errors.

We already covered the situation with support advanced edits of foreach statements within ENC #19154

In reply to: 116639124 [](ancestors = 116639124,116339869)

ivanbasov · 2017-05-16T01:44:45Z

Thank you! Fixed

In reply to: 301147392 [](ancestors = 301147392)

tmat · 2017-05-16T20:06:42Z

src/Compilers/CSharp/Test/Emit/Emit/EditAndContinue/SymbolMatcherTests.cs

@@ -769,7 +769,7 @@ public class C
    public delegate (int, int) F();
 }";
            var source1 = @"
-public struct C


struct [](start = 7, length = 6)

Why this change? #Resolved

We should compare class vs. class here not class vs. struct. The change happens in the delegate not in the class/struct.

In reply to: 116844195 [](ancestors = 116844195)

Oh, yes. Sorry I missed that.

In reply to: 116845747 [](ancestors = 116845747,116844195)

ivanbasov · 2017-05-17T20:25:16Z

need to resolve merge conflict #Closed

tmat · 2017-05-17T23:24:06Z

@dotnet/roslyn-compiler Need 2 reviews please.

tmat · 2017-05-17T23:27:11Z

src/Compilers/VisualBasic/Portable/Symbols/ArrayTypeSymbol.vb

@@ -317,6 +317,41 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Symbols
            Return IsSameType(obj, TypeCompareKind.ConsiderEverything)
        End Function

+        Friend Overrides Function Equals(t2 As TypeSymbol, comparison As TypeCompareKind) As Boolean


Equals [](start = 34, length = 6)

Each of these implementations could use some compiler unit tests. Presumably there are some C# tests that can be ported. #Resolved

Specifically, types with custom modifiers.

In reply to: 117133141 [](ancestors = 117133141)

Removed Symbol changes. Current test coverage should be enough. Thank you!

In reply to: 117133247 [](ancestors = 117133247,117133141)

tmat · 2017-05-17T23:28:34Z

src/Compilers/VisualBasic/Portable/Symbols/NamedTypeSymbol.vb

+                        Return True
+                    End If
+                End If
+            End If


No applicable in VB. VB doesn't have Dynamic type. #Resolved

Thank you! Fixed.

In reply to: 117133293 [](ancestors = 117133293)

AlekseyTs · 2017-05-18T22:55:51Z

As we discussed offline, we should switch to using IsSameType extension method, rather than adding new Equals overload. #Closed

ivanbasov · 2017-05-18T23:51:52Z

Thank you! Fixed

In reply to: 302563607 [](ancestors = 302563607)

AlekseyTs · 2017-05-19T03:01:11Z

src/Compilers/VisualBasic/Portable/Emit/EditAndContinue/VisualBasicSymbolMatcher.vb


-                    Return visitedSource = visitedOther
+                    Return visitedSource IsNot Nothing AndAlso visitedSource.IsSameType(visitedOther, TypeCompareKind.IgnoreTupleNames)


visitedSource.IsSameType(visitedOther, TypeCompareKind.IgnoreTupleNames) [](start = 63, length = 72)

We are checking if visitedSource is Nothing, it looks like we should do the same check for visitedOther. #Resolved

Thank you! Fixed

In reply to: 117395320 [](ancestors = 117395320)

Thank you!

In reply to: 117552926 [](ancestors = 117552926,117395320)

AlekseyTs · 2017-05-19T03:04:15Z

Done with review pass. #Closed

AlekseyTs · 2017-05-19T19:12:56Z

src/Compilers/VisualBasic/Portable/Emit/EditAndContinue/VisualBasicSymbolMatcher.vb


-                    Return visitedSource = visitedOther
+                    ' If both visitedSource And visitedOther are Nothing, return false meaning that the method was not able to verify the equality.


And [](start = 44, length = 3)

"And" vs "and". #Resolved

Thank you! Fixed

In reply to: 117555598 [](ancestors = 117555598)

AlekseyTs

LGTM

cston · 2017-05-19T19:35:31Z

src/Compilers/VisualBasic/Test/Emit/CodeGen/CodeGenTuples.vb

@@ -14803,7 +14803,8 @@ options:=TestOptions.DebugExe, additionalRefs:=s_valueTupleRefs)
        End Sub

        Private Shared Sub AssertTestDisplayString(symbols As ImmutableArray(Of Symbol), ParamArray baseLine As String())
-            AssertEx.Equal(symbols.Select(Function(s) s.ToTestDisplayString()), baseLine)
+            ' Re-ordering arguments because expected Is usually goes first.
+            AssertEx.Equal(baseLine, symbols.Select(Function(s) s.ToTestDisplayString()))


is rather than Is, and perhaps just expected is first. #Resolved

Thank you! Fixed

In reply to: 117559468 [](ancestors = 117559468)

cston · 2017-05-19T20:03:44Z

src/Compilers/VisualBasic/Test/Emit/Emit/EditAndContinue/SymbolMatcherTests.vb

+            Dim member = compilation1.GetMember(Of FieldSymbol)("C.x")
+            Dim other = matcher.MapDefinition(member)
+            ' Types must match because just an element name was changed.
+            Assert.NotNull(other)


Perhaps Assert.Equal("... signature ...", other.ToTestDisplayString()), here and in other tests below.
#Resolved

Good idea! Thank you! Done

In reply to: 117564324 [](ancestors = 117564324)

cston · 2017-05-19T20:08:15Z

src/Compilers/VisualBasic/Test/Emit/Emit/EditAndContinue/SymbolMatcherTests.vb

+            Dim other = matcher.MapDefinition(member)
+            ' Tuple delegate defines a type. We should be able to match old and new types by name.
+            Assert.NotNull(other)
+        End Sub


Consider testing type changes from non-tuple to tuple and tuple to non-tuple. #Resolved

Thank you! Added

In reply to: 117565093 [](ancestors = 117565093)

cston · 2017-05-19T21:08:01Z

LGTM

ivanbasov · 2017-05-19T22:02:30Z

Tagging @MattGertz or @MeiChin-Tsai for ask mode or escrow approval

MattGertz · 2017-05-19T22:04:35Z

OK by me. Once Mei-Chin approves, please go ahead & set the label.

MeiChin-Tsai · 2017-05-20T00:47:36Z

approved.

ivanbasov added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label May 5, 2017

ivanbasov requested a review from tmat May 5, 2017 00:11

dnfclas added the cla-already-signed label May 5, 2017

ivanbasov requested a review from jcouv May 8, 2017 19:03

tmat reviewed May 9, 2017

View reviewed changes

ivanbasov force-pushed the enc branch from d920d9b to 753f869 Compare May 11, 2017 00:38

ivanbasov added Interactive-EnC and removed PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels May 11, 2017

tmat reviewed May 11, 2017

View reviewed changes

tmat reviewed May 12, 2017

View reviewed changes

jcouv reviewed May 12, 2017

View reviewed changes

ivanbasov changed the title ~~ENC for tuples in foreach~~ Supporting complex types in ENC (WAS: ENC for tuples in foreach) May 16, 2017

tmat reviewed May 16, 2017

View reviewed changes

ivanbasov added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label May 17, 2017

ivanbasov force-pushed the enc branch from 379ce04 to 58cc886 Compare May 17, 2017 20:35

ivanbasov removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label May 17, 2017

tmat added Area-Compilers Area-Interactive labels May 17, 2017

tmat added this to the 15.3 milestone May 17, 2017

tmat requested review from AlekseyTs and VSadov May 17, 2017 23:24

tmat reviewed May 17, 2017

View reviewed changes

ivanbasov force-pushed the enc branch from 58cc886 to 57c654c Compare May 18, 2017 22:08

symbol matcher tuples support in C# and VB

8bb6f85

ivanbasov force-pushed the enc branch from 57c654c to 8bb6f85 Compare May 18, 2017 23:51

AlekseyTs reviewed May 19, 2017

View reviewed changes

code review feedback

95673c0

AlekseyTs reviewed May 19, 2017

View reviewed changes

AlekseyTs approved these changes May 19, 2017

View reviewed changes

cr feedback

17f58e7

cston reviewed May 19, 2017

View reviewed changes

code review feedback

c936a3d

ivanbasov added the Approved to merge label May 20, 2017

ivanbasov merged commit af16c2a into dotnet:master May 20, 2017

ivanbasov deleted the enc branch May 20, 2017 00:50

ivanbasov added a commit to ivanbasov/roslyn that referenced this pull request May 22, 2017

Supporting complex types in ENC (dotnet#19283)

4e8f80c

ivanbasov mentioned this pull request May 22, 2017

Supporting complex types in ENC (#19283) #19695

Merged

ivanbasov added a commit that referenced this pull request May 22, 2017

Supporting complex types in ENC (#19283) (#19695)

cd2112c


		Return visitedSource = visitedOther
		Return visitedSource IsNot Nothing AndAlso visitedSource.IsSameType(visitedOther, TypeCompareKind.IgnoreTupleNames)


		Return visitedSource = visitedOther
		' If both visitedSource And visitedOther are Nothing, return false meaning that the method was not able to verify the equality.

Supporting complex types in ENC (WAS: ENC for tuples in foreach) #19283

Supporting complex types in ENC (WAS: ENC for tuples in foreach) #19283

Conversation

ivanbasov commented May 5, 2017 • edited Loading

tmat May 9, 2017 • edited by ivanbasov Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat May 11, 2017 • edited by ivanbasov Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat commented May 11, 2017 • edited Loading

tmat May 11, 2017 • edited by ivanbasov Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat May 11, 2017 • edited by ivanbasov Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivanbasov commented May 11, 2017 • edited Loading

ivanbasov commented May 11, 2017 • edited Loading

tmat May 12, 2017 • edited by ivanbasov Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv May 12, 2017 • edited by ivanbasov Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv May 12, 2017 • edited by ivanbasov Loading

Choose a reason for hiding this comment

jcouv commented May 12, 2017 • edited by ivanbasov Loading

jcouv May 12, 2017 • edited by ivanbasov Loading

Choose a reason for hiding this comment

tmat May 12, 2017 • edited by ivanbasov Loading

Choose a reason for hiding this comment

jcouv May 12, 2017 • edited by ivanbasov Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivanbasov commented May 16, 2017

tmat May 16, 2017 • edited by ivanbasov Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivanbasov commented May 17, 2017 • edited Loading

tmat commented May 17, 2017

tmat May 17, 2017 • edited by ivanbasov Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat May 17, 2017 • edited by ivanbasov Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented May 18, 2017 • edited Loading

ivanbasov commented May 18, 2017

AlekseyTs May 19, 2017 • edited by ivanbasov Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented May 19, 2017 • edited Loading

AlekseyTs May 19, 2017 • edited by ivanbasov Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

cston May 19, 2017 • edited by ivanbasov Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston May 19, 2017 • edited by ivanbasov Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston May 19, 2017 • edited by ivanbasov Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston commented May 19, 2017

ivanbasov commented May 19, 2017

MattGertz commented May 19, 2017

MeiChin-Tsai commented May 20, 2017

ivanbasov commented May 5, 2017 •

edited

Loading

tmat May 9, 2017 •

edited by ivanbasov

Loading

tmat May 11, 2017 •

edited by ivanbasov

Loading

tmat commented May 11, 2017 •

edited

Loading

tmat May 11, 2017 •

edited by ivanbasov

Loading

tmat May 11, 2017 •

edited by ivanbasov

Loading

ivanbasov commented May 11, 2017 •

edited

Loading

ivanbasov commented May 11, 2017 •

edited

Loading

tmat May 12, 2017 •

edited by ivanbasov

Loading

jcouv May 12, 2017 •

edited by ivanbasov

Loading

jcouv May 12, 2017 •

edited by ivanbasov

Loading

jcouv commented May 12, 2017 •

edited by ivanbasov

Loading

jcouv May 12, 2017 •

edited by ivanbasov

Loading

tmat May 12, 2017 •

edited by ivanbasov

Loading

jcouv May 12, 2017 •

edited by ivanbasov

Loading

tmat May 16, 2017 •

edited by ivanbasov

Loading

ivanbasov commented May 17, 2017 •

edited

Loading

tmat May 17, 2017 •

edited by ivanbasov

Loading

tmat May 17, 2017 •

edited by ivanbasov

Loading

AlekseyTs commented May 18, 2017 •

edited

Loading

AlekseyTs May 19, 2017 •

edited by ivanbasov

Loading

AlekseyTs commented May 19, 2017 •

edited

Loading

AlekseyTs May 19, 2017 •

edited by ivanbasov

Loading

cston May 19, 2017 •

edited by ivanbasov

Loading

cston May 19, 2017 •

edited by ivanbasov

Loading

cston May 19, 2017 •

edited by ivanbasov

Loading