Disable EnC for active methods with v7 syntax and for any method with v7 switch statement #13754

jcouv · 2016-09-13T03:37:39Z

Adding a syntactic check for any v7 syntax around active statement during EnC. This is modeled after a similar check in VB for "on error or resume" statement.
Also adding a semantic check for v7 switch statement (looking at governing type of the switch).

Addresses some of the work items in #12379

@gafter I think it would be good to re-open your PR (#13498) with public API for checking v7 switch statement. I copied that method to unblock myself for now.

Since tuples and ref shouldn't affect EnC slots, I'm thinking to allow them. Any thoughts?
If this change looks good, I will do a similar change for VB.

@dotnet/roslyn-compiler for review.

gafter · 2016-09-13T16:52:54Z

Re

@gafter I think it would be good to re-open your PR (#13498) with public API for checking v7 switch statement. I copied that method to unblock myself for now.

There would only be a single client for that API - this code. We prefer to see at least two and preferably three potential clients to add a public API.

gafter · 2016-09-13T16:54:18Z

src/Compilers/CSharp/Portable/PublicAPI.Unshipped.txt

@@ -268,6 +268,7 @@ override Microsoft.CodeAnalysis.CSharp.Syntax.WhenClauseSyntax.Accept<TResult>(M
 static Microsoft.CodeAnalysis.CSharp.CSharpExtensions.GetDeclaredSymbol(this Microsoft.CodeAnalysis.SemanticModel semanticModel, Microsoft.CodeAnalysis.CSharp.Syntax.DeclarationPatternSyntax declarationSyntax, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) -> Microsoft.CodeAnalysis.ILocalSymbol
 static Microsoft.CodeAnalysis.CSharp.CSharpExtensions.GetDeclaredSymbol(this Microsoft.CodeAnalysis.SemanticModel semanticModel, Microsoft.CodeAnalysis.CSharp.Syntax.SingleVariableDesignationSyntax designationSyntax, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) -> Microsoft.CodeAnalysis.ISymbol
 static Microsoft.CodeAnalysis.CSharp.CSharpExtensions.GetForEachStatementInfo(this Microsoft.CodeAnalysis.SemanticModel semanticModel, Microsoft.CodeAnalysis.CSharp.Syntax.CommonForEachStatementSyntax forEachStatement) -> Microsoft.CodeAnalysis.CSharp.ForEachStatementInfo
+static Microsoft.CodeAnalysis.CSharp.CSharpExtensions.IsValidV6SwitchGoverningType(this Microsoft.CodeAnalysis.ITypeSymbol type) -> bool


IsValidV6SwitchGoverningType [](start = 54, length = 28)

Please do not add this public API. Instead use the code from #13334 (comment)

That works too. Thanks

gafter · 2016-09-13T17:02:11Z

Should this method be extended to handle new node kinds?

Refers to: src/Features/CSharp/Portable/EditAndContinue/StatementSyntaxComparer.cs:572 in 2b29f9a. [](commit_id = 2b29f9a7070131fb066bb9cac769ecb2e455f0f1, deletion_comment = False)

AlekseyTs · 2016-09-13T17:10:41Z

CC @dotnet/roslyn-interactive

AlekseyTs · 2016-09-13T17:13:52Z

src/Features/CSharp/Portable/CSharpFeaturesResources.resx

+    <comment>{Locked="ref"} "ref" is a C# keyword and should not be localized.</comment>
+  </data>
+  <data name="v7_switch" xml:space="preserve">
+    <value>v7 switch</value>


v7 [](start = 11, length = 2)

Should we find a friendlier description?

Updated to "C# 7 enhanced switch statement".
When I tried EnC in VS, I was not able to observe the error message or the diagnostics, except for an error popup with a generic message (there was an error with this edit & continue) and squiggles on the span. I didn't see anything in the error or output panes.
Do you know if the error message appears somewhere else?

AlekseyTs · 2016-09-13T17:16:10Z

        LocalDeclarationStatement,         // tied to parent

Should we add Deconstruction Declaration here?

Refers to: src/Features/CSharp/Portable/EditAndContinue/StatementSyntaxComparer.cs:192 in 2b29f9a. [](commit_id = 2b29f9a7070131fb066bb9cac769ecb2e455f0f1, deletion_comment = False)

AlekseyTs · 2016-09-13T17:23:09Z

src/Features/CSharp/Portable/EditAndContinue/CSharpEditAndContinueAnalyzer.cs

+            switch (n.Kind())
+            {
+                case SyntaxKind.IsPatternExpression:
+                case SyntaxKind.DeconstructionDeclarationStatement:


DeconstructionDeclarationStatement [](start = 32, length = 34)

Should we also handle VariableComponentAssignmentSyntax?

Sure. That scenario was already caught by the tuples check, but this will make it safer to incrementally enable scenarios.

jcouv · 2016-09-13T18:42:31Z

@gafter @AlekseyTs Regarding StatementSyntaxComparer.cs (around line 192), I only made the smallest change here to unblock the scenarios.
So it's possible this code will need to be updated, but I don't yet know a scenario that needs it.

As far as I can tell, the reason the new d-foreach statement reached this (the unreachable default in method TryComputeWeightedDistance) is because it holds a block.
I'm planning to leave this comparer as-is for now and continue investigating if there are scenarios that need it.

jcouv · 2016-09-13T19:27:21Z

I changed the PR to point to the preview5 branch.

gafter · 2016-09-13T21:29:32Z

jcouv · 2016-09-14T01:42:15Z

@dotnet-bot retest vsi please

jcouv · 2016-09-14T01:42:42Z

@dotnet-bot test eta please

amcasey · 2016-09-14T02:14:13Z

src/Features/CSharp/Portable/EditAndContinue/CSharpEditAndContinueAnalyzer.cs

                case SyntaxKind.SelectClause:
                    return ((SelectClauseSyntax)node).SelectKeyword.Span;

                case SyntaxKind.GroupClause:
                    return ((GroupClauseSyntax)node).GroupKeyword.Span;

+                case SyntaxKind.ForEachComponentStatement:


Do we gain anything by grouping cases with the same body? Won't the compiler do that anyway? I'm guessing the old order was deliberate.

I could not discern any order and found it confusing to have them spread out. I'll revert that refactoring.

Undid the refactoring.

jaredpar · 2016-09-14T18:13:56Z

test eta please

jaredpar · 2016-09-14T18:26:34Z

test eta please

jcouv · 2016-09-14T18:30:24Z

@dotnet/roslyn-compiler for a second review. This is fairly urgent as preview-5 ask mode closes later today. Thanks

jcouv · 2016-09-14T20:05:04Z

@dotnet-bot test windows_vsi_p1_open_prtest please

jcouv added Area-Compilers Feature - Ref Locals and Returns Ref Locals and Returns Feature - Local Functions Local Functions Feature - Pattern Matching Pattern Matching Feature - Tuples Tuples Feature - Out Variable Declaration Out Variable Declaration labels Sep 13, 2016

jcouv added this to the 2.0 (RC) milestone Sep 13, 2016

jcouv assigned gafter and jcouv Sep 13, 2016

dnfclas added the cla-already-signed label Sep 13, 2016

jcouv force-pushed the v7-enc2 branch from 8d86d25 to 2b29f9a Compare September 13, 2016 16:38

gafter reviewed Sep 13, 2016
View reviewed changes

AlekseyTs reviewed Sep 13, 2016
View reviewed changes

jcouv force-pushed the v7-enc2 branch from 2b29f9a to 1bb4e0e Compare September 13, 2016 17:27

This was referenced Sep 13, 2016

Edit-and-continue needs to be disabled for foreach loops that contain out vars #12736

Closed

Disable EnC in presence of C# 7 switch statement #12380

Closed

jcouv changed the base branch from master to dev15-preview-5 September 13, 2016 19:26

jcouv force-pushed the v7-enc2 branch from 2d162ee to f50d4c9 Compare September 13, 2016 21:21

gafter removed their assignment Sep 13, 2016

jcouv added this to the 2.0 (Preview 5) milestone Sep 13, 2016

Pilchie added the Approved to merge label Sep 14, 2016

amcasey reviewed Sep 14, 2016
View reviewed changes

jcouv added 9 commits September 14, 2016 10:57

WIP for blocking EnC in active methods with C#7 features

47efc6b

WIP 2

deb7cb9

WIP 3

5457fd3

WIP 3

7993a14

v7 switch statement

3d18f0f

Undoing public API change

649e0ab

Add test with v7 switch

b818e01

Adding test

fafbbed

PR feedback (1)

0e044e2

jcouv force-pushed the v7-enc2 branch from f50d4c9 to 0e044e2 Compare September 14, 2016 17:59

PR feedback (2)

c040a3e

jcouv merged commit 24cafcb into dotnet:dev15-preview-5 Sep 14, 2016

jcouv deleted the v7-enc2 branch September 14, 2016 21:09

This was referenced Oct 7, 2016

Disable EnC in presence of new VB nodes #14363

Closed

Review tuple-related commits and port stragglers to VB #13938

Closed

miloush mentioned this pull request Feb 5, 2017

Edit and Continue for out variable declarations #16960

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable EnC for active methods with v7 syntax and for any method with v7 switch statement #13754

Disable EnC for active methods with v7 syntax and for any method with v7 switch statement #13754

jcouv commented Sep 13, 2016 •

edited

Loading

gafter commented Sep 13, 2016

gafter Sep 13, 2016

jcouv Sep 13, 2016

gafter commented Sep 13, 2016

AlekseyTs commented Sep 13, 2016

AlekseyTs Sep 13, 2016

jcouv Sep 13, 2016

AlekseyTs commented Sep 13, 2016

AlekseyTs Sep 13, 2016

jcouv Sep 13, 2016

jcouv commented Sep 13, 2016

jcouv commented Sep 13, 2016

gafter commented Sep 13, 2016

jcouv commented Sep 14, 2016

jcouv commented Sep 14, 2016

amcasey Sep 14, 2016

jcouv Sep 14, 2016

jcouv Sep 14, 2016

jaredpar commented Sep 14, 2016

jaredpar commented Sep 14, 2016

jcouv commented Sep 14, 2016

jcouv commented Sep 14, 2016

Disable EnC for active methods with v7 syntax and for any method with v7 switch statement #13754

Disable EnC for active methods with v7 syntax and for any method with v7 switch statement #13754

Conversation

jcouv commented Sep 13, 2016 • edited Loading

gafter commented Sep 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter commented Sep 13, 2016

AlekseyTs commented Sep 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Sep 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Sep 13, 2016

jcouv commented Sep 13, 2016

gafter commented Sep 13, 2016

jcouv commented Sep 14, 2016

jcouv commented Sep 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar commented Sep 14, 2016

jaredpar commented Sep 14, 2016

jcouv commented Sep 14, 2016

jcouv commented Sep 14, 2016

jcouv commented Sep 13, 2016 •

edited

Loading