Skip to content

Commit a7c5e25

Browse files
ethan-kustersethan-kusters
authored
Fix regression with duplicated generated implementation sections (#321)
With the recent fix for supporting generated implementation sections for symbols that include periods (like `Comparable...<`), we introduced a regression where it's possible to end up with a duplicate default implementation section. This can happen when a protocol has a mix of documentation coverage. For example ```swift public protocol Foo { /// This is my great doc. func fooMemberWithDocComment() func fooMemberWithoutDocComment() } public extension Foo { func fooMemberWithDocComment() { } func fooMemberWithoutDocComment() { } } public struct Bar: Foo {} ``` Here `Bar` would end up with two "Foo Implementations" sections. This is because we're able to access the original parent symbol for `fooMemberWithDocComment()` but not for `fooMemberWithoutDocComment` so these relationships end up with a different unique identifier but the same collection name: "Foo Implementations". The fix is to not rely on the parent relationship being there and instead look at the origin symbol's parent path components. We can rely on the origin symbol always being there for protocols defined in the current module. This also resolves an issue where conforming to compmlicated protocols in a different module would produce a misnamed collection. For example: ```swift // FirstTarget infix operator .<.. : RangeFormationPrecedence public protocol OtherFancyProtocol { static func .<.. (lhs: OtherFancyProtocol, rhs: OtherFancyProtocol) } public extension OtherFancyProtocol { static func .<.. (lhs: OtherFancyProtocol, rhs: OtherFancyProtocol) {} } ``` ```swift // SecondTarget import FirstTarget public struct OtherFancyProtocolConformer: OtherFancyProtocol {} ``` Here we would end up with a default collection named "< Implementations" instead of "OtherFancyProtocol Implementations". The solution is to figure out the actual name of the symbol based on the source symbol which is always guaranteed to be there. We can use this to split out the parent symbol's name from the source origin display name. Resolves rdar://95808950
1 parent eb8e694 commit a7c5e25

File tree

6 files changed

+8310
-14
lines changed

6 files changed

+8310
-14
lines changed

‎Sources/SwiftDocC/Infrastructure/DocumentationContext.swift‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1264,7 +1264,6 @@ public class DocumentationContext: DocumentationContextDataProviderDelegate {
12641264
for (_, relationships) in combinedRelationships {
12651265
try GeneratedDocumentationTopics.createInheritedSymbolsAPICollections(
12661266
relationships: relationships,
1267-
parentOfFunction: { try? symbolsURLHierarchy.parent(of: $0) },
12681267
context: self,
12691268
bundle: bundle
12701269
)

‎Sources/SwiftDocC/Infrastructure/Symbol Graph/GeneratedDocumentationTopics.swift‎

Lines changed: 33 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -40,15 +40,35 @@ enum GeneratedDocumentationTopics {
4040
/// - reference: The parent type reference.
4141
/// - originDisplayName: The origin display name as provided by the symbol graph.
4242
/// - extendedModuleName: Extended module name.
43-
mutating func add(_ childReference: ResolvedTopicReference, to reference: ResolvedTopicReference, originDisplayName: String, originParentSymbol: ResolvedTopicReference?, extendedModuleName: String) throws {
43+
mutating func add(_ childReference: ResolvedTopicReference, to reference: ResolvedTopicReference, childSymbol: SymbolGraph.Symbol, originDisplayName: String, originSymbol: SymbolGraph.Symbol?, extendedModuleName: String) throws {
4444
let fromType: String
4545
let typeSimpleName: String
46-
if let originParentSymbol = originParentSymbol, !originParentSymbol.pathComponents.isEmpty {
47-
// If we have a resolved symbol for the parent of `sourceOrigin`, use that for the names
48-
fromType = originParentSymbol.pathComponents.joined(separator: ".")
49-
typeSimpleName = originParentSymbol.pathComponents.last!
46+
if let originSymbol = originSymbol, originSymbol.pathComponents.count > 1 {
47+
// If we have a resolved symbol for the source origin, use its path components to
48+
// find the name of the parent by dropping the last path component.
49+
let parentSymbolPathComponents = originSymbol.pathComponents.dropLast()
50+
fromType = parentSymbolPathComponents.joined(separator: ".")
51+
typeSimpleName = parentSymbolPathComponents.last!
52+
} else if let childSymbolName = childSymbol.pathComponents.last,
53+
originDisplayName.count > (childSymbolName.count + 1)
54+
{
55+
// In the case where we don't have a resolved symbol for the source origin,
56+
// this allows us to still accurately handle cases like this:
57+
//
58+
// "displayName": "SuperFancyProtocol..<..(_:_:)"
59+
//
60+
// Where there's no way for us to determine which of the periods is the one
61+
// splitting the name of the parent type and the symbol name. Using the count
62+
// of the symbol name (+1 for the period splitting the names)
63+
// from the source is a reliable way to support this.
64+
65+
let parentSymbolName = originDisplayName.dropLast(childSymbolName.count + 1)
66+
fromType = String(parentSymbolName)
67+
typeSimpleName = String(parentSymbolName.split(separator: ".").last ?? parentSymbolName)
5068
} else {
51-
// If we don't have a resolved `sourceOrigin` parent, fall back to parsing its display name
69+
// This should never happen but is a last safeguard for the case where
70+
// the child symbol is unexpectedly short. In this case, we can attempt to just parse
71+
// the parent symbol name out of the origin display name.
5272

5373
// Detect the path components of the providing the default implementation.
5474
let typeComponents = originDisplayName.split(separator: ".")
@@ -209,7 +229,7 @@ enum GeneratedDocumentationTopics {
209229
/// - symbolsURLHierarchy: A symbol graph hierarchy as created during symbol registration.
210230
/// - context: A documentation context to update.
211231
/// - bundle: The current documentation bundle.
212-
static func createInheritedSymbolsAPICollections(relationships: Set<SymbolGraph.Relationship>, parentOfFunction: (ResolvedTopicReference) -> ResolvedTopicReference?, context: DocumentationContext, bundle: DocumentationBundle) throws {
232+
static func createInheritedSymbolsAPICollections(relationships: Set<SymbolGraph.Relationship>, context: DocumentationContext, bundle: DocumentationBundle) throws {
213233
var inheritanceIndex = InheritedSymbols()
214234

215235
// Walk the symbol graph relationships and look for parent <-> child links that stem in a different module.
@@ -223,14 +243,14 @@ enum GeneratedDocumentationTopics {
223243
let parent = context.symbolIndex[relationship.target],
224244
// Resolve the child
225245
let child = context.symbolIndex[relationship.source],
246+
// Get the child symbol
247+
let childSymbol = child.symbol,
226248
// Get the swift extension data
227-
let extends = child.symbol?.mixins[SymbolGraph.Symbol.Swift.Extension.mixinKey] as? SymbolGraph.Symbol.Swift.Extension {
228-
var originParentSymbol: ResolvedTopicReference? = nil
229-
if let originSymbol = context.symbolIndex[origin.identifier] {
230-
originParentSymbol = parentOfFunction(originSymbol.reference)
231-
}
249+
let extends = childSymbol.mixins[SymbolGraph.Symbol.Swift.Extension.mixinKey] as? SymbolGraph.Symbol.Swift.Extension {
250+
let originSymbol = context.symbolIndex[origin.identifier]?.symbol
251+
232252
// Add the inherited symbol to the index.
233-
try inheritanceIndex.add(child.reference, to: parent.reference, originDisplayName: origin.displayName, originParentSymbol: originParentSymbol, extendedModuleName: extends.extendedModule)
253+
try inheritanceIndex.add(child.reference, to: parent.reference, childSymbol: childSymbol, originDisplayName: origin.displayName, originSymbol: originSymbol, extendedModuleName: extends.extendedModule)
234254
}
235255
}
236256

‎Tests/SwiftDocCTests/Rendering/RenderNodeTranslatorTests.swift‎

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
import Foundation
1212
import XCTest
1313
@testable import SwiftDocC
14+
import SwiftDocCTestUtilities
1415
import Markdown
1516

1617
class RenderNodeTranslatorTests: XCTestCase {
@@ -650,6 +651,120 @@ class RenderNodeTranslatorTests: XCTestCase {
650651

651652
}
652653

654+
func testAutomaticImplementationsWithExtraDotsFromExternalModule() throws {
655+
let inheritedDefaultImplementationsFromExternalModuleSGF = Bundle.module.url(
656+
forResource: "InheritedDefaultImplementationsFromExternalModule.symbols",
657+
withExtension: "json",
658+
subdirectory: "Test Resources"
659+
)!
660+
661+
let testBundle = try Folder(
662+
name: "unit-test.docc",
663+
content: [
664+
InfoPlist(displayName: "TestBundle", identifier: "com.test.example"),
665+
CopyOfFile(original: inheritedDefaultImplementationsFromExternalModuleSGF),
666+
]
667+
).write(inside: createTemporaryDirectory())
668+
669+
try assertDefaultImplementationCollectionTitles(
670+
in: try loadRenderNode(at: "/documentation/SecondTarget/FancyProtocolConformer", in: testBundle),
671+
[
672+
"FancyProtocol Implementations",
673+
]
674+
)
675+
676+
try assertDefaultImplementationCollectionTitles(
677+
in: try loadRenderNode(at: "/documentation/SecondTarget/OtherFancyProtocolConformer", in: testBundle),
678+
[
679+
"OtherFancyProtocol Implementations",
680+
]
681+
)
682+
683+
try assertDefaultImplementationCollectionTitles(
684+
in: try loadRenderNode(at: "/documentation/SecondTarget/FooConformer", in: testBundle),
685+
[
686+
"Foo Implementations",
687+
]
688+
)
689+
}
690+
691+
func testAutomaticImplementationsFromCurrentModuleWithMixOfDocCoverage() throws {
692+
let inheritedDefaultImplementationsSGF = Bundle.module.url(
693+
forResource: "InheritedDefaultImplementations.symbols",
694+
withExtension: "json",
695+
subdirectory: "Test Resources"
696+
)!
697+
let inheritedDefaultImplementationsAtSwiftSGF = Bundle.module.url(
698+
forResource: "[email protected]",
699+
withExtension: "json",
700+
subdirectory: "Test Resources"
701+
)!
702+
703+
let testBundle = try Folder(
704+
name: "unit-test.docc",
705+
content: [
706+
InfoPlist(displayName: "TestBundle", identifier: "com.test.example"),
707+
CopyOfFile(original: inheritedDefaultImplementationsSGF),
708+
CopyOfFile(original: inheritedDefaultImplementationsAtSwiftSGF),
709+
]
710+
).write(inside: createTemporaryDirectory())
711+
712+
try assertDefaultImplementationCollectionTitles(
713+
in: try loadRenderNode(at: "/documentation/FirstTarget/Bar", in: testBundle),
714+
[
715+
"Foo Implementations",
716+
]
717+
)
718+
719+
try assertDefaultImplementationCollectionTitles(
720+
in: try loadRenderNode(at: "/documentation/FirstTarget/OtherStruct", in: testBundle),
721+
[
722+
"Comparable Implementations",
723+
"Equatable Implementations",
724+
]
725+
)
726+
727+
try assertDefaultImplementationCollectionTitles(
728+
in: try loadRenderNode(at: "/documentation/FirstTarget/SomeStruct", in: testBundle),
729+
[
730+
"Comparable Implementations",
731+
"Equatable Implementations",
732+
"FancyProtocol Implementations",
733+
"OtherFancyProtocol Implementations",
734+
]
735+
)
736+
}
737+
738+
func assertDefaultImplementationCollectionTitles(
739+
in renderNode: RenderNode,
740+
_ expectedTitles: [String],
741+
file: StaticString = #file,
742+
line: UInt = #line
743+
) throws {
744+
let defaultImplementationSection = try XCTUnwrap(
745+
renderNode.topicSections.first(where: { $0.title == "Default Implementations" }),
746+
"Expected to find default implementations topic section.",
747+
file: file,
748+
line: line
749+
)
750+
751+
let references = defaultImplementationSection.identifiers.compactMap { identifier in
752+
renderNode.references[identifier] as? TopicRenderReference
753+
}
754+
755+
XCTAssertEqual(references.map(\.title), expectedTitles, file: file, line: line)
756+
}
757+
758+
func loadRenderNode(at path: String, in bundleURL: URL) throws -> RenderNode {
759+
let (_, bundle, context) = try loadBundle(from: bundleURL)
760+
761+
let reference = ResolvedTopicReference(bundleIdentifier: bundle.identifier, path: path, sourceLanguage: .swift)
762+
var translator = RenderNodeTranslator(context: context, bundle: bundle, identifier: reference, source: nil)
763+
let node = try context.entity(with: reference)
764+
let symbol = try XCTUnwrap(node.semantic as? Symbol)
765+
return try XCTUnwrap(translator.visitSymbol(symbol) as? RenderNode)
766+
}
767+
653768
func testAutomaticTaskGroupTopicsAreSorted() throws {
654769
let (bundle, context) = try testBundleAndContext(named: "DefaultImplementations")
655770
let structReference = ResolvedTopicReference(bundleIdentifier: bundle.identifier, path: "/documentation/DefaultImplementations/Foo", sourceLanguage: .swift)

0 commit comments

Comments
 (0)