Add solutions to remove invalid metadata directives in documentation comments #1277

agisilaos · 2025-08-15T08:00:06Z

Fixes: #1111

Summary

This PR addresses issue #1111 by adding automatic solutions for the org.swift.docc.Metadata.Invalid<directive>InDocumentationComment diagnostic warnings. Previously, when users included invalid metadata directives in documentation comments (such as @DisplayName, @PageKind, @CallToAction, etc.), DocC would only emit warnings without providing any way to automatically fix the issues.

Expected User Experience:

When users include invalid metadata directives in documentation comments, they now receive warnings with actionable solutions
Each warning includes a "Remove invalid '' directive" solution that can be applied automatically
This eliminates the need for manual editing and provides immediate feedback on how to fix the issue

Implementation Overview:

Enhanced the validateForUseInDocumentationComment method in Metadata.swift to include solutions for each invalid directive
Each solution uses a Replacement with an empty string to remove the problematic directive
Follows the established pattern used throughout the codebase for directive removal solutions
Added comprehensive test coverage to verify solutions are provided correctly

Dependencies

This PR has no external dependencies. It builds upon the existing solution infrastructure already present in the codebase and follows established patterns for directive removal.

Testing

Steps:

Provide setup instructions.
- Check out the feature branch: git checkout feature/add-solutions-for-invalid-metadata-directives
- Build the project: swift build
- Run the test suite: swift test
Explain in detail how the functionality can be tested.
- Run the specific test: swift test --filter testEmitsWarningsForInvalidMetadataChildrenInDocumentationComments
- Run metadata tests: swift test --filter MetadataTests
- Run symbol tests: swift test --filter SymbolTests
- Run comprehensive tests: swift test --filter "MetadataTests|SymbolTests" --parallel
Test Content to Verify:
- The existing test testEmitsWarningsForInvalidMetadataChildrenInDocumentationComments now verifies that each problem has exactly one solution
- New test testSolutionForInvalidMetadataDirectiveRemovesDirective demonstrates the solution functionality
- All tests verify that solutions have the correct summary format and replacement properties
Manual Testing:
- Create a Swift file with invalid metadata directives in documentation comments:
```
/**
 @Metadata {
   @DisplayName("Invalid in doc comments")
   @PageKind(sampleCode)
 }
 */
public func testFunction() {}
```
- Run DocC on the file and verify that warnings include solutions to remove the directives

Checklist

Added tests
Ran the ./bin/test script and it succeeded
Updated documentation if necessary

…comments - Implement solutions for org.swift.docc.Metadata.Invalid<directive>InDocumentationComment diagnostics - Each invalid directive now has a solution that removes the directive - Follows the same pattern as other directive removal solutions in the codebase - Added comprehensive tests to verify solutions are provided correctly - Fixes issue swiftlang#1111

d-ronnqvist

Thanks for opening this PR.

The code generally looks good but it would be good to avoid repeating the code that creates this diagnostic and only conditionally create solutions or no solutions.

d-ronnqvist · 2025-08-15T15:24:56Z

Tests/SwiftDocCTests/Semantics/SymbolTests.swift

+
+            let replacement = try XCTUnwrap(solution.replacements.first)
+            XCTAssertEqual(replacement.replacement, "", "Replacement should be empty string to remove the directive")
+            XCTAssertNotNil(replacement.range, "Replacement should have a valid range")


minor (non-blocking): It might be worth verifying that the range covers the expected portion of the content.

One way to check that could be to compare it against the range of the problem itself.

Fixed it here: 6af147f

d-ronnqvist · 2025-08-15T15:27:12Z

Sources/SwiftDocC/Semantics/Metadata/Metadata.swift

@@ -223,18 +223,38 @@ public final class Metadata: Semantic, AutomaticDirectiveConvertible {
            .map { (type(of: $0).directiveName, $0.originalMarkup.range) }

        problems.append(
-            contentsOf: namesAndRanges.map { (name, range) in
-                Problem(
+            contentsOf: namesAndRanges.compactMap { (name, range) in


Question; why change this to compactMap? At a glance I don't see optional return values in this closure.

That was a mistake, good catch I fixed it!

d-ronnqvist · 2025-08-15T15:30:17Z

Tests/SwiftDocCTests/Semantics/SymbolTests.swift

+
+        let replacement = try XCTUnwrap(solution.replacements.first)
+        XCTAssertEqual(replacement.replacement, "")
+        XCTAssertNotNil(replacement.range)


minor: Since this is checking a single specific directive it's easier to verify that expected range here than in the other test that loops over list of warnings.

Fixed: 6af147f

d-ronnqvist · 2025-08-15T15:49:18Z

Sources/SwiftDocC/Semantics/Metadata/Metadata.swift

+                    return Problem(
+                        diagnostic: Diagnostic(
+                            source: symbolSource,
+                            severity: .warning,
+                            range: range,
+                            identifier: "org.swift.docc.\(Metadata.directiveName).Invalid\(name)InDocumentationComment",
+                            summary: "Invalid use of \(name.singleQuoted) directive in documentation comment; configuration will be ignored",
+                            explanation: "Specify this configuration in a documentation extension file"
+                        )
+                    )


minor: It would be nice to avoid repeating this code (especially the user-facing string that are most likely to be updated in the future). Otherwise there's a risk that someone improves the summary or explanation of this warnings but only makes the change in one of these places.

You can avoid that repetition by only creating the [Solution], not the full Problem, inside the if let range check. If the range is nil you can fall back to []. That gives you a value in both code paths of the if-else-statement that you can pass to the only code path that creates this diagnostic/problem.

Fixed it here: d095925

d-ronnqvist · 2025-08-15T15:53:21Z

Tests/SwiftDocCTests/Semantics/SymbolTests.swift

+        XCTAssertEqual(replacement.replacement, "")
+        XCTAssertNotNil(replacement.range)
+
+        // The replacement should remove the entire @DisplayName directive


Question: are these comments referring to the test assertions above? If so, you can move them each into the "message" of the respective test assertions above.

Fixed: 8cec277

- Create diagnostic once instead of duplicating it - Use conditional solutions array based on range availability - Single Problem creation path for better maintainability

- Verify replacement range matches problem's diagnostic range - Ensures solution targets the correct content - Improves test coverage for range accuracy

- Move explanatory comments into assertion message parameters - Provides better context when tests fail - Makes tests more self-documenting

agisilaos changed the title ~~Add solutions to remove invalid metadata directives in documentation …~~ Add solutions to remove invalid metadata directives in documentation comments Aug 15, 2025

d-ronnqvist reviewed Aug 15, 2025

View reviewed changes

agisilaos added 3 commits August 16, 2025 07:51

Fix code duplication in validateForUseInDocumentationComment method

d095925

- Create diagnostic once instead of duplicating it - Use conditional solutions array based on range availability - Single Problem creation path for better maintainability

Add range verification test for invalid metadata directive solutions

6af147f

- Verify replacement range matches problem's diagnostic range - Ensures solution targets the correct content - Improves test coverage for range accuracy

Move test comments into assertion messages for better clarity

8cec277

- Move explanatory comments into assertion message parameters - Provides better context when tests fail - Makes tests more self-documenting

agisilaos requested a review from d-ronnqvist August 16, 2025 06:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add solutions to remove invalid metadata directives in documentation comments #1277

Add solutions to remove invalid metadata directives in documentation comments #1277

agisilaos commented Aug 15, 2025

Uh oh!

d-ronnqvist left a comment

Uh oh!

d-ronnqvist Aug 15, 2025

Uh oh!

agisilaos Aug 16, 2025

Uh oh!

d-ronnqvist Aug 15, 2025

Uh oh!

agisilaos Aug 16, 2025

Uh oh!

d-ronnqvist Aug 15, 2025

Uh oh!

agisilaos Aug 16, 2025

Uh oh!

d-ronnqvist Aug 15, 2025

Uh oh!

agisilaos Aug 16, 2025

Uh oh!

d-ronnqvist Aug 15, 2025

Uh oh!

agisilaos Aug 16, 2025

Uh oh!

Uh oh!

Add solutions to remove invalid metadata directives in documentation comments #1277

Are you sure you want to change the base?

Add solutions to remove invalid metadata directives in documentation comments #1277

Conversation

agisilaos commented Aug 15, 2025

Summary

Dependencies

Testing

Checklist

Uh oh!

d-ronnqvist left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!