Test EnC on expression variables in initializers and queries #25438

jcouv · 2018-03-13T04:40:35Z

AlekseyTs · 2018-03-13T06:20:28Z

src/Compilers/CSharp/Test/Emit/PDB/PDBTests.cs

@@ -8187,5 +8187,294 @@ .maxstack  3
 }
 ", sequencePoints: "Program+<Test>d__0.MoveNext", source: source);
        }
+
+        [Fact]
+        public void ExpressionVariable_InFieldInitializer()


ExpressionVariable_InFieldInitializer [](start = 20, length = 37)

This change is likely to be affected by the change in #25441. Also, that PR probably covers all the scenarios covered by the new tests here. #Resolved

jcouv · 2018-03-13T16:57:28Z

Hold on, some tests are failing. I'll investigate. #Closed

AlekseyTs · 2018-03-13T17:00:15Z

@tmat Could you please review EnC tests? Are they sufficient to declare that EnC works as expected for the feature? #Resolved

jcouv · 2018-03-13T17:16:08Z

Adjusted the tests. It possible that I didn't restore properly on my home machine, so was using an older dependency for pdb->xml translation.
Although this is now consistent with neighboring tests, this still feels like a regression to me (the previous format seems better). But that's unrelated to this PR.

-    <file id=""1"" name="""" language=""C#"" />
+    <file id=""1"" name="""" language=""3f5162f8-07c6-11d3-9053-00c04fa302a1"" languageVendor=""994b45c4-e6e9-11d2-903f-00c04fa302a1"" documentType=""5a869d0b-6611-11d3-bd2a-0000f80849bd"" />

AlekseyTs · 2018-03-13T18:51:57Z

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

+            var n2 = compilation2.GetMember<MethodSymbol>("Program.N");
+
+            var v0 = CompileAndVerify(compilation0);
+            v0.VerifyIL("Program.N()", @"


Program.N() [](start = 25, length = 11)

I think we should also examine IL for the selector lambda that was modified. #Closed

AlekseyTs · 2018-03-13T18:53:10Z

Done with review pass (iteration 2) #Resolved

AlekseyTs · 2018-03-13T21:52:11Z

@tmat Could you please review EnC tests? Are they sufficient to declare that EnC works as expected for the feature? #Resolved

tmat · 2018-03-14T17:53:58Z

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

+        }
+
+        [Fact]
+        public void ExpressionVariable_InConstructorInitializer()


ExpressionVariable_InConstructorInitializer [](start = 20, length = 43)

There is already test public void OutVar() that's similar. Could you rename it to ExpressionVariable_InStatement or something like that and move these tests close together? #Pending

tmat · 2018-03-14T18:01:06Z

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

+{
+  // Code size       53 (0x35)
+  .maxstack  4
+  .locals init ([unchanged] V_0,


[unchanged] V_0, [](start = 16, length = 16)

This indicates that the query variable was not reused. This is likely just because you're missing the markers <N:#> around the expression in var query = ... declaration.
Also, as Aleksey points out we should test how the lambda was updated. You'll need more markers to map the clauses of the query.
See http://source.roslyn.io/#Roslyn.Compilers.CSharp.Emit.UnitTests/Emit/EditAndContinue/EditAndContinueTests.cs,5338 for another LINQ EnC test. #Pending

tmat · 2018-03-14T18:01:39Z

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

+            select M(a, out int <N:0>x</N:0>) + x;
+    }
+}");
+             var compilation0 = CreateCompilation(source0.Tree, options: ComSafeDebugDll);


[](start = 12, length = 1)

Nit: extra space #Pending

tmat · 2018-03-14T18:03:10Z

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

+");
+        }
+
+        [Fact]


[Fact] [](start = 7, length = 7)

Are patterns in initializers implemented in a different branch? If they are in this branch we should add similar tests for patterns as well here. See PatternVariable_* tests above. #Pending

gafter

jcouv · 2018-03-14T22:50:34Z

@tmat for another look. Thanks

tmat · 2018-03-15T01:49:06Z

Aleksey's feedback doesn't seem to be addressed yet.

AlekseyTs · 2018-03-15T18:52:53Z

On another PR, @tmat also recommended to

Also https://github.com/dotnet/roslyn/blob/master/src/Compilers/CSharp/Test/Emit/Emit/EditAndContinue/EditAndContinueClosureTests.cs#L2356 verify that the members generated for the lambda and closures are correctly reused when edited.

I think he was talking about scenarios when declared Expression Variables are captured in lambdas.

jcouv · 2018-03-15T23:44:21Z

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

+                ImmutableArray.Create(
+                    new SemanticEdit(SemanticEditKind.Update, ctor1, ctor2, GetSyntaxMapFromMarkers(source1, source0), preserveLocalVariables: true)));
+
+            diff2.VerifySynthesizedMembers("C.<>c__DisplayClass0_0#1: {x, <.ctor>b__0}", "C.<>c__DisplayClass0_0#2: {x, <.ctor>b__0}", "C: {<>c__DisplayClass0_0#2, <>c__DisplayClass0_0#1}");


@AlekseyTs I'll add the other two variants of this test (in field and in query) tonight.

I don't fully understand the test helpers, but I feel like this is not what we expect: new synthesized types and members get added with new edits... I'll let @tmat confirm what's expected.

but I feel like this is not what we expect

Make sure to use latest from the feature branch, just in case. My latest changes affect PDB

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

Ok, I'll rebase the PR to latest branch bits.

I've rebased, but the tests were not affected.

AlekseyTs

LGTM (iteration 7). Thanks!

jcouv · 2018-03-20T03:54:21Z

Cool
@tmat for review. Thanks

jcouv · 2018-03-20T20:47:43Z

@tmat for review. Thanks

jcouv · 2018-03-20T21:32:41Z

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

+                ImmutableArray.Create(
+                    new SemanticEdit(SemanticEditKind.Update, ctor0, ctor1, GetSyntaxMapFromMarkers(source0, source1), preserveLocalVariables: true)));
+
+            diff1.VerifySynthesizedMembers("C: {<>c__DisplayClass0_0#1}", "C.<>c__DisplayClass0_0#1: {x, <Method>b__0}");


Tomas said the #1 indicates a new class was generated, which is unexpected.
It might be an issue with the test. I'll investigate (he said to debug EncVariableSlotAllocator.TryGetPreviousClosure to figure this out)

tmat · 2018-03-21T21:59:31Z

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

@@ -22,6 +22,9 @@

 namespace Microsoft.CodeAnalysis.CSharp.EditAndContinue.UnitTests
 {
+    /// <summary>
+    /// Tip: debug EncVariableSlotAllocator.TryGetPreviousClosure to figure out missing markers in your test.


TryGetPreviousClosure [](start = 44, length = 21)

... or other TryGet methods depending on what matching is being done.

jcouv · 2018-03-21T23:27:11Z

Thanks!
I went ahead and merged since test-only changes don't need ask-mode approval.

jcouv added the Area-Compilers label Mar 13, 2018

jcouv added this to the 15.7 milestone Mar 13, 2018

jcouv self-assigned this Mar 13, 2018

jcouv requested a review from a team as a code owner March 13, 2018 04:40

AlekseyTs reviewed Mar 13, 2018

View reviewed changes

jcouv added the Test Test failures in roslyn-CI label Mar 14, 2018

gafter self-assigned this Mar 14, 2018

tmat reviewed Mar 14, 2018

View reviewed changes

gafter approved these changes Mar 14, 2018

View reviewed changes

gafter removed their assignment Mar 14, 2018

jcouv commented Mar 15, 2018

View reviewed changes

gafter mentioned this pull request Mar 16, 2018

Test plan for ExpressionVariables #25073

Closed

44 tasks

jcouv added 6 commits March 16, 2018 00:23

Test EnC on expression variables

f531f6a

Adjust tests

4aa1435

Remove PDB tests

ca7d368

Address PR feedback from Tomas

33b8b69

Address PR feedback from Aleksey

3ad9869

Add test with lambdas capturing out var

e551523

jcouv force-pushed the exprvar-enc branch from 8294d50 to e551523 Compare March 16, 2018 07:35

Add remaining tests with lambdas

6194855

jcouv changed the base branch from features/ExpressionVariables to dev15.7.x March 17, 2018 19:00

AlekseyTs approved these changes Mar 19, 2018

View reviewed changes

jcouv commented Mar 20, 2018

View reviewed changes

Add missing markers

f1a93b8

tmat reviewed Mar 21, 2018

View reviewed changes

tmat approved these changes Mar 21, 2018

View reviewed changes

Update comment

ea87eb7

jcouv merged commit 86067f2 into dotnet:dev15.7.x Mar 21, 2018

jcouv deleted the exprvar-enc branch March 21, 2018 23:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test EnC on expression variables in initializers and queries #25438

Test EnC on expression variables in initializers and queries #25438

jcouv commented Mar 13, 2018

AlekseyTs Mar 13, 2018 •

edited

Loading

jcouv commented Mar 13, 2018 •

edited

Loading

AlekseyTs commented Mar 13, 2018 •

edited by jcouv

Loading

jcouv commented Mar 13, 2018

AlekseyTs Mar 13, 2018 •

edited

Loading

AlekseyTs commented Mar 13, 2018 •

edited by jcouv

Loading

AlekseyTs commented Mar 13, 2018 •

edited by jcouv

Loading

tmat Mar 14, 2018 •

edited by jcouv

Loading

tmat Mar 14, 2018 •

edited by jcouv

Loading

tmat Mar 14, 2018 •

edited by jcouv

Loading

tmat Mar 14, 2018 •

edited by jcouv

Loading

gafter left a comment

jcouv commented Mar 14, 2018

tmat commented Mar 15, 2018

AlekseyTs commented Mar 15, 2018

jcouv Mar 15, 2018

AlekseyTs Mar 16, 2018

jcouv Mar 16, 2018

jcouv Mar 16, 2018

AlekseyTs left a comment

jcouv commented Mar 20, 2018

jcouv commented Mar 20, 2018

jcouv Mar 20, 2018

tmat Mar 21, 2018

jcouv commented Mar 21, 2018

+              ");
+                      }
+                      [Fact]

Test EnC on expression variables in initializers and queries #25438

Test EnC on expression variables in initializers and queries #25438

Conversation

jcouv commented Mar 13, 2018

AlekseyTs Mar 13, 2018 • edited Loading

Choose a reason for hiding this comment

jcouv commented Mar 13, 2018 • edited Loading

AlekseyTs commented Mar 13, 2018 • edited by jcouv Loading

jcouv commented Mar 13, 2018

AlekseyTs Mar 13, 2018 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Mar 13, 2018 • edited by jcouv Loading

AlekseyTs commented Mar 13, 2018 • edited by jcouv Loading

tmat Mar 14, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

tmat Mar 14, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

tmat Mar 14, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

tmat Mar 14, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

gafter left a comment

Choose a reason for hiding this comment

jcouv commented Mar 14, 2018

tmat commented Mar 15, 2018

AlekseyTs commented Mar 15, 2018

jcouv Mar 15, 2018

Choose a reason for hiding this comment

AlekseyTs Mar 16, 2018

Choose a reason for hiding this comment

jcouv Mar 16, 2018

Choose a reason for hiding this comment

jcouv Mar 16, 2018

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

jcouv commented Mar 20, 2018

jcouv commented Mar 20, 2018

jcouv Mar 20, 2018

Choose a reason for hiding this comment

tmat Mar 21, 2018

Choose a reason for hiding this comment

jcouv commented Mar 21, 2018

AlekseyTs Mar 13, 2018 •

edited

Loading

jcouv commented Mar 13, 2018 •

edited

Loading

AlekseyTs commented Mar 13, 2018 •

edited by jcouv

Loading

AlekseyTs Mar 13, 2018 •

edited

Loading

AlekseyTs commented Mar 13, 2018 •

edited by jcouv

Loading

AlekseyTs commented Mar 13, 2018 •

edited by jcouv

Loading

tmat Mar 14, 2018 •

edited by jcouv

Loading

tmat Mar 14, 2018 •

edited by jcouv

Loading

tmat Mar 14, 2018 •

edited by jcouv

Loading

tmat Mar 14, 2018 •

edited by jcouv

Loading