-
Notifications
You must be signed in to change notification settings - Fork 143
Extensions to External Types (Hierarchical) #369
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
Extensions to External Types (Hierarchical) #369
Conversation
06f70dc
to
8eef6cd
Compare
Extensions for external types is so essential - I only just realised it's not yet supported by DocC! |
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 generally looks really good. My two main pieces of feedback:
- Please add a feature flag for the extension support (to opt-out even if a symbol graph file with the extension format is passed as input to DocC)
- There shouldn't be any differences in the result of
SymbolGraphLoader.loadAll(using:)
between the two decoding strategies.
I hope to finish going through ExtendedTypesFormatTransformation
later this week.
for kind in AutomaticCuration.groupKindOrder where kind != .module { | ||
if !SymbolGraph.Symbol.KindIdentifier.allCases.contains(kind) { |
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.
As mentioned in another comment: should the AutomaticCuration.groupKindOrder
always be registered or is that only applicable to this test?
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 only applicable to this test.
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.
Have I understood correctly that this change is needed because new kinds (the extended types) were added to the auto curation list that aren't decodable in a main symbol graph file by default?
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.
If so, this test would be better if it decoded and transformed the extended symbols from an extension symbol graph file, the same way that they would be decoded and processed in a real documentation build.
I don't feel that this needs to be addressed in this PR but it would be good to file an issue and add a TODO/FIXME comment.
Sources/SwiftDocC/Model/Rendering/DocumentationContentRenderer.swift
Outdated
Show resolved
Hide resolved
Tests/SwiftDocCTests/Infrastructure/SymbolGraph/SymbolGraphLoaderTests.swift
Show resolved
Hide resolved
Tests/SwiftDocCTests/Infrastructure/SymbolGraph/SymbolGraphLoaderTests.swift
Outdated
Show resolved
Hide resolved
Tests/SwiftDocCTests/Infrastructure/ReferenceResolverTests.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftDocC/Infrastructure/Symbol Graph/SymbolGraphLoader.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftDocC/Infrastructure/Symbol Graph/ExtendedTypesFormatTransformation.swift
Outdated
Show resolved
Hide resolved
89fd7c2
to
03af8e2
Compare
/// called iteratively on the (modified) target and the next source element. | ||
private static func attachDocComments<T: MutableCollection>(to targets: inout T, | ||
using source: (T.Element) -> [SymbolGraph.Symbol], | ||
onConflict resolveConflict: (_ old: T.Element, _ new: SymbolGraph.Symbol) |
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 function has a bit of unused functionality and removing it can simplify both the function and its call site.
This is private and only called from one place which doesn't pass a value for the onConflict
argument and which either returns a source with 0 or 1 elements. Removing the onConflict
argument and changing the source
argument type to
+ using sourceDocComment: (T.Element) -> SymbolGraph.LineList?
- using source: (T.Element) -> [SymbolGraph.Symbol]
which means that the for loop can be replaced with just the assignment
target.docComment = sourceDocComment(target)
(since the guard
statement ensures that the target doc comment is nil
.)
With the change to return the doc comment directly from the closure, the call site compactMap
can chain to get the doc comment which removes the optional types in the check for the longest doc comment.
Since the extension declarations are sorted based on their precise identifiers which is also the keys of extensionBlockSymbols
I think that the call site can be written as:
attachDocComments(to: &extendedTypeSymbols.values) { target in
return extendedTypeToExtensionBlockMapping[target.identifier.precise]?
.sorted() // The order of symbols in not stable across different builds.
.compactMap({ extensionBlockSymbols[$0]?.docComment })
.max(by: { lhs, rhs in lhs.lines.count < rhs.lines.count })
}
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 function is more complex than needed because I plan on iterating on the aggregation logic once the basics are complete. I'll create an issue for that and link it here when I find time.
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.
I left two comments that I don't feel are blocking and can be addressed later as new issues:
- The
ExtendedTypesFormatTransformation.attachDocComments(to:using:)
implementation and call site can be simplified. - If the symbol graph
JSONDecoder
configurability is only to enableAutomaticCurationTests.testAutomaticTopics()
to decode extended types from a main symbol graph file then I'd prefer that the extended symbol kinds were decoded and transformed from an extension symbol graph file instead in that file.
for kind in AutomaticCuration.groupKindOrder where kind != .module { | ||
if !SymbolGraph.Symbol.KindIdentifier.allCases.contains(kind) { |
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.
If so, this test would be better if it decoded and transformed the extended symbols from an extension symbol graph file, the same way that they would be decoded and processed in a real documentation build.
I don't feel that this needs to be addressed in this PR but it would be good to file an issue and add a TODO/FIXME comment.
Based on the extra information from your comment here I wrote some additional documentation for the extended types transformation to aid my understanding as I was reading the code. Feel free to incorporate any of it if you like.
|
6f7062d
to
9f4bc0a
Compare
Excellent documentation! This documentation captures the application context way better than mine. I only adapted some parts of it to create a connection towards the function's parameters and return value. |
I added the TODO, however, I noticed there currently isn't a fitting issue template for code refactorings on GitHub. Similar TODOs are tracked via Apple's radar. |
9f4bc0a
to
57ffdee
Compare
I addressed your remaining notes @d-ronnqvist. Thanks for putting so much effort into the review! I rebased again, so please trigger the CI one more time, then this should be ready for merge. |
5864fe7
to
deba7fb
Compare
I almost forgot about the SymbolKit dependency. Please wait with the CI until swiftlang/swift-docc-symbolkit#39 is merged and I have updated the Package.swift to point back to apple main! |
@swift-ci please test |
This looks like a Linux specific failure:
I can't find those properties in JSONDecoder.swift so I'm wondering if they're now available on Linux. |
deba7fb
to
8a87601
Compare
@d-ronnqvist I updated the SymbolKit dependency to point pack to apple main and squashed the commits. I removed the two problematic This is good to go from my point of view! |
8a87601
to
c7e8eb9
Compare
@swift-ci please test |
…tlang/swift#59047 - introduce transformation generating internal Extended Types Symbol Graph Format from the Extension Block Symbol Graph Format emmitted by the compiler - register symbols and relationships used by Extended Types Symbol Graph Format - adapt SymbolGraphLoader to automatically detect if input Symbol Graph Files use the Extension Block Symbol Graph Format and apply the ExtendedTypesFormatTransformation in case - improve Swift title token parsing to correctly identify Symbol titles of nested types, which contain "." infixes added tests: - test detection of the Extension Block Symbol Graph Format and application of the ExtendedTypesFormatTransformation in SymbolGraphLoader - test the ExtendedTypesFormatTransformation - extend tests for Swift title token parsing with test-cases containing "." infixes
c7e8eb9
to
91f54a8
Compare
Sorry @d-ronnqvist, the branch was already outdated again. Ethan has been quite active on main recently 😅 |
@swift-ci please test |
@theMomax Thanks for working on this huge improvement to docc. I'm testing your solution with
and getting these errors:
Unfortunately, I can not share the source code. The error is emitted by docc in The |
@edudnyk thanks for trying out this feature. As recently discussed here, my current work only focuses on Swift extensions. That's probably the reason why all the Objective C identifiers can't be found. Nevertheless, they definitely should be filtered out before they can cause errors to be thrown and there is also one Swift identifier in your list, so I'll definitely need to investigate. Would it be possible for you to share the generated Symbol Graph Files with me personally via email? This would help me a lot trying to reproduce your issue. The Symbol Graph Files only contain information about the declarations in your code, so none of your business logic would be exposed. |
@theMomax done. If I can help you in any way to find the root cause, please let me know. |
@edudnyk this was in fact a bug in the symbol graph generation for extended external types. This PR should fix the issue. In the meantime you can just manually remove the |
@theMomax thanks for making the fix. I managed to find the smallest reproducible example with the following Xcode project. I noticed another couple of issues:
|
Sorry for the late response @edudnyk. Thank you for the project, this helps a lot. In response to the issues you found: 1.This is per spec and has nothing to do with this PR. See the documentation I added here. We are aware that this is not ideal, but it is also not a pressing issue at the moment. Feel free to create an issue if this is important to you. 2.This is definitely a bug. The problem here is that main symbol graph files should not contain extension symbols, but for the I think that the only real solution is for the Swift compiler to basically treat these extensions across re-exported targets as local extensions (which are treated as if they really were declared in |
#210 (Forums Discussion)
Summary
This PR enables users of DocC to document extensions to external types, i.e. local extensions, that extend a type declared in a different module. These extensions are presented as part of the extending module's documentation archive.
All extensions to one type are merged into an Extended Type page, where "Type" can be either of struct, class, enum, protocol, or just "Type", if the extended type's kind is unknown. Extended Type pages list their children, conformances, etc. just as pages for normal types.
All top-level Extended Type pages, where the extended type was declared in the same module, are children of the same Extended Module page. The Extended Module pages are top level pages, i.e. by default listed in the archive overview page.
Referencing
Extended Module symbols are referenced using
``ModuleName``
, Extended Type symbols via``ModuleName/Path/To/ExtendedType``
. ThePath/To/ExtendedType
has more than one segment for extensions to nested external types. Extensions to type aliases use the name of the aliased (original) type.Please refer to
SwiftDocC.docc/SwiftDocC/LinkResolution.md
for a more detailed explanation.The new pages can be curated and extended as usual using these references.
Documentation Aggregation
Extended Module pages have no documentation by default. It is possible to add documentation via extension markup documents.
Extended Type pages have default documentation. They use the longest documentation comment of any extension block that contributed to the extended type. Again, it is possible to add documentation via extension markup documents.
Example:
Screenshots
The list of Extended Modules in the archive overview.

An example for an Extended Module page.

An example for an Extended Structure page.

The bottom part of the same Extended Structure page.

Implementation
Many of the files only had to be changed to introduce the six new symbol kinds
module.extension
,class.extension
,struct.extension
,enum.extension
,protocol.extension
, andunknown.extension
as well as the newdeclaredIn
andinContextOf
relationship kinds.Other than that, there are three components where actual logic had to be adapted:
SymbolGraphLoader
The loader detects whether or not the loaded symbol graphs contain
swift.extension
symbols. If all (non-main) files containswift.extension
symbols, theExtendedTypesFormatTransformation
is applied to (non-main) symbol graphs, and the loader configures theGraphCollector
to merge extension graphs with the extending main graph instead of the extended one. If the loader detects a mixed input where some SGFs use the extension block format and some don't, it aborts loading with an error.If there is no SFG that uses the extension block format, the loader behaves as it did prior to this PR.
ExtendedTypesFormatTransformation
This transformation generates the
swift.module.extension
andswift.TYPE_KIND.extension
symbols from theswift.extension
symbols. There is a detailed explanation available in code here.The transformation essentially restructures and aggregates the
swift.extension
symbols to match the page structure we want to have in the end. After the transformation, most of Swift-DocC's logic can handle the new content without changes.DocumentationContentRenderer+Swift -> navigatorTitle
Extension declarations only appear in top level code. Thus, if a nested external type is extended, it appears outside the context of its parent and therefore uses dots in its name, e.g.
Outer.Inner
. This behavior is reflected in the declaration fragments. The renderer could not handle dots in type names before, which resulted in the coloring being off. A more sophisticated parsing logic was added which is capable of handling names with dot infixes.Dependencies
Testing
There exist unit tests that:
- test detection of the Extension Block Symbol Graph Format and application of the
ExtendedTypesFormatTransformation in SymbolGraphLoader
- test the ExtendedTypesFormatTransformation
- test references work with the old
DocumentCacheBasedLinkResolver
as well as the newPathHierarchyBasedLinkResolver
.Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
./bin/test
script and it succeeded