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.

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.

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

📝 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.

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

Example input:

Example output:

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.

💭 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.

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,

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.

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.

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.

why trim the leading/trailing?

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