Skip to content
Merged
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
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 {
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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to "ExtendedModule" and cleaned up unused IDs.
Renamed IDs to simpler readable version.

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"])
]
)),
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"]),
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
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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cleaned it up, removed unrelated assertions, renamed IDs and module.

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