Improve support for documentation comments in Replace Property with Methods #18243

sharwell · 2017-03-28T13:05:45Z

Expand refactoring test cases to cover docs for generated setters
Expand refactoring test cases to cover cref references to the property under refactoring
Update refactoring to preserve documentation in cases revealed by new tests

Ask Mode

Customer scenario

Use the Convert Property to Method refactoring on a property which contains documentation comments.

Bugs this fixes:

#18234 (substantial improvement to previous fix)

Workarounds, if any

Manually convert the property, or fix the comments after the conversion.

Risk

Low, this change includes a substantial number of additional tests and is limited to the refactoring operation behavior.

Performance impact

Negligible. There is no impact except when the user explicitly requests this operation to occur, and is only slower in cases where the behavior was previously incorrect.

Is this a regression from a previous update?

No.

Root cause analysis:

Failure to account for related edge cases when fixing an overly-specific bug report.

How was the bug found?

Manual testing of edge cases related to (but not identical to) a filed bug.

sharwell · 2017-03-28T21:08:39Z

Tagging @dotnet/roslyn-ide for review

CyrusNajmabadi · 2017-03-28T21:10:29Z

...cePropertyWithMethods/CSharpReplacePropertyWithMethodsService.ConvertValueToParamRewriter.cs

+
+            private static bool IsValueName(XmlNameSyntax name)
+                => name.Prefix == null &&
+                   name.LocalName.ValueText == "value";


move to CSharpReplacePropertyWithMethodsService since both rewriters need it.

I'll get the equivalent one in VB as well.

CyrusNajmabadi · 2017-03-28T21:10:53Z

...cePropertyWithMethods/CSharpReplacePropertyWithMethodsService.ConvertValueToParamRewriter.cs

+
+                return node.ReplaceNode(node.Name, ConvertToParam(node.Name))
+                    .AddAttributes(SyntaxFactory.XmlNameAttribute("value"));
+            }


nit:follow pattern of VisitXmlElementEndTag

I was going to if it (the longer return statement's expression) fit on one line, but the additional call to AddAttributes seemed to pass a complexity line where the "light" syntax wasn't feeling so light anymore.

CyrusNajmabadi · 2017-03-28T21:11:44Z

...atures/CSharp/Portable/ReplacePropertyWithMethods/CSharpReplacePropertyWithMethodsService.cs

                    cancellationToken: cancellationToken));
            }

            var setMethod = property.SetMethod;
            if (setMethod != null)
            {
-                // Set-method only gets the leading trivia of the property if we didn't copy
-                // that trivia to the get-method.


We need some amount of logic to ensure that we don't copy all trivia from the method to both the getter and the setter. For example of there is a #pp directive on hte method, then it should only be copied to the first generated member.

➡️ Directive trivia, including #region, results in the analyzer not reporting a diagnostic.

can you comment the code as such to explain that this falls out becuase of that?

CyrusNajmabadi · 2017-03-28T21:12:13Z

...atures/CSharp/Portable/ReplacePropertyWithMethods/CSharpReplacePropertyWithMethodsService.cs

-
-            return methodDeclaration;
+            var leadingTrivia = propertyDeclaration.GetLeadingTrivia();
+            return methodDeclaration.WithLeadingTrivia(leadingTrivia.Select(trivia => ConvertTrivia(trivia, documentationCommentRewriter)));


general nit: wrap lines longer than github width. It makes reviewing easier.

CyrusNajmabadi · 2017-03-28T21:14:58Z

...atures/CSharp/Portable/ReplacePropertyWithMethods/CSharpReplacePropertyWithMethodsService.cs

+        protected override NameMemberCrefSyntax TryGetCrefSyntax(IdentifierNameSyntax identifierName)
+        {
+            return identifierName.Parent as NameMemberCrefSyntax;
+        }


CyrusNajmabadi · 2017-03-28T21:15:19Z

...atures/CSharp/Portable/ReplacePropertyWithMethods/CSharpReplacePropertyWithMethodsService.cs

+        protected override NameMemberCrefSyntax CreateCrefSyntax(NameMemberCrefSyntax originalCref, SyntaxToken identifierToken, SyntaxNode parameterType)
+        {
+            CrefParameterListSyntax parameterList;
+            if (parameterType is TypeSyntax)


if (paramterType is TypeSyntax typeSyntax)

➡️ typeSyntax is not used if I do this. The cast is on a different value.

On second look, the generic method may eliminate the need for the cast... looking again.

you could also choose to use typeSyntax :)

CyrusNajmabadi · 2017-03-28T21:16:49Z

...atures/CSharp/Portable/ReplacePropertyWithMethods/CSharpReplacePropertyWithMethodsService.cs

+            {
+                case SyntaxKind.LessThanToken:
+                case SyntaxKind.GreaterThanToken:
+                    string newText = rewrittenToken.IsKind(SyntaxKind.LessThanToken) ? "{" : "}";


sounds like we're fixing something up after the fact. Is there a reason we're not just generating the right syntax in the first place?

The source node is generated by SyntaxGenerator.TypeExpression. If there's an equivalent method which I can use that knows we're in a documentation comment I could use that instead.

The best way to handle this would be to have an overload that allows the caller to specify they want cref syntax for the type. This method would call into that with 'false'.

Currently the replacement is implemented for general code in SyntaxFactory.XmlCrefAttribute. It seems more appropriate to handle it at the point where the CrefSyntax is created, but that's in the generated code portion. As a workaround, I could use XmlCrefAttribute to translate the syntax, and then return just the CrefSyntax it contains.

Sorry, i'm finding the above confusing. Could you clarify what it is you intend to change?

I removed the current ReplaceBraceTokenForDocumentation method and just do this instead:

// XmlCrefAttribute replaces <T> with {T}, which is required for C# documentation comments var crefAttribute = SyntaxFactory.XmlCrefAttribute( SyntaxFactory.NameMemberCref(SyntaxFactory.IdentifierName(identifierToken), parameterList)); return (NameMemberCrefSyntax)crefAttribute.Cref;

CyrusNajmabadi · 2017-03-28T21:17:36Z

...atures/Core/Portable/ReplacePropertyWithMethods/AbstractReplacePropertyWithMethodsService.cs

+                _editor.ReplaceNode(
+                    _cref,
+                    GetCrefReference(_cref));
+            }


this method seems unnecessary (very simple, called in just one location). Can you just inline?

CyrusNajmabadi · 2017-04-07T18:50:35Z

...atures/CSharp/Portable/ReplacePropertyWithMethods/CSharpReplacePropertyWithMethodsService.cs

+            CrefParameterListSyntax parameterList;
+            if (parameterType is TypeSyntax typeSyntax)
+            {
+                CrefParameterSyntax parameter = SyntaxFactory.CrefParameter(typeSyntax);


nit: use var.

CyrusNajmabadi · 2017-04-07T18:54:43Z

...res/VisualBasic/Portable/ReplacePropertyWithMethods/VisualBasicReplacePropertyWithMethods.vb

+            Dim signature As CrefSignatureSyntax
+            Dim parameterSyntax = TryCast(parameterType, TypeSyntax)
+            If parameterSyntax IsNot Nothing Then
+                signature = SyntaxFactory.CrefSignature(SyntaxFactory.CrefSignaturePart(Nothing, parameterSyntax))


named parameters for 'nothing'.

CyrusNajmabadi · 2017-04-07T18:54:49Z

...res/VisualBasic/Portable/ReplacePropertyWithMethods/VisualBasicReplacePropertyWithMethods.vb

+                typeReference = qualifiedType.ReplaceNode(qualifiedType.GetLastDottedName(), typeReference)
+            End If
+
+            Return SyntaxFactory.CrefReference(typeReference, signature, Nothing)


here as well.

See dotnet#18261

sharwell · 2017-04-23T15:26:16Z

@CyrusNajmabadi Rebased to fix the merge conflict, then added c273150 to address the latest comments.

sharwell · 2017-05-08T16:41:58Z

@MattGertz for ask mode

sharwell added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Mar 28, 2017

dnfclas added the cla-already-signed label Mar 28, 2017

sharwell force-pushed the document-setters branch 2 times, most recently from 75240d7 to 2e213a5 Compare March 28, 2017 21:06

sharwell removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Mar 28, 2017

CyrusNajmabadi reviewed Mar 28, 2017

View reviewed changes

Pilchie added Area-Analyzers Area-IDE and removed Area-Analyzers labels Mar 29, 2017

sharwell requested a review from CyrusNajmabadi April 7, 2017 17:16

CyrusNajmabadi reviewed Apr 7, 2017

View reviewed changes

sharwell added 6 commits April 23, 2017 10:13

Expand refactoring test cases to cover docs for generated setters

f39da9d

Expand refactoring test cases to cover cref attributes

1b7b72d

Fix property to method conversion handling of documentation comments

a0b31ea

Ignore test broken due to VB formatting bug

82e79be

See dotnet#18261

Code cleanup based on code review feedback

a206a02

Code cleanup based on code review feedback

c273150

sharwell force-pushed the document-setters branch from f733ae4 to c273150 Compare April 23, 2017 15:25

sharwell added this to the 15.3 milestone May 8, 2017

MattGertz added the Approved to merge label May 8, 2017

sharwell merged commit 2f23f3b into dotnet:master May 8, 2017

sharwell deleted the document-setters branch May 8, 2017 23:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve support for documentation comments in Replace Property with Methods #18243

Improve support for documentation comments in Replace Property with Methods #18243

sharwell commented Mar 28, 2017 •

edited

Loading

sharwell commented Mar 28, 2017

CyrusNajmabadi Mar 28, 2017

sharwell Mar 28, 2017

CyrusNajmabadi Mar 28, 2017

sharwell Mar 28, 2017

CyrusNajmabadi Mar 28, 2017

sharwell Mar 28, 2017

CyrusNajmabadi Mar 28, 2017

CyrusNajmabadi Mar 28, 2017

CyrusNajmabadi Mar 28, 2017

CyrusNajmabadi Mar 28, 2017

sharwell Mar 28, 2017

sharwell Mar 28, 2017

CyrusNajmabadi Mar 28, 2017

CyrusNajmabadi Mar 28, 2017

sharwell Mar 28, 2017

CyrusNajmabadi Mar 28, 2017

sharwell Mar 28, 2017

CyrusNajmabadi Mar 28, 2017

sharwell Mar 28, 2017

CyrusNajmabadi Mar 28, 2017

CyrusNajmabadi Apr 7, 2017

CyrusNajmabadi Apr 7, 2017

CyrusNajmabadi Apr 7, 2017

sharwell commented Apr 23, 2017

sharwell commented May 8, 2017

Improve support for documentation comments in Replace Property with Methods #18243

Improve support for documentation comments in Replace Property with Methods #18243

Conversation

sharwell commented Mar 28, 2017 • edited Loading

Ask Mode

sharwell commented Mar 28, 2017

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

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

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

sharwell commented Apr 23, 2017

sharwell commented May 8, 2017

sharwell commented Mar 28, 2017 •

edited

Loading