-
Notifications
You must be signed in to change notification settings - Fork 146
Highlight declaration differences in overloaded symbol groups #928
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
Changes from all commits
1aa9926
9e06687
afb245a
6628523
ed43eef
ef2086d
bcdd07a
67c94a3
e4e756d
b035fe1
cc722b8
d66e752
c02ed90
4d17161
2130296
5103f6d
7f071f8
1121b63
de125de
daa2a70
2be50f3
a942e9c
2a47840
4f96243
7adfbdc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ | |
import Foundation | ||
import XCTest | ||
@testable import SwiftDocC | ||
import SwiftDocCTestUtilities | ||
|
||
class DeclarationsRenderSectionTests: XCTestCase { | ||
func testDecodingTokens() throws { | ||
|
@@ -76,7 +77,7 @@ class DeclarationsRenderSectionTests: XCTestCase { | |
) | ||
} | ||
} | ||
|
||
func testDoNotEmitOtherDeclarationsIfEmpty() throws { | ||
|
||
let encoder = RenderJSONEncoder.makeEncoder(prettyPrint: true) | ||
|
@@ -151,4 +152,230 @@ class DeclarationsRenderSectionTests: XCTestCase { | |
XCTAssertEqual(declarationsSection.declarations.count, 2) | ||
XCTAssert(declarationsSection.declarations.allSatisfy({ $0.platforms == [.iOS, .macOS] })) | ||
} | ||
|
||
func testHighlightDiff() throws { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be good to add more—and more complicated—tests for this. The code works really well but it'd be good to have more comprehensive testing so that we can be confident that we don't break it in the future. This is just one test with two overloads Here are the variations that I could think of for a single parameter type. We definitely don't need to add tests for all of them but I think we should have something that's an array, something with a dictionary, something with a tuple, something with an optional, something with a closure type, something with a variadic, and something with a generic argument. public func doSomething(with: Int) {}
public func doSomething(with: Int?) {}
public func doSomething(with: [Int]?) {}
public func doSomething(with: [Int?]) {}
public func doSomething(with: (Int, Int)) {}
public func doSomething(with: (Int?, Int)) {}
public func doSomething(with: (Int, Int?)) {}
public func doSomething(with: (Int, Int)?) {}
public func doSomething(with: [Int: Int]) {}
public func doSomething(with: [Int: Int]?) {}
public func doSomething(with: [Int?: Int]) {}
public func doSomething(with: [Int: Int?]) {}
public func doSomething(with: Int...) {}
public func doSomething(with: Int?...) {}
public func doSomething(with: Set<Int>) {}
public func doSomething(with: Set<Int?>) {}
public func doSomething(with: Set<Int>?) {}
public func doSomething(with: (Int) -> ()) {}
public func doSomething(with: (Int) -> Int) {}
public func doSomething(with: (Int?) -> Int) {}
public func doSomething(with: (Int) -> Int?) {}
public func doSomething(with: ((Int) -> Int)?) {} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (by the way, the diff in this test is not that "fancy". I can come think with much more complex differences between overloads) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The existing code seems to trip up with tuples and closures, because the differencing eagerly gloms onto the first closing parenthesis it finds and treats that as a common token with the closing parenthesis for the argument list in other overloads... ![]() It also makes the whitespace trimming fall apart for where clauses, since now the argument list parenthesis is considered a different token: ![]() I can write a test with these symbols, but the highlighting here is a bit unfortunate. However, to fix it "properly" would require introducing the complete symbol information into the differencing algorithm somehow, so that the entire argument type could be considered a distinct "token" and the correct parenthesis (and comma, in case of multiple arguments) could be counted as a common token. If we decide that we want to improve this, i'd like to defer that to after this PR lands so that we can get a "good-enough" implementation landed that we can iterate on. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The highlighting is great. I don't think the few cases where it could be slightly better should hold back this PR. If anything we could add a comment in the tests for the highlights that could be slightly better as extra information for anyone who wants to iterate on this in the future. In other words: the implementation looks great and we don't need to add many new tests but I think we should add a handful (maybe one with a tuple, one with a closure, and one with an array/dictionary and then fit an optional and a generic value in one of those 3 cases). How does that sound? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've updated my tests to test the following overload groups: public func overload1(param: Int) {}
public func overload1(param: Int?) {}
public func overload1(param: [Int]) {}
public func overload1(param: [Int]?) {}
public func overload1(param: Set<Int>) {}
public func overload1(param: [Int: Int]) {}
public func overload2(p1: Int, p2: Int) {}
public func overload2(p1: (Int, Int), p2: Int) {}
public func overload2(p1: Int, p2: (Int, Int)) {}
public func overload2(p1: (Int) -> (), p2: Int) {}
public func overload2(p1: (Int) -> Int, p2: Int) {}
public func overload2(p1: (Int) -> Int?, p2: Int) {}
public func overload2(p1: ((Int) -> Int)?, p2: Int) {}
public func overload3(_ p: [Int: Int]) {}
public func overload3<T: Hashable>(_ p: [T: T]) {}
public func overload3<K: Hashable, V>(_ p: [K: V]) {} I feel like this both tests the features we want (type decorators getting highlighted, whitespace trimmed off highlighted tokens, splitting I also added a convenience wrapper in the test code so that i could more concisely test that certain spans of declarations were being highlighted as expected. I'm not 100% sure that this is completely useful, but it helped when rewriting the tests to work with six or seven overloads at a time. 😅 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After some out-of-band discussion, i've rewritten the test wrapper to render plain-text comparison strings instead of using the string fragments i originally used. |
||
enableFeatureFlag(\.isExperimentalOverloadedSymbolPresentationEnabled) | ||
|
||
let symbolGraphFile = Bundle.module.url( | ||
forResource: "FancyOverloads", | ||
withExtension: "symbols.json", | ||
subdirectory: "Test Resources" | ||
)! | ||
|
||
let tempURL = try createTempFolder(content: [ | ||
Folder(name: "unit-test.docc", content: [ | ||
InfoPlist(displayName: "FancyOverloads", identifier: "com.test.example"), | ||
CopyOfFile(original: symbolGraphFile), | ||
]) | ||
]) | ||
|
||
let (_, bundle, context) = try loadBundle(from: tempURL) | ||
|
||
// Make sure that type decorators like arrays, dictionaries, and optionals are correctly highlighted. | ||
do { | ||
// func overload1(param: Int) {} // <- overload group | ||
// func overload1(param: Int?) {} | ||
// func overload1(param: [Int]) {} | ||
// func overload1(param: [Int]?) {} | ||
// func overload1(param: Set<Int>) {} | ||
// func overload1(param: [Int: Int]) {} | ||
let reference = ResolvedTopicReference( | ||
bundleIdentifier: bundle.identifier, | ||
path: "/documentation/FancyOverloads/overload1(param:)-8nk5z", | ||
sourceLanguage: .swift | ||
) | ||
let symbol = try XCTUnwrap(context.entity(with: reference).semantic as? Symbol) | ||
var translator = RenderNodeTranslator( | ||
context: context, | ||
bundle: bundle, | ||
identifier: reference, | ||
source: nil | ||
) | ||
let renderNode = try XCTUnwrap(translator.visitSymbol(symbol) as? RenderNode) | ||
let declarationsSection = try XCTUnwrap(renderNode.primaryContentSections.compactMap({ $0 as? DeclarationsRenderSection }).first) | ||
XCTAssertEqual(declarationsSection.declarations.count, 1) | ||
let declarations = try XCTUnwrap(declarationsSection.declarations.first) | ||
|
||
XCTAssertEqual( | ||
declarationAndHighlights(for: declarations.tokens), | ||
[ | ||
"func overload1(param: Int)", | ||
" ", | ||
] | ||
) | ||
|
||
XCTAssertEqual( | ||
declarations.otherDeclarations?.declarations.flatMap({ declarationAndHighlights(for: $0.tokens) }), | ||
[ | ||
"func overload1(param: Int?)", | ||
" ~ ", | ||
|
||
"func overload1(param: Set<Int>)", | ||
" ~~~~ ~ ", | ||
|
||
"func overload1(param: [Int : Int])", | ||
" ~ ~~~~~~ ", | ||
|
||
"func overload1(param: [Int])", | ||
" ~ ~ ", | ||
|
||
"func overload1(param: [Int]?)", | ||
" ~ ~~ ", | ||
] | ||
) | ||
} | ||
|
||
// Verify the behavior of the highlighter in the face of tuples and closures, which can | ||
// confuse the differencing code with excess parentheses and commas. | ||
do { | ||
// func overload2(p1: Int, p2: Int) {} | ||
// func overload2(p1: (Int, Int), p2: Int) {} | ||
// func overload2(p1: Int, p2: (Int, Int)) {} | ||
// func overload2(p1: (Int) -> (), p2: Int) {} | ||
// func overload2(p1: (Int) -> Int, p2: Int) {} | ||
// func overload2(p1: (Int) -> Int?, p2: Int) {} | ||
// func overload2(p1: ((Int) -> Int)?, p2: Int) {} // <- overload group | ||
let reference = ResolvedTopicReference( | ||
bundleIdentifier: bundle.identifier, | ||
path: "/documentation/FancyOverloads/overload2(p1:p2:)-4p1sq", | ||
sourceLanguage: .swift | ||
) | ||
let symbol = try XCTUnwrap(context.entity(with: reference).semantic as? Symbol) | ||
var translator = RenderNodeTranslator( | ||
context: context, | ||
bundle: bundle, | ||
identifier: reference, | ||
source: nil | ||
) | ||
let renderNode = try XCTUnwrap(translator.visitSymbol(symbol) as? RenderNode) | ||
let declarationsSection = try XCTUnwrap(renderNode.primaryContentSections.compactMap({ $0 as? DeclarationsRenderSection }).first) | ||
XCTAssertEqual(declarationsSection.declarations.count, 1) | ||
let declarations = try XCTUnwrap(declarationsSection.declarations.first) | ||
|
||
XCTAssertEqual( | ||
declarationAndHighlights(for: declarations.tokens), | ||
[ | ||
"func overload2(p1: ((Int) -> Int)?, p2: Int)", | ||
" ~~ ~~~~~~~~~~ " | ||
] | ||
) | ||
|
||
XCTAssertEqual( | ||
declarations.otherDeclarations?.declarations.flatMap({ declarationAndHighlights(for: $0.tokens) }), | ||
[ | ||
"func overload2(p1: (Int) -> (), p2: Int)", | ||
" ~ ~~~~~~~ ", | ||
|
||
"func overload2(p1: (Int) -> Int, p2: Int)", | ||
" ~ ~~~~~~~~ ", | ||
|
||
"func overload2(p1: (Int) -> Int?, p2: Int)", | ||
" ~ ~~~~~~~~~ ", | ||
|
||
// FIXME: adjust the token processing so that the comma inside the tuple isn't treated as common? | ||
// (it breaks the declaration pretty-printer in Swift-DocC-Render and causes it to skip pretty-printing) | ||
"func overload2(p1: (Int, Int), p2: Int)", | ||
" ~ ~~~~~ ", | ||
|
||
// FIXME: adjust the token processing so that the common parenthesis is always the final one | ||
"func overload2(p1: Int, p2: (Int, Int))", | ||
" ~ ~~~~~ ~", | ||
|
||
"func overload2(p1: Int, p2: Int)", | ||
" ", | ||
] | ||
) | ||
} | ||
|
||
// Verify that the presence of type parameters doesn't cause the opening parenthesis of an | ||
// argument list to also be highlighted, since it is combined into the same token as the | ||
// closing angle bracket in the symbol graph. Also ensure that the leading space of the | ||
// rendered where clause is not highlighted. | ||
do { | ||
// func overload3(_ p: [Int: Int]) {} // <- overload group | ||
// func overload3<T: Hashable>(_ p: [T: T]) {} | ||
// func overload3<K: Hashable, V>(_ p: [K: V]) {} | ||
let reference = ResolvedTopicReference( | ||
bundleIdentifier: bundle.identifier, | ||
path: "/documentation/FancyOverloads/overload3(_:)-xql2", | ||
sourceLanguage: .swift | ||
) | ||
let symbol = try XCTUnwrap(context.entity(with: reference).semantic as? Symbol) | ||
var translator = RenderNodeTranslator( | ||
context: context, | ||
bundle: bundle, | ||
identifier: reference, | ||
source: nil | ||
) | ||
let renderNode = try XCTUnwrap(translator.visitSymbol(symbol) as? RenderNode) | ||
let declarationsSection = try XCTUnwrap(renderNode.primaryContentSections.compactMap({ $0 as? DeclarationsRenderSection }).first) | ||
XCTAssertEqual(declarationsSection.declarations.count, 1) | ||
let declarations = try XCTUnwrap(declarationsSection.declarations.first) | ||
|
||
XCTAssertEqual( | ||
declarationAndHighlights(for: declarations.tokens), | ||
[ | ||
"func overload3(_ p: [Int : Int])", | ||
" ~~~ ~~~ ", | ||
] | ||
) | ||
|
||
XCTAssertEqual( | ||
declarations.otherDeclarations?.declarations.flatMap({ declarationAndHighlights(for: $0.tokens) }), | ||
[ | ||
"func overload3<K, V>(_ p: [K : V]) where K : Hashable", | ||
" ~~~~~~ ~ ~ ~~~~~~~~~~~~~~~~~~", | ||
|
||
"func overload3<T>(_ p: [T : T]) where T : Hashable", | ||
" ~~~ ~ ~ ~~~~~~~~~~~~~~~~~~", | ||
] | ||
) | ||
} | ||
} | ||
|
||
func testDontHighlightWhenOverloadsAreDisabled() throws { | ||
let symbolGraphFile = Bundle.module.url( | ||
forResource: "FancyOverloads", | ||
withExtension: "symbols.json", | ||
subdirectory: "Test Resources" | ||
)! | ||
|
||
let tempURL = try createTempFolder(content: [ | ||
Folder(name: "unit-test.docc", content: [ | ||
InfoPlist(displayName: "FancyOverloads", identifier: "com.test.example"), | ||
CopyOfFile(original: symbolGraphFile), | ||
]) | ||
]) | ||
|
||
let (_, bundle, context) = try loadBundle(from: tempURL) | ||
|
||
for hash in ["7eht8", "8p1lo", "858ja"] { | ||
let reference = ResolvedTopicReference( | ||
bundleIdentifier: bundle.identifier, | ||
path: "/documentation/FancyOverloads/overload3(_:)-\(hash)", | ||
sourceLanguage: .swift | ||
) | ||
let symbol = try XCTUnwrap(context.entity(with: reference).semantic as? Symbol) | ||
var translator = RenderNodeTranslator( | ||
context: context, | ||
bundle: bundle, | ||
identifier: reference, | ||
source: nil | ||
) | ||
let renderNode = try XCTUnwrap(translator.visitSymbol(symbol) as? RenderNode) | ||
let declarationsSection = try XCTUnwrap(renderNode.primaryContentSections.compactMap({ $0 as? DeclarationsRenderSection }).first) | ||
XCTAssertEqual(declarationsSection.declarations.count, 1) | ||
let declarations = try XCTUnwrap(declarationsSection.declarations.first) | ||
|
||
XCTAssert(declarations.tokens.allSatisfy({ $0.highlight == nil })) | ||
} | ||
} | ||
} | ||
|
||
/// Render a list of declaration tokens as a plain-text decoration and as a plain-text rendering of which characters are highlighted. | ||
func declarationAndHighlights(for tokens: [DeclarationRenderSection.Token]) -> [String] { | ||
[ | ||
tokens.map({ $0.text }).joined(), | ||
tokens.map({ String(repeating: $0.highlight == .changed ? "~" : " ", count: $0.text.count) }).joined() | ||
] | ||
} |
Uh oh!
There was an error while loading. Please reload this page.