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.

Sign up for GitHub

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

Highlight declaration differences in overloaded symbol groups #928

Merged
merged 25 commits into from
Jun 27, 2024
Merged

Highlight declaration differences in overloaded symbol groups #928

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
1aa9926
wip: highlight non-shared overload declaration tokens
QuietMisdreavus May 13, 2024
9e06687
pre-process declarations before/after running LCS to improve the result
QuietMisdreavus May 21, 2024
afb245a
trim leading or trailing whitespace from highlighted spans
QuietMisdreavus May 21, 2024
6628523
refactor and add comments
QuietMisdreavus May 23, 2024
ed43eef
render highlighted tokens as recursive 'highlightDiff' tokens
QuietMisdreavus May 28, 2024
ef2086d
use the stdlib implmentation of LCS
QuietMisdreavus May 28, 2024
bcdd07a
add tokens property to RenderJSONDiffable implementation
QuietMisdreavus May 29, 2024
67c94a3
add test for overload diff
QuietMisdreavus May 29, 2024
e4e756d
revert recursive tokens implementation
QuietMisdreavus Jun 3, 2024
b035fe1
review: represent highlights as an enum property
QuietMisdreavus Jun 3, 2024
cc722b8
refactor and reformat the token processing code
QuietMisdreavus Jun 4, 2024
d66e752
add test to ensure that highlights don't happen when overloads are di…
QuietMisdreavus Jun 5, 2024
c02ed90
review: keep partitions as substrings
QuietMisdreavus Jun 5, 2024
4d17161
review: make the overload declaration check a precondition
QuietMisdreavus Jun 5, 2024
2130296
review: remove cloning initializers in favor of mutating fields directly
QuietMisdreavus Jun 7, 2024
5103f6d
review: use OverloadDeclaration explicitly
QuietMisdreavus Jun 7, 2024
7f071f8
review: add isHighlighted convenience property
QuietMisdreavus Jun 7, 2024
1121b63
review: extract the overloadDeclarations assignment into a function
QuietMisdreavus Jun 11, 2024
de125de
review: precalculate platform names and languages for alternate decls
QuietMisdreavus Jun 11, 2024
daa2a70
review: allow nil platforms in alternate declarations
QuietMisdreavus Jun 11, 2024
2be50f3
review: refactor postProcessTokens loop to remove an optional
QuietMisdreavus Jun 11, 2024
a942e9c
refactor: write comparison declarations in a more concise way
QuietMisdreavus Jun 20, 2024
2a47840
review: add a wider variety of tests for overload diff highlighting
QuietMisdreavus Jun 20, 2024
4f96243
review: simplify the testing harness to use a plain-text comparator
QuietMisdreavus Jun 25, 2024
7adfbdc
Merge branch 'main' into diff-overloads
QuietMisdreavus Jun 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
382 changes: 349 additions & 33 deletions ...ces/SwiftDocC/Model/Rendering/RenderSectionTranslator/DeclarationsSectionTranslator.swift
View file
Open in desktop

Large diffs are not rendered by default.

11 changes: 10 additions & 1 deletion Sources/SwiftDocC/Model/Rendering/Symbol/DeclarationRenderSection+SymbolGraph.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,19 @@ extension DeclarationRenderSection.Token {
/// - Parameters:
/// - fragment: The symbol-graph declaration fragment to render.
/// - identifier: An optional reference to a symbol.
init(fragment: SymbolKit.SymbolGraph.Symbol.DeclarationFragments.Fragment, identifier: String?) {
init(
fragment: SymbolKit.SymbolGraph.Symbol.DeclarationFragments.Fragment,
identifier: String?,
highlight: Bool = false
) {
self.text = fragment.spelling
self.kind = Kind(rawValue: fragment.kind.rawValue) ?? .text
self.identifier = identifier
self.preciseIdentifier = fragment.preciseIdentifier
if highlight {
self.highlight = .changed
} else {
self.highlight = nil
}
}
}
26 changes: 22 additions & 4 deletions Sources/SwiftDocC/Model/Rendering/Symbol/DeclarationsRenderSection.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ public struct DeclarationRenderSection: Codable, Equatable {
/// For example, `123` is represented as a single token of kind "number".
public struct Token: Codable, Hashable, Equatable {
/// The token text content.
public let text: String
public var text: String
/// The token programming kind.
public let kind: Kind

Expand Down Expand Up @@ -114,24 +114,40 @@ public struct DeclarationRenderSection: Codable, Equatable {

/// If the token is a known symbol, its precise identifier as vended in the symbol graph.
public let preciseIdentifier: String?


/// The kind of highlight the token should be rendered with.
public var highlight: Highlight?

/// The kinds of highlights that can be applied to a token.
public enum Highlight: String, Codable, RawRepresentable {
/// A highlight representing generalized change, not specifically added or removed.
case changed
QuietMisdreavus marked this conversation as resolved.
Show resolved Hide resolved
}

/// Creates a new declaration token with optional identifier and precise identifier.
/// - Parameters:
/// - text: The text content of the token.
/// - kind: The kind of the token.
/// - identifier: If the token refers to a known symbol, its identifier.
/// - preciseIdentifier: If the refers to a symbol, its precise identifier.
public init(text: String, kind: Kind, identifier: String? = nil, preciseIdentifier: String? = nil) {
public init(
text: String,
kind: Kind,
identifier: String? = nil,
preciseIdentifier: String? = nil,
highlight: Highlight? = nil
) {
self.text = text
self.kind = kind
self.identifier = identifier
self.preciseIdentifier = preciseIdentifier
self.highlight = highlight
}

// MARK: - Codable

private enum CodingKeys: CodingKey {
case text, kind, identifier, preciseIdentifier, otherDeclarations
case text, kind, identifier, preciseIdentifier, highlight, otherDeclarations
}

public func encode(to encoder: Encoder) throws {
Expand All @@ -141,6 +157,7 @@ public struct DeclarationRenderSection: Codable, Equatable {
try container.encode(kind, forKey: .kind)
try container.encodeIfPresent(identifier, forKey: .identifier)
try container.encodeIfPresent(preciseIdentifier, forKey: .preciseIdentifier)
try container.encodeIfPresent(highlight, forKey: .highlight)
}

public init(from decoder: Decoder) throws {
Expand All @@ -150,6 +167,7 @@ public struct DeclarationRenderSection: Codable, Equatable {
kind = try container.decode(Kind.self, forKey: .kind)
preciseIdentifier = try container.decodeIfPresent(String.self, forKey: .preciseIdentifier)
identifier = try container.decodeIfPresent(String.self, forKey: .identifier)
highlight = try container.decodeIfPresent(Highlight.self, forKey: .highlight)

if let reference = identifier {
decoder.registerReferences([reference])
Expand Down
6 changes: 6 additions & 0 deletions Sources/SwiftDocC/SwiftDocC.docc/Resources/RenderNode.spec.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2616,6 +2616,12 @@
},
"preciseIdentifier": {
"type": "string"
},
"highlight": {
"type": "string",
"enum": [
"changed"
]
}
}
},
Expand Down
229 changes: 228 additions & 1 deletion Tests/SwiftDocCTests/Rendering/DeclarationsRenderSectionTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import Foundation
import XCTest
@testable import SwiftDocC
import SwiftDocCTestUtilities

class DeclarationsRenderSectionTests: XCTestCase {
func testDecodingTokens() throws {
Expand Down Expand Up @@ -76,7 +77,7 @@ class DeclarationsRenderSectionTests: XCTestCase {
)
}
}

func testDoNotEmitOtherDeclarationsIfEmpty() throws {

let encoder = RenderJSONEncoder.makeEncoder(prettyPrint: true)
Expand Down Expand Up @@ -151,4 +152,230 @@ class DeclarationsRenderSectionTests: XCTestCase {
XCTAssertEqual(declarationsSection.declarations.count, 2)
XCTAssert(declarationsSection.declarations.allSatisfy({ $0.platforms == [.iOS, .macOS] }))
}

func testHighlightDiff() throws {
Copy link
Contributor

Choose a reason for hiding this comment

The 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 func myFunc(param: Int) and func myFunc<S>(param: S) where S : StringProtocol but there's a lot more possible syntax that would be worth testing, especially for text tokens since there's logic to split those for better diffident.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

It also makes the whitespace trimming fall apart for where clauses, since now the argument list parenthesis is considered a different token:

image

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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 >( tokens) and also adds the known edge case issues (parentheses and commas throwing off the diff).

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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()
]
}
Loading