-
Notifications
You must be signed in to change notification settings - Fork 132
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 #335
Conversation
de54bff
to
a0e502d
Compare
24eba25
to
0571862
Compare
…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 reference collision detection logic to deal with emitted path components that occur when extending external nested types - adapt reference resolution logic to first search for paths in the local module by default and introduce shorthand absolute syntax with "/" prefix to mark paths as absolute (to be searched for only in the global scope) - 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
- test detection of the Extension Block Symbol Graph Format and application of the ExtendedTypesFormatTransformation in SymbolGraphLoader - test the ExtendedTypesFormatTransformation - test handling of collisions resulting from extensions to nested external types in DocumentationContext - test tests for absolute/relative reference resolution for ambiguous relative references
0571862
to
3ae5f3b
Compare
…olver to underlying PathHierarchy
@@ -1920,7 +1923,7 @@ public class DocumentationContext: DocumentationContextDataProviderDelegate { | |||
discoveryGroup.async(queue: discoveryQueue) { [unowned self] in | |||
symbolGraphLoader = SymbolGraphLoader(bundle: bundle, dataProvider: self.dataProvider) | |||
do { | |||
try symbolGraphLoader.loadAll() | |||
try symbolGraphLoader.loadAll(using: decoder) |
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.
Mostly out of curiosity: why is a decoded passed as an argument? AFIACT the context's decoder is never modified.
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 can be used (in tests) to decode symbol kinds not known to SymbolKit. See e.g. the changes in Tests/SwiftDocCTests/Infrastructure/AutomaticCurationTests.swift
.
This registration process is part of swiftlang/swift-docc-symbolkit#39. For usage details please refer to the documentation here.
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.
I've only partially reviewed the link resolution code and its documentation.
There are some changes to central code in both the documentation cache based link resolver and in the path hierarchy based link resolver that I feel need more targeted tests to verify the new behaviors and use cases.
($0.path.lowercased(), $0.sourceLanguage) | ||
} } | ||
|
||
let referenceMap = symbols.concurrentMap { symbol in |
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.
I need to take some more time to review this change in detail. Like the comment at the top of this function says:
The implementation of this function is fairly tricky because in most cases it has to preserve past behavior.
Subtle changes, even bug fixes, can result in pages getting different paths or different disambiguation which results in broken links (since the documentation cache based link resolver requires the exact right paths and disambiguation) to resolve links.
Sources/SwiftDocC/Infrastructure/Link Resolution/DocumentationCacheBasedLinkResolver.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftDocC/Infrastructure/Link Resolution/DocumentationCacheBasedLinkResolver.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy.swift
Outdated
Show resolved
Hide resolved
|
||
### Handling Nested Types | ||
|
||
Sometimes it can happen that a symbol appears in your documentation catalog, but one or more of its original anchestors do not. This happens, for example, when extending a nested type from a different module. In those cases, the path components representing the missing anchestors are not part of the symbol page's link. |
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.
Why does the page's link not include all path components? To me that sounds like either a bug or an unintended implementation detail that we wouldn't want to define as the expected behavior.
@@ -79,7 +79,9 @@ let package = Package( | |||
// Test utility library | |||
.target( | |||
name: "SwiftDocCTestUtilities", | |||
dependencies: []), | |||
dependencies: [ | |||
"SymbolKit" |
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.
Mostly out of curiosity: what functionality of the utilities target is dealing with SymbolKit directly? My understanding is that the utilities target is mainly meant to cover the command-line interface and its related functionality but not any core logic.
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.
I factored out makeSymbolGraph(moduleName:symbols:relationships:)
into XCTestCase
as it was used in more than one test file. See Sources/SwiftDocCTestUtilities/SymbolGraphCreation.swift
Thank you for the very detailed PR description. It really helps to understand the changes. |
Thinking about this a bit more it would be good to have an off-by-default feature flag to allow developers opt in to the extension support. That would lower the risks for merging this, allow some time to live on it before enabling it by default and providing developers with a workaround if this raises issues in their projects.
I'm torn on what the resulting path for the extended page should be. I see the reason for not wanting path components in the URL that doesn't correspond to a page but skipping directly to the extended symbol leaves out important information about the symbol. For example, it's not uncommon for apps to extend On the link syntax in the content, As further examples; "AppKit" has both Similarly, DocC has both
I view this almost like a separate bug fix. It may be better to split this fix from the rest of the extension support feature and land it in a separate PR. |
@d-ronnqvist thanks so much for you extensive feedback! I see that there are a couple of critical changes in this PR. I'd like to take this opportunity and bring some broader context to this discussion, which could help us to arrive at an agreeable solution. First off, I think we need to make a final decision on the topics of path contractions, absolute path resolution, and shadowing. Path ContractionsThis decision is only relevant to the extended type feature coming with this PR.
You understanding is correct and I am, too, not 100% happy with the current situation. I've discussed this topic with @daniel-grumberg, @QuietMisdreavus, and @franklinsch before. IMO it makes sense that Therefore, I think the best solution in that case would be to synthesize Extended Type symbols for the respective path components in the The outer/intermediary Extended Type pages would, of course, contain a list of the nested extended types (as well as maybe the contents of extensions to the outer type). Thus, the intermediary extended type pages would not be totally empty. Nested Extended Type pages would no longer be listed on the respective Extended Module page. With that approach, the respective modifications to the path collision algorithms could be reverted. However, that is an "if" and I'd like to hear everyone's opinions on the topic! Absolute Path ResolutionThis topic is more consequential for the whole of DocC. Furthermore, @d-ronnqvist your PR #337 and your discussion about combined documentation of multiple targets changed the context quite a bit, so the ideas I had in mind when implementing this may no longer be valid. My implementation assumed that multiple targets would be compiled together by the I think we all agree that there is a need for "absolute" symbol links, i.e. such starting with a slash, which are not resolved in their local context, but always start after the "documentation" path component. However, the details of how to determine the correct documentation archive for such link is still discussed in the forums' thread above and the exact syntax is not really known. Therefore, it might be better to ignore the topic of absolute links in this PR as compiling multiple targets together using the ShadowingAgain, this topic is very consequential for the whole of DocC. My original proposal explicitly stated relative links to external symbols should be possible so that the external function would automatically be shadowed once the external symbol is overridden by a local extension: From the proposal:
@d-ronnqvist if I understood your post here correctly, you want to change this behavior. You do not want to allow relative links to refer to different modules/targets, correct? This would mean there is no shadowing at all, which would be a difference to how it works in Swift. The beauty of allowing these relative paths would be that they automatically switch over to the local extension once an external symbol is overridden locally. Another discussion is how much or little shadowing we want locally. Consider the following example: struct Identifier {
func foo() { }
}
struct Kind {
struct Identifier {
/// ``Identifier/foo()`` can be resolved by the new implementation whereas the
/// original algorithm would fail as `Kind.Idenitfier` has no child called `foo()`.
func bar() { }
}
} Before this PR (and in Again, I'd like to hear you opinions about the case specific to extensions, as well as the more general local case! A Path Forward
I think that depending on the decisions we make on the above topics, it may be better to delay the changes to path resolution to David's work on combined documentation, making this PR way more lightweight in size and impact. After all, the additional content from extensions already has an "off-by-default" feature flag on the Swift compiler (see Otherwise, (depending on how long you want to keep the |
I'm realizing that there would also need to be a way to use the full path but without creating pages extra pages for the extension or any in-between path components. It is possible that the same server is hosting the extending module and the extended module and in that case the in-between pages would exist but be served from another documentation archive. (This would already be possible today without the upcoming multi-target documentation feature).
Yes, for DocC itself it makes sense to pass input for all the targets to a single
That's would be a good question to bring up in the forums. I imagine that each external resolver would read the documentation archive's linkable entities and vend the modules that it can resolve to DocC. If multiple dependency documentation archives vend the same module name that would probably be a warning. When resolving an symbol link with leading slash DocC would try to resolve it locally and then check if any of the external resolvers had listed the first path component as one of their modules.
👍
I am proposing that external symbol links need to start with a leading slash but that it's optional for local symbol links. My understanding is that extensions would be part of the extending target's documentation build so it would be considered a "local" symbol (meaning that it's passed as symbol graph input and not resolved from an already built archive) even if it's not from the main target. This would allow a link with leading slash to be shadowed by a local extension, since DocC would attempt to resolve links locally before attempting to resolve them externally.
This I view as a bug in the
👍
It's very hard to say how long we plan on keeping |
That's not correct. Extended Type Pages are hosted at |
Great! Not an issue then. My mistake. I’ll make a note of it for possible future improvements to combined documentation. |
bffb3ef
to
7d34951
Compare
Closed in favor of #369. See the forums discussion for reference. |
#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, or protocol. Extended Type pages list their children, conformances, etc. just as pages for normal types.
All 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/ExtendedTypeName``
. TheExtendedTypeName
for an extension to a nested external type consists only of the name of the innermost type. 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 five new symbol kinds
module.extension
,class.extension
,struct.extension
,enum.extension
, andprotocol.extension
as well as the newdeclaredIn
relationship kind.Other than that, there are five 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 and 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. However, there are still a few new edge cases that needed special attention:DocumentationContentRenderer+Swift -> navigatorTitle
All extended types are direct children to the respective extended module page, independent of the extended types' original hierarchy. 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
. 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.Link Resolution
The following aspects had to be adapted in DocumentCacheBasedLinkResolver
as well as the new
PathHierarchyBasedLinkResolver(or, its underlying
PathHierarchy`). This topic is also explained in the SwiftDocC documentation.Path Contractions
When extending nested external types such as
Outer.Inner
, the extended type page forInner
is considered a direct child of the respective extended module page, not the extended type page forOuter
(which may or may not exist). This makes sense as there is no hierarchy among these extensions. One does not writeextension Outer { extension Inner { /* ... */ } }
, butextension Outer.Inner { /* ... */ }
. This has the consequence, that SwiftDocC removes theOuter
element from the path, as there is no corresponding page for it.MyModule/ExternalModule/Inner
becomes the path for the extended type page for the nested typeExternalModule.Outer.Inner
. This must be considered when detecting collisions.The respective changes can be found here:
DocumentationCacheBasedLinkResolver
L351-401PathHierarchy
L169-177Absolute Paths and Relative Path Shadowing
This problem is not specific to the extended type contents added by this PR, but they make it become more apparent.
A relative path can be a valid part of multiple different absolute paths. Consider the following example:
Now, how could
foo()
link to the outerA
? It could do so by usingMODULE_NAME/A
. However, ifMODULE_NAME
happens to also beA
, then it just can't. This might seem very unlikely, however, it is a common case once we have multiple modules and extended module pages in the same documentation archive (as explained under "Reference Resolution" in my sample project.To resolve this issue, I adapted the code to make absolute paths (starting with a slash) truly absolute and always start after the
documentation
component, i.e. the first component after the leading slash is always the module name. Previously, they were handled just like relative paths.The
PathHierarchyBasedLinkResolver
also used to fully commit on the first potential root for a relative path where the first path component matched. Thus,``A/A``
wouldn't resolve in the example above, asA.A
has no member namedA
. Now, it continues searching up the tree finding the outerA
, which has a member namedA
.The relevant changes can be found here:
DocumentationCacheBasedLinkResolver
L81-92PathHierarchy
L314-598Dependencies
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 handling of collisions resulting from extensions to nested external types
- test for absolute/relative reference resolution for ambiguous relative
references
Tests handling references work with the old
DocumentCacheBasedLinkResolver
as well as the newPathHierarchyBasedLinkResolver
.In addition to that you can use this sample project to explore edge cases manually. Build instructions can be found in the project readme. The live documentation, which is hosted here, explains the edge cases and points to the relevant symbols for each case.
Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
./bin/test
script and it succeeded