Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added warning for multiple root pages #1170 #1177

Closed
Closed
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
166 changes: 166 additions & 0 deletions Sources/SwiftDocC/Infrastructure/DocumentationContext.swift
Original file line number Diff line number Diff line change
Expand Up @@ -2809,11 +2809,177 @@ public class DocumentationContext {
}
}

//Detects and emits warnings for multiple root pages in the documentation.

private func emitWarningsForMultipleRoots() {
//Check for multiple root modules from symbol graphs
let rootModuleReferences = rootModules ?? []
let rootModulesFromSymbolGraph = rootModuleReferences.filter { reference in
guard let node = topicGraph.nodeWithReference(reference) else { return false }
//include actual modules from symbol graphs
return node.kind == .module && !node.isVirtual
}

if rootModulesFromSymbolGraph.count > 1 {
//Multiple modules from symbol graph files
let moduleNames = rootModulesFromSymbolGraph.map { $0.lastPathComponent }

//create diagnostic notes for each module
var notes = [DiagnosticNote]()
for moduleRef in rootModulesFromSymbolGraph {
let moduleName = moduleRef.lastPathComponent
notes.append(DiagnosticNote(
source: nil,
Copy link
Contributor

Choose a reason for hiding this comment

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

This code doesn't compile.

message: "Module '\(moduleName)' is one of multiple root modules"
))
}

let diagnostic = Diagnostic(
source: nil,
severity: .warning,
range: nil,
identifier: "org.swift.docc.MultipleModuleRoots",
summary: "Documentation contains symbol graphs for more than one main module: \(moduleNames.joined(separator: ", ").singleQuoted)",
explanation: "This may lead to unexpected behavior as it's not clear which module should be the primary root.",
notes: notes
)
diagnosticEngine.emit(Problem(diagnostic: diagnostic, possibleSolutions: []))
}

//check for manual technology roots
let manualRootPages = topicGraph.nodes.values.filter { node in
guard let documentationNode = documentationCache[node.reference] else { return false }
// Find nodes with @TechnologyRoot directive that aren't from symbol graphs
return documentationNode.semantic is Article &&
(documentationNode.semantic as? Article)?.metadata?.technologyRoot != nil
}

//multiple manual @TechnologyRoot pages
if manualRootPages.count > 1 {
//a mapping of root pages to their URLs for diagnostic notes
var rootPageURLs = [ResolvedTopicReference: URL]()
for rootPage in manualRootPages {
if let documentURL = try? documentURL(for: rootPage.reference) {
rootPageURLs[rootPage.reference] = documentURL
}
}

for rootPage in manualRootPages {
if let documentURL = try? documentURL(for: rootPage.reference),
let documentationNode = documentationCache[rootPage.reference],
let article = documentationNode.semantic as? Article,
let technologyRoot = article.metadata?.technologyRoot {

//diagnostic notes for the other root pages
var notes = [DiagnosticNote]()
for otherRootPage in manualRootPages where otherRootPage.reference != rootPage.reference {
if let otherURL = rootPageURLs[otherRootPage.reference] {
let technologyRootRange = (documentationCache[otherRootPage.reference]?.semantic as? Article)?.metadata?.technologyRoot?.originalMarkup.range
notes.append(DiagnosticNote(
source: otherURL,
range: technologyRootRange,
message: "'\(otherRootPage.title)' is also marked as a technology root"
))
}
}

let diagnostic = Diagnostic(
source: documentURL,
severity: .warning,
range: technologyRoot.originalMarkup.range,
identifier: "org.swift.docc.MultipleManualRoots",
summary: "Multiple @TechnologyRoot pages found in documentation",
explanation: "The page \(rootPage.title.singleQuoted) is marked as a technology root, but there are other technology root pages in the documentation. This may lead to unexpected behavior.",
notes: notes
)

let solutions: [Solution]
if let range = technologyRoot.originalMarkup.range {
solutions = [
Solution(summary: "Remove the TechnologyRoot directive", replacements: [Replacement(range: range, replacement: "")])
]
} else {
solutions = []
}

diagnosticEngine.emit(Problem(diagnostic: diagnostic, possibleSolutions: solutions))
}
}
}

//@TechnologyRoot page + module from symbol graph
if manualRootPages.count == 1 && rootModulesFromSymbolGraph.count == 1 {
let rootPage = manualRootPages.first!

//skip warning if either is a descendant of the other
let moduleReference = rootModulesFromSymbolGraph.first!
if !isDescendant(of: rootPage.reference, moduleReference) && !isDescendant(of: moduleReference, rootPage.reference) {
if let documentURL = try? documentURL(for: rootPage.reference),
let documentationNode = documentationCache[rootPage.reference],
let article = documentationNode.semantic as? Article,
let technologyRoot = article.metadata?.technologyRoot {

let moduleName = moduleReference.lastPathComponent

//create a diagnostic note for the module
let notes = [DiagnosticNote(
source: nil,
message: "Module '\(moduleName)' is already serving as a root"
)]

let diagnostic = Diagnostic(
source: documentURL,
severity: .warning,
range: technologyRoot.originalMarkup.range,
identifier: "org.swift.docc.ManualRootWithModuleRoot",
summary: "Manual @TechnologyRoot found with a module root from symbol graph file",
explanation: "The page \(rootPage.title.singleQuoted) is marked as a technology root, but the documentation also contains a module \(moduleName.singleQuoted) from a symbol graph file. This may lead to unexpected behavior.",
notes: notes
)

let solutions: [Solution]
if let range = technologyRoot.originalMarkup.range {
solutions = [
Solution(summary: "Remove the TechnologyRoot directive", replacements: [Replacement(range: range, replacement: "")])
]
} else {
solutions = []
}

diagnosticEngine.emit(Problem(diagnostic: diagnostic, possibleSolutions: solutions))
}
}
}
}

/**
Checks if a reference is a descendant of another reference in the topic graph.

- Parameters:
- possibleDescendant: The reference to check if it's a descendant
- possibleAncestor: The reference to check if it's an ancestor
- Returns: `true` if possibleDescendant is a descendant of possibleAncestor, `false` otherwise
*/
private func isDescendant(of possibleDescendant: ResolvedTopicReference, _ possibleAncestor: ResolvedTopicReference) -> Bool {
var current = possibleDescendant
while let parentReferences = try? parents(of: current), !parentReferences.isEmpty {
if parentReferences.contains(possibleAncestor) {
return true
}
// Check the first parent (we just need to find any path to the ancestor)
current = parentReferences[0]
}
return false
Comment on lines +2964 to +2972
Copy link
Contributor

Choose a reason for hiding this comment

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

This implementation is sensitive to cycles and could end up traversing the graph forever.

For comparison, see DocumentationCurator.isReference(_:anAncestorOf:)

Copy link
Author

Choose a reason for hiding this comment

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

This implementation is sensitive to cycles and could end up traversing the graph forever.

For comparison, see DocumentationCurator.isReference(_:anAncestorOf:)

I understand your concern. I apologize for taking up your time. This is my first time dealing with a large codebase, so it’s a bit challenging to grasp everything. However, I’ll ensure that the code functions correctly before I commit it to the repository.

Thank You!

}

/**
Analysis that runs after all nodes are successfully registered in the context.
Useful for checks that need the complete node graph.
*/
func topicGraphGlobalAnalysis() {
//check for multiple root pages and emit warnings
emitWarningsForMultipleRoots()

// Run any checks added to the context.
let problems = knownIdentifiers.flatMap { reference in
return configuration.topicAnalysisConfiguration.additionalChecks.flatMap { check in
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -169,4 +169,134 @@ class DocumentationContext_RootPageTests: XCTestCase {

XCTAssertEqual(context.problems.count, 0)
}

func testWarnsAboutMultipleModuleRoots() throws {
let moduleOneSymbolGraph = SymbolGraph(
metadata: SymbolGraph.Metadata(
formatVersion: SymbolGraph.FormatVersion(major: 1, minor: 0, patch: 0),
Copy link
Contributor

Choose a reason for hiding this comment

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

This code doesn't compile. There is no SymbolGraph.FormatVersion type. Did you try building and running this test?

generator: "unit-test"
),
module: SymbolGraph.Module(
name: "ModuleOne",
platform: .init(architecture: nil, vendor: nil, operatingSystem: nil, environment: nil),
version: nil
),
symbols: [],
relationships: []
)

let moduleTwoSymbolGraph = SymbolGraph(
metadata: SymbolGraph.Metadata(
formatVersion: SymbolGraph.FormatVersion(major: 1, minor: 0, patch: 0),
generator: "unit-test"
),
module: SymbolGraph.Module(
name: "ModuleTwo",
platform: .init(architecture: nil, vendor: nil, operatingSystem: nil, environment: nil),
version: nil
),
symbols: [],
relationships: []
)

let moduleOneSymbolGraphURL = try createTempFile(name: "module-one.symbols.json", content: try JSONEncoder().encode(moduleOneSymbolGraph))
Copy link
Contributor

Choose a reason for hiding this comment

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

This code doesn't compile.

let moduleTwoSymbolGraphURL = try createTempFile(name: "module-two.symbols.json", content: try JSONEncoder().encode(moduleTwoSymbolGraph))

let (_, context) = try loadBundle(copying: "TestBundle", excludingPaths: ["TestBundle.symbols.json"], additionalSymbolGraphs: [moduleOneSymbolGraphURL, moduleTwoSymbolGraphURL])
Copy link
Contributor

Choose a reason for hiding this comment

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

This code doesn't compile. Even if did, there is no "TestBundle" test bundle, so it would fail when the test ran.


// Verify warning about multiple roots
let multipleRootsProblem = try XCTUnwrap(context.problems.first(where: { $0.diagnostic.identifier == "org.swift.docc.MultipleModuleRoots" }))
XCTAssertEqual(multipleRootsProblem.diagnostic.severity, .warning)
XCTAssertTrue(multipleRootsProblem.diagnostic.summary.contains("ModuleOne") && multipleRootsProblem.diagnostic.summary.contains("ModuleTwo"), "The warning should mention both module names")

// Verify diagnostic notes
XCTAssertEqual(multipleRootsProblem.diagnostic.notes.count, 2, "There should be a note for each module")
XCTAssertTrue(multipleRootsProblem.diagnostic.notes.contains { $0.message.contains("ModuleOne") })
XCTAssertTrue(multipleRootsProblem.diagnostic.notes.contains { $0.message.contains("ModuleTwo") })
}

func testWarnsAboutMultipleManualRoots() throws {
let (_, context) = try loadBundle(catalog:
Folder(name: "MultipleRoots.docc", content: [
TextFile(name: "RootOne.md", utf8Content: """
# Root One
@Metadata {
@TechnologyRoot
}
First root article
"""),

TextFile(name: "RootTwo.md", utf8Content: """
# Root Two
@Metadata {
@TechnologyRoot
}
Second root article
"""),

InfoPlist(displayName: "MultipleRoots", identifier: "com.test.multipleroots"),
])
)

// Verify warning about multiple manual roots
let multipleManualRootProblems = context.problems.filter { $0.diagnostic.identifier == "org.swift.docc.MultipleManualRoots" }
XCTAssertEqual(multipleManualRootProblems.count, 2, "There should be a warning for each manual root page")

for problem in multipleManualRootProblems {
XCTAssertEqual(problem.diagnostic.severity, .warning)
XCTAssertTrue(problem.diagnostic.source?.lastPathComponent == "RootOne.md" || problem.diagnostic.source?.lastPathComponent == "RootTwo.md")
XCTAssertEqual(problem.possibleSolutions.count, 1, "There should be a solution to remove the TechnologyRoot directive")

// Verify diagnostic notes
XCTAssertEqual(problem.diagnostic.notes.count, 1, "There should be a note for the other root page")
let otherRootName = problem.diagnostic.source?.lastPathComponent == "RootOne.md" ? "Root Two" : "Root One"
XCTAssertTrue(problem.diagnostic.notes.first?.message.contains(otherRootName) ?? false, "The note should mention the other root page")
}
}

func testWarnsAboutManualRootWithModuleRoot() throws {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test also has several compiler errors.

let symbolGraph = SymbolGraph(
metadata: SymbolGraph.Metadata(
formatVersion: SymbolGraph.FormatVersion(major: 1, minor: 0, patch: 0),
generator: "unit-test"
),
module: SymbolGraph.Module(
name: "TestModule",
platform: .init(architecture: nil, vendor: nil, operatingSystem: nil, environment: nil),
version: nil
),
symbols: [],
relationships: []
)

let symbolGraphURL = try createTempFile(name: "test-module.symbols.json", content: try JSONEncoder().encode(symbolGraph))

let tempFolder = try createTempFolder(content: [
Folder(name: "MixedRoots.docc", content: [
TextFile(name: "ManualRoot.md", utf8Content: """
# Manual Root
@Metadata {
@TechnologyRoot
}
A manual root page
"""),

InfoPlist(displayName: "MixedRoots", identifier: "com.test.mixedroots"),
])
])

let (_, context) = try loadBundle(from: URL(fileURLWithPath: tempFolder).appendingPathComponent("MixedRoots.docc"), additionalSymbolGraphs: [symbolGraphURL])

//verify warning about manual root with module root
let manualWithModuleProblem = try XCTUnwrap(context.problems.first(where: { $0.diagnostic.identifier == "org.swift.docc.ManualRootWithModuleRoot" }))
XCTAssertEqual(manualWithModuleProblem.diagnostic.severity, .warning)
XCTAssertEqual(manualWithModuleProblem.diagnostic.source?.lastPathComponent, "ManualRoot.md")
XCTAssertTrue(manualWithModuleProblem.diagnostic.summary.contains("Manual @TechnologyRoot found with a module root"))
XCTAssertTrue(manualWithModuleProblem.diagnostic.explanation.contains("TestModule"))
XCTAssertEqual(manualWithModuleProblem.possibleSolutions.count, 1, "There should be a solution to remove the TechnologyRoot directive")

//verify diagnostic notes
XCTAssertEqual(manualWithModuleProblem.diagnostic.notes.count, 1, "There should be a note about the module")
XCTAssertTrue(manualWithModuleProblem.diagnostic.notes.first?.message.contains("TestModule") ?? false, "The note should mention the module name")
}
}