Doc comments: Handle CDATA correctly and avoid normalizing code elements #53140

Youssef1313 · 2021-05-04T12:37:19Z

Fixes #53782
Fixes #53135
Fixes #37503
Fixes #22431

Before:

After:

Youssef1313 · 2021-05-04T12:42:49Z

...eatures/Core/Portable/DocumentationComments/AbstractDocumentationCommentFormattingService.cs

-                Builder.Add(new TaggedText(TextTags.Text, s, Style, NavigationTarget.target, NavigationTarget.hint));
+                Builder.Add(new TaggedText(TextTags.Text, NormalizeLineEndings(s), Style, NavigationTarget.target, NavigationTarget.hint));

                _anyNonWhitespaceSinceLastPara = true;
+
+                static string NormalizeLineEndings(string input) => Regex.Replace(input, "(?<!\r)\n", "\r\n");


I'd certainly prefer a better solution here. The issue here is that XText.Value produces a string with Unix-style line endings (\n), causing the end result to have mixed line-endings 😕

this warrants a comment explaning what the regex means and waht this is here for.

I normally use input.Replace("\r\n", "\n").Replace("\n", Environment.NewLine), but XML is required to normalize line endings to \n so I'm guessing you can just use input.Replace("\n", "\r\n") and be done.

@sharwell I'm a little bit worried if AppendString can be called with a string already containing \r\n, then doing Replace("\n", "\r\n") will duplicate \r.

Is it's guaranteed that XText.Value will always be normalized to \n?

https://www.w3.org/TR/xml/#sec-line-ends seems to claim that. I'll convert to Replace("\n", "\r\n"). Thanks @sharwell.

Youssef1313 · 2021-05-04T12:43:28Z

...rvices/SymbolDisplayService/AbstractSymbolDisplayService.AbstractSymbolDescriptionBuilder.cs

-                    symbol.GetDocumentationParts(_semanticModel, _position, formatter, CancellationToken).ToImmutableArray());
+                    symbol.GetDocumentationParts(_semanticModel, _position, formatter, CancellationToken));

                _documentationMap.Add(
                    SymbolDescriptionGroups.RemarksDocumentation,
-                    symbol.GetRemarksDocumentationParts(_semanticModel, _position, formatter, CancellationToken).ToImmutableArray());
+                    symbol.GetRemarksDocumentationParts(_semanticModel, _position, formatter, CancellationToken));


This only avoids creating a new array since GetDocumentationParts already returns an immutable array.

ToImmutableArray() won't clone the array if the input is already an immutable array, but this does avoid boxing the IEnumerable<T> for the call and is cleaner so 👍

ToImmutableArray() won't clone the array if the input is already an immutable array,

@sharwell Yeah that seem the case:

https://github.com/dotnet/runtime/blob/59d9d0d16858635a8c228be2ab7ad8882b0d1c71/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray.cs#L372-L380

So the docs of CA2009 are inaccurate/incorrect?

https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca2009

Performance issue: Unnecessary creation of a duplicate immutable collection. The original collection was already immutable and can be used directly.

sharwell · 2021-05-04T16:11:23Z

Ohhhh snap 🎉

CyrusNajmabadi · 2021-05-04T16:14:39Z

...eatures/Core/Portable/DocumentationComments/AbstractDocumentationCommentFormattingService.cs

+                // This is because XText.Value returns a string contaning `\n` as the line endings, causing
+                // the end result to have mixed line-endings. So normalize everything to `\r\n`.
+                static string NormalizeLineEndings(string input) => Regex.Replace(input, "(?<!\r)\n", "\r\n");
+


Suggested change

sharwell · 2021-05-04T16:13:32Z

src/EditorFeatures/CSharpTest/QuickInfo/SemanticQuickInfoSourceTests.cs

+
+        [WorkItem(37503, "https://github.com/dotnet/roslyn/issues/37503")]
+        [Fact, Trait(Traits.Feature, Traits.Features.QuickInfo)]
+        public async Task DoNotNormalizeWhitespaceForCode()


❗ Can you add both of these test cases to a new test class IntellisenseQuickInfoBuilderTests_Code.cs, which is modeled after IntellisenseQuickInfoBuilderTests_Styles.

sharwell · 2021-05-04T16:14:44Z

src/EditorFeatures/Test/DocCommentFormatting/DocCommentFormattingTests.cs

@@ -50,7 +50,7 @@ public void ExampleAndCodeTags()
            results in <c>p</c>'s having the value (2,8).
            </example>";

-            var expected = "This method changes the point's location by the given x- and y-offsets. For example:\r\n\r\nPoint p = new Point(3,5); p.Translate(-1,3);\r\n\r\nresults in p's having the value (2,8).";
+            var expected = "This method changes the point's location by the given x- and y-offsets. For example:\r\n\r\n\r\n            Point p = new Point(3,5);\r\n            p.Translate(-1,3);\r\n            \r\n\r\nresults in p's having the value (2,8).";


📝 This is one of the cases that made me uncomfortable, but I'll reserve judgement until I see the new builder tests.

@sharwell I'm expecting you'll still be uncomfortable after seeing the new tests 😄
Does it make sense to trim line-endings from code elements?

I'm going to look at screenshots before requesting changes to the actual normalization approach. Maybe there's no problem.

sharwell · 2021-05-04T17:35:07Z

src/EditorFeatures/Test2/IntelliSense/IntellisenseQuickInfoBuilderTests_Code.vb

+                        New ClassifiedTextRun(ClassificationTypeNames.WhiteSpace, " "),
+                        New ClassifiedTextRun(ClassificationTypeNames.Text, "Not        this", ClassifiedTextRunStyle.UseClassificationFont))),
+                New ClassifiedTextElement(
+                    New ClassifiedTextRun(ClassificationTypeNames.Text, "\r\nline 1\r\nline     2\r\n", ClassifiedTextRunStyle.UseClassificationFont)),


💭 I would expect this to be rendered like <br/>

Example input:

roslyn/src/EditorFeatures/Test2/IntelliSense/IntellisenseQuickInfoBuilderTests.vb

Lines 326 to 327 in 3e1cf13

/// Documentation line 1.<br/>

/// Documentation line 2.

Example output:

roslyn/src/EditorFeatures/Test2/IntelliSense/IntellisenseQuickInfoBuilderTests.vb

Lines 371 to 376 in 3e1cf13

New ContainerElement(

ContainerElementStyle.Stacked,

New ClassifiedTextElement(

New ClassifiedTextRun(ClassificationTypeNames.Text, "Documentation line 1.")),

New ClassifiedTextElement(

New ClassifiedTextRun(ClassificationTypeNames.Text, "Documentation line 2.")))),

I'll have to run the code to see if it looks the same with the current form (unless you have a screenshot handy).

I'll add screenshots shortly.

@sharwell Screenshot added.

Youssef1313

While playing around this, I found that indentation of code isn't preserved. This is due to:

roslyn/src/Workspaces/Core/Portable/Shared/Utilities/DocumentationComment.cs

Line 182 in 1cca63b

_comment.SummaryText = TrimEachLine(reader.ReadInnerXml());

I'll open an issue to address this as a follow-up. Not sure if we should simply drop calling TrimEachLine or if it should understand the structure of XML and avoids trimming things inside <code> but trim other lines.

sharwell · 2021-05-07T21:57:39Z

src/EditorFeatures/Test2/IntelliSense/IntellisenseQuickInfoBuilderTests_Code.vb

+                        <Document>
+                            class MyClass {
+                                /// &lt;summary&gt;
+                                /// Normalize    this, but &lt;c&gt;Not        this&lt;/c&gt;


💭 I thought <c> would be normalized. We should check with the publishing tools to see if this is the case.

@sharwell This is still code, so I'm expecting it to preserve spaces. But no idea what tools like docfx do.

The main case where we definitely don't want to keep whitespace is this:

/// <summary> /// Some <c>code /// example</c> is here. /// </summary>

The above should render exactly the same as this:

/// <summary> /// Some <c>code example</c> is here. /// </summary>

It may be worth adding tests to show these are the same.

src/EditorFeatures/Test2/IntelliSense/IntellisenseQuickInfoBuilderTests_Code.vb

Youssef1313 · 2021-05-12T08:17:46Z

src/Features/Core/Portable/Common/TaggedTextStyle.cs


-        Code = 0x8,
+        InlineCode = 1 << 4,


I added a new TaggedTextStyle to differentiate <c> and <code>.

Currently Code value is 1000 and InlineCode is 10000. Should InlineCode be 1001 or whatever value to share the same most significant bit? (i.e, indicate that InlineCode is a subset of Code)?

This will allow the following check: (style & TaggedTextStyle.Code) == TaggedTextStyle.Code to pass whether style is Code or InlineCode.

The current approach seems fine to me.

On second thought, it seems like the new addition should be:

PreserveWhitespace = 1 << 4,

Youssef1313 · 2021-05-12T08:22:07Z

src/Features/LanguageServer/Protocol/Extensions/ProtocolConversions.cs

@@ -695,7 +695,7 @@ static string GetStyledText(TaggedText taggedText, bool isInCodeBlock)
                    TaggedTextStyle.Strong => $"**{text}**",
                    TaggedTextStyle.Emphasis => $"_{text}_",
                    TaggedTextStyle.Underline => $"<u>{text}</u>",
-                    TaggedTextStyle.Code => $"`{text}`",
+                    TaggedTextStyle.Code or TaggedTextStyle.InlineCode => $"`{text}`",


Few lines above this, there is:

if (!isInCodeBlock) text = text.Replace(" ", " ");

This doesn't feel correct. See the following test:

https://github.com/dibarbet/roslyn/blob/0ef88b0a686892546b57e721ba65738872bf928d/src/Features/LanguageServer/ProtocolUnitTests/Hover/HoverTests.cs#L195

@sharwell Do you agree here? Do you want me to fix LSP in this PR or make that a separate PR? (I prefer LSP work to be separate - but no objection doing it in this PR)

cc: @dibarbet

Separate is fine.

sharwell

Let's change <code> to use TaggedTextStyle.Code | TaggedTextStyle.PreserveWhitespace so the whole enumeration continues to work as individual flags.

Youssef1313 · 2021-05-18T18:23:20Z

@sharwell Anything else needed here?

Youssef1313 · 2021-05-18T20:34:48Z

@Youssef1313 We'd like to rebase this onto release/dev16.11. Are you able to do this or would you like us to go ahead and do it?

Done.

sharwell · 2021-06-24T14:25:42Z

@jinujoseph for approval to merge

CyrusNajmabadi · 2021-06-24T17:17:38Z

...eatures/Core/Portable/DocumentationComments/AbstractDocumentationCommentFormattingService.cs

+                // XText.Value returns a string with `\n` as the line endings, causing
+                // the end result to have mixed line-endings. So normalize everything to `\r\n`.
+                // https://www.w3.org/TR/xml/#sec-line-ends
+                static string NormalizeLineEndings(string input) => input.Replace("\n", "\r\n");


why do we need this though? what is wrong with the doc comment having \n in it?

➡️ This function ensures that non-normalized XML content uses the same end-of-line character as s_newlinePart (defined above). It could be cleaned up in the future (e.g. if we wanted to use Environment.Newline instead), but the approach in this PR keeps the result consistent with current precedent.

CyrusNajmabadi · 2021-06-24T17:18:16Z

...eatures/Core/Portable/DocumentationComments/AbstractDocumentationCommentFormattingService.cs

            {
+                // cast is safe since XCData inherits XText


should the above just be if (node is XText xtext) ?

➡️ Both approaches are fine. There isn't really a downside to the current approach so I prefer to not make a change here unless it's being done along with other changes.

CyrusNajmabadi · 2021-06-24T17:18:53Z

...eatures/Core/Portable/DocumentationComments/AbstractDocumentationCommentFormattingService.cs

@@ -550,6 +559,13 @@ private static string TrimCrefPrefix(string value)
        private static void AppendTextFromTextNode(FormatterState state, XText element)
        {
            var rawText = element.Value;
+            if ((state.Style & TaggedTextStyle.PreserveWhitespace) == TaggedTextStyle.PreserveWhitespace)
+            {
+                // Don't normalize code from middle. Only trim leading/trailing new lines.


why trim the leading/trailing?

➡️ This implements a fix which a test revealed:
#53140 (comment)

Youssef1313 · 2021-06-24T18:33:30Z

@sharwell Can you close the linked issues? Thanks!

sharwell · 2021-06-24T18:34:15Z

@Youssef1313 Already done thanks 🎉🎉

dotnet-issue-labeler bot added the Area-IDE label May 4, 2021

Youssef1313 force-pushed the quick-info-cdata branch from 8d277b4 to f3fe0ed Compare May 4, 2021 12:40

Youssef1313 marked this pull request as ready for review May 4, 2021 12:40

Youssef1313 requested a review from a team as a code owner May 4, 2021 12:40

Youssef1313 commented May 4, 2021

View reviewed changes

Youssef1313 force-pushed the quick-info-cdata branch from b94e97b to a67f650 Compare May 4, 2021 14:52

CyrusNajmabadi approved these changes May 4, 2021

View reviewed changes

CyrusNajmabadi reviewed May 4, 2021

View reviewed changes

sharwell requested changes May 4, 2021

View reviewed changes

sharwell reviewed May 4, 2021

View reviewed changes

Youssef1313 commented May 4, 2021

View reviewed changes

jinujoseph added Feature Request Community The pull request was submitted by a contributor who is not a Microsoft employee. labels May 4, 2021

jinujoseph assigned sharwell May 4, 2021

Youssef1313 mentioned this pull request May 4, 2021

Fix incorrect info in CA2009 docs dotnet/docs#24042

Merged

sharwell reviewed May 7, 2021

View reviewed changes

src/EditorFeatures/Test2/IntelliSense/IntellisenseQuickInfoBuilderTests_Code.vb Outdated Show resolved Hide resolved

sharwell approved these changes May 11, 2021

View reviewed changes

Youssef1313 commented May 12, 2021

View reviewed changes

Youssef1313 mentioned this pull request May 12, 2021

Fix LSP tooltips that are generated as markdown from doc comments #53358

Open

sharwell requested changes May 12, 2021

View reviewed changes

sharwell approved these changes May 12, 2021

View reviewed changes

Youssef1313 mentioned this pull request May 13, 2021

<code> doc comment should preserve indentation #53384

Open

Youssef1313 added 7 commits May 18, 2021 22:27

Add comment

c0e6992

Address feedback

0cc823b

Fix

c3a86c8

Fix code normalization

e01d57a

Address feedback

69ad011

Address feedback

b6159fc

Fix

636bc29

Youssef1313 force-pushed the quick-info-cdata branch from 97eee47 to 636bc29 Compare May 18, 2021 20:33

Youssef1313 requested a review from a team as a code owner May 18, 2021 20:33

Youssef1313 changed the base branch from main to release/dev16.11 May 18, 2021 20:33

sharwell added the Needs Shiproom Approval label May 18, 2021

Youssef1313 mentioned this pull request May 30, 2021

Quick info in IntelliSense strips whitespaces from <code> blocks #53782

Closed

jinujoseph removed the Feature Request label Jun 24, 2021

CyrusNajmabadi reviewed Jun 24, 2021

View reviewed changes

jinujoseph added Approved to merge and removed Needs Shiproom Approval labels Jun 24, 2021

sharwell merged commit c177fa8 into dotnet:release/dev16.11 Jun 24, 2021

Youssef1313 deleted the quick-info-cdata branch June 24, 2021 18:32

This was referenced Jun 26, 2021

CDATA code blocks are not rendered #54417

Closed

Newlines in XML doc <code> elements are not displayed in the IDE, creating an unreadable jumble #54414

Closed

sharwell mentioned this pull request Jul 6, 2021

<code> tags in XML comments should render as monospaced, syntax highlighted code in the IntelliSense popups. #39140

Closed

Youssef1313 mentioned this pull request Oct 20, 2021

VS Quick Info code sample formatting not preserved #57262

Closed

	New ContainerElement(
	ContainerElementStyle.Stacked,
	New ClassifiedTextElement(
	New ClassifiedTextRun(ClassificationTypeNames.Text, "Documentation line 1.")),
	New ClassifiedTextElement(
	New ClassifiedTextRun(ClassificationTypeNames.Text, "Documentation line 2.")))),

Doc comments: Handle CDATA correctly and avoid normalizing code elements #53140

Doc comments: Handle CDATA correctly and avoid normalizing code elements #53140

Conversation

Youssef1313 commented May 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Youssef1313 May 4, 2021 • 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

sharwell commented May 4, 2021

Choose a reason for hiding this comment

sharwell May 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell May 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell May 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Youssef1313 left a comment • edited Loading

Choose a reason for hiding this comment

sharwell May 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell May 11, 2021 • 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

Youssef1313 May 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell left a comment

Choose a reason for hiding this comment

Youssef1313 commented May 18, 2021

Youssef1313 commented May 18, 2021

sharwell commented Jun 24, 2021

Choose a reason for hiding this comment

sharwell Jun 24, 2021 • 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

Choose a reason for hiding this comment

Youssef1313 commented Jun 24, 2021

sharwell commented Jun 24, 2021

Youssef1313 commented May 4, 2021 •

edited

Loading

Youssef1313 May 4, 2021 •

edited

Loading

sharwell May 4, 2021 •

edited

Loading

sharwell May 4, 2021 •

edited

Loading

sharwell May 4, 2021 •

edited

Loading

Youssef1313 left a comment •

edited

Loading

sharwell May 7, 2021 •

edited

Loading

sharwell May 11, 2021 •

edited

Loading

Youssef1313 May 12, 2021 •

edited

Loading

sharwell Jun 24, 2021 •

edited

Loading