Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
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
93 changes: 93 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 Expand Up @@ -2381,6 +2429,51 @@ public class DocumentationContext {
return node.reference
}

// Warn when the documentation contains both symbols (modules) and @TechnologyRoot pages
// This is an unsupported setup that creates multiple roots in the documentation hierarchy
if !rootModules.isEmpty && !rootPageArticles.isEmpty {
let problems = rootPageArticles.map { rootPageArticle -> Problem in
// Create notes pointing to symbol graph files that are causing the multiple roots issue
let symbolGraphNotes: [DiagnosticNote] = bundle.symbolGraphURLs.map { symbolGraphURL in
let fileName = symbolGraphURL.lastPathComponent
let zeroRange = SourceLocation(line: 1, column: 1, source: nil)..<SourceLocation(line: 1, column: 1, source: nil)
return DiagnosticNote(
source: symbolGraphURL,
range: zeroRange,
message: "Symbol graph file '\(fileName)' creates a module root"
)
}

let diagnostic = Diagnostic(
source: rootPageArticle.source,
severity: .warning,
range: rootPageArticle.value.metadata?.technologyRoot?.originalMarkup.range,
identifier: "org.swift.docc.TechnologyRootWithSymbols",
summary: "Documentation contains both symbols and articles with @TechnologyRoot directives",
explanation: """
When documentation contains symbols (from symbol graph files), @TechnologyRoot directives create an unsupported setup with multiple roots in the documentation hierarchy.
Remove the @TechnologyRoot directive so that this page is treated as an article under the module.
""",
notes: symbolGraphNotes
)

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(problems)
}

// Articles that will be automatically curated can be resolved but they need to be pre registered before resolving links.
let rootNodeForAutomaticCuration = soleRootModuleReference.flatMap(topicGraph.nodeWithReference(_:))
if configuration.convertServiceConfiguration.allowsRegisteringArticlesWithoutTechnologyRoot || rootNodeForAutomaticCuration != nil {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -169,4 +169,125 @@ 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.
"""),
])
)

// 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 = try XCTUnwrap(problem.possibleSolutions.first)
XCTAssertEqual(solution.summary, "Remove the 'TechnologyRoot' directive")
XCTAssertEqual(solution.replacements.count, 1)
}
}

func testWarnsAboutSymbolsWithTechnologyRootPages() async throws {
// Test the third case: documentation contains symbols (has a module) and also has @TechnologyRoot pages
let (_, context) = try await loadBundle(catalog:
Folder(name: "symbols-with-root.docc", content: [
// Symbol graph with a module
JSONFile(name: "MyModule.symbols.json", content: makeSymbolGraph(moduleName: "MyModule")),

// Article with @TechnologyRoot directive
TextFile(name: "GettingStarted.md", utf8Content: """
# Getting Started
@Metadata {
@TechnologyRoot
}
Learn how to use MyModule.
"""),

// Another article with @TechnologyRoot directive
TextFile(name: "Overview.md", utf8Content: """
# Overview
@Metadata {
@TechnologyRoot
}
Overview of the technology.
"""),
])
)

// Verify that we emit warnings for @TechnologyRoot directives when symbols are present
let symbolsWithRootProblems = context.problems.filter { $0.diagnostic.identifier == "org.swift.docc.TechnologyRootWithSymbols" }
XCTAssertEqual(symbolsWithRootProblems.count, 2, "Should emit warnings for both @TechnologyRoot directives when symbols are present")
Comment on lines +247 to +248
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 of multiple root pages, this test assertion becomes a good example of why it's important to not ignore unexpected diagnostics in tests.

The way the code is written right now, this setup would result in both "MultipleTechnologyRoots" warnings and "TechnologyRootWithSymbols" warnings for the same @TechnologyRoot directives but because the test filters the problems to a smaller subset it misses these warnings and because of this doesn't reflect the experience that a real user would have in this scenario.


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

// Verify each warning has a solution to remove the TechnologyRoot directive
for problem in symbolsWithRootProblems {
XCTAssertEqual(problem.possibleSolutions.count, 1)
let solution = try XCTUnwrap(problem.possibleSolutions.first)
XCTAssertEqual(solution.summary, "Remove the 'TechnologyRoot' directive")
XCTAssertEqual(solution.replacements.count, 1)

// Verify that diagnostic notes point to the symbol graph file
XCTAssertEqual(problem.diagnostic.notes.count, 1, "Should have a note pointing to the symbol graph file")
let note = try XCTUnwrap(problem.diagnostic.notes.first)
XCTAssertTrue(note.source.lastPathComponent.hasSuffix(".symbols.json"), "Note should point to a symbol graph file")
XCTAssertTrue(note.message.contains("Symbol graph file"), "Note should mention symbol graph file")
}
}

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
JSONFile(name: "ModuleA.symbols.json", content: makeSymbolGraph(moduleName: "ModuleA")),

// Second module symbol graph
JSONFile(name: "ModuleB.symbols.json", content: makeSymbolGraph(moduleName: "ModuleB")),

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)
}
}