A code fix for a set of Documentation Comment diagnostics returned by the compiler #12902

Hosch250 · 2016-08-03T18:59:09Z

Reference #7070

Pilchie · 2016-08-03T19:38:55Z

Thanks for the submission - tagging @dotnet/roslyn-ide for review

jmarolf · 2016-08-03T20:02:34Z

test vsi please

brettfo · 2016-08-03T20:04:04Z

...tures/CSharpTest/DocumentationComments/CodeFixes/RemoveDocCommentNodeCodeFixProviderTests.cs

+        <Document>
+class Program1
+{
+    /// &lt;summary&gt;


Minor note: if you wrap the body in <![CDATA[ and ]]> tags you don't need < and &gt which might make this easier to read. E.g.,

var initial = @" <Workspace> <Project ...> <Document> <![CDATA[ class Program 1 { /// <summary> ... ]]> ... ";

brettfo · 2016-08-03T20:12:09Z

Aside from my minor comments, looks good to me.

CyrusNajmabadi · 2016-08-03T20:25:35Z

...tures/CSharpTest/DocumentationComments/CodeFixes/RemoveDocCommentNodeCodeFixProviderTests.cs

+    public void Fizz(int value) {}
+}
+";
+            var parseOptions = Options.Regular.WithDocumentationMode(DocumentationMode.Diagnose);


so the pattern i prefer in cases like this is that you have your own TestAsync function in this class which creates theese parse options and calls the original TestAsync. Then all the functions in this class call that Test function. Test code follows hte same rules as normal code and we shoudl apply DRY.

(yes, i know there are TONS of cases where we violate this. but no reason to continue bad habits when we can avoid them). Thanks!

Also, the core test method you call must pass in:

, compareTokens: false

The reason for this is that if you don't pass this in, then our test code will explicitly ignore trivia. That's obviously bad because this entire feature is precisely about changing trivia. :)

This is also the benefit of having a single test method. You change one and everything else picks it up.

CyrusNajmabadi · 2016-08-03T20:30:32Z

As usual @Hosch250 your commitment to testing is phenomenal. Your contributions are very appreciated by the entire team.

jmarolf · 2016-08-03T20:39:19Z

Looks like test failure is due to: #11032

retest windows_vsi_p1_open_prtest please
retest windows_vsi_p3_open_prtest please

CyrusNajmabadi · 2016-08-03T23:13:06Z

.../VisualBasicTest/DocumentationComments/CodeFixes/RemoveDocCommentNodeCodeFixProviderTests.vb

@@ -14,351 +14,373 @@ Namespace Microsoft.CodeAnalysis.Editor.VisualBasic.UnitTests.DocumentationComme
                Nothing,
                New VisualBasicRemoveDocCommentNodeCodeFixProvider())
        End Function
+
+        Private Overloads Async Function TestAsync(ByVal initial As String, ByVal expected As String) As Task
+


remove newline.

CyrusNajmabadi · 2016-08-03T23:15:50Z

LGTM. @Pilchie Any concerns here?

Hosch250 · 2016-08-04T01:26:18Z

If you hold off a bit on merging this, I'll add the fix for CS1572 also. The others all involve removing the entire block, or adding elements (I think).

jmarolf · 2016-08-04T01:40:52Z

@Hosch250 alright, thanks, I'll hold off on merging until I hear from you.

Hosch250 · 2016-08-04T01:41:14Z

Just running my tests right now, so it shouldn't be long :)

…nding the correct node slightly.

CyrusNajmabadi · 2016-08-04T04:10:46Z

...Core/Portable/DocumentationComments/CodeFixes/AbstractRemoveDocCommentNodeCodeFixProvider.cs

+        {
+            var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
+            var commentParent = root.FindNode(span);
+            var triviaNode = commentParent.GetLeadingTrivia().Single(s => s.GetLocation().SourceSpan.Contains(span));


s => s.Span.Contains(span)

Hosch250 · 2016-08-04T04:11:52Z

I will fix these and add fixes for BC42313 and BC42315 as well (I just need to add them to the list of fixable diagnostics and add tests).

CyrusNajmabadi · 2016-08-04T04:13:06Z

...Core/Portable/DocumentationComments/CodeFixes/AbstractRemoveDocCommentNodeCodeFixProvider.cs

+
+            var triviaNodeStructure = triviaNode.GetStructure();
+
+            var paramNode = triviaNodeStructure.ChildNodes().Single(s => s.Span.Contains(span));


single is dangerous here. What if you had something like:

/// <summary> /// <param ... > /// </summary>

Couldn't both the summary node and the param node contain the span in question?

I'd far prefer something like SingleOrDefault just to be resilient to strange parse tree structures. (same applies to the higher use of Single).

I will change this, but, IIRC, SingleOrDefault still throws if there is more than one matching node. I'll have to add tests for this possibility.

You are correct. 'FirstOrDefault' then. :)

Or, somethgin that actually tries to get the right node but won't crash :)

Yes, I will have to redesign this.

Actually, no, that situation you show will not cause a crash. I only retrieve the child nodes, which is the first level under the given element. This would crash if I had retrieved the descendant nodes, but the only way it could crash under the child nodes would be if the span covered two elements. I will change it to SingleOrDefault and return an unchanged document if I can't find a matching node, however, unless you think this isn't safe enough.

Ah good point. Can you still do FirstOrDefault then, and just bail in the default case. I don't want us to introduce a crash with Single late in the process that only happens in rare cases, but we don't run into until some customer hits it :)

Yes, that would be good--I wouldn't want to cause a crash either.

CyrusNajmabadi · 2016-08-04T04:16:39Z

A few more comments. If @Pilchie is cool with this, and the comments are addressed, then 👍

Thanks!

Hosch250 · 2016-08-04T04:26:24Z

I may not be able to get this in tonight. Working on addressing the comments and adding tests right now.

Hosch250 · 2016-08-04T04:53:59Z

I just caught a bug:

/// <summary>
/// <param name="args"></param>
/// <param name="args"></param>
/// </summary>

That fires the diagnostic off, and running my quick fix will kill the whole summary tag. Now, no diagnostic comment should ever be like that, but I'll address this bug tomorrow anyway (especially since I've seen some that are like that).

… deleted

Hosch250 · 2016-08-04T16:22:34Z

@CyrusNajmabadi Bug fixed and ready for review again.

Pilchie · 2016-08-04T18:14:16Z

I won't have a chance to review until Saturday, but this can be merged before then if @CyrusNajmabadi and @jmarolf are happy with it.

Hosch250 · 2016-08-05T15:37:56Z

test vsi please

Hosch250 · 2016-08-05T16:13:01Z

Is the reason for failure #11032 again?

Hosch250 · 2016-08-05T16:18:28Z

...Core/Portable/DocumentationComments/CodeFixes/AbstractRemoveDocCommentNodeCodeFixProvider.cs

+
+            // There *should* always be one node here if the diagnostic fires,
+            // but we will use `FirstOrDefault` just to safeguard against a sly bug
+            var triviaNode = commentParent.GetLeadingTrivia().FirstOrDefault(s => s.Span.Contains(span));


I can use the flag to search in trivia as well as non-trivia nodes here to simplify this.

Hosch250 · 2016-08-05T20:06:46Z

test vsi please

Hosch250 · 2016-08-05T20:40:23Z

@CyrusNajmabadi I'm ready for this to be reviewed again whenever you are ready. I made a few slight changes to use the pre-written node-finding API, and fixed the bug I mentioned on Wednesday night.

Hosch250 · 2016-08-06T01:16:31Z

Thank you for the information.

CyrusNajmabadi · 2016-08-06T01:40:47Z

...Core/Portable/DocumentationComments/CodeFixes/AbstractRemoveDocCommentNodeCodeFixProvider.cs

+                paramNode = paramNode.Parent;
+            }
+
+            if (paramNode == null)


This whole check should happen prior to calling "RegisterCodeFix". Because if we can't transform the document then we shouldn't offer a fix.

CyrusNajmabadi · 2016-08-06T01:42:08Z

@Hosch250 If you extract that code into a helper you can then call it both before and after the 'RegisterXXX' call.

Make that change and i'll happily merge this. If you want, i'm also ok making that change myself. Your call!

Hosch250 · 2016-08-06T01:45:06Z

I will do it.

Hosch250 · 2016-08-06T02:27:22Z

Done, sorry it took so long. I will be back in about an hour in case you have more questions.

CyrusNajmabadi · 2016-08-06T02:53:48Z

Merged in. Thanks a ton!

jmarolf · 2016-08-06T06:13:05Z

Thanks for merging @CyrusNajmabadi. and thanks for the awesome contribution @Hosch250!

Hosch250 added 2 commits August 3, 2016 13:49

Partially implements #7070

3843a36

Merge branch 'master' of https://github.com/dotnet/roslyn

93f487b

dnfclas added the cla-already-signed label Aug 3, 2016

Pilchie added Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Aug 3, 2016

brettfo reviewed Aug 3, 2016
View reviewed changes

CyrusNajmabadi reviewed Aug 3, 2016
View reviewed changes

Respond to comments

04b0d3d

CyrusNajmabadi reviewed Aug 3, 2016
View reviewed changes

Respond to comments (2)

23a14d3

Add CS1572 to the list of fixable diagnostics. Modify my method of fi…

83b6e84

…nding the correct node slightly.

CyrusNajmabadi reviewed Aug 4, 2016
View reviewed changes

Respond to comments (3). Add fix for BC42313 and BC42315

b88786e

Fix bug where nested XML nodes caused the outermost parent node to be…

666317b

… deleted

janrapp mentioned this pull request Aug 5, 2016

Add parameter xml documentation code fix provider [work in progress] #10520

Closed

4 tasks

Hosch250 reviewed Aug 5, 2016
View reviewed changes

Clean up finding the correct node.

87bdd51

CyrusNajmabadi reviewed Aug 6, 2016
View reviewed changes

Respond to comments (4)

1bd65c6

CyrusNajmabadi merged commit 811ddd1 into dotnet:master Aug 6, 2016

Hosch250 mentioned this pull request Feb 23, 2017

Incorrect use of cancellation token Vannevelj/VSDiagnostics#609

Closed


		var triviaNodeStructure = triviaNode.GetStructure();

		var paramNode = triviaNodeStructure.ChildNodes().Single(s => s.Span.Contains(span));

A code fix for a set of Documentation Comment diagnostics returned by the compiler #12902

A code fix for a set of Documentation Comment diagnostics returned by the compiler #12902

Conversation

Hosch250 commented Aug 3, 2016

Pilchie commented Aug 3, 2016

jmarolf commented Aug 3, 2016

Choose a reason for hiding this comment

brettfo commented Aug 3, 2016

Choose a reason for hiding this comment

CyrusNajmabadi Aug 3, 2016 • edited Loading

Choose a reason for hiding this comment

CyrusNajmabadi commented Aug 3, 2016

jmarolf commented Aug 3, 2016

Choose a reason for hiding this comment

CyrusNajmabadi commented Aug 3, 2016

Hosch250 commented Aug 4, 2016

jmarolf commented Aug 4, 2016

Hosch250 commented Aug 4, 2016

Choose a reason for hiding this comment

Hosch250 commented Aug 4, 2016

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

Hosch250 Aug 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Aug 4, 2016

Hosch250 commented Aug 4, 2016

Hosch250 commented Aug 4, 2016 • edited Loading

Hosch250 commented Aug 4, 2016

Pilchie commented Aug 4, 2016

Hosch250 commented Aug 5, 2016

Hosch250 commented Aug 5, 2016

Choose a reason for hiding this comment

Hosch250 commented Aug 5, 2016

Hosch250 commented Aug 5, 2016

Hosch250 commented Aug 6, 2016

Choose a reason for hiding this comment

CyrusNajmabadi commented Aug 6, 2016

Hosch250 commented Aug 6, 2016

Hosch250 commented Aug 6, 2016

CyrusNajmabadi commented Aug 6, 2016

jmarolf commented Aug 6, 2016

CyrusNajmabadi Aug 3, 2016 •

edited

Loading

Hosch250 Aug 4, 2016 •

edited

Loading

Hosch250 commented Aug 4, 2016 •

edited

Loading