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

Support disambiguating links with type signature information #643

Merged

Conversation

d-ronnqvist
Copy link
Contributor

@d-ronnqvist d-ronnqvist commented Jun 23, 2023

Bug/issue #, if applicable: rdar://112224233

Summary

Adds support for disambiguating symbol links with information from the symbol graph's functionSignature data.

This new syntax was pitched on the Swift Forums in this thread.

With these changes DocC will support type signature disambiguation in links and will use it in diagnostics but topic references and page URLs will continue to only use kind-and-hash disambiguation to ensure that all pages have the same URLs as without these changes.

Dependencies

None.

Testing

Describe how a reviewer can test the functionality of your PR. Provide test content to test with if
applicable.

Steps:

  1. Provide setup instructions.
  2. Explain in detail how the functionality can be tested.

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

@d-ronnqvist
Copy link
Contributor Author

I feel that the code in this PR is getting too long to be easily reviewed but splitting it into multiple files in the same PR makes it much much harder to know what changed and what moved. Because of this I opened #662 to only split the code into multiple files and then this PR can focus on just the new feature.

@d-ronnqvist d-ronnqvist force-pushed the readable-symbol-link-disambiguation branch from 0939305 to 4b2581b Compare December 1, 2023 15:45
@d-ronnqvist d-ronnqvist force-pushed the readable-symbol-link-disambiguation branch from 4b2581b to 7341760 Compare December 1, 2023 15:51
# Conflicts:
#	Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy+Error.swift
#	Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy+Find.swift
#	Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy+PathComponent.swift
#	Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy.swift
#	Tests/SwiftDocCTests/Infrastructure/PathHierarchyTests.swift
@d-ronnqvist d-ronnqvist force-pushed the readable-symbol-link-disambiguation branch from 9605330 to 3827762 Compare December 14, 2023 14:52
@d-ronnqvist d-ronnqvist added the merge conflicts This pull request has merge conflicts that need to be resolved label Jan 29, 2024
@d-ronnqvist
Copy link
Contributor Author

I'm breaking this up into smaller pieces (#824 and #828) so that there won't be as many merge conflicts to deal with. I'll rebase this PR once both those have been merged.

# Conflicts:
#	Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy+DisambiguatedPaths.swift
#	Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy+Error.swift
#	Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy+Find.swift
#	Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy+PathComponent.swift
#	Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy+Serialization.swift
#	Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy.swift
#	Tests/SwiftDocCTests/Infrastructure/PathHierarchyTests.swift
@d-ronnqvist d-ronnqvist removed the merge conflicts This pull request has merge conflicts that need to be resolved label Mar 4, 2024
@d-ronnqvist d-ronnqvist marked this pull request as ready for review March 4, 2024 17:04
@d-ronnqvist
Copy link
Contributor Author

The main implementation is ready for review. I will update the user facing documentation to describe this new syntax.

@d-ronnqvist d-ronnqvist changed the title Experimental support for disambiguating links with type signatures Support disambiguating links with type signature information Mar 4, 2024
@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

Also, format the error message better in single-line presentation
… of the overloaded symbols

Also, improve paths for overload groups and other preferred symbols
@d-ronnqvist d-ronnqvist force-pushed the readable-symbol-link-disambiguation branch from aea9c21 to f57ac6c Compare September 11, 2024 13:48
@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist
Copy link
Contributor Author

I resolved the merge conflicts so this is reviewable again.

I also updated the code to make some improvements for overload groups and C++ operators which are new changes since last time I updated this PR.

@d-ronnqvist
Copy link
Contributor Author

I have some more improvements in mind for the richness of the error messages about ambiguous links, but my plan is to defer that to a follow up PR, sometime after this is merged 🤞

Copy link
Contributor

@patshaughnessy patshaughnessy left a comment

Choose a reason for hiding this comment

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

Amazing work!

I left a lot of questions and a few suggestions mostly around the readability of the code. You added extensive new unit tests, which is great, but I can imagine some quirks or odd corner conditions could come up someday. I'd suggest we merge this and then keep an eye on how the disambiguation is working over time as we use these new disambiguation styles more and more.

@@ -47,7 +53,7 @@ extension PathHierarchy {

func gatherLinksFrom(_ containers: some Sequence<DisambiguationContainer>) {
for container in containers {
let disambiguatedChildren = container.disambiguatedValuesWithCollapsedUniqueSymbols(includeLanguage: false)
let disambiguatedChildren = container.disambiguatedValuesWithCollapsedUniqueSymbols(includeLanguage: false, allowAdvancedDisambiguation: true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Following on the previous comment: Here you set the values for these to false and true. It might be hard for someone in the future to track down where you set these values. Maybe using FeatureFlags or maybe some struct-wide attribute of PathHierarchy instead of function parameters would make the code less verbose and easier to understand?

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'm not too concerned about this since these are mostly file private arguments that are passed by the caller.

return lhs.fullName < rhs.fullName
}
.map { (fullName: String, suggestedDisambiguation: String) -> Solution in
// In contexts that display the solution message on a single line by removing newlines, this extra whitespace makes it look correct ─────────────╮
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure I understand what this is referring to.

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 default diagnostic formatter outputs diagnostic solutions (phrased as "suggestions") on a single line. For example:

4 | /// Here's a link that uses the wrong symbol kind disambiguation in a link:
5 + /// - ``myFunction(with:and:)-struct``
  |                              ╰─suggestion: Remove '-struct' for'func myFunction(with one: Int, and two: String) -> Bool'

If you look closely at the suggestion text you can see that there's a missing space between "for" and the symbol name.

With these changes this suggestion text isn't missing that space:

suggestion: Remove '-struct' for 'func myFunction(with one: Int, and two: String) -> Bool'
                                ^
                                with this change there is an added space here

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist
Copy link
Contributor Author

I started updating the user facing documentation related to link disambiguation but found that I want to make slightly larger documentation changes that I first thought and I feel that those updates would benefit from a separate review.

I will merge this PR now since it's a big improvement and there doesn't seem to be any major feedback anymore. If there's more changes that you're like to request, please message me and I'll make a follow up PR. I already have further refinements and enhancements in mind so I plan to continue to iterate on this code over the coming months anyway.

@d-ronnqvist d-ronnqvist merged commit f8d16c5 into swiftlang:main Sep 18, 2024
2 checks passed
@d-ronnqvist d-ronnqvist deleted the readable-symbol-link-disambiguation branch September 18, 2024 07:52
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