Refactoring on inferred tuple names and anonymous type members (VB) #18930

jcouv · 2017-04-23T03:43:40Z

Same change as #18726, but for VB.
There are some minor differences: (1) the syntax for anonymous types is a bit different in VB, (2) the identifier comparisons are case-insensitive, and (3) there is no deconstruction in VB.

Note that there is an existing issue with the semantic model on the name of tuple elements in VB (#16697). For instance, in (i:=i, 2), both i's are treated as references to the local called i.
So some InlineTemporary tests are skipped for now.

jcouv · 2017-04-23T03:46:42Z

src/EditorFeatures/VisualBasicTest/CodeActions/IntroduceVariable/IntroduceVariableTests.vb

@@ -1827,7 +1827,7 @@ Class C
        Const {|Rename:V|} As Integer = 5
        Console.WriteLine(V)
 #End ExternalSource
-    End Sub
+   End Sub


Note: This change occurred when I moved complify earlier in the IntroduceVariable logic. I didn't investigate in details.

jcouv · 2017-04-23T03:48:06Z

src/Features/Core/Portable/IntroduceVariable/AbstractIntroduceVariableService.cs

@@ -392,5 +393,50 @@ where NodeMatchesExpression(originalSemanticModel, currentSemanticModel, syntaxF
                                                       .Where(p => p.ContainingSymbol.IsAnonymousFunction());
            return anonymousMethodParameters;
        }
+
+        protected static async Task<(SemanticDocument newSemanticDocument, ISet<TExpressionSyntax> newMatches)> ComplexifyParentingStatements(


Note: I pulled this method up from the C# derived class. The only change is that ExpressionSyntax was made generic, using TExpressionSyntax. #Resolved

nice! #Resolved

jcouv · 2017-04-23T03:50:28Z

...VisualBasic/Portable/IntroduceVariable/VisualBasicIntroduceVariableService_IntroduceLocal.vb


            Dim oldOutermostBlock = container
            If oldOutermostBlock.IsSingleLineExecutableBlock() Then
                oldOutermostBlock = oldOutermostBlock.Parent
            End If

            Dim matches = FindMatches(document, expression, document, oldOutermostBlock, allOccurrences, cancellationToken)
+
+            Dim complexified = Await ComplexifyParentingStatements(document, matches, cancellationToken).ConfigureAwait(False)


Note: Before the change, the complexification was happening after the local was introduced. But that is too late, as we need the original syntax to make names explicit in tuples and anonymous types.
The new flow mirrors the C# implementation of IntroduceVariable. #Resolved

I don't recall why this was done this way in VB. However, matching the flow of C# makes sense to me. #Resolved

jcouv · 2017-04-24T03:20:04Z

@CyrusNajmabadi I'm thinking about what is required to merge the inferred tuple names branch back into master. The crash with IntroduceVariable is probably the biggest issue, but it's also not new.
I'm wondering if I could mitigate the issue until the larger issue with semantic model on tuple element names in VB is fixed. Maybe blocking the refactoring or putting a special case (the name of a tuple element doesn't contain expressions to be extracted into variables)?

CyrusNajmabadi · 2017-04-24T03:21:51Z

Sorry, i'm not sure what this is referring to:

The crash with IntroduceVariable is probably the biggest issue

jcouv · 2017-04-24T03:35:54Z

@CyrusNajmabadi More context:
A number of IntroduceVariable tests that I added in this PR are skipped. IntroduceVariable looks for the expression to extract (let's say i) and replaces it with the parenthesized local (let's say (local)), whenever i is found and matches what you're trying to extract.
The problem is that the semantic model for tuple element names is busted. So the symbol for i in (i:=1, 2) is found to be a local of the same name (assuming it exists). So the rewriter will attempt to produce ((local):=1, 2) which does not fit the syntax model (crash).

CyrusNajmabadi · 2017-04-24T03:37:50Z

So the rewriter will attempt to produce ((local):=1, 2) which does not fit the syntax model (crash).

Can you just fix that?

jcouv · 2017-04-24T04:02:56Z

I should be able to. The only concern is that such fix will become irrelevant once the semantic model is fixed.
I'll give it a shot. Thanks

jcouv · 2017-04-24T17:27:44Z

@CyrusNajmabadi That worked. I was debating to put a comment to remove that VisitNameColonEquals method once the semantic model is fixed, but I think that special case is reasonable to leave permanently.

@DustinCampbell @dotnet/roslyn-ide This PR is ready for review. You will find it very similar to the last one (for C#). Thanks!

jcouv · 2017-04-25T18:04:41Z

@DustinCampbell @dotnet/roslyn-ide This PR is ready for review. Thanks.
I'm aiming to merge the tuple-names feature branch back into master on Thursday.

DustinCampbell · 2017-04-26T13:06:49Z

src/EditorFeatures/VisualBasicTest/CodeActions/ExtractMethod/ExtractMethodTests.vb

+    Sub Main()
+        Dim a = 1
+        Dim t = {|Rename:GetT|}(a)
+        System.Console.Write(t.a)


Does this code compile, since the tuple returned by GetT does not have a as a member? #Resolved

It won't compile. The refactoring uses VB 15.3 feature, but the project is only VB 15.
My understanding is there is a broader issue to allow making refactorings sensitive to language version. #Resolved

DustinCampbell · 2017-04-26T13:12:23Z

...VisualBasic/Portable/IntroduceVariable/VisualBasicIntroduceVariableService_IntroduceLocal.vb


            Dim oldOutermostBlock = container
            If oldOutermostBlock.IsSingleLineExecutableBlock() Then
                oldOutermostBlock = oldOutermostBlock.Parent
            End If

            Dim matches = FindMatches(document, expression, document, oldOutermostBlock, allOccurrences, cancellationToken)
+
+            Dim complexified = Await ComplexifyParentingStatements(document, matches, cancellationToken).ConfigureAwait(False)


I don't recall why this was done this way in VB. However, matching the flow of C# makes sense to me. #Resolved

DustinCampbell · 2017-04-26T13:14:22Z

...ualBasic/Portable/Simplification/AbstractVisualBasicSimplifier.AbstractExpressionRewriter.vb

@@ -122,7 +139,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Simplification
                Return newToken
            End Function

-            Protected Function SimplifyExpression(Of TExpression As ExpressionSyntax)(
+            Protected Function SimplifyExpression(Of TExpression As SyntaxNode)(


Why was this necessary? It seems a bit weird given the method name, the type parameter name, and the parameter name, expression.

See corresponding C# implementation of SimplifyExpression. Notice that TExpression is constrained to SyntaxNode.

Specifically, the new Reducer (VisualBasicInferredMemberNameReducer.Rewriter) needs to simplify a SimpleArgumentSyntax (for tuples) and a NamedFieldInitializerSyntax (for anonymous types). Copied relevant code below:

Public Overrides Function VisitSimpleArgument(node As SimpleArgumentSyntax) As SyntaxNode CancellationToken.ThrowIfCancellationRequested() Dim newNode = MyBase.VisitSimpleArgument(node) If node.IsParentKind(SyntaxKind.TupleExpression) Then Return SimplifyExpression( node, newNode:=newNode, simplifier:=AddressOf SimplifyTupleName) End If Return newNode End Function Public Overrides Function VisitNamedFieldInitializer(node As NamedFieldInitializerSyntax) As SyntaxNode Dim newNode = MyBase.VisitNamedFieldInitializer(node) Return SimplifyExpression( node, newNode:=newNode, simplifier:=AddressOf SimplifyNamedFieldInitializer) End Function End Class

Then we really need to change the name. SimplifyExpression means to me that it will be getting an expression. I have no problem with you calling this SimplifyExpressionOrSimpleArgumentOrFieldInitializer. Or just having three helpers that each handle a single type.

jcouv · 2017-04-26T17:20:40Z

Thanks @DustinCampbell Do I need to get a second review?

DustinCampbell · 2017-04-26T17:23:43Z

I'm good with it. @CyrusNajmabadi, did you want to take a look?

CyrusNajmabadi · 2017-04-26T19:23:43Z

...CodeRefactorings/InlineTemporary/InlineTemporaryCodeRefactoringProvider.ReferenceRewriter.vb

+                If node.IsParentKind(SyntaxKind.SimpleArgument) AndAlso
+                    node.Parent.IsParentKind(SyntaxKind.TupleExpression) Then
+
+                    Return node


this needs documentation as to why you are explicitly not changing anything in this situatoin. #Resolved

CyrusNajmabadi · 2017-04-26T19:24:29Z

...ualBasic/Portable/Simplification/AbstractVisualBasicSimplifier.AbstractExpressionRewriter.vb

+                    Return node.Parent
+                End If
+
+                Return Nothing


this whole method needs commenting to explain what's going on. #Resolved

So, for this one, I'm not really sure. I'll talk to Dustin. #Resolved

Removed this method.

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

CyrusNajmabadi · 2017-04-26T19:26:19Z

...kspaces/VisualBasic/Portable/Simplification/VisualBasicInferredMemberNameReducer.Rewriter.vb

+                    newNode:=newNode,
+                    simplifier:=AddressOf SimplifyNamedFieldInitializer)
+            End Function
+


nit: in general, i do not like extraneous newline between the 'end' statements of VB constructs.

CyrusNajmabadi · 2017-04-26T19:26:28Z

src/Workspaces/VisualBasic/Portable/Simplification/VisualBasicInferredMemberNameReducer.vb

+        ) As SimpleArgumentSyntax
+
+            If node.NameColonEquals Is Nothing OrElse Not node.IsParentKind(SyntaxKind.TupleExpression) Then
+                Return node


needs comments.

jcouv · 2017-04-26T20:50:15Z

@dotnet/roslyn-compiler This is the corresponding change for VB to public API change I made last week. It adds TryGetInferredMemberName and IsReservedTupleElementName.

jasonmalinowski

Did we get tests for renaming the field being captured by a tuple creation? We've (still 😦) blocked that for anonymous types and a few other places (@dpoeschl knows more), and we might have problems here too.

jasonmalinowski · 2017-04-27T00:57:51Z

src/Compilers/VisualBasic/Portable/Syntax/SyntaxExtensions.vb

+        <Extension>
+        Public Function TryGetInferredMemberName(expression As ExpressionSyntax) As String
+            Dim ignore As XmlNameSyntax = Nothing
+            Dim nameToken As SyntaxToken = expression.ExtractAnonymousTypeMemberName(ignore)


This will null ref is expression is null.

Added a null check for safety.

jasonmalinowski · 2017-04-27T00:58:56Z

src/Compilers/VisualBasic/Portable/Syntax/SyntaxFacts.vb

@@ -1171,5 +1172,17 @@ Namespace Microsoft.CodeAnalysis.VisualBasic

            Return False
        End Function
+
+        ''' <summary>
+        ''' Checks whether the element name Is reserved.


Not sure why "Is" is capitalized.

Copy and paste from C#, then VB auto-formatting. I'll fix.

jasonmalinowski · 2017-04-27T01:01:33Z

src/EditorFeatures/VisualBasicTest/CodeActions/ExtractMethod/ExtractMethodTests.vb

+Namespace System
+    Structure ValueTuple(Of T1, T2)
+    End Structure
+End Namespace", TestOptions.Regular.WithLanguageVersion(LanguageVersion.VisualBasic15))


Why aren't we testing with 15.3 then? To be clear, you're OK doing a blanket upgrade of TestOptions.Regular to something higher.

See TestTupleWithInferredNames above. It uses VB15.3.

jasonmalinowski · 2017-04-27T01:03:44Z

...CodeRefactorings/InlineTemporary/InlineTemporaryCodeRefactoringProvider.ReferenceRewriter.vb

+                    node.Parent.IsParentKind(SyntaxKind.TupleExpression) Then
+
+                    ' Temporaries should not be inlined in the name portion of a named tuple element
+                    ' This special case should be removed once https://github.com/dotnet/roslyn/issues/16697 is fixed


Is fixing #16697 on the docket for this work?

jasonmalinowski · 2017-04-27T01:07:33Z

@jcouv, @VSadov, @rchande: you might want to take a look at the integration test failures here in this run. It might be flaky results, but might also indicate some new issues in the branch?

VSadov · 2017-04-27T01:17:20Z

BasicReferenceHighlighting.Highlighting failed in both Debug and Release so could be related to the changes.
Other failures are more likely to be just flaky tests

cston · 2017-04-27T04:24:39Z

src/Compilers/VisualBasic/Portable/Syntax/SyntaxFacts.vb

+        ''' Checks whether the element name is reserved.
+        '''
+        ''' For example:
+        ''' "Item3" is reserved (at certain positions).


What does at certain positions refer to? It looks like "Item3" will always return True.

I'll update the comment. This refers to previous shape of the API (which was returning a position).

cston · 2017-04-27T04:29:54Z

src/Compilers/VisualBasic/Test/Syntax/Syntax/SyntaxFactsTest.vb

+    <InlineData("M()", "M")>
+    <InlineData("x.M()", "M")>
+    <InlineData("TypeOf(x)", Nothing)>
+    <InlineData("GetType(x)", Nothing)>


Perhaps test: "Me", "[Me]", "x.Me", "x!y", "-x", "M()()", "New C()"

cston · 2017-04-27T04:31:08Z

src/Compilers/VisualBasic/Test/Syntax/Syntax/SyntaxFactsTest.vb

+    <InlineData("GetHashCode", True)>
+    <InlineData("item1", True)>
+    <InlineData("item10", True)>
+    <InlineData("Alice", False)>


cston · 2017-04-27T14:37:24Z

Compiler changes LGTM

jaredpar · 2017-04-27T15:46:53Z

APi changes look good.

Refactoring on inferred tuple names (VB)

8caa0d0

dnfclas added the cla-already-signed label Apr 23, 2017

jcouv commented Apr 23, 2017

View reviewed changes

jcouv self-assigned this Apr 23, 2017

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Apr 24, 2017

Enable skipped tests

63f0414

jcouv removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Apr 24, 2017

DustinCampbell approved these changes Apr 26, 2017

View reviewed changes

CyrusNajmabadi reviewed Apr 26, 2017

View reviewed changes

jcouv force-pushed the tuple-names-refactoring branch from 74122f5 to 47987fb Compare April 26, 2017 20:53

Address PR feedback from Cyrus

304b908

jcouv force-pushed the tuple-names-refactoring branch from 47987fb to 304b908 Compare April 26, 2017 23:16

jasonmalinowski approved these changes Apr 27, 2017

View reviewed changes

Addressing PR feedback from Jason

cd36d02

cston reviewed Apr 27, 2017

View reviewed changes

Address PR feedback from Chuck

8aa78eb

jcouv merged commit 3069ba1 into dotnet:features/tuple-names Apr 27, 2017

jcouv deleted the tuple-names-refactoring branch April 27, 2017 15:50

Refactoring on inferred tuple names and anonymous type members (VB) #18930

Refactoring on inferred tuple names and anonymous type members (VB) #18930

Conversation

jcouv commented Apr 23, 2017 • edited Loading

jcouv Apr 23, 2017 • edited Loading

Choose a reason for hiding this comment

jcouv Apr 23, 2017 • edited Loading

Choose a reason for hiding this comment

CyrusNajmabadi Apr 26, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Apr 23, 2017 • edited Loading

Choose a reason for hiding this comment

DustinCampbell Apr 26, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv commented Apr 24, 2017

CyrusNajmabadi commented Apr 24, 2017

jcouv commented Apr 24, 2017

CyrusNajmabadi commented Apr 24, 2017

jcouv commented Apr 24, 2017

jcouv commented Apr 24, 2017

jcouv commented Apr 25, 2017

DustinCampbell Apr 26, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Apr 26, 2017 • edited Loading

Choose a reason for hiding this comment

DustinCampbell Apr 26, 2017 • edited by jcouv 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

jcouv commented Apr 26, 2017

DustinCampbell commented Apr 26, 2017

CyrusNajmabadi Apr 26, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Apr 26, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Apr 26, 2017 • edited 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

jcouv commented Apr 26, 2017

jasonmalinowski left a comment

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

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

Choose a reason for hiding this comment

jasonmalinowski commented Apr 27, 2017

VSadov commented Apr 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston commented Apr 27, 2017

jaredpar commented Apr 27, 2017

jcouv commented Apr 23, 2017 •

edited

Loading

jcouv Apr 23, 2017 •

edited

Loading

jcouv Apr 23, 2017 •

edited

Loading

CyrusNajmabadi Apr 26, 2017 •

edited by jcouv

Loading

jcouv Apr 23, 2017 •

edited

Loading

DustinCampbell Apr 26, 2017 •

edited by jcouv

Loading

DustinCampbell Apr 26, 2017 •

edited by jcouv

Loading

jcouv Apr 26, 2017 •

edited

Loading

DustinCampbell Apr 26, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Apr 26, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Apr 26, 2017 •

edited by jcouv

Loading

jcouv Apr 26, 2017 •

edited

Loading

cston Apr 27, 2017 •

edited

Loading