Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions Sources/SwiftDocC/Infrastructure/DocumentationContext.swift
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that this code is checking for all 3 scenarios, there are 2 things that I notice:

  • The 3 checks and diagnostics are fairly similar, both conceptually (diagnosing multiple root pages of some sort) and in code (doing similar checks and creating similar diagnostics and solutions).
  • It's odd to me that these 3 checks happen in 3 different places.

It would be good to address both of these by extracting all 3 into a common place (for example private func emitWarningsForMultipleRootPages() {...}) and reduce some of that duplication.

Original file line number Diff line number Diff line change
Expand Up @@ -1317,6 +1317,22 @@ public class DocumentationContext {
uniqueRelationships.formUnion(unifiedSymbolGraph.orphanRelationships)
}

// Warn when the documentation contains more than one main module
if moduleReferences.count > 1 {
let diagnostic = Diagnostic(
source: nil,
severity: .warning,
range: nil,
identifier: "org.swift.docc.MultipleMainModules",
summary: "Documentation contains more than one main module",
explanation: """
The documentation inputs contain symbol graphs for more than one main module: \(moduleReferences.keys.sorted().joined(separator: ", ")).
This may lead to unexpected behaviors in the generated documentation.
"""
)
diagnosticEngine.emit(Problem(diagnostic: diagnostic))
}

try shouldContinueRegistration()

// Only add the symbol mapping now if the path hierarchy based resolver is the main implementation.
Expand Down Expand Up @@ -2309,6 +2325,38 @@ public class DocumentationContext {
let (tutorialTableOfContentsResults, tutorials, tutorialArticles, allArticles, documentationExtensions) = result
var (otherArticles, rootPageArticles) = splitArticles(allArticles)

// Warn when the documentation contains more than one root page
if rootPageArticles.count > 1 {
let extraRootPageProblems = rootPageArticles.map { rootPageArticle -> Problem in
let diagnostic = Diagnostic(
source: rootPageArticle.source,
severity: .warning,
range: rootPageArticle.value.metadata?.technologyRoot?.originalMarkup.range,
identifier: "org.swift.docc.MultipleTechnologyRoots",
summary: "Documentation contains more than one root page",
explanation: """
The documentation contains \(rootPageArticles.count) articles with \(TechnologyRoot.directiveName.singleQuoted) directives.
Only one article should be marked as a technology root to avoid unexpected behaviors.
"""
)

guard let range = rootPageArticle.value.metadata?.technologyRoot?.originalMarkup.range else {
return Problem(diagnostic: diagnostic)
}

let solution = Solution(
summary: "Remove the \(TechnologyRoot.directiveName.singleQuoted) directive",
replacements: [
Replacement(range: range, replacement: "")
]
)

return Problem(diagnostic: diagnostic, possibleSolutions: [solution])
}

diagnosticEngine.emit(extraRootPageProblems)
}

let globalOptions = (allArticles + documentationExtensions).compactMap { article in
return article.value.options[.global]
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -169,4 +169,175 @@ class DocumentationContext_RootPageTests: XCTestCase {

XCTAssertEqual(context.problems.count, 0)
}

func testWarnsAboutMultipleTechnologyRootDirectives() async throws {
let (_, context) = try await loadBundle(catalog:
Folder(name: "multiple-roots.docc", content: [
TextFile(name: "FirstRoot.md", utf8Content: """
# First Root
@Metadata {
@TechnologyRoot
}
This is the first root page.
"""),

TextFile(name: "SecondRoot.md", utf8Content: """
# Second Root
@Metadata {
@TechnologyRoot
}
This is the second root page.
"""),

TextFile(name: "ThirdRoot.md", utf8Content: """
# Third Root
@Metadata {
@TechnologyRoot
}
This is the third root page.
"""),

InfoPlist(displayName: "TestBundle", identifier: "com.test.example"),
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI (non-blocking): This Info.plist file hasn't been needed for some time. I understand that you looked at other tests in this file and followed what they were doing (which they likely did for historical reasons) but this test (and the other test) would work the same without this file because the test doesn't verify any of the information that the Info.plist file configures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed here: 4ec94c0

])
)

// Verify that we emit warnings for multiple TechnologyRoot directives
let multipleRootsProblems = context.problems.filter { $0.diagnostic.identifier == "org.swift.docc.MultipleTechnologyRoots" }
XCTAssertEqual(multipleRootsProblems.count, 3, "Should emit warnings for all three TechnologyRoot directives")

// Verify the warnings are associated with the correct files
let problemSources = multipleRootsProblems.compactMap { $0.diagnostic.source?.lastPathComponent }.sorted()
XCTAssertEqual(problemSources, ["FirstRoot.md", "SecondRoot.md", "ThirdRoot.md"])

// Verify each warning has a solution to remove the TechnologyRoot directive
for problem in multipleRootsProblems {
XCTAssertEqual(problem.possibleSolutions.count, 1)
let solution = problem.possibleSolutions.first!
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: If this is nil it will trap and stop running the remainder of the tests. A more defensive solution would be to use XCTUnwrap to handle the nil value. This will gracefully fail the tests by reporting a test failure without interrupting other tests.

Suggested change
let solution = problem.possibleSolutions.first!
let solution = try XCTUnwrap(problem.possibleSolutions.first)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed here: aa33e18

XCTAssertEqual(solution.summary, "Remove the 'TechnologyRoot' directive")
XCTAssertEqual(solution.replacements.count, 1)
}
}

func testWarnsAboutMultipleMainModules() async throws {
// Create a bundle with multiple symbol graphs for different modules
let (_, context) = try await loadBundle(catalog:
Folder(name: "multiple-modules.docc", content: [
// First module symbol graph
TextFile(name: "ModuleA.symbols.json", utf8Content: """
{
"metadata": {
"formatVersion": {
"major": 1,
"minor": 0,
"patch": 0
},
"generator": "unit-test"
},
"module": {
"name": "ModuleA",
"platform": {
"architecture": "x86_64",
"vendor": "apple",
"operatingSystem": {
"name": "macosx",
"minimumVersion": {
"major": 10,
"minor": 15
}
},
"environment": null
}
},
"symbols": [
{
"identifier": {
"precise": "ModuleA",
"interfaceLanguage": "swift"
},
"names": {
"title": "ModuleA",
"navigator": null,
"subHeading": null,
"prose": null
},
"pathComponents": ["ModuleA"],
"docComment": null,
"accessLevel": "public",
"kind": {
"identifier": "module",
"displayName": "Module"
},
"mixins": {}
}
],
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI: The module isn't a defined symbol in the file. Instead DocC creates the module symbol when it processes the rest of the symbol graph data.

The correct way do define a minimal (empty) symbol graph file would be "symbols": [],

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed it here: ded8478

"relationships": []
}
"""),

// Second module symbol graph
TextFile(name: "ModuleB.symbols.json", utf8Content: """
{
"metadata": {
"formatVersion": {
"major": 1,
"minor": 0,
"patch": 0
},
"generator": "unit-test"
},
"module": {
"name": "ModuleB",
"platform": {
"architecture": "x86_64",
"vendor": "apple",
"operatingSystem": {
"name": "macosx",
"minimumVersion": {
"major": 10,
"minor": 15
}
},
"environment": null
}
},
"symbols": [
{
"identifier": {
"precise": "ModuleB",
"interfaceLanguage": "swift"
},
"names": {
"title": "ModuleB",
"navigator": null,
"subHeading": null,
"prose": null
},
"pathComponents": ["ModuleB"],
"docComment": null,
"accessLevel": "public",
"kind": {
"identifier": "module",
"displayName": "Module"
},
"mixins": {}
}
],
"relationships": []
}
"""),
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a makeSymbolGraph(moduleName:platform:symbols:relationships:) helper that mosts tests use. It makes the test significantly shorter and easier to read and helps with data correctness as well compared to spelling out the raw JSON string.

Suggested change
TextFile(name: "ModuleA.symbols.json", utf8Content: """
{
"metadata": {
"formatVersion": {
"major": 1,
"minor": 0,
"patch": 0
},
"generator": "unit-test"
},
"module": {
"name": "ModuleA",
"platform": {
"architecture": "x86_64",
"vendor": "apple",
"operatingSystem": {
"name": "macosx",
"minimumVersion": {
"major": 10,
"minor": 15
}
},
"environment": null
}
},
"symbols": [
{
"identifier": {
"precise": "ModuleA",
"interfaceLanguage": "swift"
},
"names": {
"title": "ModuleA",
"navigator": null,
"subHeading": null,
"prose": null
},
"pathComponents": ["ModuleA"],
"docComment": null,
"accessLevel": "public",
"kind": {
"identifier": "module",
"displayName": "Module"
},
"mixins": {}
}
],
"relationships": []
}
"""),
// Second module symbol graph
TextFile(name: "ModuleB.symbols.json", utf8Content: """
{
"metadata": {
"formatVersion": {
"major": 1,
"minor": 0,
"patch": 0
},
"generator": "unit-test"
},
"module": {
"name": "ModuleB",
"platform": {
"architecture": "x86_64",
"vendor": "apple",
"operatingSystem": {
"name": "macosx",
"minimumVersion": {
"major": 10,
"minor": 15
}
},
"environment": null
}
},
"symbols": [
{
"identifier": {
"precise": "ModuleB",
"interfaceLanguage": "swift"
},
"names": {
"title": "ModuleB",
"navigator": null,
"subHeading": null,
"prose": null
},
"pathComponents": ["ModuleB"],
"docComment": null,
"accessLevel": "public",
"kind": {
"identifier": "module",
"displayName": "Module"
},
"mixins": {}
}
],
"relationships": []
}
"""),
JSONFile(name: "ModuleA.symbols.json", content: makeSymbolGraph(moduleName: "ModuleA")),
JSONFile(name: "ModuleB.symbols.json", content: makeSymbolGraph(moduleName: "ModuleB")),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed here: agisilaos@ded8478


InfoPlist(displayName: "TestBundle", identifier: "com.test.example"),
])
)

// Verify that we emit a warning for multiple main modules
let multipleModulesProblem = try XCTUnwrap(context.problems.first(where: { $0.diagnostic.identifier == "org.swift.docc.MultipleMainModules" }))
XCTAssertEqual(multipleModulesProblem.diagnostic.severity, .warning)
XCTAssertTrue(multipleModulesProblem.diagnostic.summary.contains("more than one main module"))
XCTAssertTrue(multipleModulesProblem.diagnostic.explanation?.contains("ModuleA, ModuleB") == true)

// Verify the warning doesn't have a source location since it's about the overall input structure
XCTAssertNil(multipleModulesProblem.diagnostic.source)
Copy link
Contributor

Choose a reason for hiding this comment

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

minor (non-blocking): We could refer to one of the symbol graph files here and use DiagnosticNote values to help the developer find the other symbol graph files so that this diagnostic becomes a bit more actionable.

See for example DocumentationContext.emitWarningsForSymbolsMatchedInMultipleDocumentationExtensions(with:)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed here: 121c159

XCTAssertNil(multipleModulesProblem.diagnostic.range)
}
}