Skip to content
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

Merged

Conversation

theMomax
Copy link
Contributor

#210 (Forums Discussion)

Note This is an alternative implementation to #335, with the only notable difference being that extensions to nested external types are organized in a hierarchy.

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``. The Path/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:

/// This will be discarded.
public extension String { /* ... */ }

/// This will be the documentation for ``Swift/String``, as
/// it is the longest (in terms of numbers of lines) documentation comment
/// above an extension to `String` that is defined in this module.
public extension String { /* ... */ }

Screenshots

The list of Extended Modules in the archive overview.
image

An example for an Extended Module page.
image

An example for an Extended Structure page.
image

The bottom part of the same Extended Structure page.
image

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, and unknown.extension as well as the new declaredIn and inContextOf 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 contain swift.extension symbols, the ExtendedTypesFormatTransformation is applied to (non-main) symbol graphs, and the loader configures the GraphCollector 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 and swift.TYPE_KIND.extension symbols from the swift.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.

Note RenderNodeTranslator+Swift was removed because it was a copy of DocumentationContentRenderer+Swift where only the latter was used in production code and only the former was used in tests.

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 new PathHierarchyBasedLinkResolver.

Checklist

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

  • Added tests
  • Ran the ./bin/test script and it succeeded
  • Updated documentation if necessary

@theMomax theMomax force-pushed the extensions-to-external-types-hierarchical branch from 06f70dc to 8eef6cd Compare August 29, 2022 09:03
@maxxfrazer
Copy link

Extensions for external types is so essential - I only just realised it's not yet supported by DocC!

@d-ronnqvist d-ronnqvist self-requested a review September 6, 2022 21:27
@d-ronnqvist d-ronnqvist self-assigned this Sep 6, 2022
Copy link
Contributor

@d-ronnqvist d-ronnqvist left a 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) {
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor

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.

@theMomax theMomax mentioned this pull request Sep 7, 2022
3 tasks
@theMomax theMomax force-pushed the extensions-to-external-types-hierarchical branch from 89fd7c2 to 03af8e2 Compare September 7, 2022 13:24
/// 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)
Copy link
Contributor

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 })
}

Copy link
Contributor Author

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.

Copy link
Contributor

@d-ronnqvist d-ronnqvist left a 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 enable AutomaticCurationTests.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) {
Copy link
Contributor

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.

@d-ronnqvist
Copy link
Contributor

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.

/// Transforms the extension symbol graph file to better match the hierarchical symbol structure that DocC uses when processing and rendering documentation.
///
/// ## Discussion
///
/// Performing this transformation before the symbols are registered allows the handling of extensions to be centralized in one place
///
/// ### Extensions in symbol graph files
///
/// In the symbol graph file, each [extension declaration](extension-decl), i.e. `extension X { ... }`, has one corresponding symbol with a
/// `extension` kind. Each extension declaration symbol has one `extensionTo` relationship to the symbol that it extends.
///
/// ```swift
/// extension ExtendedSymbol { ... }
/// //  │          ▲
/// //  ╰──────────╯ extensionTo
/// ```
///
/// Each symbol that's defined in the extension declaration has a `memberOf` relationship to the extension declaration symbol.
///
/// ```swift
/// extension ExtendedSymbol {
/// //  ▲
/// //  ╰────────╮ memberOf
///     func addedSymbol() { ... }
/// }
/// ```
///
/// If the extension adds protocol conformances, the extension declaration symbol has `conformsTo` relationships for each adopted protocol.
///
/// ```swift
/// extension ExtendedSymbol: AdoptedProtocol { ... }
/// //  │                           ▲
/// //  ╰───────────────────────────╯ conformsTo
/// ```
///
/// ### Transformation
///
/// #### Extended type symbols
///
/// For each extended symbol, all extension declarations are combined into a single "extended type" symbol with the combined
/// `memberOf` and `conformsTo` relationships for those extension declarations. The extended symbol has the most visible
/// access off all of the extensions and the longest documentation comment of all the extensions.
///
/// ```swift
/// /// Long comment that                       //      The combined "ExtendedSymbol" extended type
/// /// spans two lines.                        //      symbol gets its documentation comment from
/// internal extension ExtendedSymbol { ... }   // ◀─── this extension declaration
///                                             //      ..
/// /// Short single-line comment.              //      and its "public" access control level from
/// public extension ExtendedSymbol { ... }     // ◀─── this extension declaration
/// ```
///
/// The kind of the extended type symbol include information about the extended symbol's kind:
///
///  - ``SymbolKit/SymbolGraph/Symbol/KindIdentifier/extendedStruct``
///  - ``SymbolKit/SymbolGraph/Symbol/KindIdentifier/extendedClass``
///  - ``SymbolKit/SymbolGraph/Symbol/KindIdentifier/extendedEnum``
///  - ``SymbolKit/SymbolGraph/Symbol/KindIdentifier/extendedProtocol``
///  - ``SymbolKit/SymbolGraph/Symbol/KindIdentifier/unknownExtendedType``
///
/// #### Documentation hierarchy
///
/// For the extended module, a new "extended module" symbol is created. Each top-level extended symbol has a `declaredIn` relationship to the extended module symbol:
///
/// ```
/// ┌─────────────┐             ┌──────────────────┐
/// │  Extended   │  declaredIn │ "ExtendedSymbol" │
/// │   Module    │◀────────────│   Extended Type  │
/// └─────────────┘             └──────────────────┘
/// ```
///
/// For extension declarations where the extended type is nested within another type, an "extended type" symbol is created for each symbol
/// in the hierarchy.
///
/// ```swift
/// extension Outer.Inner { ... }
/// ```
///
/// Each nested  "extended type" symbol has a `inContextOf` relationship to its "extended type" parent symbol.
///
/// ```
/// ┌─────────────┐             ┌─────────────┐              ┌─────────────┐
/// │  Extended   │  declaredIn │   "Outer"   │  inContextOf │   "Inner"   │
/// │   Module    │◀────────────│Extended Type│◀─────────────│Extended Type│
/// └─────────────┘             └─────────────┘              └─────────────┘
/// ```
///
/// [extension-decl]: https://docs.swift.org/swift-book/ReferenceManual/Declarations.html#ID378
///
/// #### Path components
///
/// The URLs for symbols declared in extensions include both the extend_ed_ and extend_ing_ module names:
///
/// ```
/// /ExtendingModule/ExtendedModule/Path/To/ExtendedSymbol/NewSymbol
/// ```
///
/// To accomplish this, all extended type symbols' path components are prefixed with the extended module.
///
/// After transforming the extensions symbol graph, the extended symbols are merged with the extending module's symbol which adds
/// the leading extending module name to the extended symbol's URLs.

@theMomax theMomax force-pushed the extensions-to-external-types-hierarchical branch from 6f7062d to 9f4bc0a Compare September 14, 2022 06:19
@theMomax
Copy link
Contributor Author

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.

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.

@theMomax
Copy link
Contributor Author

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.

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.

@theMomax theMomax force-pushed the extensions-to-external-types-hierarchical branch from 9f4bc0a to 57ffdee Compare September 14, 2022 06:58
@theMomax
Copy link
Contributor Author

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.

@theMomax theMomax force-pushed the extensions-to-external-types-hierarchical branch from 5864fe7 to deba7fb Compare September 14, 2022 16:40
@theMomax
Copy link
Contributor Author

I almost forgot about the SymbolKit dependency. Please wait with the CI until apple/swift-docc-symbolkit#39 is merged and I have updated the Package.swift to point back to apple main!

@d-ronnqvist
Copy link
Contributor

@swift-ci please test

@d-ronnqvist
Copy link
Contributor

This looks like a Linux specific failure:

/home/build-user/swift-docc/Sources/SwiftDocC/Infrastructure/Symbol Graph/SymbolGraphConcurrentDecoder.swift:198:18: error: value of type 'Self' has no member 'allowsJSON5'
            self.allowsJSON5 = old.allowsJSON5
            ~~~~ ^~~~~~~~~~~
/home/build-user/swift-docc/Sources/SwiftDocC/Infrastructure/Symbol Graph/SymbolGraphConcurrentDecoder.swift:198:36: error: value of type 'JSONDecoder' has no member 'allowsJSON5'
            self.allowsJSON5 = old.allowsJSON5
                               ~~~ ^~~~~~~~~~~
/home/build-user/swift-docc/Sources/SwiftDocC/Infrastructure/Symbol Graph/SymbolGraphConcurrentDecoder.swift:199:18: error: value of type 'Self' has no member 'assumesTopLevelDictionary'
            self.assumesTopLevelDictionary = old.assumesTopLevelDictionary
            ~~~~ ^~~~~~~~~~~~~~~~~~~~~~~~~
/home/build-user/swift-docc/Sources/SwiftDocC/Infrastructure/Symbol Graph/SymbolGraphConcurrentDecoder.swift:199:50: error: value of type 'JSONDecoder' has no member 'assumesTopLevelDictionary'
            self.assumesTopLevelDictionary = old.assumesTopLevelDictionary
                                             ~~~ ^~~~~~~~~~~~~~~~~~~~~~~~~

I can't find those properties in JSONDecoder.swift so I'm wondering if they're now available on Linux.

@theMomax theMomax force-pushed the extensions-to-external-types-hierarchical branch from deba7fb to 8a87601 Compare September 15, 2022 07:12
@theMomax
Copy link
Contributor Author

@d-ronnqvist I updated the SymbolKit dependency to point pack to apple main and squashed the commits. I removed the two problematic JSONDecoder properties entirely, as that code is part of the decoder injection logic, which we want to remove in a follow-up PR anyway, since it's only relevant to AutomaticCurationTests.testAutomaticTopics().

This is good to go from my point of view!

@theMomax theMomax force-pushed the extensions-to-external-types-hierarchical branch from 8a87601 to c7e8eb9 Compare September 16, 2022 05:08
@d-ronnqvist
Copy link
Contributor

@swift-ci please test

…e/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
@theMomax theMomax force-pushed the extensions-to-external-types-hierarchical branch from c7e8eb9 to 91f54a8 Compare September 16, 2022 21:25
@theMomax
Copy link
Contributor Author

Sorry @d-ronnqvist, the branch was already outdated again. Ethan has been quite active on main recently 😅

@d-ronnqvist
Copy link
Contributor

@swift-ci please test

@d-ronnqvist d-ronnqvist merged commit 0324af2 into apple:main Sep 16, 2022
@d-ronnqvist d-ronnqvist removed their assignment Sep 16, 2022
@edudnyk
Copy link

edudnyk commented Nov 4, 2022

@theMomax Thanks for working on this huge improvement to docc.

I'm testing your solution with swift-DEVELOPMENT-SNAPSHOT-2022-11-01-a toolchain on an Xcode project that uses SwiftUI, providing these build settings:

SWIFT_USE_INTEGRATED_DRIVER = YES
OTHER_SWIFT_FLAGS = $(inherited) -emit-extension-block-symbols

and getting these errors:

error: Symbol with identifier 's:s7KeyPathC' couldn't be found
error: Symbol with identifier 'c:objc(cs)UIColor' couldn't be found
error: Symbol with identifier 'c:objc(cs)UIView' couldn't be found
error: Symbol with identifier 'c:objc(cs)UINavigationBar' couldn't be found
error: Symbol with identifier 'c:objc(cs)UITabBar' couldn't be found

Unfortunately, I can not share the source code.

The error is emitted by docc in SymbolGraphRelationshipsBuilder.swift:213, in addInheritanceRelationship(edge:selector:in:symbolIndex:engine:) function having the edge like this:

Screenshot 2022-11-04 at 05 03 09

The symbolIndex[edge.source] is nil in this case.

@theMomax
Copy link
Contributor Author

theMomax commented Nov 4, 2022

@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.

@edudnyk
Copy link

edudnyk commented Nov 5, 2022

@theMomax done. If I can help you in any way to find the root cause, please let me know.

@theMomax
Copy link
Contributor Author

theMomax commented Nov 6, 2022

@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 inheritsFrom relationships where the source is among the five identifiers that couldn't be found from the symbol graph files. Afterwards docc should be able do build the archive without errors.

@edudnyk
Copy link

edudnyk commented Nov 6, 2022

@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:

  1. The variables defined in the extension to SomeToken which is the type alias to KeyPath<SomeAccessors, Subj>, are shown on the KeyPath type, but not on the type alias type. Maybe they can be scoped to the type alias extension?
  2. Having Lib1 depending on Lib2, and the Tests reexporting Lib1 and Lib2, and in Lib1 having the extensions defined to the types declared in Lib2, the variables defined in these extensions, are shown on the root level of the doc archive.

Screenshot 2022-11-06 at 15 01 29

@theMomax
Copy link
Contributor Author

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 Tests target it does. It contains the extension symbols for extensions Lib1 defines on Lib2, as well as the normal symbols from Lib2. However, all of these symbols are re-exported under the same name, so essentially "Tests" is extending itself. There would be a relatively easy fix here in Swift-DocC, but that would result in the doc archive for Tests containing an Extended Module Tests, which is very counterintuitive behavior (in the image the Tests target is named ExportedExtendedLib):

image

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 Tests itself). I'll work on a fix for that. Until then, the workaround is to not use the -emit-extension-block-symbols flag with @_exported import and instead compile documentation archives for each of the re-exported targets individually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SR-15982] SymbolKit crashes when encoding unknown mixins
4 participants