Skip to content

Update AV2305: Document all public, protected and internal types and members#381

Open
dennisdoomen wants to merge 1 commit intodevelopfrom
copilot/pr298-update-av2305
Open

Update AV2305: Document all public, protected and internal types and members#381
dennisdoomen wants to merge 1 commit intodevelopfrom
copilot/pr298-update-av2305

Conversation

@dennisdoomen
Copy link
Copy Markdown
Owner

This PR updates guideline AV2305.

It was split out of #298 so the change can be reviewed independently.

Files:

  • _rules/2305.md

Part of the replacement for #298.

Split from #298.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

**Note:** For internal types and members that are only used within a single assembly, XML documentation is optional. However, a brief summary comment can still be helpful for future maintainers.

**Note:** You don't need to use `/// <inheritdoc/>` on overriding or implementing members. Visual Studio and Rider will automatically inherit documentation from the base type or interface.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure about that? DocFX certainly doesn't pick them up when emitted. There's an additional pitfall to watch out for; the following is wrong:

/// <summary>A worker</summary>
public interface IWorker;
/// <inheritdoc/>
public class Worker: IWorker;

Instead, it needs to be:

/// <inheritdoc cref="IWorker"/>
public class Worker: IWorker;

Without the cref, you'll get the documentation for System.Object on Worker.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've never reused documentation from the interface itself on the implementing types. I've only done this for members. And yes, even if you omit that tag, both Rider and R# will display the right documentation...

Image

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I didn't know about that. Vanilla VS behaves the same. I know that Roslyn provides richer information when the source is available (in the same solution), so I tried referencing an assembly (with an .xml file next to it, but not a .pdb), and even then, it works for docs on members. But once <GenerateDocumentationFile>True</GenerateDocumentationFile> is added to the project file, it still emits CS1591 warnings.

{A187F7EB-7296-402C-8E3E-C35C762869F7}

But apparently, the warnings are by design:

That inheritdoc behavior is purely an IDE-time thing to help with features like quick-info. It's not a feature of the language, and docs are not inherited as far as the compiler and the rules around doc files are concerned. So the compiler is working as intended here.

Also, it doesn't work on types at all.

image

What API documentation generator is used by FluentAssertions, and does that also show docs on members from the interface without using <inheritdoc/>?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I never inherit type-level documentation from an interface or base-class. I always write something new as the type is different and usually has a different purpose.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's less common, I suppose in depends on the kind of project. For example, a public library with well-documented interfaces for extensibility plus internal default implementations.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even then, I would never inherit the interface-level docs. The interface defines a contract that the implementing class fulfill. That class has a specific functional purpose within that code base, which I document.

So my suggestion is to keep my note, but mention something like "...unless your specific tool needs it"

Copy link
Copy Markdown
Collaborator

@bkoelman bkoelman Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an example:

// lots of docs, describing lifetime, constraints, which errors to raise, etc.
public interface IRepository<T>;

/// <inheritdoc cref="IRepository{T}"/>
internal class ArticleRepository : IRepository<Article>;

Can you answer the following?

What API documentation generator is used by FluentAssertions, and does that also show docs on members from the interface without using <inheritdoc/>?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an example:

// lots of docs, describing lifetime, constraints, which errors to raise, etc.
public interface IRepository;

///
internal class ArticleRepository : IRepository

;

Even in this example, I really don't see the value of inheriting the documentation from the interface. In this particular case, most of the code only has to deal with the interface and never see the implementation. And for the folks that need to maintain the implementation, they can read the docs from the interface to understand how their implementation should behave.

What API documentation generator is used by FluentAssertions, and does that also show docs on members from the interface without using ?

What generator are you talking about? The .xml files are generated by the dotnet build CLI.

---
Documenting your code allows Visual Studio, [Visual Studio Code](https://code.visualstudio.com/) or [Jetbrains Rider](https://www.jetbrains.com/rider/) to pop-up the documentation when your class is used somewhere else. Furthermore, by properly documenting your classes, tools can generate professionally looking class documentation.

**Note:** For internal types and members that are only used within a single assembly, XML documentation is optional. However, a brief summary comment can still be helpful for future maintainers.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This used to be a solid rule, but now it has become subjective. It depends on how each developer feels about when something is helpful, and making it optional means that some developers will never add them. This defeats the goal of a universal style in the codebase, regardless of who implements it.

The accompanying analyzer flags when internal members are not documented (the other types are already covered by other tools). How is the analyzer supposed to handle the changed rule?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're suggesting to remove the note completely?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion on what should be documented on internal members, but I think the rule should be deterministic. Some options:

  • Don't document internal types/members at all
  • Internal types/members require at least a summary
  • Internal types/members need to be fully documented (summary, type parameters, parameters, and return value)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For internal types and members that are only used within a single assembly

This implies that when using InternalsVisibleTo (for example, for tests), internal types and members must be fully documented. Is that desired?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion on what should be documented on internal members, but I think the rule should be > deterministic. Some options:

Don't document internal types/members at all
Internal types/members require at least a summary
Internal types/members need to be fully documented (summary, type parameters, parameters, and return value)

This is a problem for me. I want people to document everything, but only if it adds value. I don't want to be dogmatic about it. Repeating the name of the method in a single-line doc is a waste of effort. And quite often I don't even bother documenting some parameters or return values. E.g. nobody needs to understand what a CancellationToken is or what the purpose is of the returned Task.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I totally agree with that, and typically solve that by adding:

/// <summary/>

This expresses a clear intent that I considered, but decided not to document, while still getting warnings for new code. The empty marker reminds reviewers to assess whether documentng is desirable.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the tools get it in the way, I disable the tool. These days, I use an AI Skill to let it judge the need for that.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If everyone on the team, including unexperienced developers, start disabling tools that annoy them, chaos emerges. The tools provide a safety net for a reason, their goal is to alert: warn early to prevent long-term problems. A suppression is fine when a justification can be provided (or is obvious). Delegating to AI without thinking about it violates the rule recently introduced: "I have no idea, the AI did that."

There's always a balance: a tool with primarily false positives is counterproductive and should be disabled. A team may decide this rule (always document) doesn't apply to their codebase and delete it (and turn off its analyzer). For teams that do care, requiring at least a summary (potentially empty) for internal types and members seems reasonable to me. The analyzer can be adapted accordingly to remain silent on missing parameter/return docs if the summary is empty (as opposed to absent).

While teams can easily edit the text of rules, they can't easily adapt the analyzer. So the question is: what would strike a good balance for the majority of users that take this rule as-is?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, in that case, we remove that note as the consensus seems to be to also document internal types and members. A tech lead or architect can always reduce the level of that rule to suggestion

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants