Skip to content

[DocC Live Preview] Support parameter and return type disambiguations #2216

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

Merged

Conversation

matthewbastien
Copy link
Member

Adds support for parameter and return type disambiguations to textDocument/doccDocumentation requests. See the docc documentation for more info.

The information for parameter and return types of functions does not exist in the index. So, we have to fetch the symbol graph from sourcekitd in order to get all of the info we need. This adds some overhead, but not enough to cause any significant performance impacts in my testing.

@matthewbastien
Copy link
Member Author

Depends on PR from swift-docc: swiftlang/swift-docc#1253

@matthewbastien
Copy link
Member Author

swiftlang/swift-docc#1253

@swift-ci please test

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice clean-up with added functionality 🤩

else {
throw ResponseError.internalError("Unable to find Swift language service for \(location.documentUri)")
}
return try await languageService.withSnapshotFromDiskOpenedInSourcekitd(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, we run multiple cursor info requests for a single docc request, right? Wit this implementation we need to open a new sourcekitd document for every request, which means that we also need to build as many ASTs, which is really the expensive part for the cursor info request. Would it be possible to only open each document once?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct. I can cache the snapshots instead of throwing them away, but that will require a rework of withSnapshotFromDiskOpenedInSourcekitd(uri:fallbackSettingsAfterTimeout:body:). I'll see how feasible that is.

FWIW I don't expect there to be too many lookups, but it'll only get worse with the size of the project and the number of colliding symbol names...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m happy to take this change as-is as well and address it in a follow-up PR (or file an issue if you don’t think that you’ll get to it soon).

Comment on lines +42 to +49
for occurrence in topLevelSymbolOccurrences {
let info = try await doccSymbolInformation(ofUSR: occurrence.symbol.usr, fetchSymbolGraph: fetchSymbolGraph)
if let info, info.matches(symbolLink) {
result.append(occurrence)
}
// If we have exactly one component left, check to see if it's the module name
if components.count == 1 {
let lastComponent = components.removeLast()
guard lastComponent.name == topLevelSymbolOccurrence.location.moduleName else {
return false
}
}
// Ensure that this is deterministic by sorting the results
result.sort()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be just a filter?

Suggested change
for occurrence in topLevelSymbolOccurrences {
let info = try await doccSymbolInformation(ofUSR: occurrence.symbol.usr, fetchSymbolGraph: fetchSymbolGraph)
if let info, info.matches(symbolLink) {
result.append(occurrence)
}
// If we have exactly one component left, check to see if it's the module name
if components.count == 1 {
let lastComponent = components.removeLast()
guard lastComponent.name == topLevelSymbolOccurrence.location.moduleName else {
return false
}
}
// Ensure that this is deterministic by sorting the results
result.sort()
let result = try await topLevelSymbolOccurrences.asyncFilter { occurrence in
let info = try await doccSymbolInformation(ofUSR: occurrence.symbol.usr, fetchSymbolGraph: fetchSymbolGraph)
return info?.matches(symbolLink) ?? false
}.sorted()

Copy link
Member Author

@matthewbastien matthewbastien Jul 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to use asyncFilter(_:), but got the following error when I tried it:

Capture of 'self' with non-Sendable type 'CheckedIndex' in a '@Sendable' closure

and it doesn't look like we can easily make CheckedIndex conform to Sendable.

Copy link
Member

@ahoppen ahoppen Jul 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like none of the functions in Sequence+AsyncMap.swift need @_inheritActorContext and @Sendable. Removing those also allows you to use asyncFilter here. Not sure why I added those annotations. But we can also make that a follow-up change.

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks. Happy to take care of the two open conversations in follow-up PRs if you prefer that.

@ahoppen
Copy link
Member

ahoppen commented Jul 25, 2025

@swift-ci Please test

@bnbarham
Copy link
Contributor

swiftlang/swift-docc#1253

@swift-ci please test

@matthewbastien
Copy link
Member Author

Thanks for the review! I'll submit a follow-up PR to address the two remaining comments.

@matthewbastien matthewbastien merged commit cc50129 into swiftlang:main Jul 30, 2025
3 checks passed
@matthewbastien matthewbastien deleted the docc-func-disambiguation branch July 31, 2025 15:33
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.

3 participants