Skip to content
Merged
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
30 changes: 29 additions & 1 deletion Sources/SwiftDocC/Model/ParametersAndReturnValidator.swift
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,35 @@ struct ParametersAndReturnValidator {
guard let signature = mixin.getValueIfPresent(for: SymbolGraph.Symbol.FunctionSignature.self) else {
continue
}
signatures[DocumentationDataVariantsTrait(for: selector)] = signature

let trait = DocumentationDataVariantsTrait(for: selector)
// Check if we've already encountered a different signature for another platform
guard var existing = signatures.removeValue(forKey: trait) else {
signatures[trait] = signature
continue
}

// An internal helper function that compares parameter names
func hasSameNames(_ lhs: SymbolGraph.Symbol.FunctionSignature.FunctionParameter, _ rhs: SymbolGraph.Symbol.FunctionSignature.FunctionParameter) -> Bool {
lhs.name == rhs.name && lhs.externalName == rhs.externalName
}
// If the two signatures have different parameters, add any missing parameters.
// This allows for documenting parameters that are only available on some platforms.
//
// Note: Doing this redundant `elementsEqual(_:by:)` check is significantly faster in the common case when all platforms have the same signature.
// In the rare case where platforms have different signatures, the overhead of checking `elementsEqual(_:by:)` first is too small to measure.
if !existing.parameters.elementsEqual(signature.parameters, by: hasSameNames) {
for case .insert(offset: let offset, element: let element, _) in signature.parameters.difference(from: existing.parameters, by: hasSameNames) {
existing.parameters.insert(element, at: offset)
}
}
Comment on lines +298 to +302
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this if statement is not needed because if there's no difference then the array will be empty and the loop will iterate over nothing (which is the desired behaviour)

Suggested change
if !existing.parameters.elementsEqual(signature.parameters, by: hasSameNames) {
for case .insert(offset: let offset, element: let element, _) in signature.parameters.difference(from: existing.parameters, by: hasSameNames) {
existing.parameters.insert(element, at: offset)
}
}
for case .insert(offset: let offset, element: let element, _) in signature.parameters.difference(from: existing.parameters, by: hasSameNames) {
existing.parameters.insert(element, at: offset)
}

Copy link
Contributor Author

@d-ronnqvist d-ronnqvist Jul 10, 2024

Choose a reason for hiding this comment

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

The extra if-statement isn't necessary but it's faster in the common case where the API has the same signature on all platforms.

Here's a chart (that I made just now) of a microbenchmark where I create 1000 fake symbols, that each exist on two platforms and have an increasing number of fake parameters named "AAAAAAA", "BBBBBBB", "CCCCCCC", etc. on both platforms.

chart

You can see that the equalElements(_:by:) check makes a fairly fast early exit compared to much more sophisticated differences(from:by) computation that finds no differences. As the symbols get more and more parameters this difference is lessened but at fewer than 10 parameters, it's a rather significant difference.

If I update the benchmark to have minor differences between the parameters of each platform for all 1000 fake symbols, the overhead of the redundant equalElements(_:by:) check is barely measurable:

chart

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a shorter version of this comment to the code so that someone else (including me when I inevitably forget about this) doesn't remove the if-statement in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the deep-dive, sounds great!


// If the already encountered signature has a void return type, replace it with the non-void return type.
// This allows for documenting the return values that are only available on some platforms.
if existing.returns != signature.returns, existing.returns == knownVoidReturnValuesByLanguage[.init(id: selector.interfaceLanguage)] {
existing.returns = signature.returns
}
signatures[trait] = existing
}

guard !signatures.isEmpty else { return nil }
Expand Down
58 changes: 58 additions & 0 deletions Tests/SwiftDocCTests/Model/ParametersAndReturnValidatorTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,62 @@ class ParametersAndReturnValidatorTests: XCTestCase {
XCTAssertEqual(returnsSections[.objectiveC]?.content.map({ $0.format() }).joined(), "Some return value description.")
}

func testFunctionWithDifferentSignaturesOnDifferentPlatforms() throws {
let url = try createTempFolder(content: [
Folder(name: "unit-test.docc", content: [
// One parameter, void return
JSONFile(name: "Platform1-ModuleName.symbols.json", content: makeSymbolGraph(
platform: .init(operatingSystem: .init(name: "Platform1")),
docComment: nil,
sourceLanguage: .objectiveC,
parameters: [(name: "first", externalName: nil)],
returnValue: .init(kind: .typeIdentifier, spelling: "void", preciseIdentifier: "c:v")
)),
// Two parameters, void return
JSONFile(name: "Platform2-ModuleName.symbols.json", content: makeSymbolGraph(
platform: .init(operatingSystem: .init(name: "Platform2")),
docComment: nil,
sourceLanguage: .objectiveC,
parameters: [(name: "first", externalName: nil), (name: "second", externalName: nil)],
returnValue: .init(kind: .typeIdentifier, spelling: "void", preciseIdentifier: "c:v")
)),
// One parameter, BOOL return
JSONFile(name: "Platform3-ModuleName.symbols.json", content: makeSymbolGraph(
platform: .init(operatingSystem: .init(name: "Platform3")),
docComment: nil,
sourceLanguage: .objectiveC,
parameters: [(name: "first", externalName: nil),],
returnValue: .init(kind: .typeIdentifier, spelling: "BOOL", preciseIdentifier: "c:@T@BOOL")
)),
TextFile(name: "Extension.md", utf8Content: """
# ``functionName(...)``

A documentation extension that documents both parameters

- Parameters:
- first: Some description of the parameter that is available on all three platforms.
- second: Some description of the parameter that is only available on platform 2.
- Returns: Some description of the return value that is only available on platform 3.
""")
])
])
let (_, bundle, context) = try loadBundle(from: url)

XCTAssert(context.problems.isEmpty, "Unexpected problems: \(context.problems.map(\.diagnostic.summary))")

let reference = ResolvedTopicReference(bundleIdentifier: bundle.identifier, path: "/documentation/ModuleName/functionName(...)", sourceLanguage: .swift)
let node = try context.entity(with: reference)
let symbol = try XCTUnwrap(node.semantic as? Symbol)

let parameterSections = symbol.parametersSectionVariants
XCTAssertEqual(parameterSections[.objectiveC]?.parameters.map(\.name), ["first", "second"])
XCTAssertEqual(parameterSections[.objectiveC]?.parameters.first?.contents.map({ $0.format() }).joined(), "Some description of the parameter that is available on all three platforms.")
XCTAssertEqual(parameterSections[.objectiveC]?.parameters.last?.contents.map({ $0.format() }).joined(), "Some description of the parameter that is only available on platform 2.")

let returnSections = symbol.returnsSectionVariants
XCTAssertEqual(returnSections[.objectiveC]?.content.map({ $0.format() }).joined(), "Some description of the return value that is only available on platform 3.")
}

func testFunctionWithErrorParameterButVoidType() throws {
let url = try createTempFolder(content: [
Folder(name: "unit-test.docc", content: [
Expand Down Expand Up @@ -619,6 +675,7 @@ class ParametersAndReturnValidatorTests: XCTestCase {
}

private func makeSymbolGraph(
platform: SymbolGraph.Platform = .init(),
docComment: String?,
sourceLanguage: SourceLanguage,
parameters: [(name: String, externalName: String?)],
Expand All @@ -634,6 +691,7 @@ class ParametersAndReturnValidatorTests: XCTestCase {

return makeSymbolGraph(
moduleName: "ModuleName",
platform: platform,
symbols: [
.init(
identifier: .init(precise: "symbol-id", interfaceLanguage: sourceLanguage.id),
Expand Down