Refactoring on inferred tuple names #18726

jcouv · 2017-04-15T07:08:18Z

@CyrusNajmabadi
I took a rough stab at this.
I'm not really sure about handling trivia and null parents.
This is not handling deconstructions yet (need some LDM input), or VB.
Relates to #18480

…them

CyrusNajmabadi · 2017-04-16T20:24:43Z

...orkspaces/CSharp/Portable/Simplification/AbstractCSharpReducer.AbstractExpressionRewriter.cs

@@ -46,6 +46,16 @@ private static SyntaxNode GetParentNode(SyntaxNode node)
                    return GetParentNode(cref);
                }

+                if (node.IsKind(SyntaxKind.Argument) && node.Parent.IsKind(SyntaxKind.TupleExpression))


arguments can be parented by tuple expressions? #Resolved

got it. i didn't realize you reused ArgumentSyntax here. nifty :)

CyrusNajmabadi · 2017-04-16T20:25:36Z

src/Workspaces/CSharp/Portable/Simplification/CSharpInferredMemberNameReducer.Rewriter.cs

+                return SimplifyExpression(
+                    node,
+                    newNode: base.VisitArgument(node),
+                    simplifier: SimplifyTupleName);


this is waht we do elsewhere, but these delegate allocations make me super sad :) #Resolved

I don't see the allocation. SimplifyTupleName is static.

Unless something changed and we are now caching static methods, then this will be an allocation. I was under the impression that we had not done the optimization to cache these yet.

this is waht we do elsewhere, but these delegate allocations make me super sad :)

Says the guy who lives for IEnumerable<T>.Do(...) extension methods. 😉

CyrusNajmabadi · 2017-04-16T20:26:14Z

src/Workspaces/CSharp/Portable/Simplification/CSharpInferredMemberNameReducer.cs

+        {
+            string inferredName = node.IsParentKind(SyntaxKind.TupleExpression) ?
+                CSharpSimplificationService.ExtractAnonymousTypeMemberName(node.Expression).ValueText :
+                null;


ide style is:

var inferredName = node.IsParentKind(SyntaxKind.TupleExpression) ? CSharpSimplificationService.ExtractAnonymousTypeMemberName(node.Expression).ValueText : null; ``` #Resolved

ExtractAnonymousTypeMemberName maybe "GetInferredTupleOrAnonymousTypeMemerName? #Resolved

The name of the method matches the method I copied from compiler. I'd rather keep the name (or avoid copying the code entirely).

Or just rename hte compiler method name :) But it's no biggie.

CyrusNajmabadi · 2017-04-16T20:27:31Z

src/Workspaces/CSharp/Portable/Simplification/CSharpInferredMemberNameReducer.cs

+                return node;
+            }
+
+            return node.WithNameColon(null);


probably needs to be node.WithNameColon(null).WithTriviaFrom(node). #Resolved

CyrusNajmabadi · 2017-04-16T20:28:18Z

src/Workspaces/CSharp/Portable/Simplification/CSharpInferredMemberNameReducer.cs

+        }
+
+        private static ArgumentSyntax SimplifyTupleName(ArgumentSyntax node, SemanticModel semanticModel, OptionSet optionSet, CancellationToken cancellationToken)
+        {


can we bail out immediately if the node doesn't even have an name-colon? #Resolved

CyrusNajmabadi · 2017-04-16T20:28:35Z

src/Workspaces/CSharp/Portable/Simplification/CSharpInferredMemberNameReducer.cs

+
+        private static SyntaxNode SimplifyAnonymousTypeMemberName(AnonymousObjectMemberDeclaratorSyntax node, SemanticModel semanticModel, OptionSet optionSet, CancellationToken canellationToken)
+        {
+            string inferredName = CSharpSimplificationService.ExtractAnonymousTypeMemberName(node.Expression).ValueText;


var #Resolved

CyrusNajmabadi · 2017-04-16T20:28:48Z

src/Workspaces/CSharp/Portable/Simplification/CSharpInferredMemberNameReducer.cs

+        {
+            string inferredName = CSharpSimplificationService.ExtractAnonymousTypeMemberName(node.Expression).ValueText;
+
+            if (inferredName == null || inferredName != node?.NameEquals?.Name.Identifier.ValueText)


if it can return null, it should be TryGetXXX #Resolved

CyrusNajmabadi · 2017-04-16T20:28:54Z

src/Workspaces/CSharp/Portable/Simplification/CSharpInferredMemberNameReducer.cs

+                return node;
+            }
+
+            return node.WithNameEquals(null);


same comment about trivia. #Resolved

CyrusNajmabadi · 2017-04-16T20:29:34Z

src/Workspaces/CSharp/Portable/Simplification/CSharpSimplificationService.Expander.cs

+                if (node.Parent.IsKind(SyntaxKind.TupleExpression))
+                {
+                    var tuple = (TupleExpressionSyntax)node.Parent;
+                    var nonDuplicateTupleNames = ExtractTupleNames(tuple);


is this allocating a list per argument, then checking each argument against that list? #Resolved

If so, i'd prefer a non-allocating way of doing all of this. #Resolved

I'm thinking of having the Expander cache those. I'd create a map from tuple to hashset of names. It will be lazily populated, and entries can be removed once the visitor is done with a tuple.
Does that sound ok?

so, i've been needing to go through and basically address the fact that the simplifier is a memory hog. So i'd prefer we remove unnecessary allocations. Having a set created for every tuple, which will likely never contain more than 4 items in it just seems super wasteful.

(and i actually believe just linearly scanning the tuple will be faster in practice). This ia case where big-O may say something, but actual practical constants impact you.

Based on your comment below, I'll change to no allocation.

CyrusNajmabadi · 2017-04-16T20:31:17Z

src/Workspaces/CSharp/Portable/Simplification/CSharpSimplificationService.Expander.cs

+                {
+                    var identifier = SyntaxFactory.Identifier(inferredName);
+                    identifier = TryEscapeIdentifierToken(identifier, node, _semanticModel);
+                    newArgument = newArgument.WithNameColon(SyntaxFactory.NameColon(SyntaxFactory.IdentifierName(identifier)))


we probably want to move the leading trivia of the argument to the name-colon. #Resolved

I added a test with trivia, but it seems to be attached to the open paren. I'm not sure how to get trivia attached to the first tuple element.

CyrusNajmabadi · 2017-04-16T20:32:29Z

src/Workspaces/CSharp/Portable/Simplification/CSharpSimplificationService.Expander.cs

@@ -271,6 +290,46 @@ public override SyntaxNode VisitArgument(ArgumentSyntax node)
                return newArgument;
            }

+            // PROTOTYPE(tuple-names) this can be memoized


i wouldn't even allocate/memoize here. tuples are going to be small enough that you can check if the name you care about shows up in the tuple exactly once quite cheaply. #Resolved

CyrusNajmabadi · 2017-04-16T20:32:53Z

src/Workspaces/CSharp/Portable/Simplification/CSharpSimplificationService.Expander.cs

+                    var identifier = SyntaxFactory.Identifier(inferredName);
+                    identifier = TryEscapeIdentifierToken(identifier, node, _semanticModel);
+                    newDeclarator = newDeclarator.WithNameEquals(SyntaxFactory.NameEquals(SyntaxFactory.IdentifierName(identifier)))
+                         .WithAdditionalAnnotations(Simplifier.Annotation);


same comment about triivia. #Resolved

CyrusNajmabadi · 2017-04-16T20:33:01Z

src/Workspaces/CSharp/Portable/Simplification/CSharpSimplificationService.Expander.cs

+                    newDeclarator = newDeclarator.WithNameEquals(SyntaxFactory.NameEquals(SyntaxFactory.IdentifierName(identifier)))
+                         .WithAdditionalAnnotations(Simplifier.Annotation);
+                }
+                return newDeclarator;


blank line after }

CyrusNajmabadi · 2017-04-21T20:33:49Z

src/Workspaces/CSharp/Portable/Simplification/CSharpSimplificationService.Expander.cs


                var newArgument = (ArgumentSyntax)base.VisitArgument(node);

+                if (inferredName != null && node.NameColon == null)


You only needed the inferred name if nameColon was not null. So you should move that outer code in here. (though likely a helper method would be good. but htat helper method should be called inside this if block.

CyrusNajmabadi · 2017-04-21T20:34:57Z

src/Workspaces/CSharp/Portable/Simplification/CSharpSimplificationService.Expander.cs

+                {
+                    var identifier = SyntaxFactory.Identifier(inferredName);
+                    identifier = TryEscapeIdentifierToken(identifier, node, _semanticModel);
+                    newArgument = newArgument.WithLeadingTrivia(SyntaxTriviaList.Empty)


i think there's a WithoutLeadingTrivia.

Also, consider just saying newArgument....WithTriviaFrom(newArgument).

CyrusNajmabadi · 2017-04-21T20:35:36Z

src/Workspaces/CSharp/Portable/Simplification/CSharpSimplificationService.Expander.cs

+                bool found = false;
+                foreach (var argument in tuple.Arguments)
+                {
+                    string elementName = null;


consider ?: expression here.

CyrusNajmabadi · 2017-04-21T20:35:55Z

src/Workspaces/CSharp/Portable/Simplification/CSharpSimplificationService.Expander.cs

+                    }
+                    else
+                    {
+                        elementName = argument.Expression.TryGetInferredMemberName().ValueText;


ValueText is safe on a default tuple?

CyrusNajmabadi · 2017-04-21T20:36:07Z

src/Workspaces/CSharp/Portable/Simplification/CSharpSimplificationService.Expander.cs

+                        }
+                        found = true;
+                    }
+


extra newline.

CyrusNajmabadi · 2017-04-21T20:36:36Z

src/Workspaces/CSharp/Portable/Simplification/CSharpSimplificationService.Expander.cs

+
+            public override SyntaxNode VisitAnonymousObjectMemberDeclarator(AnonymousObjectMemberDeclaratorSyntax node)
+            {
+                var inferredName = node.Expression.TryGetInferredMemberName().ValueText;


not needed until you verify that .NameEquals is null. move inside lower if block.

CyrusNajmabadi · 2017-04-21T20:36:46Z

src/Workspaces/CSharp/Portable/Simplification/CSharpSimplificationService.Expander.cs

+                        .WithLeadingTrivia(SyntaxTriviaList.Empty)
+                        .WithNameEquals(SyntaxFactory.NameEquals(SyntaxFactory.IdentifierName(identifier))
+                            .WithLeadingTrivia(node.GetLeadingTrivia()))
+                        .WithAdditionalAnnotations(Simplifier.Annotation);


same comment about trivia.

CyrusNajmabadi · 2017-04-21T20:39:00Z

LGTM. However, i had some tiny questions i'd like some clarifation on and some nits. #Resolved

CyrusNajmabadi · 2017-04-21T20:39:28Z

Also, will need @jaredpar signoff on the public API change. That said, i now like the new API and think it's valuable to have

CyrusNajmabadi · 2017-04-21T20:40:21Z

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

+static Microsoft.CodeAnalysis.CSharp.LanguageVersionFacts.TryParse(this string version, out Microsoft.CodeAnalysis.CSharp.LanguageVersion result) -> bool
+static Microsoft.CodeAnalysis.CSharp.SyntaxFacts.IsReservedTupleElementName(string elementName, out int index) -> bool


consider just exposing the form that doesn't return the index. THe index seems to be just internally by the compiler right now right? You only discard it on the IDE side. So really, all we're exposign is the generally useful concept of "is this name reserved inside a tuple".

CyrusNajmabadi · 2017-04-21T20:41:04Z

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

+static Microsoft.CodeAnalysis.CSharp.LanguageVersionFacts.TryParse(this string version, out Microsoft.CodeAnalysis.CSharp.LanguageVersion result) -> bool
+static Microsoft.CodeAnalysis.CSharp.SyntaxFacts.IsReservedTupleElementName(string elementName, out int index) -> bool
+static Microsoft.CodeAnalysis.CSharp.SyntaxFacts.TryGetInferredMemberName(this Microsoft.CodeAnalysis.CSharp.Syntax.ExpressionSyntax input) -> Microsoft.CodeAnalysis.SyntaxToken


I think you only ever use .ValueText on hte IDE side. As such, consider just exposing a method that returns the string value (which also fits the name a lot better).

VSadov

LGTM

CyrusNajmabadi · 2017-04-21T22:08:56Z

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

+static Microsoft.CodeAnalysis.CSharp.SyntaxFacts.IsReservedTupleElementName(string elementName) -> bool
+static Microsoft.CodeAnalysis.CSharp.SyntaxFacts.TryGetInferredMemberName(this Microsoft.CodeAnalysis.CSharp.Syntax.ExpressionSyntax input) -> string


nice. love the new APIs.

CyrusNajmabadi · 2017-04-21T22:10:19Z

src/Workspaces/CSharp/Portable/Simplification/CSharpSimplificationService.Expander.cs

@@ -311,7 +306,7 @@ private static bool CanMakeNameExplicitInTuple(TupleExpressionSyntax tuple, stri
                    }
                    else
                    {
-                        elementName = argument.Expression.TryGetInferredMemberName().ValueText;
+                        elementName = argument.Expression?.TryGetInferredMemberName();


dont' think expression can be null. but i don't mind being careful.

CyrusNajmabadi · 2017-04-21T22:11:24Z

src/Workspaces/CSharp/Portable/Simplification/CSharpSimplificationService.cs

+                    return false;
+                }
+            }
+            while (true);


consdier making this "while (parent != null)". Consider moving the return false out of hte do-while.

CyrusNajmabadi · 2017-04-21T22:11:34Z

CyrusNajmabadi · 2017-04-21T22:12:11Z

@DustinCampbell I feel that your concerns have been well addressed by julien. Are you ok with him checking in at this point? Or do you want to rereview?

Thanks!

DustinCampbell

Looks much better. Thanks for taking the extra time to get things right.

CyrusNajmabadi · 2017-04-21T22:22:57Z

src/Workspaces/CSharp/Portable/Simplification/CSharpSimplificationService.cs

-                {
-                    return false;
-                }
+                parent = tuple.IsKind(SyntaxKind.TupleExpression) ? parent.Parent.Parent : null;


i just realized this seems wrong. ```a = (b, c).d" would say that it's a tuple deconstruction withthis logic right? i think you want to verify that parent.Parent is an ArgumentSyntax.

Or, at least explain what you're doing parent.parent.parent in terms of what yuo're trying to skip over.

Good catch. Thanks!

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

jcouv added the Area-Compilers label Apr 15, 2017

jcouv self-assigned this Apr 15, 2017

jcouv added this to the 15.3 milestone Apr 15, 2017

jcouv force-pushed the tuple-names-refactoring branch from 022fa8a to 43d94a4 Compare April 15, 2017 07:10

jcouv and others added 4 commits April 15, 2017 09:39

Add test and high-level design for refactoring on tuple inferred names

269107d

Complexify makes tuple inferred names explicit, and simplify removes …

d5ea871

…them

Do the same for inferred names of anonymous type members

fb60246

Missing files

653f56d

jcouv force-pushed the tuple-names-refactoring branch 2 times, most recently from 574ca5f to 5f49619 Compare April 15, 2017 18:43

Fix escaping problem. Add tests for duplicate names

e37d34c

jcouv force-pushed the tuple-names-refactoring branch 2 times, most recently from 28857bf to 2d2e4f1 Compare April 16, 2017 17:02

Exclude duplicate tuple names when complexifying

7cb9a9d

jcouv force-pushed the tuple-names-refactoring branch from 2d2e4f1 to 7cb9a9d Compare April 16, 2017 17:07

Add test for back compat break

47eb9e0

CyrusNajmabadi reviewed Apr 16, 2017

View reviewed changes

CyrusNajmabadi reviewed Apr 21, 2017

View reviewed changes

jcouv force-pushed the tuple-names-refactoring branch 2 times, most recently from fb0202f to 28e957d Compare April 21, 2017 21:43

VSadov approved these changes Apr 21, 2017

View reviewed changes

Address PR feedback from Cyrus

cf698b2

jcouv force-pushed the tuple-names-refactoring branch from 28e957d to cf698b2 Compare April 21, 2017 22:04

CyrusNajmabadi reviewed Apr 21, 2017

View reviewed changes

DustinCampbell approved these changes Apr 21, 2017

View reviewed changes

Simplify IsTupleInDeconstruction method

a5bbc2b

CyrusNajmabadi reviewed Apr 21, 2017

View reviewed changes

Fix bug with IsTupleInDeconstruction

d612db7

jcouv merged this pull request into dotnet:features/tuple-names Apr 21, 2017

jcouv deleted the tuple-names-refactoring branch April 21, 2017 23:58

jcouv mentioned this pull request Apr 23, 2017

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

Merged


		var newArgument = (ArgumentSyntax)base.VisitArgument(node);

		if (inferredName != null && node.NameColon == null)

		static Microsoft.CodeAnalysis.CSharp.LanguageVersionFacts.TryParse(this string version, out Microsoft.CodeAnalysis.CSharp.LanguageVersion result) -> bool
		static Microsoft.CodeAnalysis.CSharp.SyntaxFacts.IsReservedTupleElementName(string elementName, out int index) -> bool

Refactoring on inferred tuple names #18726

Refactoring on inferred tuple names #18726

Conversation

jcouv commented Apr 15, 2017 • edited Loading

CyrusNajmabadi Apr 16, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Apr 16, 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

CyrusNajmabadi Apr 16, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Apr 16, 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

CyrusNajmabadi Apr 16, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Apr 16, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Apr 16, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Apr 16, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Apr 16, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Apr 16, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Apr 16, 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

Choose a reason for hiding this comment

CyrusNajmabadi Apr 16, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Apr 16, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Apr 16, 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

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

CyrusNajmabadi commented Apr 21, 2017 • edited by jcouv Loading

CyrusNajmabadi commented Apr 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VSadov 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

CyrusNajmabadi commented Apr 21, 2017

CyrusNajmabadi commented Apr 21, 2017

DustinCampbell left a 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 15, 2017 •

edited

Loading

CyrusNajmabadi Apr 16, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Apr 16, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Apr 16, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Apr 16, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Apr 16, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Apr 16, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Apr 16, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Apr 16, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Apr 16, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Apr 16, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Apr 16, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Apr 16, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Apr 16, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Apr 16, 2017 •

edited by jcouv

Loading

CyrusNajmabadi commented Apr 21, 2017 •

edited by jcouv

Loading