Adds support for namespace XML doc comments

[daveaglick]

This PR addresses the lack of support for namespace XML comments as described in #15474. Since it was a fairly simple change, I decided to go ahead and implement so that the PR could be used for discussion (and hopefully merged eventually).

Customer scenario

.NET documentation generators have been forced to come up with their own ways of representing namespace documentation, and nearly all of them have tried to approach namespace-level documentation in one way or another. This shows that the lack of official compiler support for namespace documentation comments is an ongoing deficit. This PR adds support for reporting XML documentation comments at the namespace level in both Roslyn symbol representations and the generated XML documentation output file.

Bugs this fixes:

#15474

Workarounds, if any

Reflection can be used to get the formatted XML documentation for a namespace (see #15474).

Risk

There is a chance that changes to the generated XML documentation output file to include namespace XML documentation comments might break consumers of the file. This is mitigated by the fact that unless a namespace has XML documentation comments in the first place, they won't be output to the file. The presence of such comments on a namespace prior to this change is unlikely as they wouldn't have been picked up by the compiler and so would have been of little use.

Performance impact

I suspect (though do not know for sure) that this has a low performance impact since it uses the same mechanisms of compiling documentation comments as do other symbols, which have already been optimized.

Is this a regression from a previous update?

No.

Root cause analysis:

This is new functionality, and a test has been added to verify correct operation.

How was the bug found?

While developing a Roslyn-based .NET documentation generator and requiring information about namespaces to be available.

[daveaglick]

It looks like I'm getting CI errors with the message:

No test reports found for the metric 'xUnit.Net' with the resolved pattern '**/xUnitResults/*.xml'. Configuration error?

I'm not sure what's going wrong here - I was at least able to verify all tests pass locally.

[vcsjones]

@daveaglick

error BC37258: Error writing to XML documentation file: Unexpected value 'Namespace' of type 'Microsoft.CodeAnalysis.SymbolKind' [/mnt/j/workspace/dotnet_roslyn/master/linux_debug_prtest/src/Workspaces/VisualBasic/Portable/BasicWorkspace.vbproj]
Indication 1

[vcsjones]

Risk

If anyone is treating CS1591 as an error, this change will break their build, it looks like. I suspect that's unlikely, but possible.

[daveaglick]

If anyone is treating CS1591 as an error, this change will break their build, it looks like. I suspect that's unlikely, but possible.

It shouldn't - I intentionally added a check to the C# version to exempt namespaces from the missing documentation check. Apparently got it wrong on the VB side though - attempting to fix.

[vcsjones]

I intentionally added a check to the C# version to exempt namespaces from the missing documentation check.

OK. I read the check backwards.

[daveaglick]

Looks like this is sorted for now, thanks for the help @vcsjones!

[Pilchie]

@jaredpar please take a look.

[daveaglick]

Just a quick ping on this issue - is it worth doing a catch-up rebase at this point or should I wait for a review/feedback?

[daveaglick]

Taking @sharwell up on Twitter offer to review before 15.3 stabilization - would this one be appropriate? I can debase before review if needed. cc @jaredpar

[gafter]

This probably requires LDM review. I think we would consider it a tiny language change.

I've championed the language proposal dotnet/csharplang#315 so it will be triaged by the LDM.

I think it is too late to consider this for 15.3 (language version 7.1).

[jaredpar]

Sorry that we missed this PR for this long. I recently realized that we've missed a lot of PRs and have been trying to work my way down the open list triaging the ones I missed. Just saw the notification for this pop up today.

As @gafter said this is actually a language change hence we have to start by getting approval their first before we can take the PR.

Looking through the PR though had a couple of high level feedback items.

1. Didn't see any testing for same namespace having multiple XML doc comments (across files for instance). Unclear what the behavior should be.
2. IDE testing: probably need to make sure we complete the /// tags when typing this in the IDE.

[sharwell]

@daveaglick I see this has been sitting for a while; sorry to see that happen after you put the work in. The good news for you is @gafter has championed the issue, so you have at least one internal advocate for the proposal who's driving towards a resolution (i.e. an actual up or down decision on including it in the language).

It turns out language changes are some of the hardest to move forward. I have another documentation-comment related item which I implemented and it's in roughly the same situation you find yourself in. So while I can't "make it happen" for 15.3, I'll go through the PR and proposal and see if I can't provide some feedback that can lead to a sooner rather than later outcome.

[daveaglick]

Thanks all! No problem on the timing - just happy to see it's getting some attention 😄. I'll go back and engage with the proposal issue while this works through LDM review and circle back to the PR once that's done.

[sharwell]

@daveaglick I just read the comment from @jaredpar, and he pretty much nailed my first two pieces of feedback regarding testing. I would highly recommend you review the SHFB documentation related to namespace comments, and consider whether this feature as you've defined/implemented it could be considered a true replacement for the workarounds currently being used there.

[jaredpar]

We apologize, but we are closing this PR due to code drift. We regret letting this PR go so long without attention, but at this point the code base has changed too much for us to revisit older PRs. Going forward, we will manage PRs better under an SLA currently under draft in issue #26266 – we encourage you to add comments to that draft as you see fit. Although that SLA is still in draft form, we nevertheless want to move forward with the identification of older PRs to give contributors as much early notice as possible to review and update them.

If you are interested in pursuing this PR, please reset it against the head of master and we will reopen it. Thank you!