Add a new link resolver that finds documentation in a hierarchy of path components #337

d-ronnqvist · 2022-07-19T00:26:42Z

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

Summary

This adds an alternative link resolution implementation that searches a hierarchy of path components with disambiguation information to find documentation. This new resolver is off-by-default but can be enabeld by via a user default or environmental variable.

This new implementation is meant to eventually replace the current documentation cache based implementation but only after we're confident in the quality. Since the documentation cache based link resolver implementation is meant to be removed in the future, I didn't see a value in defining a common interface for both implementations.

Because both implementation exist side-by-side almost no code is removed in this PR. Breaking down the 21.7K added lines of code; 18.7K are from new symbol graph file test data and the remaining 3K LOC is half new implementation and half new unit tests.

This new implementation was originally prototyped here #251. The user facing differences it brings is more flexibility with disambiguation for symbols and that more actionable information in diagnostics for unresolved links.

Dependencies

n/a

Testing

Without any additional configuration DocC should continue to behave exactly the same.

To test the new link resolver implementation:

Set either the DocCUseHierarchyBasedLinkResolver user default or the DOCC_USE_HIERARCHY_BASED_LINK_RESOLVER environmental variable to a true boolean value.
Run any docc convert command.

The expected behaviors are:

All links that resolve in the default DocC configuration should continue to resolve
Some links that didn't resolve in the default configuration may resolve successfully in the new implementation (for example if they are overly disambiguated).
Links that fail to resolve (especially links to symbols) should have more actionable diagnostics.

You can also set the DOCC_REPORT_LINK_RESOLUTION_MISMATCHES environmental variable to a true boolean value to enable automatic reporting of mismatched behaviors between the two implementations. Opting in to automatic reporting means that portions of both systems will run side-by-side performing redundant work so it comes with some performance cost.

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

This resolver is off-by-default but can be enabled via configuration. Also, add some high level documentation of how link resolution and link disambiguation works.

d-ronnqvist · 2022-07-19T17:48:01Z

@swift-ci please test

d-ronnqvist · 2022-07-19T17:55:13Z

All the code in this PR so far should be in a reviewable state. I'm thinking about also including some additional changes in this PR to diff the necessary symbol disambiguation suffixes for the two implementations and add them as automatic redirects in the linkable entities file. I plan on making those changes regardless. The main question is if I do it in this PR or in a follow up PR.

d-ronnqvist · 2022-07-19T18:00:35Z

@swift-ci please test

franklinsch · 2022-07-20T09:37:05Z

Thanks @d-ronnqvist! I haven't reviewed this closely yet, but have you also considered a mode for running both the new and old resolvers side-by-side and have DocC flag any mismatches? I believe there was discussion around that but I can't remember the outcome.

d-ronnqvist · 2022-07-20T21:45:45Z

have you also considered a mode for running both the new and old resolvers side-by-side and have DocC flag any mismatches?

Yes, that's what I was referring to with "some additional changes [...] to diff the necessary symbol disambiguation suffixes for the two implementations".

Originally I was only thinking about flagging mismatches for the disambiguation suffixes that DocC adds to resolved references but now that I think about it it may also make sense to flag links that resolved in the old resolver but didn't in the new resolver.

d-ronnqvist · 2022-07-21T00:14:01Z

I pushed a new commit adding the ability to report mismatches between the two implementations (both mismatches in the computed disambiguations for symbols or mismatches in the link resolution results separately).

In my limited testing with our test bundles I only saw two differences (in the new "MixedLanguageFrameworkWithLanguageRefinements" test bundle added in this PR). In both cases I feel that the new reference is more correct.

In the first case; MyObjectiveCOptionNone is the Objective-C name for an empty OptionSet and doesn't have a Swift counterpart so I feel that it makes more sense that the container type is disambiguated as the Objective-C version of the container symbol.

+ /documentation/MixedFramework/MyObjectiveCOption-c.enum/MyObjectiveCOptionNone  
- /documentation/MixedFramework/MyObjectiveCOption-swift.struct/MyObjectiveCOptionNone

In the second case; init is the Objective-C spelling of an initializer—the Swift version would be init()—so I feel that it makes more sense that the container type is the Objective-C name of that symbol (the containing class symbol has different names in different languages):

+ /documentation/MixedFramework/MySwiftClassObjectiveCName/init
- /documentation/MixedFramework/MySwiftClassSwiftName/init

d-ronnqvist · 2022-07-21T00:18:22Z

@swift-ci please test

Sources/SwiftDocC/Infrastructure/DocumentationContext.swift

franklinsch · 2022-07-22T13:15:12Z

Sources/SwiftDocC/Infrastructure/DocumentationContext.swift

-            let symbolReferences = documentationCacheBasedLinkResolver.referencesForSymbols(in: symbolGraphLoader.unifiedGraphs, bundle: bundle, context: self)
+            let symbolReferences: [SymbolGraph.Symbol.Identifier : [ResolvedTopicReference]]
+            if LinkResolutionMigrationConfiguration.shouldUseHierarchyBasedLinkResolver {
+                symbolReferences = hierarchyBasedLinkResolver!.referencesForSymbols(in:symbolGraphLoader.unifiedGraphs, bundle: bundle, context: self)


nit

Suggested change

symbolReferences = hierarchyBasedLinkResolver!.referencesForSymbols(in:symbolGraphLoader.unifiedGraphs, bundle: bundle, context: self)

symbolReferences = hierarchyBasedLinkResolver!.referencesForSymbols(in: symbolGraphLoader.unifiedGraphs, bundle: bundle, context: self)

Sources/SwiftDocC/Infrastructure/DocumentationContext.swift

Sources/SwiftDocC/Infrastructure/Link Resolution/LinkResolutionMigration.swift

franklinsch · 2022-07-22T13:18:33Z

Sources/SwiftDocC/Infrastructure/DocumentationContext.swift

+            for tutorial in tutorials {
+                hierarchyBasedResolver.addTutorial(tutorial)
+            }
+            for article in tutorialArticles {
+                hierarchyBasedResolver.addTutorialArticle(article)
+            }
+            for technology in technologies {
+                hierarchyBasedResolver.addTechnology(technology)


Sources/SwiftDocC/SwiftDocC.docc/SwiftDocC/LinkResolution.md

franklinsch · 2022-07-22T13:29:05Z

Sources/SwiftDocC/SwiftDocC.docc/SwiftDocC/LinkResolution.md

@@ -0,0 +1,59 @@
+# Linking between documentation


Great idea writing this!

Sources/SwiftDocC/SwiftDocC.docc/SwiftDocC/LinkResolution.md

franklinsch · 2022-07-22T13:31:28Z

Tests/SwiftDocCTests/Infrastructure/PathHierarchyTests.swift

+        let id = try find(path: rawPath, parent: parent, onlyFindSymbols: true)
+        return lookup[id]!.symbol!
+    }
+}


Extremely solid testing here 🎩

…rpart in another language Also, improve error message when no part of the unresolved symbol link can be found.

Also, fix broken symbol link in DocC documentation.

…ches

# Conflicts: # Sources/SwiftDocC/Semantics/MarkupReferenceResolver.swift # Tests/SwiftDocCTests/Rendering/RenderNodeTranslatorTests.swift

d-ronnqvist · 2022-07-28T21:19:14Z

@swift-ci please test

Co-authored-by: Franklin Schrans <[email protected]>

d-ronnqvist · 2022-07-28T21:20:21Z

@swift-ci please test

d-ronnqvist · 2022-07-28T21:26:20Z

I think that's all the changes and all the functionality that I wanted to add in this PR. Developers can now run either of the link resolver implementations and optionally report mismatches between the two implementations. Both of these are off by default (meaning that the documentation cache based link resolver continues to be the default).

d-ronnqvist · 2022-07-29T18:59:29Z

@swift-ci please test

franklinsch

Looks great to me, I just have a question about testing.

franklinsch · 2022-08-01T12:29:05Z

Tests/SwiftDocCTests/DocumentationService/ConvertService/ConvertServiceTests.swift

-                renderNode.abstract?.first,
-                .codeVoice(code: "myFunction()")
-            )
+            if LinkResolutionMigrationConfiguration.shouldUseHierarchyBasedLinkResolver {


for this test and the others, would shouldUseHierarchyBasedLinkResolver be set at the command-line when invoking testing? Should we have a variant of this test that runs with shouldUseHierarchyBasedLinkResolver == true so that we test both implementations? Otherwise we might miss regressions with one or other implementation. We can extract the test method body into a separate function and call it in different tests with shouldUseHierarchyBasedLinkResolver true or false.

It's possible to run the tests using the new implementation by setting DOCC_USE_HIERARCHY_BASED_LINK_RESOLVER=YES as an environmental variable.

I think that It's equally important to catch regressions in the tests that have the same behavior in both implementations as it is in the handful of tests that have differences in behavior across the two implementations.

If the time increase is acceptable I would suggest updating "bin/test" to run all the tests with both implementations. If not, then maybe we can run the new implementation before merging for PRs where we think it's needed and at some point flip the default value so that the new implementation always runs and we'd run the previous implementation before merging PRs.

@franklinsch Do you feel that this change to run the tests with both link resolver implementations and this other change to skip a slow test that doesn't report results cover the testing needs for this?

d-ronnqvist · 2022-08-02T17:26:26Z

@swift-ci please test

Add a new link resolver that searches a hierarchy of path components

82eff64

This resolver is off-by-default but can be enabled via configuration. Also, add some high level documentation of how link resolution and link disambiguation works.

d-ronnqvist mentioned this pull request Jul 19, 2022

[Prototype] Improve link resolution for symbol links #251

Closed

4 tasks

Conditionally skip PathHierarchy tests

faf888e

d-ronnqvist requested review from QuietMisdreavus, daniel-grumberg, ethan-kusters and franklinsch July 19, 2022 18:00

d-ronnqvist added 3 commits July 20, 2022 14:48

Handle collisions when all matches are the same symbol

7efe03d

Merge branch 'main' into hierarchy-based-link-resolver

0be4a3c

Add opt-in ability to report mismatches between the two link resolvers.

4d59bec

franklinsch reviewed Jul 22, 2022

View reviewed changes

This was referenced Jul 25, 2022

Stable names for headings and internal <doc:> links #345

Open

Symbol linking disambiguation help in warnings #346

Closed

d-ronnqvist linked an issue Jul 26, 2022 that may be closed by this pull request

Symbol linking disambiguation help in warnings #346

Closed

d-ronnqvist added 5 commits July 27, 2022 22:22

Improve disambiguation of symbols that only collide with their counte…

d5eec42

…rpart in another language Also, improve error message when no part of the unresolved symbol link can be found.

Update link examples in DocC documentation about documentation links.

f72e6a6

Also, fix broken symbol link in DocC documentation.

Update gathering and reporting of link resolver implementation mismat…

18a9eff

…ches

Merge branch 'main' into hierarchy-based-link-resolver

a8510ca

# Conflicts: # Sources/SwiftDocC/Semantics/MarkupReferenceResolver.swift # Tests/SwiftDocCTests/Rendering/RenderNodeTranslatorTests.swift

Update for snippet changes and the new Snippets.docc test data

1d7537e

Fix minor pluralization in documentation article

4691574

Co-authored-by: Franklin Schrans <[email protected]>

d-ronnqvist linked an issue Jul 28, 2022 that may be closed by this pull request

Improve the core link resolution implementation #311

Closed

Merge branch 'main' into hierarchy-based-link-resolver

0ac7563

franklinsch reviewed Aug 1, 2022

View reviewed changes

d-ronnqvist added 2 commits August 2, 2022 10:25

Run tests twice (once with each link resolver implementation)

f36aef8

Skip slow performance test that isn't compared to previous results

4a9068f

franklinsch approved these changes Aug 3, 2022

View reviewed changes

d-ronnqvist merged commit 587c879 into swiftlang:main Aug 3, 2022

theMomax mentioned this pull request Aug 13, 2022

Extensions to External Types #335

Closed

3 tasks

d-ronnqvist deleted the hierarchy-based-link-resolver branch October 25, 2024 09:00

	symbolReferences = hierarchyBasedLinkResolver!.referencesForSymbols(in:symbolGraphLoader.unifiedGraphs, bundle: bundle, context: self)
	symbolReferences = hierarchyBasedLinkResolver!.referencesForSymbols(in: symbolGraphLoader.unifiedGraphs, bundle: bundle, context: self)

Add a new link resolver that finds documentation in a hierarchy of path components #337

Add a new link resolver that finds documentation in a hierarchy of path components #337

Uh oh!

Conversation

d-ronnqvist commented Jul 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Dependencies

Testing

Checklist

Uh oh!

d-ronnqvist commented Jul 19, 2022

Uh oh!

d-ronnqvist commented Jul 19, 2022

Uh oh!

d-ronnqvist commented Jul 19, 2022

Uh oh!

franklinsch commented Jul 20, 2022

Uh oh!

d-ronnqvist commented Jul 20, 2022

Uh oh!

d-ronnqvist commented Jul 21, 2022

Uh oh!

d-ronnqvist commented Jul 21, 2022

Uh oh!

Uh oh!

franklinsch Jul 22, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

franklinsch Jul 22, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

franklinsch Jul 22, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

franklinsch Jul 22, 2022

Choose a reason for hiding this comment

Uh oh!

d-ronnqvist commented Jul 28, 2022

Uh oh!

d-ronnqvist commented Jul 28, 2022

Uh oh!

d-ronnqvist commented Jul 28, 2022

Uh oh!

d-ronnqvist commented Jul 29, 2022

Uh oh!

franklinsch left a comment

Choose a reason for hiding this comment

Uh oh!

franklinsch Aug 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d-ronnqvist Aug 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d-ronnqvist Aug 2, 2022

Choose a reason for hiding this comment

Uh oh!

d-ronnqvist commented Aug 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

d-ronnqvist commented Jul 19, 2022 •

edited

Loading

franklinsch Aug 1, 2022 •

edited

Loading

d-ronnqvist Aug 1, 2022 •

edited

Loading