-
Notifications
You must be signed in to change notification settings - Fork 162
(#1257) Combined module link issue - public extension of dependent module causes resolution failure #1327
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
base: main
Are you sure you want to change the base?
(#1257) Combined module link issue - public extension of dependent module causes resolution failure #1327
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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" | ||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||||||||||||
| 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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@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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
There was a problem hiding this comment.
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