When documenting members of extensions to external protocols, display the external protocol's name #732

patshaughnessy · 2023-10-05T21:57:48Z

Explanation: When documenting members of extensions to external protocols, display the
external protocol's name.
Scope: A small amount of new text appears on documentation pages for one type of Swift symbol.
GitHub Issue: none.
Risk: Low. Minor code changes that only impact how protocol extensions of external protocols are documented.
Testing: Ran the ./bin/test script and it succeeded
Reviewer: @d-ronnqvist

rdar://112219546

Summary

When documenting members of extensions to external protocols, display the
external protocol's name. This is helpful because these pages don’t have the
context of being curated inside their “owning” type or protocol.

Expand the section that we use for conditional constraints to display the
external protocol name. For example, this screenshot shows the documentation for
the stablePartition(by:) symbol in the Swift Algorithms package. Since this
page is curated under "Partitioning and Rotating" it's not obvious that the
function is a member of a protocol extension, or that the protocol extension is
extending the MutableCollection protocol from the Swift Standard Library.

This code change will now show the external protocol's name,
MutableCollection, just below the declaration:

Note the new text "Available when Self is MutableCollection."

The new "Self is ExternalProtocol" message will be combined with any other
existing generic constraints. For example, uniquePermutations(ofCount:)
already requires that the associated type Element conform to the Hashable
protocol.

In this screenshot note the combined text "Available when Element conforms to
Hashable and Self is Collection."

Dependencies

This depends on a new initializer added to SymbolKit in swiftlang/swift-docc-symbolkit#63. We need to merge that PR first, before running the CI tests for this, or merging this.

Testing

The Swift DocC benchmark command
did not show any significant performance regressions or differences.
Rebuilding the documentation for the Swift Algorithms package causes the names of external protocols to appear as expected.
Rebuilding the documentation for other large Swift projects did not introduce any unexpected changes or regressions.
Unit tests were added, including a new test bundle ModuleWithProtocolExtensions.docc that includes symbol graph files showing the use of an external protocol.

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

[ x ] Added tests
[ x ] Ran the ./bin/test script and it succeeded
~~[ ] Updated documentation if necessary~~

the external protocol's name. This is helpful because these pages don’t have the context of being curated inside their “owning” type or protocol. Expand the section that we use for conditional constraints to display the external protocol name, for example: "Self is ExternalProtocol". The new "Self is ExternalProtocol" message will be combined with any other existing generic constraints. Do not do this for extended structs, classes or other extended types, since this generic constraint syntax wouldn't be accurate for them. Also as a related bug fix remove the extendedModule property of the Semanic Symbol class, and instead calculate this based on swiftExtensionVariants. Same for constraintsVariants. Fix the initializer of Semantic Symbol, which previously allowed a nil default value for the extendedModule property, allowing callers to accidentally leave it nil.

patshaughnessy · 2023-10-06T23:01:32Z

@swift-ci please test

patshaughnessy · 2023-10-06T23:34:04Z

@swift-ci please test

patshaughnessy · 2023-10-06T23:59:13Z

@swift-ci please test

patshaughnessy · 2023-10-09T14:42:26Z

@swift-ci please test

patshaughnessy · 2023-10-09T17:52:01Z

@swift-ci please test

d-ronnqvist · 2023-10-10T22:21:10Z

Sources/SwiftDocC/Semantics/Symbol/Symbol.swift

+            // Double check the existing extension uses the same module and type. If it does not,
+            // we must have a tooling or data consistency problem.
+            assert(
+                existing.extendedModule == extendedModule, //&& existing.typeKind == typeKind,


Should the commented existing.typeKind == typeKind assertion be included or should it be removed?

Oops I had disabled the second condition while debugging something. I added it back.

d-ronnqvist · 2023-10-10T22:22:05Z

Sources/SwiftDocC/Semantics/Symbol/Symbol.swift

+        - typeKind: The ``SymbolGraph/Symbol/KindIdentifier`` of the symbol this extension extends.
+        - constraint: The new generic constraints to add
+        - trait: Which generic constraints variant to append to
+     - Returns: True if the constraint was appended to an existing or new swift extension variant. False if there was an existing swift extension variant for a different module.


This function doesn't have a return value.

d-ronnqvist · 2023-10-10T22:32:27Z

Sources/SwiftDocC/Semantics/Symbol/Symbol.swift

+    // MARK: - Private helpers
+
+    /// Return all of this symbol's Swift extension variants.
+    func swiftExtensionVariants() -> [DocumentationDataVariantsTrait : SymbolGraph.Symbol.Swift.Extension] {


If nothing other then Symbol needs to call this we could make it private

Yes thank you. Marked private.

- Remove incorrect doc comment about a return value - Fix assertion in Symbol#addConstraint - Symbol#swiftExtensionVariants is private

patshaughnessy · 2023-10-10T23:26:04Z

@swift-ci please test

d-ronnqvist · 2023-10-10T23:50:32Z

Sources/SwiftDocC/Semantics/Symbol/Symbol.swift

+    public func addConstraint(
+        extendedModule: String,
+        typeKind: SymbolGraph.Symbol.KindIdentifier? = nil,
+        constraint newConstraint: SymbolGraph.Symbol.Swift.GenericConstraint,
+        trait: DocumentationDataVariantsTrait = DocumentationDataVariantsTrait.swift


It's good that you are supporting multiple traits here but I'm wondering if we really need to.

Considering that the added constraint is Swift specific, I think maybe we could remove the trait argument and rename the method to indicate that this is only applicable to Swift. That would allow for the implementation to ignore the other traits which realistically should never have this mixin.

Maybe something like this?

/// Append a new generic constraint for the given extended module /// - Parameters: /// - extendedModule: The name of the extended module. /// - extendedSymbolKind: The kind of the extended symbol. /// - constraint: The new generic constraints to add. public func addSwiftExtensionConstraint( extendedModule: String, extendedSymbolKind: SymbolGraph.Symbol.KindIdentifier? = nil, constraint newConstraint: SymbolGraph.Symbol.Swift.GenericConstraint ) {

- Rename Symbol#addConstraint to addSwiftExtensionConstraint and assume the only relevant trait is DocumentationDataVariantsTrait.swift. - Also rename the typeKind parameter to extendedSymbolKind.

patshaughnessy · 2023-10-11T22:21:40Z

@swift-ci please test

d-ronnqvist · 2023-10-12T17:52:01Z

Sources/SwiftDocC/Infrastructure/Symbol Graph/SymbolGraphRelationshipsBuilder.swift

+    ///   - engine: A diagnostic collecting engine.
+


Suggested change

/// - engine: A diagnostic collecting engine.

/// - documentationCache: A documentation node lookup by the node's resolved reference.

d-ronnqvist · 2023-10-12T17:55:26Z

Tests/SwiftDocCTests/Semantics/SymbolTests.swift

+        // This symbol starts with 3 generic constraints. See:
+
+        // jq < "Tests/SwiftDocCTests/Test Bundles/TestBundle.docc/mykit-iOS.symbols.json" '.symbols[] | select(.identifier.precise == "s:5MyKit0A5ClassC10myFunctionyyF") | .swiftExtension'


I would maybe just remove the "jq" part of the comment.

Suggested change

// This symbol starts with 3 generic constraints. See:

// jq < "Tests/SwiftDocCTests/Test Bundles/TestBundle.docc/mykit-iOS.symbols.json" '.symbols[] | select(.identifier.precise == "s:5MyKit0A5ClassC10myFunctionyyF") | .swiftExtension'

// The original symbol has 3 Swift extension constraints:

d-ronnqvist · 2023-10-12T17:56:34Z

Tests/SwiftDocCTests/Semantics/SymbolTests.swift

+        let constraints = try XCTUnwrap(withoutArticle.constraints)
+        XCTAssertEqual(3, constraints.count)
+        var constraint = constraints[0]
+        XCTAssertEqual(SymbolGraph.Symbol.Swift.GenericConstraint.Kind.sameType, constraint.kind)


FYI: The compiler should be able to infer the types here allowing these assertions to be shorter and more readable

Suggested change

XCTAssertEqual(SymbolGraph.Symbol.Swift.GenericConstraint.Kind.sameType, constraint.kind)

XCTAssertEqual(.sameType, constraint.kind)

That's great - it must be that since I wrote the expected value first, the compiler/Xcode showed me an error until I finished typing the line. I wonder if it makes more sense to write the actual value first inside of XCTAssertEqual for this reason...

d-ronnqvist · 2023-10-12T18:01:55Z

Tests/SwiftDocCTests/Semantics/SymbolTests.swift

+        XCTAssertEqual(SymbolGraph.Symbol.Swift.GenericConstraint.Kind.superclass, constraint.kind)
+        XCTAssertEqual("Observer", constraint.leftTypeName)
+        XCTAssertEqual("NSObject", constraint.rightTypeName)


FYI: The constrain is equatable so you can compare the entire value on one XCTAsserEqual(...) if you want to:

Suggested change

XCTAssertEqual(SymbolGraph.Symbol.Swift.GenericConstraint.Kind.superclass, constraint.kind)

XCTAssertEqual("Observer", constraint.leftTypeName)

XCTAssertEqual("NSObject", constraint.rightTypeName)

XCTAssertEqual(.init(kind: .superclass, leftTypeName: "Observer", rightTypeName: "NSObject"), constraint)

d-ronnqvist

This looks good to me.

I will just point out that this is a small source breaking change because the setters for
SwiftDocC/Symbol/constraintsVariants and SwiftDocC/Symbol/constraints are removed.

However, because assigning new values to those properties didn't really work before I think it's acceptable to remove them.

If we wanted we could add them back, mark them as deprecated, and assert in debug builds.

- Correct doc comment - Clean up/refactor unit test

patshaughnessy · 2023-10-12T21:48:04Z

@swift-ci please test

patshaughnessy · 2023-10-17T17:51:42Z

@swift-ci please test

…ded-protocol-names

patshaughnessy · 2023-10-17T22:15:10Z

@swift-ci please test

patshaughnessy requested a review from d-ronnqvist October 5, 2023 21:57

Merge branch 'main' into display-extended-protocol-names

256ecdb

swift package update swift-docc-symbolkit

f124d5f

d-ronnqvist reviewed Oct 10, 2023

View reviewed changes

PR Feedback:

7a8e67e

- Remove incorrect doc comment about a return value - Fix assertion in Symbol#addConstraint - Symbol#swiftExtensionVariants is private

d-ronnqvist reviewed Oct 10, 2023

View reviewed changes

PR Feedback:

3563e66

- Rename Symbol#addConstraint to addSwiftExtensionConstraint and assume the only relevant trait is DocumentationDataVariantsTrait.swift. - Also rename the typeKind parameter to extendedSymbolKind.

d-ronnqvist reviewed Oct 12, 2023

View reviewed changes

d-ronnqvist approved these changes Oct 12, 2023

View reviewed changes

PR Feedback:

40914e4

- Correct doc comment - Clean up/refactor unit test

patshaughnessy requested review from Teapane and franklinsch October 12, 2023 23:45

franklinsch approved these changes Oct 17, 2023

View reviewed changes

Merge branch 'main' into display-extended-protocol-names

9982ba1

Merge branch 'main' of github.com:apple/swift-docc into display-exten…

86c592f

…ded-protocol-names

patshaughnessy merged commit 04be889 into swiftlang:main Oct 17, 2023
2 checks passed

patshaughnessy deleted the display-extended-protocol-names branch October 17, 2023 23:40

sofiaromorales mentioned this pull request May 23, 2024

Fix bug where symbols would get the framework name show up twice in the availability badges. #922

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

When documenting members of extensions to external protocols, display the external protocol's name #732

When documenting members of extensions to external protocols, display the external protocol's name #732

patshaughnessy commented Oct 5, 2023 •

edited

Loading

patshaughnessy commented Oct 6, 2023

patshaughnessy commented Oct 6, 2023

patshaughnessy commented Oct 6, 2023

patshaughnessy commented Oct 9, 2023

patshaughnessy commented Oct 9, 2023

d-ronnqvist Oct 10, 2023

patshaughnessy Oct 10, 2023

d-ronnqvist Oct 10, 2023

patshaughnessy Oct 10, 2023

d-ronnqvist Oct 10, 2023

patshaughnessy Oct 10, 2023

patshaughnessy commented Oct 10, 2023

d-ronnqvist Oct 10, 2023

patshaughnessy commented Oct 11, 2023

d-ronnqvist Oct 12, 2023

d-ronnqvist Oct 12, 2023

d-ronnqvist Oct 12, 2023

patshaughnessy Oct 12, 2023

d-ronnqvist Oct 12, 2023

d-ronnqvist left a comment

patshaughnessy commented Oct 12, 2023

patshaughnessy commented Oct 17, 2023

patshaughnessy commented Oct 17, 2023

	/// - engine: A diagnostic collecting engine.
	/// - documentationCache: A documentation node lookup by the node's resolved reference.

		// This symbol starts with 3 generic constraints. See:

		// jq < "Tests/SwiftDocCTests/Test Bundles/TestBundle.docc/mykit-iOS.symbols.json" '.symbols[] \| select(.identifier.precise == "s:5MyKit0A5ClassC10myFunctionyyF") \| .swiftExtension'

	XCTAssertEqual(SymbolGraph.Symbol.Swift.GenericConstraint.Kind.sameType, constraint.kind)
	XCTAssertEqual(.sameType, constraint.kind)

When documenting members of extensions to external protocols, display the external protocol's name #732

When documenting members of extensions to external protocols, display the external protocol's name #732

Conversation

patshaughnessy commented Oct 5, 2023 • edited Loading

Summary

Dependencies

Testing

Checklist

patshaughnessy commented Oct 6, 2023

patshaughnessy commented Oct 6, 2023

patshaughnessy commented Oct 6, 2023

patshaughnessy commented Oct 9, 2023

patshaughnessy commented Oct 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patshaughnessy commented Oct 10, 2023

Choose a reason for hiding this comment

patshaughnessy commented Oct 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-ronnqvist left a comment

Choose a reason for hiding this comment

patshaughnessy commented Oct 12, 2023

patshaughnessy commented Oct 17, 2023

patshaughnessy commented Oct 17, 2023

patshaughnessy commented Oct 5, 2023 •

edited

Loading