Use consistent tags in documentation comments

[bamarsha]

Most documentation comments in the compiler only use the <summary> tag. For short comments, this is fine, but in many cases the summary is several paragraphs long and would be easier to read if it used standard documentation tags to structure the information. This is especially important for exceptions - exceptions mentioned in the <summary> tag are easy to miss, whereas the special <exception> tag makes them stand out more.

I think we need to use some rules for our doc comments. My proposal is:

1. Limit <summary> to one paragraph. Anything longer than that should go in specific tags as appropriate, or the <remarks> tag if none of the other tags apply.
2. Always put exceptions in an <exception> tag.

Tasks

• [x] Use exception tags in C# documentation comments (#732)
• [x] Use exception tags in F# documentation comments (#766)
• [ ] Limit summaries to one paragraph
• [ ] Decide if other changes to the documentation tags are needed
• [ ] Enable C# documentation generation to validate doc comments

[cgranade]

Agreed entirely. Can I also request splitting parameter descriptions in to <param> elements so that they show up in intelliSense correctly?

[kevinhartman]

I'd be interested in working on this issue. Assuming I wouldn't be toe-stepping, my plan would be to start with exceptions, working out of this branch.

[bamarsha]

Hi @kevinhartman, that would be great! Let us know if you have any questions.

[kevinhartman]

Thanks @samarsha!

I just submitted a PR to add the exception tag for C# doc comments. I didn't do this for the F# code, since most of it uses the implicit summary tag (omits the <summary> tag). Thoughts on using explicit XML tags for that portion of the codebase?

I was also wondering how strictly the <paramref> tag should be used in exception text. In most cases, I was worried that it'd make the documentation less readable when looking at source files. But perhaps if the XML is being used to generate API reference docs, it'd be pretty helpful!

[bamarsha]

Thanks @kevinhartman for the PR! I'll take a look soon.

For the F# comments, I think switching from an implicit summary tag to an explicit one when an exception tag (or other tag) is needed makes sense.

I think using <paramref> is fine too - while the Q# compiler currently doesn't have generated API docs, we're planning to do that at some point, so I think planning ahead and using proper tags makes sense even if they are slightly less readable in the source. Also, IDEs can help a little bit with automatically formatting doc comments while editing code (e.g. by displaying formatted versions in the hover text).

If you have any examples where you think using <paramref> would significantly decrease readability, or if it would be too annoying to write because the parameter name is used too often or something like that, though, let me know.

[kevinhartman]

Sounds great! I can create a separate PR to handle the F# code using that approach.

I'll go ahead and add the <paramref> tag where appropriate for this PR. There are only a handful of cases where things might get a little messy.

Perhaps in a case like this it might make sense to only add reference tags for the first mention of each parameter?

[bamarsha]

Perhaps in a case like this it might make sense to only add reference tags for the first mention of each parameter?

Yeah, that could be one option, although <paramref> has a couple of advantages that work best if every occurrence of a parameter name uses the tag (similar to nameof in C#):

1. When renaming a parameter, an IDE can automatically rename <paramref> tags too.
2. <paramref> tags can be checked automatically by a tool to make sure that the doc comment refers to actual parameter names.

[kevinhartman]

Hmm, that does sound useful. I suppose I'm just wishing C# doc comments weren't XML-based!

Somewhat related, I noticed something that might be worth considering: neither Visual Studio nor VS Code seem to render exception descriptions, at least in the floating window that pops up from the callsite:

Defined here:

At first this surprised me, but thinking more about it now, this would be alleviated through the use of proper application-specific exception design, where the exception class corresponds closely to a single cause, or provides its own interface to get the cause.

Thoughts?

And another question (thanks for your patience!): In many cases, I've noticed that exception descriptions can easily be written clearly without actually naming code references or parameters. When both approaches are possible, which should be preferred?

/// <exception cref="NotSupportedException">Thrown if the path to the output folder is malformed.</exception>
/// <exception cref="IOException">Thrown if <paramref name="outputFolder"/> is a file path.</exception>
public static void PublishResults(string outputFolder) { }

[bamarsha]

Hmm, that does sound useful. I suppose I'm just wishing C# doc comments weren't XML-based!

Yes, they do tend to be very verbose. :)

Somewhat related, I noticed something that might be worth considering: neither Visual Studio nor VS Code seem to render exception descriptions, at least in the floating window that pops up from the callsite:

Interesting, it looks like Rider does show the full exception description in the hover box, though.

At first this surprised me, but thinking more about it now, this would be alleviated through the use of proper application-specific exception design, where the exception class corresponds closely to a single cause, or provides its own interface to get the cause.

I think defining new exception types makes sense sometimes, but I wouldn't want to overuse them - I think it's fine to use the generic ArgumentException in most cases. Maybe for VS and VS Code you just need to go to the full doc comment to see the conditions for the exceptions?

In many cases, I've noticed that exception descriptions can easily be written clearly without actually naming code references or parameters. When both approaches are possible, which should be preferred?

I think I slightly prefer using <paramref>, since it's shorter (you don't have to repeat the parameter description), and if there are multiple parameters, it's easier to tell which one can cause the exception since you don't need to map the description back to the parameter name in your mind. What do you think?

[kevinhartman]

Interesting, it looks like Rider does show the full exception description in the hover box, though.

Wow, Rider looks very snazzy! I wonder if the VS ReSharper plugin also includes the exception description.

I think defining new exception types makes sense sometimes, but I wouldn't want to overuse them - I think it's fine to use the generic ArgumentException in most cases.

I agree. I think most of the ArgumentException usages I've seen are entirely reasonable. For that exception particularly, I believe it's conventionally only thrown when a developer calls a method incorrectly (i.e. the caller wouldn't plan to handle it, unless they were doing something weird).

Maybe for VS and VS Code you just need to go to the full doc comment to see the conditions for the exceptions?

That seem fine to me. Knowing that Rider is an option for the dev team makes me feel content.

I think I slightly prefer using , since it's shorter (you don't have to repeat the parameter description), and if there are multiple parameters, it's easier to tell which one can cause the exception since you don't need to map the description back to the parameter name in your mind. What do you think?

I think I agree. Originally, I was thinking that if devs had to look directly at the XML for exception descriptions, it might make sense to avoid as many inline tags as possible for readability. Perhaps it could be argued that a method with a parameter set not easily described is overly complex. But, this does of course happen, and I like the idea of super concise exception descriptions rendered in all of their glory with Rider :).

[kevinhartman]

Starting on F# exception tags next!

I'll be working in this branch.

[kevinhartman]

I'm noticing that neither VS nor Rider seem to fully support XML doc comments for F#.

VS supports text formatting for the hover dialog, but does not recognize XML in comments (i.e. symbol rename won't work):

Rider seems to have zero support:

Doc generation by the F# compiler works as expected:

@samarsha, thoughts on if we should continue? I've updated my branch with the work so far if you want to take a look.

[bamarsha]

Yeah, the IDE support for F# doc comments isn't as good as C#, unfortunately. I think it still makes sense to update the doc comments, since:

1. IDE support could improve in the future - and support in VS is not bad currently, it even shows the exception condition. :)
2. The tags will be needed if we ever generate documentation for the F# projects (which we hopefully will in the near future).
3. Even if there was no IDE support, IMO, it's still easier to read a structured doc comment with XML tags than one block of unstructured text.

[kevinhartman]

Alright, next up is to work through the doc comments one-by-one to apply any restructuring and add missing code/param refs.

I plan to do this with a few PRs (maybe one or so projects at a time) since it'll probably take longer to review structural changes.

Unrelated, there are a lot of C# doc comment warnings we're not seeing since XML documentation generation is not enabled for projects. Unlike with the F# toolchain (...as far as I can tell) there's no way to request this analysis, unless documentation generation is enabled. I wonder if it makes sense to enable documentation generation for all projects (this can be done very easily in AssemblyCommon.props), and then to suppress any warnings we don't care about at the moment (e.g. missing parameter tags, missing public member doc comments, etc.). We could also promote the warnings we do care about to errors.

Alternatively, we could leave the warnings uninhibited for people to fix as they go.

Thoughs, @samarsha?

[bamarsha]

Alright, next up is to work through the doc comments one-by-one to apply any restructuring and add missing code/param refs.

I plan to do this with a few PRs (maybe one or so projects at a time) since it'll probably take longer to review structural changes.

That would be great! I really appreciate your dedication to improving the doc comments. :)

Unrelated, there are a lot of C# doc comment warnings we're not seeing since XML documentation generation is not enabled for projects. Unlike with the F# toolchain (...as far as I can tell) there's no way to request this analysis, unless documentation generation is enabled. I wonder if it makes sense to enable documentation generation for all projects

Yeah, I think StyleCop also recommends this with rule SA0001 XmlCommentAnalysisDisabled. This sounds OK to me.

and then to suppress any warnings we don't care about at the moment (e.g. missing parameter tags, missing public member doc comments, etc.). We could also promote the warnings we do care about to errors.

Yes, this sounds like a good idea. I think there might be some overlap with StyleCop, since StyleCop also generates documentation warnings (and some of them are suppressed in .editorconfig). But if the C# compiler can validate doc comments, I think it would be better to use it instead.

As for turning the warnings into errors, if the warnings are for things like missing documentation, I'm worried it might be annoying during development to have to write filler doc comments to get the code to compile when you're just testing it. It might be nice if the missing documentation errors could only happen in CI, so PRs would still need to have documentation. Warnings for invalid documentation seems fine to promote to errors locally, though.

Alternatively, we could leave the warnings uninhibited for people to fix as they go.

When we added StyleCop to our projects, I tried this by leaving the warning for undocumented public types/members on, but I don't think it has caused any progress to happen, and the excessive warnings are kind of annoying. :) So I think I would lean towards suppressing any warnings we don't want to fix right now. Alternatively, maybe we can suppress warnings for individual types/members using an attribute or compiler directive, with a note to fix them later?

[kevinhartman]

I think there might be some overlap with StyleCop, since StyleCop also generates documentation warnings

Ah! That's helpful context. I'd tried to suppress a few warnings at one point using <NoWarn> in AssemblyCommon.props, without success. I wonder if StyleCop was somehow overriding that.

As for turning the warnings into errors, if the warnings are for things like missing documentation, I'm worried it might be annoying during development to have to write filler doc comments to get the code to compile

Completely agree. If we do promote any warnings to errors, I think it should just be those corresponding to bad XML and incorrect code references for now. At some point, it might be nice to do this for all doc issues, but only for this toolchain's public interface (whatever that may be), so that generated docs we expect people to work with don't have inherent issues.

It might be nice if the missing documentation errors could only happen in CI, so PRs would still need to have documentation.

Are you suggesting that only changed / added code be subjected to full doc validation in PRs? Any idea if there's an existing tool that can do this? In theory, a script could be added to CI that does a Git diff of the target branch (i.e. main) and the PR branch, and then cross-references the changes to the warnings emitted by the C# compiler to fail the PR only for changed elements with warnings. But this seems somewhat complicated.

Alternatively, maybe we can suppress warnings for individual types/members using an attribute or compiler directive, with a note to fix them later?

I think this would be a good solution to preventing new code without proper docs from being merged, since we'd sort of grandfather-in existing code. But then, there'd potentially be many places to look for suppressed warnings. I wonder if it'd be better to try to identify (or refactor to, if necessary) individual projects that define the public interface, and hold those projects to a higher standard for XML doc validation.

tl;dr

A lot of this is probably outside the scope of this issue, but worth discussing at least for the future.

For now, I'll be working through the original goals of this issue, and then ideally submitting a PR to enable doc generation with warning suppression for anything other than invalid XML and incorrect code refs / names for all projects (@samarsha, could you perhaps add this point to the issue checklist?).

[kevinhartman]

Also, I just made this tool to help with manually fixing C# XML doc comments.

CSharpDocRewriter

I'm planning to use it to work through refactoring one project at a time, starting with CompilationManager.

If anyone else wants to help with this effort, maybe they can make use of this too. I'll continue to update this issue with which project I'm actively working through as I go to avoid toe-stepping.

[bamarsha]

@kevinhartman Just getting back into this, sorry for the delayed response. :)

Ah! That's helpful context. I'd tried to suppress a few warnings at one point using <NoWarn> in AssemblyCommon.props, without success. I wonder if StyleCop was somehow overriding that.

I'm not sure about <NoWarn>, but we use .editorconfig to control the StyleCop warnings. By the way, I made PR #807 to address all of the remaining StyleCop warnings (either by fixing the issues or disabling the warnings).

Are you suggesting that only changed / added code be subjected to full doc validation in PRs? Any idea if there's an existing tool that can do this? In theory, a script could be added to CI that does a Git diff of the target branch (i.e. main) and the PR branch, and then cross-references the changes to the warnings emitted by the C# compiler to fail the PR only for changed elements with warnings. But this seems somewhat complicated.

Oh, I wasn't thinking of running the validation only on the diff - I meant that the validation could be an extra step that PRs run, as opposed to using something like WarningsAsErrors for missing documentation, since I think that would prevent local builds from succeeding until doc comments are added. In other words I see three different levels of validation:

1. Invalid code that cannot be compiled and has to be fixed immediately.
2. WarningsAsErrors for code that could be compiled, but the warnings are important enough and/or easy enough to fix that they should be addressed immediately - e.g. we treat C# nullable reference type warnings as errors.
3. Additional validation that only needs to be addressed before the PR is merged, but can be ignored in local builds - e.g. missing documentation, code style violations, etc.

I think this would be a good solution to preventing new code without proper docs from being merged, since we'd sort of grandfather-in existing code. But then, there'd potentially be many places to look for suppressed warnings. I wonder if it'd be better to try to identify (or refactor to, if necessary) individual projects that define the public interface, and hold those projects to a higher standard for XML doc validation.

I think it might be tricky to come up with a clean separation like this, unless I'm misunderstanding. For example, each project has to have at least some public API in order to be used by other projects (there's no "within solution" accessibility level between public and internal).

For now, I'll be working through the original goals of this issue, and then ideally submitting a PR to enable doc generation with warning suppression for anything other than invalid XML and incorrect code refs / names for all projects (@samarsha, could you perhaps add this point to the issue checklist?).

Sounds good! Done. :)

Also, I just made this tool to help with manually fixing C# XML doc comments.

CSharpDocRewriter

Wow, this looks awesome! Although I'm also a little sorry that our doc comments needed a tool like this to be developed. :)

[kevinhartman]

By the way, I made PR #807 to address all of the remaining StyleCop warnings

Awesome! I will wait for that to go in before fiddling with any warning / error configurations. Feel free to pick that up as well if you feel strongly about getting those changes in quickly. This is the preliminary list of codes I thought might make sense to promote to error:

Code	Desc.
CS1570	bad XML
CS1571	duplicate param tag name
CS1572	param tag name not a param
CS1574	bad cref
CS1587	XML comment appears before something not documentable

since I think that would prevent local builds from succeeding until doc comments are added

Oh! That makes total sense. I think I somehow missed your point originally—sorry for the confusion! Yes, I completely agree that it only makes sense to promote invalid documentation issues to error.

1. ...
2. ...
3. Additional validation that only needs to be addressed before the PR is merged, but can be ignored in local builds - e.g. missing documentation, code style violations, etc.

Sounds great! Perhaps that could eventually be as simple as just failing the CI build step if any warnings are emitted.

I think it might be tricky to come up with a clean separation like this, unless I'm misunderstanding.

I think I was imagining that some of the Q# compiler's library projects might be consumed (i.e. as Nuget packages) from other solutions. In that case, I was wondering if perhaps the public interfaces of those libraries should be more rigorously documented :)

Wow, this looks awesome! Although I'm also a little sorry that our doc comments needed a tool like this to be developed. :)

:D

It probably wasn't necessary, but it makes it easier for me to focus on a task like this to click less :) I recently added support to visit only XML doc comments that have been touched by a specific author (uses Git blame under the hood).

Anyways, still working through CompilationManager. I should hopefully be able to make faster progress this week, as things have been busy with the holidays.

[bamarsha]

Awesome! I will wait for that to go in before fiddling with any warning / error configurations.

807 is merged now. :)

This is the preliminary list of codes I thought might make sense to promote to error:

Looks reasonable to me!

Sounds great! Perhaps that could eventually be as simple as just failing the CI build step if any warnings are emitted.

Currently, the compiler does build with several warnings right now. But yes, I think it would be a great goal to try to eliminate all warnings and turn them into errors in CI.

I think I was imagining that some of the Q# compiler's library projects might be consumed (i.e. as Nuget packages) from other solutions. In that case, I was wondering if perhaps the public interfaces of those libraries should be more rigorously documented :)

Yeah, the compiler projects are part of the Microsoft.Quantum.Compiler package and can be used by other solutions - but this applies to all projects equally, and anything public in any of the compiler projects is part of the public API for the NuGet package. So I'm not sure if it's possible to say that some projects have more rigorous documentation standards than other projects.

Anyways, still working through CompilationManager. I should hopefully be able to make faster progress this week, as things have been busy with the holidays.

Sounds good, let me know how it goes. :)

[filipw]

Somewhat related, I noticed something that might be worth considering: neither Visual Studio nor VS Code seem to render exception descriptions, at least in the floating window that pops up from the callsite:

This is a limitation of the quick info provider in Roslyn - https://github.com/dotnet/roslyn/issues/1515 OmniSharp also uses quick info from Roslyn hence the problem is visible there too.