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
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,9 @@ extension PathHierarchy {
if let moduleMatch = modules.first(where: { $0.matches(firstComponent) }) {
return try searchForNode(descendingFrom: moduleMatch, pathComponents: remaining.dropFirst(), onlyFindSymbols: onlyFindSymbols, rawPathForError: rawPath)
}
if modules.count == 1 {
// For absolute links, only use the single-module fallback if the first component doesn't match
// any module name
if modules.count == 1 && !isAbsolute {
do {
return try searchForNode(descendingFrom: modules.first!, pathComponents: remaining, onlyFindSymbols: onlyFindSymbols, rawPathForError: rawPath)
} catch {
Expand Down
98 changes: 97 additions & 1 deletion Tests/SwiftDocCTests/Infrastructure/PathHierarchyTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -3126,7 +3126,103 @@ class PathHierarchyTests: XCTestCase {
try assertFindsPath("/MainModule/TopLevelProtocol/extensionMember(_:)", in: tree, asSymbolID: "extensionMember1")
try assertFindsPath("/MainModule/TopLevelProtocol/InnerStruct/extensionMember(_:)", in: tree, asSymbolID: "extensionMember2")
}


func testAbsoluteLinksToOtherModuleWithExtensions() async 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 passes even without the changes in PathHierarchy+Find

enableFeatureFlag(\.isExperimentalLinkHierarchySerializationEnabled)

let importedProtocolID = "s:14ImportedModule12BaseProtocolP"
let importedTypeID = "s:14ImportedModule12ExtendedTypeV"
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: I find that mixing "imported" and "extended" in this test adds unnecessary confusion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I personally don't find that these string constants help the readability of the test all that much. If they're only used once or twice we could use basic readable inline literals like "extension-id" and "extended-type-id" instead.

let extensionSymbolID = "s:e:s:14ImportedModule12ExtendedTypeV04MainC0E15extensionMethodyyF"
let extensionMethodID = "s:14ImportedModule12ExtendedTypeV04MainC0E15extensionMethodyyF"
let mainModuleTypeID = "s:10MainModule0A4TypeV"

let extensionMixin = SymbolGraph.Symbol.Swift.Extension(
extendedModule: "ImportedModule",
typeKind: .struct,
constraints: []
)

let catalog = Folder(name: "TestCatalog.docc", content: [
JSONFile(name: "MainModule.symbols.json", content: makeSymbolGraph(
moduleName: "MainModule",
symbols: [
makeSymbol(id: mainModuleTypeID, kind: .struct, pathComponents: ["MainType"])
]
)),
Comment on lines +3146 to +3151
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: It's unnecessary for the main module symbol graph to define any symbols of its own. The "MainType" symbol is never used in this test so it can be removed to slightly simplify the test setup.

Suggested change
JSONFile(name: "MainModule.symbols.json", content: makeSymbolGraph(
moduleName: "MainModule",
symbols: [
makeSymbol(id: mainModuleTypeID, kind: .struct, pathComponents: ["MainType"])
]
)),
JSONFile(name: "MainModule.symbols.json", content: makeSymbolGraph(moduleName: "MainModule", symbols: [])),

JSONFile(name: "MainModule@ImportedModule.symbols.json", content: makeSymbolGraph(
moduleName: "MainModule",
symbols: [
makeSymbol(id: importedProtocolID, kind: .protocol, pathComponents: ["BaseProtocol"]),
makeSymbol(id: importedTypeID, kind: .struct, pathComponents: ["ExtendedType"]),
Comment on lines +3155 to +3156
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK extension symbol graph files (ExtendingName@ExtendedName) don't include the definition of the symbols being extended. Those symbol definitions would belong in the main symbol graph for the ExtendedName module instead.

Suggested change
makeSymbol(id: importedProtocolID, kind: .protocol, pathComponents: ["BaseProtocol"]),
makeSymbol(id: importedTypeID, kind: .struct, pathComponents: ["ExtendedType"]),

makeSymbol(
id: extensionSymbolID,
kind: .extension,
pathComponents: ["ExtendedType"],
otherMixins: [extensionMixin]
),
makeSymbol(
id: extensionMethodID,
kind: .method,
pathComponents: ["ExtendedType", "extensionMethod()"],
otherMixins: [extensionMixin]
)
],
relationships: [
.init(
source: extensionMethodID,
target: extensionSymbolID,
kind: .memberOf,
targetFallback: "ImportedModule.ExtendedType"
),
.init(
source: extensionSymbolID,
target: importedTypeID,
kind: .extensionTo,
targetFallback: "ImportedModule.ExtendedType"
)
]
))
])

let (_, context) = try await loadBundle(catalog: catalog)
let tree = context.linkResolver.localResolver.pathHierarchy

XCTAssertEqual(tree.modules.count, 1)
XCTAssertEqual(tree.modules.first?.name, "MainModule")

let paths = tree.caseInsensitiveDisambiguatedPaths()
XCTAssertEqual(paths[importedProtocolID], "/MainModule/ImportedModule/BaseProtocol")
XCTAssertEqual(paths[importedTypeID], "/MainModule/ImportedModule/ExtendedType-struct")
XCTAssertEqual(
paths[extensionMethodID],
"/MainModule/ImportedModule/ExtendedType/extensionMethod()"
)

// Verify that symbols can be found at their correct paths
try assertFindsPath("/MainModule/ImportedModule/BaseProtocol", in: tree, asSymbolID: importedProtocolID)
try assertFindsPath("/MainModule/ImportedModule/ExtendedType-struct", in: tree, asSymbolID: importedTypeID)
try assertFindsPath(
"/MainModule/ImportedModule/ExtendedType/extensionMethod()",
in: tree,
asSymbolID: extensionMethodID
)

// Verify that absolute paths to non-existent modules throw moduleNotFound error
// This is the fix being tested: without it, single-module fallback would trigger incorrectly
Comment on lines +3210 to +3211
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: What this comment is describing and what the test assertions below are meant to verify isn't really the same thing.

If we only wanted to verify that non-existent modules result in a moduleNotFound error, then we could use a more explicit path like "/NotFound".

What the test assertions below are meant to verify is that a link that resolves relative to the module fails to resolve as an absolute link, with a moduleNotFound error.

It would be good to both verify that "ExtendedModule/ExtendedType/extensionMethod()" resolves and that "/ExtendedModule/ExtendedType/extensionMethod()" (with a leading slash) raises the expected error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, if it's only these test assertions that are important, then the other test assertions could be removed along with that portion of the code comment.

try assertPathRaisesErrorMessage(
"/ImportedModule/BaseProtocol",
in: tree,
context: context,
expectedErrorMessage: "No module named 'ImportedModule'"
)
try assertPathRaisesErrorMessage(
"/ImportedModule/ExtendedType",
in: tree,
context: context,
expectedErrorMessage: "No module named 'ImportedModule'"
)
}

func testMissingRequiredMemberOfSymbolGraphRelationshipInOneLanguageAcrossManyPlatforms() async throws {
// We make a best-effort attempt to create a valid path hierarchy, even if the symbol graph inputs are not valid.

Expand Down