DC0006, DC0007-DC0010 Documentation Cop

[MODUSCarstenScholling]

#341

DC0006: Internal procedures must include XML documentation comments
DC0007: Public objects must include XML documentation comments
DC0008: Internal objects must include XML documentation comments
DC0009: Events must must include XML documentation comments
DC0010: Internal events must include XML documentation comments

[Arthurvdv]

Code review — PR #344: DC0006–DC0010 Documentation Cop

Hi Carsten 👋 — thanks a lot for this, and welcome aboard as a first-time contributor! 🎉 This is a substantial, well-structured addition: five new rules, a clean split between object- and procedure-level analyzers, and genuinely thorough test fixtures (public/internal × procedure/object/event, plus attribute and comment variants). The resx messages are clear and helpful. Really nice work.

Below are a few things worth a second look. None are dealbreakers — mostly correctness/consistency points we can tidy up together. CI is green across all BC versions, and I verified locally that dotnet test passes for both new analyzers.

🔧 Worth fixing

1. Severity + enablement on the new descriptors (DiagnosticDescriptors.cs)
DC0006, DC0008, DC0009, DC0010 are defaultSeverity: Hidden, isEnabledByDefault: true, while DC0007 is Info, isEnabledByDefault: true. Two thoughts:

• I'd suggest Info + isEnabledByDefault: false for the documentation rules. A documentation cop is inherently opt-in and noisy on existing codebases; Hidden + enabled is an awkward combo (it runs but never surfaces in build/IDE output), whereas Info + disabled gives a clean "opt in, then see Info squiggles" experience.
• There's also an internal inconsistency: DC0009 (EventRequiresDocumentation) is described as "integration/business events are part of the exposed API" (i.e. public surface), yet it's Hidden, while DC0007 (public objects) is Info. Whatever policy we pick, it'd be good to apply it consistently across DC0006–DC0010.

2. Drop the FileSystem is null guard in ObjectRequiresDocumentation (ObjectRequiresDocumentation.cs:34)
This looks copy-pasted from PermissionSetCoverage.cs:36, which needs it because it reads permission-set documents off disk. Here the analyzer never touches the file system (docs come from GetDocumentationCommentXml()), so the guard serves no purpose — and since Compilation.FileSystem can be null in some IDE contexts (per analyzer-development.md:250), it could silently suppress DC0007/DC0008 there. Tests pass today because RoslynTestKit supplies a file system on this path, so it's safe to just remove the two lines. (If there is a reason to keep it, a short comment explaining the case would help future readers.)

3. ObjectRequiresDocumentation is missing the IsObsolete() guard (ObjectRequiresDocumentation.cs:32)
The project convention (CLAUDE.md) is to call ctx.IsObsolete() at the top of every analysis method, and a SymbolAnalysisContext.IsObsolete() extension already exists (AnalysisContextExtensions.cs:8). ProcedureRequiresDocumentation already does this (line 28), but the object analyzer doesn't — so obsolete objects (e.g. ObsoleteState = Pending/Removed) will still be flagged for missing docs. Suggest adding if (ctx.IsObsolete()) return; after the symbol cast.

4. Case-sensitive attribute matching (SyntaxNodeExtensions.cs:12-19)

a.Name.Identifier.ToString() == "IntegrationEvent" / "BusinessEvent" / "InternalEvent"

AL is case-insensitive, so [integrationevent(...)] or [BusinessEVENT] are valid but won't match this ordinal ==. Because events are declared local procedure, a miss doesn't just misclassify — it falls through to the procedure branch, hits the LocalKeyword check, and returns with no diagnostic at all (DC0009/DC0010 false-negative). The project guidance is to use SemanticFacts.IsSameName() for AL identifiers — or see 5) for a cleaner fix.

5. The robust overloads exist but aren't used (MethodSymbolInterfaceExtensions.cs:28-35)
You added symbol-based IsIntegrationOrBusinessEvent / IsInternalEvent that match on AttributeKind (case-insensitive, reliable) — but the analyzer calls the syntax/string versions instead, leaving the symbol-based ones as dead code. In a RegisterSyntaxNodeAction on MethodDeclaration, ctx.ContainingSymbol is the IMethodSymbol, so switching to the symbol-based overloads would fix 4) and remove the duplication in one move.

🤔 Questions / minor

6. Internal integration events → DC0009 or DC0010? (ProcedureRequiresDocumentation.cs:41-58)
The event branch runs before accessibility checks, and IsIntegrationOrBusinessEvent is tested first. So an internal-scoped [IntegrationEvent] reports as DC0009 (worded as "exposed/public API") rather than DC0010 (which only fires on the distinct [InternalEvent] attribute). Looks intentional (classify by event type, not container accessibility), and there's no fixture for this combo — just confirming it matches the spec.

7. Duplicate IsTestCodeunit (ProcedureRequiresDocumentation.cs:94)
There's already a null-safe IsTestCodeunit extension in ALCops.Common. The local private copy takes a non-nullable IObjectTypeSymbol and duplicates the logic — reusing the common one would keep behavior consistent. Minor.

✅ Things I liked

• Test coverage is excellent — the attribute/comment/parameter permutations are exactly right.
• Splitting object vs procedure analyzers is clean and maps nicely to the rule IDs.
• Reuses the established HasXmlDocumentation trivia approach, keeping it consistent with DC0004.

CI is green across all BC versions and both new analyzers pass locally, so functionally it's solid — these are about robustness and consistency. I also confirmed the IntegrationEventInInternalCodeunit refactor (dropping the explicit obsolete check) is not a regression: ctx.IsObsolete() already covers the containing object. Happy to pair on any of these. Thanks again for a great first contribution! 🙌

---

Note: I didn't write this review myself — it was generated with the help of an AI assistant.

[MODUSCarstenScholling]

1. The choice between “Hidden” and “Info” was based on visibility: “Internal” = “Hidden,” “Public” = “Info.” However, setting ‘Internal’ to “Info/Enabled = false” is the better choice. For DC0009, I have now selected “Info/true” since it is also public. DC0009 was hidden initially, because it is new and probably unusual for other developers. But now it makes more sense in general.

• However: The test kit does not set it to active. So the tests fail when isEnabledByDefault is false and diag is expected. What should we do?

2. Removed FileSystem is null guard in ObjectRequiresDocumentation (ObjectRequiresDocumentation.cs:34).
3. Added obsolete check and return.
4. Makes totally sense. Removed in favor for 5.
5. Check
6. This is by intention. The method scope in events has nothing to do with InternalEvent.
7. Check

Before I push, I need clarification about the issue with 1 and the failing tests when isEnabledByDefault = false.

P.S.:
I have seen that 4 tests regarding PermissionDeclarationOrder are failing locally with CR/LF issues. Should I have a look?

HasFix("SingleLineToMultiLine") (2s 560ms): Error Message: RoslynTestKit.TransformedCodeDifferentThanExpectedException : Transformed code is different than expected:
      ===========================
      From line 3:
      - ····Permissions·=·tabledata·Alpha·=·R,␍␊
      + ····Permissions·=·tabledata·Alpha·=·R,␊

[Arthurvdv]

Thanks for look into the feedback (and your quick response!)

On 1) - Oops, that was indeed still on the backlog. I've updated main where you now can set a .ruleset file, see the examples on d143bb9

I've also reworked the RoslynTestKit, where from v1.3.0 it should handle CR/LF without any errors.

So if you would merge the main back into your branch, the depedency to RoslynTestKit should be updated to v1.4.0 (which support the .ruleset file)

[Arthurvdv]

Looks great! Thanks a lot for the PR (and the rework). Let's get this merged!

[MODUSCarstenScholling]

Great. If I find some time next week, I'll do the Naming pattern stuff.