-
Notifications
You must be signed in to change notification settings - Fork 126
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
Add a new generated page to group symbol overloads together #825
Add a new generated page to group symbol overloads together #825
Conversation
Gets link resolution working for new overload page rdar://116550528
@swift-ci Please test |
guard let indexOfHash = firstOverloadReference.path.lastIndex(of: "-") else { return } | ||
let overloadGroupPath = String(firstOverloadReference.path[..<indexOfHash]) | ||
let overloadGroupReference = ResolvedTopicReference( | ||
bundleIdentifier: firstOverloadTopicNode.reference.bundleIdentifier, | ||
path: overloadGroupPath, | ||
sourceLanguages: firstOverloadTopicNode.reference.sourceLanguages | ||
) |
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 doesn't guarantee that the reference is unambiguous. In general, directly initializing a ResolvedTopicReference
from a manually manipulated path is not recommended.
The place that knows how to create minimal unambiguous references is the link resolver (PathHierarchy
). If the overload page needs its own unambiguous reference then the path hierarchy needs to be made aware of that symbol before DocumentationContext.registerSymbols(from:symbolGraphLoader:documentationExtensions:)
is called.
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.
Are you suggesting that we move the overload grouping logic into the PathHierarchy
initializer? That seems like the most likely place to perform that aggregation if the hierarchy is expected to be immutable once all the symbols are registered.
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.
Possibly. I could happen after initialization as long as it happens before DocumentationContext.registerSymbols(from:symbolGraphLoader:documentationExtensions:)
. The logic that identifies the overloads is already in the path hierarchy so making it responsible for creating the node seems fine.
The hierarchy isn't really immutable but symbols shouldn't be added after initialization. Non-symbols don't get their references from the path hierarchy which is the cause of #593.
Also, the path hierarchy shouldn't be manipulating symbols to update the declaration token. That is not its responsibility and you wouldn't retrieve the symbol from the hierarchy anyway, just the unique identifier which maps to a ResolvedTopicReference
.
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.
There is already a mechanism for disfavoring certain nodes in case of a link collision. Does that not work for overloads?
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 might have been a consequence of adding the overload nodes to the path hierarchy after they were registered in the documentation context. Let me see if i can rework this to manipulate the nodes before then and see if this is still necessary.
let parent = lookup[parent]! | ||
|
||
let newReference = ResolvedIdentifier() | ||
let newNode = Node(symbol: symbol, name: name) |
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 too late to add symbols to the path hierarchy after it has been initialized. The added symbol won't be able to affect the other symbols.
var symbols = [SymbolGraph.Symbol]() | ||
// func myFunc(param: Int, other: Int) -> Int | ||
symbols.append(makeSymbol( | ||
name: "myFunc(param:other:)", | ||
identifier: "myFunc-1", | ||
kind: .method, | ||
subHeading: [ | ||
.init(kind: .keyword, spelling: "func", preciseIdentifier: nil), | ||
.init(kind: .text, spelling: " ", preciseIdentifier: nil), | ||
.init(kind: .identifier, spelling: "myFunc", preciseIdentifier: nil), | ||
.init(kind: .text, spelling: "(", preciseIdentifier: nil), | ||
.init(kind: .externalParameter, spelling: "param", preciseIdentifier: nil), | ||
.init(kind: .text, spelling: ": ", preciseIdentifier: nil), | ||
.init(kind: .typeIdentifier, spelling: "Int", preciseIdentifier: "Int"), | ||
.init(kind: .text, spelling: ", ", preciseIdentifier: nil), | ||
.init(kind: .externalParameter, spelling: "other", preciseIdentifier: nil), | ||
.init(kind: .text, spelling: ": ", preciseIdentifier: nil), | ||
.init(kind: .typeIdentifier, spelling: "Int", preciseIdentifier: "Int"), | ||
.init(kind: .text, spelling: ") -> ", preciseIdentifier: nil), | ||
.init(kind: .typeIdentifier, spelling: "Int", preciseIdentifier: "Int"), | ||
] | ||
)) | ||
// func myFunc(param: String, other: String) -> String | ||
symbols.append(makeSymbol( | ||
name: "myFunc(param:other:)", | ||
identifier: "myFunc-2", | ||
kind: .method, | ||
subHeading: [ | ||
.init(kind: .keyword, spelling: "func", preciseIdentifier: nil), | ||
.init(kind: .text, spelling: " ", preciseIdentifier: nil), | ||
.init(kind: .identifier, spelling: "myFunc", preciseIdentifier: nil), | ||
.init(kind: .text, spelling: "(", preciseIdentifier: nil), | ||
.init(kind: .externalParameter, spelling: "param", preciseIdentifier: nil), | ||
.init(kind: .text, spelling: ": ", preciseIdentifier: nil), | ||
.init(kind: .typeIdentifier, spelling: "String", preciseIdentifier: "String"), | ||
.init(kind: .text, spelling: ", ", preciseIdentifier: nil), | ||
.init(kind: .externalParameter, spelling: "other", preciseIdentifier: nil), | ||
.init(kind: .text, spelling: ": ", preciseIdentifier: nil), | ||
.init(kind: .typeIdentifier, spelling: "String", preciseIdentifier: "String"), | ||
.init(kind: .text, spelling: ") -> ", preciseIdentifier: nil), | ||
.init(kind: .typeIdentifier, spelling: "String", preciseIdentifier: "String"), | ||
] | ||
)) |
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 make symbols
mutable here instead of directly initializing the array?
var symbols = [SymbolGraph.Symbol]() | |
// func myFunc(param: Int, other: Int) -> Int | |
symbols.append(makeSymbol( | |
name: "myFunc(param:other:)", | |
identifier: "myFunc-1", | |
kind: .method, | |
subHeading: [ | |
.init(kind: .keyword, spelling: "func", preciseIdentifier: nil), | |
.init(kind: .text, spelling: " ", preciseIdentifier: nil), | |
.init(kind: .identifier, spelling: "myFunc", preciseIdentifier: nil), | |
.init(kind: .text, spelling: "(", preciseIdentifier: nil), | |
.init(kind: .externalParameter, spelling: "param", preciseIdentifier: nil), | |
.init(kind: .text, spelling: ": ", preciseIdentifier: nil), | |
.init(kind: .typeIdentifier, spelling: "Int", preciseIdentifier: "Int"), | |
.init(kind: .text, spelling: ", ", preciseIdentifier: nil), | |
.init(kind: .externalParameter, spelling: "other", preciseIdentifier: nil), | |
.init(kind: .text, spelling: ": ", preciseIdentifier: nil), | |
.init(kind: .typeIdentifier, spelling: "Int", preciseIdentifier: "Int"), | |
.init(kind: .text, spelling: ") -> ", preciseIdentifier: nil), | |
.init(kind: .typeIdentifier, spelling: "Int", preciseIdentifier: "Int"), | |
] | |
)) | |
// func myFunc(param: String, other: String) -> String | |
symbols.append(makeSymbol( | |
name: "myFunc(param:other:)", | |
identifier: "myFunc-2", | |
kind: .method, | |
subHeading: [ | |
.init(kind: .keyword, spelling: "func", preciseIdentifier: nil), | |
.init(kind: .text, spelling: " ", preciseIdentifier: nil), | |
.init(kind: .identifier, spelling: "myFunc", preciseIdentifier: nil), | |
.init(kind: .text, spelling: "(", preciseIdentifier: nil), | |
.init(kind: .externalParameter, spelling: "param", preciseIdentifier: nil), | |
.init(kind: .text, spelling: ": ", preciseIdentifier: nil), | |
.init(kind: .typeIdentifier, spelling: "String", preciseIdentifier: "String"), | |
.init(kind: .text, spelling: ", ", preciseIdentifier: nil), | |
.init(kind: .externalParameter, spelling: "other", preciseIdentifier: nil), | |
.init(kind: .text, spelling: ": ", preciseIdentifier: nil), | |
.init(kind: .typeIdentifier, spelling: "String", preciseIdentifier: "String"), | |
.init(kind: .text, spelling: ") -> ", preciseIdentifier: nil), | |
.init(kind: .typeIdentifier, spelling: "String", preciseIdentifier: "String"), | |
] | |
)) | |
let symbols = [ | |
// func myFunc(param: Int, other: Int) -> Int | |
makeSymbol( | |
name: "myFunc(param:other:)", | |
identifier: "myFunc-1", | |
kind: .method, | |
subHeading: [ | |
.init(kind: .keyword, spelling: "func", preciseIdentifier: nil), | |
.init(kind: .text, spelling: " ", preciseIdentifier: nil), | |
.init(kind: .identifier, spelling: "myFunc", preciseIdentifier: nil), | |
.init(kind: .text, spelling: "(", preciseIdentifier: nil), | |
.init(kind: .externalParameter, spelling: "param", preciseIdentifier: nil), | |
.init(kind: .text, spelling: ": ", preciseIdentifier: nil), | |
.init(kind: .typeIdentifier, spelling: "Int", preciseIdentifier: "Int"), | |
.init(kind: .text, spelling: ", ", preciseIdentifier: nil), | |
.init(kind: .externalParameter, spelling: "other", preciseIdentifier: nil), | |
.init(kind: .text, spelling: ": ", preciseIdentifier: nil), | |
.init(kind: .typeIdentifier, spelling: "Int", preciseIdentifier: "Int"), | |
.init(kind: .text, spelling: ") -> ", preciseIdentifier: nil), | |
.init(kind: .typeIdentifier, spelling: "Int", preciseIdentifier: "Int"), | |
] | |
), | |
// func myFunc(param: String, other: String) -> String | |
makeSymbol( | |
name: "myFunc(param:other:)", | |
identifier: "myFunc-2", | |
kind: .method, | |
subHeading: [ | |
.init(kind: .keyword, spelling: "func", preciseIdentifier: nil), | |
.init(kind: .text, spelling: " ", preciseIdentifier: nil), | |
.init(kind: .identifier, spelling: "myFunc", preciseIdentifier: nil), | |
.init(kind: .text, spelling: "(", preciseIdentifier: nil), | |
.init(kind: .externalParameter, spelling: "param", preciseIdentifier: nil), | |
.init(kind: .text, spelling: ": ", preciseIdentifier: nil), | |
.init(kind: .typeIdentifier, spelling: "String", preciseIdentifier: "String"), | |
.init(kind: .text, spelling: ", ", preciseIdentifier: nil), | |
.init(kind: .externalParameter, spelling: "other", preciseIdentifier: nil), | |
.init(kind: .text, spelling: ": ", preciseIdentifier: nil), | |
.init(kind: .typeIdentifier, spelling: "String", preciseIdentifier: "String"), | |
.init(kind: .text, spelling: ") -> ", preciseIdentifier: nil), | |
.init(kind: .typeIdentifier, spelling: "String", preciseIdentifier: "String"), | |
] | |
), | |
] |
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.
Purely because i wanted to save the level of indentation. 😅 I can change it if this is more desired.
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.
For me, it's mostly about what a mutable variable signals. I kept looking later in the code thinking "surely there is another value getting added somewhere later" and not finding any.
/// However, doing this by itself will return `func myFunc(param: ) -> ` as the result. After | ||
/// stripping out any `typeIdentifier` fragments, this method then cleans up any visual | ||
/// oddities like the space in the parameters list and the trailing return-type arrow. | ||
func simplifyForOverloads() -> Self { |
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.
Please add dedicated tests for this function with more complex example data. This current implementation doesn't work well for array types, optional types, inout parameters, borrowed/consuming parameters, tuples, closures, variadics, parameter packs, and more.
For example, I defined this function
public func someFunction<Result: Equatable>(
first: borrowing [Int?],
second: inout Int,
third: (Int, Int),
fourth: @autoclosure () throws -> Void,
fifth: Int...,
sixth: some Sequence<Int>,
seventh: any Hashable
) -> Result? {
fatalError()
}
which gets this sub heading in the symbol graph file (the full declaration has even more tokens)
func someFunction<Result>(first: borrowing [Int?], second: inout Int, third: (Int, Int), fourth: () throws -> Void, fifth: Int..., sixth: some Sequence<Int>, seventh: any Hashable) -> Result?
Calling simplifyForOverloads()
on that sub heading results in
func someFunction<Result>(first: borrowing [?], second: inout , third: (,), fourth: () throws, fifth: ..., sixth: some <>, seventh: any)?
which still contains a lot of tokens which I would expect to be filtered out.
Similarly, a function definition like
public func somePackedFunction<each T: Collection>(
_ item: repeat each T
) -> (repeat (each T).Element?) {
fatalError()
}
has this sub heading
func somePackedFunction<each T>(repeat each T) -> (repeat (each T).Element?)
and calling simplifyForOverloads()
on it results in
func somePackedFunction<each T>(repeat each) -> (repeat (each).?)
which also contains a number of tokens that I would expect to be filtered out.
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.
Maybe it would be worth building on top / adding tokens to the page title as opposed to stripping out unwanted fragments from the subheading/navigator, since it seems like you want a slightly longer form of the title? I don't know how often new tokens are added (maybe rarely), but that could reduce the number of unexpected tokens that might slip through here if new tokens are added to subheadings in the future.
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.
@d-ronnqvist I think there are alternate filterings and techniques i can try to deal with the situations you've mentioned. Thanks for the examples; i'll work up some tests for these.
@mayaepps The issue with starting from the title is that the title is plain text whereas the navigator and subheading are based on declaration fragments. This is important since it affects how Swift-DocC-Render will color a curated symbol link, for example with this test page i made with the current implementation:
I believe it's worth keeping as much rich information as we can, to preserve this behavior.
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.
You could possible build up the simplified subheading based on the parameters names (and external parameter names) in the type signature.
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.
Right, i feel like we could get away with filtering out everything but keyword
, identifier
, externalParam
, and text
fragments, and then stripping out everything that comes after the return-type arrow.
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 think it's more complicated than that. Parenthesis and braces are text
fragments, so removing identifiers but leaving text results in (,)
, []
and ?
for tuples, arrays, and optionals. Likewise, inout
, borrowing
, some
, and each
are keywords.
|
||
fileprivate typealias Fragment = SymbolKit.SymbolGraph.Symbol.DeclarationFragments.Fragment | ||
|
||
extension [Fragment] { |
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.
Out of curiosity, what made you decide to implement this logic here as opposed to in SymbolKit (such as adding a new property to DeclarationFragments)?
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.
Not wanting to split the PR across repos, mainly. 😅
This logic is pretty specific to the kind of display that DocC is wanting to do, and isn't representative of what data is coming directly from the symbol graph JSON file. I feel like SymbolKit is more of a data-representation library, and adding more custom-logic pieces like this somewhat muddles that.
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.
Agreed. This feels like it belongs in DocC.
/// However, doing this by itself will return `func myFunc(param: ) -> ` as the result. After | ||
/// stripping out any `typeIdentifier` fragments, this method then cleans up any visual | ||
/// oddities like the space in the parameters list and the trailing return-type arrow. | ||
func simplifyForOverloads() -> Self { |
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.
Maybe it would be worth building on top / adding tokens to the page title as opposed to stripping out unwanted fragments from the subheading/navigator, since it seems like you want a slightly longer form of the title? I don't know how often new tokens are added (maybe rarely), but that could reduce the number of unexpected tokens that might slip through here if new tokens are added to subheadings in the future.
@@ -421,6 +421,8 @@ extension PathHierarchy.DisambiguationContainer { | |||
return subtree[hash] | |||
} else if subtree.count == 1 { | |||
return subtree.values.first | |||
} else if let overloadGroupNode = subtree["overloadGroup"] { |
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 may become irrelevant depending on the above comment) but I wonder if it would be worth making "overloadGroup" a constant somewhere so it's less of a magic value?
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 was assuming that this specific find logic would not be necessary anymore.
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.
Right, i think by implementing it how David suggests, this part of the diff can be removed.
Based on discussions both here and offline, i'm closing this PR in favor of #846. |
Bug/issue #, if applicable: rdar://116550528
Summary
In an effort to simplify symbol overloads, this PR creates a new generated "overload group" page for each group of overloads it encounters. This page has a simplified page subheading based on the first overload's information, and links to the first overload's declaration. This page's path is the same as the first overload's path, without a disambiguating hash.
Dependencies
None
Testing
Steps:
main
branch Swift-DocC-Render built in$DOCC_HTML_DIR
, runswift run docc preview --enable-experimental-overloaded-symbol-presentation Tests/SwiftDocCTests/Test\ Bundles/OverloadedSymbols.docc
OverloadedEnum
symbol.firstTestMemberName(_:)
page is added to the automatic curation alongside the specific overloads.Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
./bin/test
script and it succeeded