-
Notifications
You must be signed in to change notification settings - Fork 157
Populate abbreviated declaration fragments in external render nodes #1255
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
Populate abbreviated declaration fragments in external render nodes #1255
Conversation
8401492
to
ee465d6
Compare
Some tests are failing in The full declaration fragments are used to determine the full name of the symbol for diagnostic reporting: swift-docc/Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy+Error.swift Lines 115 to 117 in 1b4a185
I think we will need a new field in |
ee465d6
to
79c9844
Compare
I've updated the implementation so that all unit tests should now pass. |
@swift-ci please test |
1 similar comment
@swift-ci please test |
Thanks for the incredible work on documenting this PR, Andrea! The only question I have about the implementation is if TopicRenderReference only takes fragments, and not subheadingFragments [1], why do we need to add subHeadingDeclarationFragments into the ResolvedInformation [2]? Can't we simply keep the summarized version and default its value to the full declaration from the LinkDestinationSummary in case this is nil? [1] https://github.com/swiftlang/swift-docc/pull/1255/files#diff-c195a5776f681e560e52fc959b22479573bbc847217e4ba4b66c0a0f29f31424L169 |
Tests/SwiftDocCTests/LinkTargets/LinkDestinationSummaryTests.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftDocC/Infrastructure/External Data/OutOfProcessReferenceResolver.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftDocC/Infrastructure/Link Resolution/LinkResolver+NavigatorIndex.swift
Show resolved
Hide resolved
Sources/SwiftDocC/Infrastructure/External Data/OutOfProcessReferenceResolver.swift
Outdated
Show resolved
Hide resolved
2785db2
to
aa7f7dd
Compare
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.
Overall this looks good but I feel that we should make the public API clearer (since that's harder to change later). Also, we should add the new JSON key to the specification file.
{ | ||
"kind" : "keyword", | ||
"spelling" : "typedef" | ||
}, | ||
{ | ||
"kind" : "text", | ||
"spelling" : " " | ||
}, | ||
{ | ||
"kind" : "keyword", | ||
"spelling" : "enum" | ||
}, | ||
{ | ||
"kind" : "text", | ||
"spelling" : " " | ||
"spelling" : "+ " | ||
}, | ||
{ | ||
"kind" : "identifier", | ||
"spelling" : "Foo" | ||
}, | ||
{ | ||
"kind" : "text", | ||
"spelling" : " : " | ||
}, | ||
{ | ||
"kind" : "typeIdentifier", | ||
"spelling" : "NSString", | ||
"preciseIdentifier": "c:@T@NSInteger" | ||
}, | ||
{ | ||
"kind": "text", | ||
"spelling": " {\n ...\n} " | ||
}, | ||
{ | ||
"kind": "identifier", | ||
"spelling": "Foo" | ||
}, | ||
{ | ||
"kind": "text", | ||
"spelling": ";" | ||
"spelling" : "myStringFunction:error:" |
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.
Question: Is this representative of real data? It looks odd to me that the return type isn't included.
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.
Yes, from looking at real examples I believe this is representative of real data and that the return type is not included in the subheading -- however I'm not the most familiar with Objective C symbol graphs so I might be wrong.
This PR is depending on #1300 being merged, as the changes it was depending on were reverted. There are still some comments to address here so I will do that next. I measured the performance impact of these changes using the
The overall performance impact is, over 10 iterations:
where 9413c59 is the commit I was comparing against. The results above show that the DocC archive size is now smaller compared to previously. |
/// | ||
/// This value is `nil` if the referenced page is not a symbol. | ||
var fragmentsVariants: VariantCollection<[DeclarationRenderSection.Token]?> { | ||
topicRenderReference.fragmentsVariants |
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.
Question: since this is for the navigator, should it return the navigator title?
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.
It's meant to return the subheading declaration fragments AFAICT -- for internal links in the navigator, we store this information:
swift-docc/Sources/SwiftDocC/Indexing/Navigator/RenderNode+NavigatorIndex.swift
Lines 71 to 76 in 257c62d
var navigatorTitle: [DeclarationRenderSection.Token]? { | |
wrapped.navigatorTitleVariants.value(for: traits) | |
} | |
var fragments: [DeclarationRenderSection.Token]? { | |
wrapped.fragmentsVariants.value(for: traits) | |
} |
Where this type is wrapping RenderMetadata
and these fragments are derived from the symbol subheading fragments:
swift-docc/Sources/SwiftDocC/Model/Rendering/RenderNode/RenderMetadata.swift
Lines 124 to 131 in 257c62d
/// Abbreviated declaration to display in links. | |
public var fragments: [DeclarationRenderSection.Token]? { | |
get { getVariantDefaultValue(keyPath: \.fragmentsVariants) } | |
set { setVariantDefaultValue(newValue, keyPath: \.fragmentsVariants) } | |
} | |
/// The variants for the fragments of a page. | |
public var fragmentsVariants: VariantCollection<[DeclarationRenderSection.Token]?> = .init(defaultValue: nil) |
node.metadata.fragmentsVariants = contentRenderer.subHeadingFragments(for: documentationNode) |
And then finally there's some logic when deriving the final navigator title, to choose to use either the subheading fragments or the navigator fragments:
swift-docc/Sources/SwiftDocC/Indexing/Navigator/RenderNode+NavigatorIndex.swift
Lines 141 to 157 in 257c62d
/// Returns a navigator title preferring the fragments inside the metadata, if applicable. | |
func navigatorTitle() -> String? { | |
let tokens: [DeclarationRenderSection.Token]? | |
// FIXME: Use `metadata.navigatorTitle` for all Swift symbols (github.com/swiftlang/swift-docc/issues/176). | |
if identifier.sourceLanguage == .swift || (metadata.navigatorTitle ?? []).isEmpty { | |
let pageType = navigatorPageType() | |
guard !typesThatShouldNotUseNavigatorTitle.contains(pageType) else { | |
return metadata.title | |
} | |
tokens = metadata.fragments | |
} else { | |
tokens = metadata.navigatorTitle | |
} | |
return tokens?.map(\.text).joined() ?? metadata.title | |
} |
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 looks good to me.
…gments` These declaration fragments are directly used to populate `TopicRenderReference.fragmentsVariants` [1], which is described as "The abbreviated declaration of the symbol to display in links" [2]. This change clarifies that the declaration fragments are expected to be the abbreviated declaration fragments. [1]: https://github.com/swiftlang/swift-docc/blob/65aaf926ec079ddbd40f29540d4180a70af99e5e/Sources/SwiftDocC/Infrastructure/External%20Data/OutOfProcessReferenceResolver.swift#L169 [2]: https://github.com/swiftlang/swift-docc/blob/65aaf926ec079ddbd40f29540d4180a70af99e5e/Sources/SwiftDocC/Model/Rendering/References/TopicRenderReference.swift#L50-L53
The only use-case for storing the full declaration fragments in `LinkDestinationSummary` rather than the shortened declaration fragments intended for display, is to derive the full name of the symbol in diagnostics [1] [2]. We have a number of tests [3] which verify that the diagnostic emitted when a symbol is referenced locally is the same as when the symbol is referenced externally. However, we want to stop storing the full declaration fragments in favour of the shortened ones because: - the full fragments are never displayed when referenced externally, as they are too long - the full fragments take up additionally storage space, and encoding/decoding time, to only be used to derive the full name of the symbol This commit updates the logic such that, we pre-derive the full name of the symbol from its declaration fragments, and then store that as part of `LinkDestinationSummary`, so that in a subsequent commit we can stop storing the full declaration fragments and store only the shortened ones instead. The diagnostic logic has also been updated to use the new field rather than the full declaration fragments. [1]: https://github.com/swiftlang/swift-docc/blob/1b4a1850dd2785a8ebabded139ae0af3551bb029/Sources/SwiftDocC/Infrastructure/Link%20Resolution/ExternalPathHierarchyResolver.swift#L61-L63 [2]: https://github.com/swiftlang/swift-docc/blob/1b4a1850dd2785a8ebabded139ae0af3551bb029/Sources/SwiftDocC/Infrastructure/Link%20Resolution/PathHierarchy%2BError.swift#L115-L117 [3]: https://github.com/swiftlang/swift-docc/blob/9413c599817abc9dd4f8feeed57a998b19a05a6f/Tests/SwiftDocCTests/Infrastructure/ExternalPathHierarchyResolverTests.swift#L907-L918
LinkDestinationSummary contains a summary of an element that you can link to from outside the documentation bundle. [1] This information is meant to be used by a server to provide information to an out-of-process resolver to resolve links to external entities, so that the partner implementation of `LinkDestinationSummary` is `OutOfProcessReferenceResolver.ResolvedInformation` [2]. However, currently `OutOfProcessReferenceResolver.ResolvedInformation. declarationFragments` is expecting the abbreviated declaration fragments, but we are storing the full fragments instead. [3] Instead, we should be storing the abbreviated declaration fragments, which are stored as the `subHeading` of the symbol. [4] This subheading is further processed during the render node transformation phase [5], and stored as `renderNode.metadata.fragmentsVariants`. This commit modifies `LinkDestinationSummary` such that its declaration fragments are the abbreviated declaration fragments from `renderNode.metadata.fragmentsVariants` rather than the full declaration fragments. The final result will be that declaration fragments for external links will behave the same as local links when referencing them in the Topics section. They will both now use the abbreviated declaration fragments. [1]: https://github.com/swiftlang/swift-docc/blob/65aaf926ec079ddbd40f29540d4180a70af99e5e/Sources/SwiftDocC/LinkTargets/LinkDestinationSummary.swift#L66 [2]: https://github.com/swiftlang/swift-docc/blob/65aaf926ec079ddbd40f29540d4180a70af99e5e/Sources/SwiftDocC/Infrastructure/External%20Data/OutOfProcessReferenceResolver.swift#L558-L562 [3]: https://github.com/swiftlang/swift-docc/blob/65aaf926ec079ddbd40f29540d4180a70af99e5e/Sources/SwiftDocC/LinkTargets/LinkDestinationSummary.swift#L445 [4]: https://github.com/swiftlang/swift-docc-symbolkit/blob/ebe89c7da4cf03ded04cd708f3399087c6f2dad7/Sources/SymbolKit/SymbolGraph/Symbol/Names.swift#L28-L31
Now that the declaration fragments will be the abbreviated declaration fragments from `LinkDestinationSummary`, we can propagate those to the navigator metadata for them to be used to inform the title of the navigator item [1]. Fixes rdar://156488052. [1]: https://github.com/swiftlang/swift-docc/blob/65aaf926ec079ddbd40f29540d4180a70af99e5e/Sources/SwiftDocC/Indexing/Navigator/RenderNode%2BNavigatorIndex.swift#L140
The navigator title, which contains abbreviated fragments of a symbol's declaration for display in navigation [1], was missing from LinkDestinationSummary. The navigator title is used here [2], but the information wasn't being populated anywhere [3]. This commit captures the navigator title information from the render metadata, and stores it as part of `LinkDestinationSummary`. While the original information comes from the symbol graph [4], the `DocumentationContentRenderer` has some special logic to sanitise the declaration fragments, particularly for Swift [5]. In order to ensure that we have parity between internal and external navigator titles, used the render metadata rather than the original information from the symbol graph. This mirrors what we're already doing with the shortened declaration fragments, which also are processed with the same logic. Finally, `LinkDestinationSummary.makeTopicRenderReference()` has been updated to populate the navigator title variants, such that after this commit navigator titles will be correctly populated for external links in the navigator. Fixes rdar://156488184. [1]: https://github.com/swiftlang/swift-docc/blob/257c62d1ef11efc023667fda89c0cd4419a8afa3/Sources/SwiftDocC/Model/Rendering/References/TopicRenderReference.swift#L60-L63 [2]: https://github.com/swiftlang/swift-docc/blob/257c62d1ef11efc023667fda89c0cd4419a8afa3/Sources/SwiftDocC/Indexing/Navigator/RenderNode%2BNavigatorIndex.swift#L153 [3]: https://github.com/swiftlang/swift-docc/blob/257c62d1ef11efc023667fda89c0cd4419a8afa3/Sources/SwiftDocC/Infrastructure/Link%20Resolution/ExternalPathHierarchyResolver.swift#L225 [4]: https://github.com/swiftlang/swift-docc-symbolkit/blob/3fc0c6880f712ce38137d0752f24b568fce1522e/Sources/SymbolKit/SymbolGraph/Symbol/Names.swift#L23-L26 [5]: https://github.com/swiftlang/swift-docc/blob/257c62d1ef11efc023667fda89c0cd4419a8afa3/Sources/SwiftDocC/Model/Rendering/DocumentationContentRenderer.swift#L87-L105
Now that the navigator title is supported in external references, we can add some tests which verify this logic is working as expected. Added some tests which verify that the navigator title is used as expected for both Swift and Objective C symbols.
`plainTextDeclaration` is clearer and makes it harder to confuse it with `title`. It is also closer to how the value is derived. Also updates the JSON schema in `LinkableEntities.json` to add the new `plainTextDeclaration` property to the `LinkDestinationSummary` and `LinkDestinationSummaryVariant` schemas.
`declarationFragments` was not descriptive enough about the contents of the property, so renamed to `subheadingDeclarationFragments` and deprecated `declarationFragments` for backwards compatibility. The JSON encoding key is kept the same (`fragments`) for backwards compatibility.
Renamed the property in `LinkDestinationSummary` which stores the declaration fragments for the navigator to `navigatorDeclarationFragments`, to follow the same naming convention as `subheadingDeclarationFragments` which fulfils a very similar purpose. The documentation comments mirror each other, with one explaining that it's intended for use in topic groups, and the other that it's intended for use in navigators.
Clarifies the function name so that it clearly states that only top level nodes are returned, as well as improving the grammar at the call site.
f6149e0
to
edd24c2
Compare
@swift-ci please test |
Bug/issue #, if applicable: rdar://152652751&156488184
Summary
Improves the quality of the navigator titles for external render nodes. Navigator titles are derived from the item's declaration fragments when at least one of the following is met 1:
These declaration fragments are expected to be the abbreviated declaration fragments 3 of the symbol.
This PR helps capture the abbreviated declaration fragments of the symbol for external entities, so that they can be used in the navigator. The final goal is to have external entities in the navigator render no differently from local entities.
Implementation
There was no change needed to the
LinkResolver.ExternalEntity
type, as some declaration fragments were already being stored as part ofLinkResolver.ExternalEntity.TopicRenderReference.fragmentsVariants
4.The main issue was that while
OutOfProcessReferenceResolver.ResolvedInformation
relied on the fragments that it was receiving from theOutOfProcessReferenceResolver
to be the abbreviated declaration fragments,LinkDestinationSummary
was only encoding the full declaration fragments only as part of its summary:swift-docc/Sources/SwiftDocC/LinkTargets/LinkDestinationSummary.swift
Line 445 in 1b4a185
swift-docc/Sources/SwiftDocC/LinkTargets/LinkDestinationSummary.swift
Line 454 in 1b4a185
The abbreviated declaration fragments are different from the full declaration fragments and defined as a different field (
subHeading
5) in the symbol graph (and therefore cannot be derived by DocC), but they're ultimately processed and captured as part ofRenderMetadata.fragmentsVariants
6.The main changes needed were to update the documentation of
OutOfProcessReferenceResolver.ResolvedInformation
andLinkDestinationSummary
to have an extra property,subHeadingDeclarationFragments
, which stores the abbreviated declaration fragments. These declaration fragments can then be used when initialisingLinkResolver.ExternalEntity.TopicRenderReference.fragmentsVariants
.More detail on the implementation can be found in commit 832f1eb.
Alternatives considered
Considered modifying
LinkDestinationSummary.declarationFragments
instead of adding a new optional field, however the full declaration fragments are used to determine the full name 7 of the symbol for diagnostic reporting 8.By introducing a new field we also ensure this is a non-breaking change.
Navigator comparison (using swift-docc-render):
This compares what it looks like to refer to those symbols within its local DocC bundle versus what it would look like to curate them as external links:
And if we also bring in the symbol kind changes from #1251, it becomes even closer:
Dependencies
N/A
Testing
Verified by using 2 bundles -- one for generating linkable entities, another for referencing an external link from the other bundle -- and verifying that the declaration fragments in the linkable entities are the abbreviated, subheading declaration fragments.
Steps:
swift run docc preview ExampleA.docc --emit-digest
a. Look at http://localhost:8000/documentation/mykit
b. Look at the Topics section and the navigator titles for both the class and the function
DOCC_LINK_RESOLVER_EXECUTABLE=ExampleB.docc/bin/test-data-external-resolver swift run docc preview ExampleB.docc
Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
AddedUpdated tests./bin/test
script and it succeeded